Merge pull request #263 from Quentin-M/rhel_unique_fixedin

pgsql: Do not insert entry in Vulnerability_FixedIn_Feature if existing

README.md

@@ -73,9 +73,9 @@ This runs a PostgreSQL database insecurely and locally in a container.
 This method should only be used for testing.
 
 ```sh
-$ curl -L https://raw.githubusercontent.com/coreos/clair/master/docker-compose.yml -o $HOME/docker-compose.yml
+$ curl -L https://raw.githubusercontent.com/coreos/clair/v1.2.5/docker-compose.yml -o $HOME/docker-compose.yml
 $ mkdir $HOME/clair_config
-$ curl -L https://raw.githubusercontent.com/coreos/clair/master/config.example.yaml -o $HOME/clair_config/config.yaml
+$ curl -L https://raw.githubusercontent.com/coreos/clair/v1.2.5/config.example.yaml -o $HOME/clair_config/config.yaml
 $ $EDITOR $HOME/clair_config/config.yaml # Edit database source to be postgresql://postgres:password@postgres:5432?sslmode=disable
 $ docker-compose -f $HOME/docker-compose.yml up -d
 ```
@@ -93,9 +93,9 @@ This is the recommended method for production deployments.
 
 ```sh
 $ mkdir $HOME/clair_config
-$ curl -L https://raw.githubusercontent.com/coreos/clair/master/config.example.yaml -o $HOME/clair_config/config.yaml
+$ curl -L https://raw.githubusercontent.com/coreos/clair/v1.2.5/config.example.yaml -o $HOME/clair_config/config.yaml
 $ $EDITOR $HOME/clair_config/config.yaml # Add the URI for your postgres database
-$ docker run -d -p 6060-6061:6060-6061 -v $HOME/clair_config:/config quay.io/coreos/clair -config=/config/config.yaml
+$ docker run -d -p 6060-6061:6060-6061 -v $HOME/clair_config:/config quay.io/coreos/clair:v1.2.5 -config=/config/config.yaml
 ```
 
 ### Source
@@ -119,10 +119,10 @@ $ ./$GOBIN/clair -config=config.yaml
 
 ## Documentation
 
-Documentation can be found in a `README.md` file located in the directory of the component.
+The latest stable documentation can be found [on the CoreOS website]. Documentation for the current branch can be found [inside the Documentation directory][docs-dir] at the root of the project's source code.
 
