Repository: deislabs/ratify
Pull request #712 (2023-03-14 17:42:53)

fix: switch reference normalization to use docker parsing
# Description

## What this PR does / why we need it:
New testing revealed that the subject reference parsing logic was not comprehensive and didn't account for non `library` docker.io repositories. Upon further investigation, I found that Ratify is using the wrong reference parsing method from the upstream distribution repo. We should rely entirely on upstream's normalization methods to normalize subject refs into fully qualified references (adding `latest` if tag/digest is not present, use docker.io/library or docker.io/<repo path> when domain is not specified, etc.) This PR updates the reference parsing method to use the upstream normalization function. It also updates the tests to add more coverage.

## Type of change

Please delete options that are not relevant.

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] This change requires a documentation update

# How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

- [ ] Unit Tests pass
- [ ] e2e tests pass

# Checklist:

- [ ] Does the affected code have corresponding tests?
- [ ] Are the changes documented, not just with inline documentation, but also with conceptual documentation such as an overview of a new feature, or task-based documentation like a tutorial? Consider if this change should be announced on your project blog.
- [ ] Does this introduce breaking changes that would require an announcement or bumping the major version?
- [ ] Do all new files have appropriate license header?


akashsinghal: One other finding is that this affects the request cache that was implemented at the http server handler level. Currently the cache key for that is the raw subject ref passed in from the External Data request. As a result, if someone specifies `hello-world` for one workload and `docker.io/library/hello-world` for another workload, they would be 2 different cache keys; however they resolve to the same normalized docker ref --> `docker.io/library/hello-world:latest`
codecov[bot]: ## [Codecov](https://codecov.io/gh/deislabs/ratify/pull/712?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report
Patch coverage: **`81.48`**% and project coverage change: **`-0.11`** :warning:
> Comparison is base [(`5a90911`)](https://codecov.io/gh/deislabs/ratify/commit/5a90911e5e2eca99d1cfc0abe87383c3eef4edc5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) 49.17% compared to head [(`6680af9`)](https://codecov.io/gh/deislabs/ratify/pull/712?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) 49.06%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #712      +/-   ##
==========================================
- Coverage   49.17%   49.06%   -0.11%     
==========================================
  Files          57       57              
  Lines        3278     3275       -3     
==========================================
- Hits         1612     1607       -5     
- Misses       1492     1493       +1     
- Partials      174      175       +1     
```


| [Impacted Files](https://codecov.io/gh/deislabs/ratify/pull/712?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | |
|---|---|---|
| [httpserver/handlers.go](https://codecov.io/gh/deislabs/ratify/pull/712?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-aHR0cHNlcnZlci9oYW5kbGVycy5nbw==) | `77.77% <79.16%> (-2.35%)` | :arrow_down: |
| [pkg/utils/utils.go](https://codecov.io/gh/deislabs/ratify/pull/712?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cGtnL3V0aWxzL3V0aWxzLmdv) | `100.00% <100.00%> (+10.00%)` | :arrow_up: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

</details>

[:umbrella: View full report in Codecov by Sentry](https://codecov.io/gh/deislabs/ratify/pull/712?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).


Patch:
diff --git a/httpserver/handlers.go b/httpserver/handlers.go
index 1758743..5a813b2 100644
--- a/httpserver/handlers.go
+++ b/httpserver/handlers.go
@@ -64,43 +64,48 @@ func (server *Server) verify(ctx context.Context, w http.ResponseWriter, r *http
 		go func(subject string) {
 			defer wg.Done()
 			routineStartTime := time.Now()
-
-			unlock := server.keyMutex.Lock(subject)
+			returnItem := externaldata.Item{
+				Key: subject,
+			}
+			defer func() {
+				mu.Lock()
+				results = append(results, returnItem)
+				mu.Unlock()
+			}()
+			subjectReference, err := pkgUtils.ParseSubjectReference(subject)
+			if err != nil {
+				returnItem.Error = err.Error()
+				return
+			}
+			resolvedSubjectReference := subjectReference.Original
+			unlock := server.keyMutex.Lock(resolvedSubjectReference)
 			defer unlock()
 
-			logrus.Infof("verifying subject %v", subject)
+			logrus.Infof("verifying subject %v", resolvedSubjectReference)
 			var result types.VerifyResult
-			res := server.cache.get(subject)
+			res := server.cache.get(resolvedSubjectReference)
 			if res != nil {
-				logrus.Debugf("cache hit for subject %v", subject)
+				logrus.Debugf("cache hit for subject %v", resolvedSubjectReference)
 				result = *res
 			} else {
-				logrus.Debugf("cache miss for subject %v", subject)
-				returnItem := externaldata.Item{
-					Key: subject,
-				}
-
+				logrus.Debugf("cache miss for subject %v", resolvedSubjectReference)
 				verifyParameters := executor.VerifyParameters{
-					Subject: subject,
+					Subject: resolvedSubjectReference,
 				}
 
 				if result, err = server.GetExecutor().VerifySubject(ctx, verifyParameters); err != nil {
 					returnItem.Error = err.Error()
+					return
 				}
-				server.cache.set(subject, &result)
+				server.cache.set(resolvedSubjectReference, &result)
 
 				if res, err := json.MarshalIndent(result, "", "  "); err == nil {
 					fmt.Println(string(res))
 				}
 			}
 
-			mu.Lock()
-			defer mu.Unlock()
-			results = append(results, externaldata.Item{
-				Key:   subject,
-				Value: fromVerifyResult(result),
-			})
-			logrus.Debugf("verification: execution time for image %s: %dms", subject, time.Since(routineStartTime).Milliseconds())
+			returnItem.Value = fromVerifyResult(result)
+			logrus.Debugf("verification: execution time for image %s: %dms", resolvedSubjectReference, time.Since(routineStartTime).Milliseconds())
 		}(utils.SanitizeString(subject))
 	}
 	wg.Wait()
diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go
index d7fdb34..daae900 100644
--- a/pkg/utils/utils.go
+++ b/pkg/utils/utils.go
@@ -26,8 +26,6 @@ import (
 	"github.com/opencontainers/go-digest"
 )
 
-const defaultRegistryPrefix = "docker.io/library/"
-
 // ParseDigest parses the given string and returns a validated Digest object.
 func ParseDigest(digestStr string) (digest.Digest, error) {
 	digest, err := digest.Parse(digestStr)
@@ -40,22 +38,12 @@ func ParseDigest(digestStr string) (digest.Digest, error) {
 
 // ParseSubjectReference parses the given subject and returns a valid reference
 func ParseSubjectReference(subRef string) (common.Reference, error) {
-	parseResult, err := reference.Parse(subRef)
+	parseResult, err := reference.ParseDockerRef(subRef)
 	if err != nil {
 		return common.Reference{}, fmt.Errorf("failed to parse subject reference: %w", err)
 	}
 
 	var subjectRef common.Reference
-	if named, ok := parseResult.(reference.Named); ok {
-		subjectRef.Path = named.Name()
-		if reference.Domain(named) == "" {
-			subRef = fmt.Sprint(defaultRegistryPrefix, subRef)
-			subjectRef.Path = fmt.Sprint(defaultRegistryPrefix, subjectRef.Path)
-		}
-	} else {
-		return common.Reference{}, fmt.Errorf("failed to parse subject reference Path")
-	}
-
 	if digested, ok := parseResult.(reference.Digested); ok {
 		subjectRef.Digest = digested.Digest()
 	}
@@ -63,8 +51,8 @@ func ParseSubjectReference(subRef string) (common.Reference, error) {
 	if tag, ok := parseResult.(reference.Tagged); ok {
 		subjectRef.Tag = tag.Tag()
 	}
-
-	subjectRef.Original = subRef
+	subjectRef.Original = parseResult.String()
+	subjectRef.Path = parseResult.Name()
 
 	return subjectRef, nil
 }


Test patch:
diff --git a/httpserver/server_test.go b/httpserver/server_test.go
index d3ed1e5..dd3cc2b 100644
--- a/httpserver/server_test.go
+++ b/httpserver/server_test.go
@@ -28,6 +28,8 @@ import (
 	"github.com/deislabs/ratify/pkg/executor/core"
 	"github.com/deislabs/ratify/pkg/ocispecs"
 	config "github.com/deislabs/ratify/pkg/policyprovider/configpolicy"
+	"github.com/docker/distribution/reference"
+	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 
 	"github.com/deislabs/ratify/pkg/policyprovider/types"
@@ -335,3 +337,61 @@ func TestServer_MultipleRequestsForSameSubject_Success(t *testing.T) {
 		}
 	})
 }
+
+// TestServer_MultipleSubjects_Success tests multiple subjects are verified concurrently
+func TestServer_Verify_ParseReference_Failure(t *testing.T) {
+	testImageNames := []string{"&&"}
+	t.Run("server_verify_parsereference_failure", func(t *testing.T) {
+		body := new(bytes.Buffer)
+
+		if err := json.NewEncoder(body).Encode(externaldata.NewProviderRequest(testImageNames)); err != nil {
+			t.Fatalf("failed to encode request body: %v", err)
+		}
+		request := httptest.NewRequest(http.MethodPost, "/ratify/gatekeeper/v1/verify", bytes.NewReader(body.Bytes()))
+		logrus.Infof("policies successfully created. %s", body.Bytes())
+
+		responseRecorder := httptest.NewRecorder()
+
+		ex := &core.Executor{
+			PolicyEnforcer: config.PolicyEnforcer{},
+			ReferrerStores: []referrerstore.ReferrerStore{&mocks.TestStore{}},
+			Verifiers:      []verifier.ReferenceVerifier{&core.TestVerifier{}},
+			Config: &exconfig.ExecutorConfig{
+				VerificationRequestTimeout: nil,
+				MutationRequestTimeout:     nil,
+			},
+		}
+
+		getExecutor := func() *core.Executor {
+			return ex
+		}
+
+		server := &Server{
+			GetExecutor: getExecutor,
+			Context:     request.Context(),
+
+			keyMutex: keyMutex{},
+			cache:    newSimpleCache(DefaultCacheTTL, DefaultCacheMaxSize),
+		}
+
+		handler := contextHandler{
+			context: server.Context,
+			handler: processTimeout(server.verify, server.GetExecutor().GetVerifyRequestTimeout(), false),
+		}
+
+		handler.ServeHTTP(responseRecorder, request)
+		var respBody externaldata.ProviderResponse
+		if err := json.NewDecoder(responseRecorder.Result().Body).Decode(&respBody); err != nil {
+			t.Fatalf("failed to decode response body: %v", err)
+		}
+		retFirstKey := respBody.Response.Items[0].Key
+		retFirstErr := respBody.Response.Items[0].Error
+		if retFirstKey != testImageNames[0] {
+			t.Fatalf("Expected first subject response to be %s but got %s", testImageNames[0], retFirstKey)
+		}
+		expectedErr := errors.Wrap(reference.ErrReferenceInvalidFormat, "failed to parse subject reference")
+		if retFirstErr != expectedErr.Error() {
+			t.Fatalf("Expected first subject error to be %s but got %s", expectedErr.Error(), retFirstErr)
+		}
+	})
+}
diff --git a/pkg/executor/core/executor_test.go b/pkg/executor/core/executor_test.go
index d277388..f9a8485 100644
--- a/pkg/executor/core/executor_test.go
+++ b/pkg/executor/core/executor_test.go
@@ -92,7 +92,7 @@ func TestVerifySubject_Verify_NoReferrers(t *testing.T) {
 	}
 
 	verifyParameters := e.VerifyParameters{
-		Subject: "localhost:5000/net-monitor:v1@sha256:a0fc570a245b09ed752c42d600ee3bb5b4f77bbd70d8898780b7ab43454530eb",
+		Subject: "localhost:5000/net-monitor:v1",
 	}
 
 	if _, err := ex.verifySubjectInternal(context.Background(), verifyParameters); !errors.Is(err, ErrReferrersNotFound) {
@@ -137,7 +137,7 @@ func TestVerifySubject_CanVerify_ExpectedResults(t *testing.T) {
 	}
 
 	verifyParameters := e.VerifyParameters{
-		Subject: "localhost:5000/net-monitor:v1@sha256:a0fc570a245b09ed752c42d600ee3bb5b4f77bbd70d8898780b7ab43454530eb",
+		Subject: "localhost:5000/net-monitor:v1",
 	}
 
 	result, err := ex.verifySubjectInternal(context.Background(), verifyParameters)
@@ -193,7 +193,7 @@ func TestVerifySubject_VerifyFailures_ExpectedResults(t *testing.T) {
 	}
 
 	verifyParameters := e.VerifyParameters{
-		Subject: "localhost:5000/net-monitor:v1@sha256:a0fc570a245b09ed752c42d600ee3bb5b4f77bbd70d8898780b7ab43454530eb",
+		Subject: "localhost:5000/net-monitor:v1",
 	}
 
 	result, err := ex.verifySubjectInternal(context.Background(), verifyParameters)
@@ -245,7 +245,7 @@ func TestVerifySubject_VerifySuccess_ExpectedResults(t *testing.T) {
 	}
 
 	verifyParameters := e.VerifyParameters{
-		Subject: "localhost:5000/net-monitor:v1@sha256:a0fc570a245b09ed752c42d600ee3bb5b4f77bbd70d8898780b7ab43454530eb",
+		Subject: "localhost:5000/net-monitor:v1",
 	}
 
 	result, err := ex.verifySubjectInternal(context.Background(), verifyParameters)
@@ -305,7 +305,7 @@ func TestVerifySubject_MultipleArtifacts_ExpectedResults(t *testing.T) {
 	}
 
 	verifyParameters := e.VerifyParameters{
-		Subject: "localhost:5000/net-monitor:v1@sha256:a0fc570a245b09ed752c42d600ee3bb5b4f77bbd70d8898780b7ab43454530eb",
+		Subject: "localhost:5000/net-monitor:v1",
 	}
 
 	result, err := ex.verifySubjectInternal(context.Background(), verifyParameters)
diff --git a/pkg/utils/utils_test.go b/pkg/utils/utils_test.go
index 06e2f27..1bd985e 100644
--- a/pkg/utils/utils_test.go
+++ b/pkg/utils/utils_test.go
@@ -52,7 +52,6 @@ func TestParseSubjectReference_ReturnsExpected(t *testing.T) {
 			input: "localhost:5000/net-monitor:v1@sha256:a0fc570a245b09ed752c42d600ee3bb5b4f77bbd70d8898780b7ab43454530eb",
 			output: common.Reference{
 				Path:   "localhost:5000/net-monitor",
-				Tag:    "v1",
 				Digest: getDigest("sha256:a0fc570a245b09ed752c42d600ee3bb5b4f77bbd70d8898780b7ab43454530eb"),
 			},
 		},
@@ -82,6 +81,20 @@ func TestParseSubjectReference_ReturnsExpected(t *testing.T) {
 			output:         common.Reference{},
 			expectedErrMsg: "failed to parse subject reference",
 		},
+		{
+			input: "hello-world",
+			output: common.Reference{
+				Path: "docker.io/library/hello-world",
+				Tag:  "latest",
+			},
+		},
+		{
+			input: "prom/prometheus",
+			output: common.Reference{
+				Path: "docker.io/prom/prometheus",
+				Tag:  "latest",
+			},
+		},
 	}
 
 	for _, testcase := range testcases {
@@ -93,10 +106,6 @@ func TestParseSubjectReference_ReturnsExpected(t *testing.T) {
 				t.Fatalf("parsing subject reference expected to fail with err %v actual %v", testcase.expectedErrMsg, err)
 			}
 		} else {
-			if !testcase.isDefaultTest && actual.Original != testcase.input {
-				t.Fatalf("parsing subject reference failed expected original %v actual %v", testcase.input, actual.Original)
-			}
-
 			if actual.Path != testcase.output.Path {
 				t.Fatalf("parsing subject reference failed expected path %v actual %v", testcase.output.Path, actual.Path)
 			}