-
Notifications
You must be signed in to change notification settings - Fork 42
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
Add GetApplicableTrustPolicy function #51
Conversation
Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comment, we should use terminology matching the spec, otherwise it gets confusing sometimes.
- trustPolicy instead of policyStatement
- (signature) verificationLevel vs verificationPreset
Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>
Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>
Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>
verification/policy.go
Outdated
} else if wildcardPolicy != nil { | ||
return wildcardPolicy, nil | ||
} else { | ||
return nil, fmt.Errorf("registry scope %q has no applicable trust policy", artifactPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, fmt.Errorf("registry scope %q has no applicable trust policy", artifactPath) | |
return nil, fmt.Errorf("artifact %q has no applicable trust policy", artifactUri) |
verification/helpers.go
Outdated
// TODO support more types of URI like "domain.com/repository", "domain.com/repository:tag", "domain.com/repository@sha256:digest" | ||
i := strings.LastIndex(artifactUri, ":") | ||
if i < 0 { | ||
return "", fmt.Errorf("registry URI %q could not be parsed, make sure it is the fully qualified registry URI without the scheme/protocol. e.g domain.com:80/my/repository:digest", artifactUri) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return "", fmt.Errorf("registry URI %q could not be parsed, make sure it is the fully qualified registry URI without the scheme/protocol. e.g domain.com:80/my/repository:digest", artifactUri) | |
return "", fmt.Errorf("artifact URI %q could not be parsed, make sure it is the fully qualified OCI artifact URI without the scheme/protocol. e.g domain.com:80/my/repository:digest", artifactUri) |
Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>
verification/policy.go
Outdated
} else if wildcardPolicy != nil { | ||
return wildcardPolicy, nil | ||
} else { | ||
return nil, fmt.Errorf("artifact %q has no applicable trust policy", artifactPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, I'd prefer artifactUri
in the error message which is the user provided input, artifactPath
is calculated in our internal logic.
return nil, fmt.Errorf("artifact %q has no applicable trust policy", artifactPath) | |
return nil, fmt.Errorf("artifact %q has no applicable trust policy", artifactUri) |
Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>
Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>
// Constants | ||
supportedPolicyVersions := []string{"1.0"} | ||
supportedVerificationPresets := []string{"strict", "permissive", "audit", "skip"} | ||
supportedVerificationLevels := []string{"strict", "permissive", "audit", "skip"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you going to replaces this with levels define in #55 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I have a TODO to do that.
} else if isPresent(artifactPath, policyStatement.RegistryScopes) { | ||
applicablePolicy = policyStatement.deepCopy() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qq: Where are we validating that there is only one applicable trust policy fir a given repository?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@shizhMSFT merging this PR with two approvals. |
Followed the rules from https://github.com/notaryproject/notaryproject/blob/main/trust-store-trust-policy-specification.md#selecting-a-trust-policy-based-on-artifact-uri
Signed-off-by: rgnote 5878554+rgnote@users.noreply.github.com