-- [Notifier](https://github.com/coreos/clair/blob/master/notifier/README.md)
-- [v1 API](https://github.com/coreos/clair/blob/master/api/v1/README.md)
+[on the CoreOS website]: https://coreos.com/clair/docs/latest/
+[docs-dir]: /Documentation
 
 ### Architecture at a Glance
 

database/namespace_mapping.go

@@ -40,4 +40,5 @@ var UbuntuReleasesMapping = map[string]string{
 	"vivid":   "15.04",
 	"wily":    "15.10",
 	"xenial":  "16.04",
+	"yakkety": "16.10",
 }

database/pgsql/feature.go

@@ -129,11 +129,11 @@ func (pgSQL *pgSQL) insertFeatureVersion(featureVersion database.FeatureVersion)
 	}
 
 	// Find or create FeatureVersion.
-	var newOrExisting string
+	var created bool
 
 	t = time.Now()
 	err = tx.QueryRow(soiFeatureVersion, featureID, &featureVersion.Version).
-		Scan(&newOrExisting, &featureVersion.ID)
+		Scan(&created, &featureVersion.ID)
 	observeQueryTime("insertFeatureVersion", "soiFeatureVersion", t)
 
 	if err != nil {
@@ -141,8 +141,9 @@ func (pgSQL *pgSQL) insertFeatureVersion(featureVersion database.FeatureVersion)
 		return 0, handleError("soiFeatureVersion", err)
 	}
 
-	if newOrExisting == "exi" {
-		// That featureVersion already exists, return its id.
+	if !created {
+		// The featureVersion already existed, no need to link it to
+		// vulnerabilities.
 		tx.Commit()
 
 		if pgSQL.cache != nil {

database/pgsql/queries.go

@@ -63,9 +63,9 @@ const (
 			WHERE NOT EXISTS (SELECT id FROM FeatureVersion WHERE feature_id = $1 AND version = $2)
 			RETURNING id
 		)
-		SELECT 'exi', id FROM FeatureVersion WHERE feature_id = $1 AND version = $2
+		SELECT false, id FROM FeatureVersion WHERE feature_id = $1 AND version = $2
 		UNION
-		SELECT 'new', id FROM new_featureversion`
+		SELECT true, id FROM new_featureversion`
 
 	searchVulnerabilityFixedInFeature = `
 		SELECT id, vulnerability_id, version FROM Vulnerability_FixedIn_Feature
@@ -160,10 +160,16 @@ const (
 		VALUES($1, $2, $3, $4, $5, $6, CURRENT_TIMESTAMP)
 		RETURNING id`
 
-	insertVulnerabilityFixedInFeature = `
-		INSERT INTO Vulnerability_FixedIn_Feature(vulnerability_id, feature_id, version)
-		VALUES($1, $2, $3)
-		RETURNING id`
+	soiVulnerabilityFixedInFeature = `
+		WITH new_fixedinfeature AS (
+			INSERT INTO Vulnerability_FixedIn_Feature(vulnerability_id, feature_id, version)
+			SELECT CAST($1 AS INTEGER), CAST($2 AS INTEGER), CAST($3 AS VARCHAR)
+			WHERE NOT EXISTS (SELECT id FROM Vulnerability_FixedIn_Feature WHERE vulnerability_id = $1 AND feature_id = $2)
+			RETURNING id
+		)
+		SELECT false, id FROM Vulnerability_FixedIn_Feature WHERE vulnerability_id = $1 AND feature_id = $2
+		UNION
+		SELECT true, id FROM new_fixedinfeature`
 
 	searchFeatureVersionByFeature = `SELECT id, version FROM FeatureVersion WHERE feature_id = $1`
 
@@ -205,17 +211,22 @@ const (
 		WHERE name = $1`
 
 	searchNotificationLayerIntroducingVulnerability = `
+	WITH subquery AS (
 		SELECT l.ID, l.name
-		FROM Vulnerability v, Vulnerability_Affects_FeatureVersion vafv, FeatureVersion fv, Layer_diff_FeatureVersion ldfv, Layer l
-		WHERE v.id = $1
-					AND v.id = vafv.vulnerability_id
-					AND vafv.featureversion_id = fv.id
-					AND fv.id = ldfv.featureversion_id
-					AND ldfv.modification = 'add'
-					AND ldfv.layer_id = l.id
-					AND l.id >= $2
+		FROM Vulnerability_Affects_FeatureVersion vafv, FeatureVersion fv, Layer_diff_FeatureVersion ldfv, Layer l
+		WHERE l.id >= $2
+			AND vafv.vulnerability_id = $1
+			AND vafv.featureversion_id = fv.id
+			AND ldfv.featureversion_id = fv.id
+			AND ldfv.modification = 'add'
+			AND ldfv.layer_id = l.id
 		ORDER BY l.ID
-		LIMIT $3`
+	)
+
+	SELECT *
+	FROM subquery
+	LIMIT $3;
+`
 
 	// complex_test.go
 	searchComplexTestFeatureVersionAffects = `

database/pgsql/vulnerability.go

@@ -433,18 +433,25 @@ func (pgSQL *pgSQL) insertVulnerabilityFixedInFeatureVersions(tx *sql.Tx, vulner
 
 	for _, fv := range fixedIn {
 		var fixedInID int
+		var created bool
 
-		// Insert Vulnerability_FixedIn_Feature.
+		// Find or create entry in Vulnerability_FixedIn_Feature.
 		err = tx.QueryRow(
-			insertVulnerabilityFixedInFeature,
+			soiVulnerabilityFixedInFeature,
 			vulnerabilityID, fv.Feature.ID,
 			&fv.Version,
-		).Scan(&fixedInID)
+		).Scan(&created, &fixedInID)
 
 		if err != nil {
 			return handleError("insertVulnerabilityFixedInFeature", err)
 		}
 
+		if !created {
+			// The relationship between the feature and the vulnerability already
+			// existed, no need to update Vulnerability_Affects_FeatureVersion.
+			continue
+		}
+
 		// Insert Vulnerability_Affects_FeatureVersion.
 		err = linkVulnerabilityToFeatureVersions(tx, fixedInID, vulnerabilityID, fv.Feature.ID, fv.Version)
 		if err != nil {

database/pgsql/vulnerability_test.go

@@ -225,25 +225,24 @@ func TestInsertVulnerability(t *testing.T) {
 	v1.Description = "TestInsertVulnerabilityLink2"
 	v1.Link = "TestInsertVulnerabilityLink2"
 	v1.Severity = types.High
-	// Update f3 in f4, add fixed in f5, add fixed in f6 which already exists, removes fixed in f7 by
-	// adding f8 which is f7 but with MinVersion.
-	v1.FixedIn = []database.FeatureVersion{f4, f5, f6, f8}
+	// Update f3 in f4, add fixed in f5, add fixed in f6 which already exists,
+	// removes fixed in f7 by adding f8 which is f7 but with MinVersion, and
+	// add fixed by f5 a second time (duplicated).
+	v1.FixedIn = []database.FeatureVersion{f4, f5, f6, f8, f5}
 
 	err = datastore.InsertVulnerabilities([]database.Vulnerability{v1}, true)
 	if assert.Nil(t, err) {
 		v1f, err := datastore.FindVulnerability(n1.Name, v1.Name)
 		if assert.Nil(t, err) {
+			// Remove f8 from the struct for comparison as it was just here to cancel f7.
+			// Remove one of the f5 too as it was twice in the struct but the database
+			// implementation should have dedup'd it.
+			v1.FixedIn = v1.FixedIn[:len(v1.FixedIn)-2]
+
 			// We already had f1 before the update.
 			// Add it to the struct for comparison.
 			v1.FixedIn = append(v1.FixedIn, f1)
 
-			// Removes f8 from the struct for comparison as it was just here to cancel f7.
-			for i := 0; i < len(v1.FixedIn); i++ {
-				if v1.FixedIn[i].Feature.Name == f8.Feature.Name {
-					v1.FixedIn = append(v1.FixedIn[:i], v1.FixedIn[i+1:]...)
-				}
-			}
-
 			equalsVuln(t, &v1, &v1f)
 		}
 	}

updater/fetchers/rhel/rhel.go

@@ -290,7 +290,8 @@ func toFeatureVersions(criteria criteria) []database.FeatureVersion {
 			}
 		}
 
-		if osVersion > firstConsideredRHEL {
+		if osVersion >= firstConsideredRHEL {
+			// TODO(vbatts) this is where features need multiple labels ('centos' and 'rhel')
 			featureVersion.Feature.Namespace.Name = "centos" + ":" + strconv.Itoa(osVersion)
 		} else {
 			continue

updater/fetchers/ubuntu/ubuntu.go

@@ -36,7 +36,7 @@ import (
 
 const (
 	trackerURI        = "https://launchpad.net/ubuntu-cve-tracker"
-	trackerRepository = "lp:ubuntu-cve-tracker"
+	trackerRepository = "https://launchpad.net/ubuntu-cve-tracker"
 	updaterFlag       = "ubuntuUpdater"
 	cveURL            = "http://people.ubuntu.com/~ubuntu-security/cve/%s"
 )

worker/detectors/namespace.go

@@ -21,6 +21,7 @@ import (
 	"sync"
 
 	"github.com/coreos/clair/database"
+	"github.com/coreos/pkg/capnslog"
 )
 
 // The NamespaceDetector interface defines a way to detect a Namespace from input data.
@@ -34,6 +35,8 @@ type NamespaceDetector interface {
 }
 
 var (
+	nlog = capnslog.NewPackageLogger("github.com/coreos/clair", "worker/detectors")
+
 	namespaceDetectorsLock sync.Mutex
 	namespaceDetectors     = make(map[string]NamespaceDetector)
 )
@@ -62,8 +65,9 @@ func RegisterNamespaceDetector(name string, f NamespaceDetector) {
 
 // DetectNamespace finds the OS of the layer by using every registered NamespaceDetector.
 func DetectNamespace(data map[string][]byte) *database.Namespace {
-	for _, detector := range namespaceDetectors {
+	for name, detector := range namespaceDetectors {
 		if namespace := detector.Detect(data); namespace != nil {
+			nlog.Debugf("detector: %q; namespace: %q\n", name, namespace.Name)
 			return namespace
 		}
 	}

worker/detectors/namespace/osrelease/osrelease.go

@@ -24,6 +24,8 @@ import (
 )
 
 var (
+	//log = capnslog.NewPackageLogger("github.com/coreos/clair", "worker/detectors/namespace/osrelease")
+
 	osReleaseOSRegexp      = regexp.MustCompile(`^ID=(.*)`)
 	osReleaseVersionRegexp = regexp.MustCompile(`^VERSION_ID=(.*)`)
 )
@@ -42,6 +44,12 @@ func init() {
 func (detector *OsReleaseNamespaceDetector) Detect(data map[string][]byte) *database.Namespace {
 	var OS, version string
 
+	for _, filePath := range detector.getExcludeFiles() {
+		if _, hasFile := data[filePath]; hasFile {
+			return nil
+		}
+	}
+
 	for _, filePath := range detector.GetRequiredFiles() {
 		f, hasFile := data[filePath]
 		if !hasFile {
@@ -74,3 +82,8 @@ func (detector *OsReleaseNamespaceDetector) Detect(data map[string][]byte) *data
 func (detector *OsReleaseNamespaceDetector) GetRequiredFiles() []string {
 	return []string{"etc/os-release", "usr/lib/os-release"}
 }
+
+// getExcludeFiles returns the list of files that are ought to exclude this detector from Detect()
+func (detector *OsReleaseNamespaceDetector) getExcludeFiles() []string {
+	return []string{"etc/redhat-release", "usr/lib/centos-release"}
+}

worker/detectors/namespace/redhatrelease/redhatrelease.go

@@ -20,9 +20,15 @@ import (
 
 	"github.com/coreos/clair/database"
 	"github.com/coreos/clair/worker/detectors"
+	"github.com/coreos/pkg/capnslog"
 )
 
-var redhatReleaseRegexp = regexp.MustCompile(`(?P<os>[^\s]*) (Linux release|release) (?P<version>[\d]+)`)
+var (
+	log = capnslog.NewPackageLogger("github.com/coreos/clair", "worker/detectors/namespace/redhatrelease")
+
+	centosReleaseRegexp = regexp.MustCompile(`(?P<os>[^\s]*) (Linux release|release) (?P<version>[\d]+)`)
+	redhatReleaseRegexp = regexp.MustCompile(`(?P<os>Red Hat Enterprise Linux) (Client release|Server release|Workstation release) (?P<version>[\d]+)`)
+)
 
 // RedhatReleaseNamespaceDetector implements NamespaceDetector and detects the OS from the
 // /etc/centos-release, /etc/redhat-release and /etc/system-release files.
@@ -31,6 +37,7 @@ var redhatReleaseRegexp = regexp.MustCompile(`(?P<os>[^\s]*) (Linux release|rele
 // eg. CentOS release 5.11 (Final)
 // eg. CentOS release 6.6 (Final)
 // eg. CentOS Linux release 7.1.1503 (Core)
+// eg. Red Hat Enterprise Linux Server release 7.2 (Maipo)
 type RedhatReleaseNamespaceDetector struct{}
 
 func init() {
@@ -44,10 +51,21 @@ func (detector *RedhatReleaseNamespaceDetector) Detect(data map[string][]byte) *
 			continue
 		}
 
-		r := redhatReleaseRegexp.FindStringSubmatch(string(f))
+		var r []string
+
+		// try for RHEL
+		r = redhatReleaseRegexp.FindStringSubmatch(string(f))
+		if len(r) == 4 {
+			// TODO(vbatts) this is a hack until https://github.com/coreos/clair/pull/193
+			return &database.Namespace{Name: "centos" + ":" + r[3]}
+		}
+
+		// then try centos first
+		r = centosReleaseRegexp.FindStringSubmatch(string(f))
 		if len(r) == 4 {
 			return &database.Namespace{Name: strings.ToLower(r[1]) + ":" + r[3]}
 		}
+
 	}
 
 	return nil