Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

insertVulnerabilityFixedInFeature: pq: duplicate key value violates unique constraint #238

Closed
jonaz opened this issue Aug 30, 2016 · 11 comments
Assignees
Labels
kind/bug things are not as they seem priority/urgent critical functionality

Comments

@jonaz
Copy link

jonaz commented Aug 30, 2016

Just tested clair for the first time. Followed the instructions in README.md running with docker-compose so the image is: quay.io/coreos/clair

After a few hours running i get duplicate key error when updateing.

2016-08-30 11:44:11.079358 E | pgsql: insertVulnerabilityFixedInFeature: pq: duplicate key value violates unique constraint "vulnerability_fixedin_feature_vulnerability_id_feature_id_key"
database.Vulnerability{Model:database.Model{ID:0}, Name:"RHSA-2009:0382", Namespace:database.Namespace{Model:database.Model{ID:0}, Name:"centos:5"}, Description:"libvirt is a C API for managing and interacting with the virtualization capabilities of Linux and other operating systems. libvirt also provides tools for remotely managing virtualized systems. The libvirtd daemon was discovered to not properly check user connection permissions before performing certain privileged actions, such as requesting migration of an unprivileged guest domain to another system. A local user able to establish a read-only connection to libvirtd could use this flaw to perform actions that should be restricted to read-write connections. (CVE-2008-5086) libvirt_proxy, a setuid helper application allowing non-privileged users to communicate with the hypervisor, was discovered to not properly validate user requests. Local users could use this flaw to cause a stack-based buffer overflow in libvirt_proxy, possibly allowing them to run arbitrary code with root privileges. (CVE-2009-0036) All users are advised to upgrade to these updated packages, which contain backported patches which resolve these issues. After installing the update, libvirtd must be restarted manually (for example, by issuing a \"service libvirtd restart\" command), and guest systems rebooted, for this change to take effect.", Link:"https://rhn.redhat.com/errata/RHSA-2009-0382.html", Severity:"Medium", Metadata:database.MetadataMap(nil), FixedIn:[]database.FeatureVersion{database.FeatureVersion{Model:database.Model{ID:0}, Feature:database.Feature{Model:database.Model{ID:0}, Name:"libvirt-devel", Namespace:database.Namespace{Model:database.Model{ID:0}, Name:"centos:5"}}, Version:types.Version{epoch:0, version:"0.3.3", revision:"14.el5_3.1"}, AffectedBy:[]database.Vulnerability(nil), AddedBy:database.Layer{Model:database.Model{ID:0}, Name:"", EngineVersion:0, Parent:(*database.Layer)(nil), Namespace:(*database.Namespace)(nil), Features:[]database.FeatureVersion(nil)}}, database.FeatureVersion{Model:database.Model{ID:0}, Feature:database.Feature{Model:database.Model{ID:0}, Name:"libvirt", Namespace:database.Namespace{Model:database.Model{ID:0}, Name:"centos:5"}}, Version:types.Version{epoch:0, version:"0.3.3", revision:"14.el5_3.1"}, AffectedBy:[]database.Vulnerability(nil), AddedBy:database.Layer{Model:database.Model{ID:0}, Name:"", EngineVersion:0, Parent:(*database.Layer)(nil), Namespace:(*database.Namespace)(nil), Features:[]database.FeatureVersion(nil)}}, database.FeatureVersion{Model:database.Model{ID:0}, Feature:database.Feature{Model:database.Model{ID:0}, Name:"libvirt-python", Namespace:database.Namespace{Model:database.Model{ID:0}, Name:"centos:5"}}, Version:types.Version{epoch:0, version:"0.3.3", revision:"14.el5_3.1"}, AffectedBy:[]database.Vulnerability(nil), AddedBy:database.Layer{Model:database.Model{ID:0}, Name:"", EngineVersion:0, Parent:(*database.Layer)(nil), Namespace:(*database.Namespace)(nil), Features:[]database.FeatureVersion(nil)}}, database.FeatureVersion{Model:database.Model{ID:0}, Feature:database.Feature{Model:database.Model{ID:0}, Name:"libvirt-python", Namespace:database.Namespace{Model:database.Model{ID:0}, Name:"centos:5"}}, Version:types.Version{epoch:0, version:"0.3.3", revision:"14.el5_3.1"}, AffectedBy:[]database.Vulnerability(nil), AddedBy:database.Layer{Model:database.Model{ID:0}, Name:"", EngineVersion:0, Parent:(*database.Layer)(nil), Namespace:(*database.Namespace)(nil), Features:[]database.FeatureVersion(nil)}}, database.FeatureVersion{Model:database.Model{ID:0}, Feature:database.Feature{Model:database.Model{ID:0}, Name:"libvirt-devel", Namespace:database.Namespace{Model:database.Model{ID:0}, Name:"centos:5"}}, Version:types.Version{epoch:0, version:"0.3.3", revision:"14.el5_3.1"}, AffectedBy:[]database.Vulnerability(nil), AddedBy:database.Layer{Model:database.Model{ID:0}, Name:"", EngineVersion:0, Parent:(*database.Layer)(nil), Namespace:(*database.Namespace)(nil), Features:[]database.FeatureVersion(nil)}}, database.FeatureVersion{Model:database.Model{ID:0}, Feature:database.Feature{Model:database.Model{ID:0}, Name:"libvirt", Namespace:database.Namespace{Model:database.Model{ID:0}, Name:"centos:5"}}, Version:types.Version{epoch:0, version:"0.3.3", revision:"14.el5_3.1"}, AffectedBy:[]database.Vulnerability(nil), AddedBy:database.Layer{Model:database.Model{ID:0}, Name:"", EngineVersion:0, Parent:(*database.Layer)(nil), Namespace:(*database.Namespace)(nil), Features:[]database.FeatureVersion(nil)}}}, LayersIntroducingVulnerability:[]database.Layer(nil), FixedBy:types.Version{epoch:0, version:"", revision:""}}
2016-08-30 11:44:11.079604 E | updater: an error occured when inserting vulnerabilities for update: database: an error occured when querying the backend
2016-08-30 11:44:11.100811 I | updater: updating vulnerabilities
2016-08-30 11:44:11.100885 I | updater: fetching vulnerability updates
2016-08-30 11:44:11.100991 I | updater/fetchers/ubuntu: fetching Ubuntu vulnerabilities
2016-08-30 11:44:11.101055 I | updater/fetchers/rhel: fetching Red Hat vulnerabilities
2016-08-30 11:44:11.101401 I | updater/fetchers/debian: fetching Debian vulnerabilities
2016-08-30 11:56:59.028545 I | updater: adding metadata to vulnerabilities

@dmwoods38
Copy link

Same thing here, has anyone resolved this issue?

@Quentin-M
Copy link
Contributor

Hi,

This issue looks repeatable and pretty critical, I'll free some time next week to take a look!

@Quentin-M Quentin-M self-assigned this Oct 6, 2016
@Quentin-M Quentin-M added kind/bug things are not as they seem component/updater priority/urgent critical functionality labels Oct 6, 2016
@Windfarer
Copy link

the same issue

@jason-odess
Copy link

We're also getting this error. Using the same instructions in the README with docker-compose. Any updates on this issue?

@ruqqq
Copy link

ruqqq commented Oct 26, 2016

Are there any workarounds for this yet?

@phaseshift42
Copy link

Red Hat's OVAL files com.redhat.rhsa-20090382.xml and com.redhat.rhsa-20090045.xml describe RHSA-2009:0382-01 and RHSA-2009:0382-02 respectively; these are two releases of the same RHSA with only minor differences. Clair 1.2.x disregards the '-xx' revision suffix and attempts to insert data for both releases as RHSA-2009:0382, which violates the database uniqueness constraint.

I've had success working around this by explicitly skipping com.redhat.rhsa-20090382.xml (the older release) with a minor edit to the RHEL fetcher:

--- a/updater/fetchers/rhel/rhel.go
+++ b/updater/fetchers/rhel/rhel.go
@@ -116,7 +116,7 @@ func (f *RHELFetcher) FetchUpdate(datastore database.Datastore) (resp updater.Fe
                r := rhsaRegexp.FindStringSubmatch(line)
                if len(r) == 2 {
                        rhsaNo, _ := strconv.Atoi(r[1])
-                       if rhsaNo > firstRHSA {
+                       if rhsaNo > firstRHSA && rhsaNo != 20090382 {
                                rhsaList = append(rhsaList, rhsaNo)
                        }
                }

@Windfarer
Copy link

@Quentin-M Do you have any plan to fix this?

@Quentin-M
Copy link
Contributor

Quentin-M commented Nov 11, 2016

The revision suffix is never parsed because the name is extracted from the definitions>definition>metadata>title field, which does not include it.

While the Debian and Ubuntu fetchers maintain an internal list of vulnerabilities to keep them unique at return time, the RHEL/OVAL fetcher does not. However, doVulnerabilitiesNamespacing catches the duplicates and merge them. Unfortunately, its logic is somewhat limited in the sense that it does not verify that FixedIn features are unique. The single vulnerability will the duplicated FixedIn goes all the way to insertVulnerabilityFixedInFeatureVersions, which tries to insert the relationship between the vulnerability and the feature twice.

Therefore, while we could fix the issue at multiple levels, I think that the most durable solution would be to harden insertVulnerabilityFixedInFeatureVersions itself against this kind of non-coherent input. We could either use a map to dedup the IDs or simply rely on PostgreSQL by ignoring this particular error there, the second solution would be the fastest in the general case. Would you agree with this solution @jzelinskie?

Now, what's scarier about this whole thing is that RHEL may have added an errata with an old date? Both advisories are dated 2009 / 2011, timestamped 2015 but we didn't encounter this issue before and I don't see any code change that might have introduced it. Also, the updated errata (20090045) has an identification number lower than the original one (20090382)..

@jzelinskie
Copy link
Contributor

We could either use a map to dedup the IDs or simply rely on PostgreSQL by ignoring this particular error there, the second solution would be the fastest in the general case. Would you agree with this solution @jzelinskie?

I think it's safe to rely on the unique constraint in the database. Clair's storage is inherently relational, so I don't think we ever plan to support a database layer that doesn't have unique indices.
Writing a test that asserts the failure for the storage engine seems like a good way to ensure any future database implementations are aware of this requirement in behavior.

@Quentin-M
Copy link
Contributor

Quentin-M commented Nov 11, 2016

@jzelinskie Agreed. In the early stages of the project, most of the database tests were implementation agnostic and were running on each registered implementation. But it got removed after an internal discussion for a silly reason that I can't seem to remember. I extended the pgsql's tests to cover this case. What about creating a follow-up issue to extract as many tests as possible from the pgsql implementation and put them at the database level instead?

@jzelinskie
Copy link
Contributor

sgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug things are not as they seem priority/urgent critical functionality
Development

No branches or pull requests

8 participants