-
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
Basic Signature Verification #79
Conversation
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>
Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>
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.
Nice work keeping the code modular and readable !! 🚀
Verify performs verification for each of the verification types supported in notation | ||
See https://github.com/notaryproject/notaryproject/blob/main/trust-store-trust-policy-specification.md#signature-verification | ||
*/ | ||
func (v *Verifier) Verify(ctx context.Context, artifactUri string) ([]*SignatureVerificationOutcome, error) { |
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.
Caller has to loop through the result array to find if there was a successful verification outcome, I'd prefer wrapping the array in a ArtifactVerificationOutcome which also provides you a top level successful outcome, and signed annotations.
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.
Callers don't have to loop through the array to figure out whether verification was successful or not. They just need to check whether the error is null or not. They need to look at the array only if they want to make use of the individual results.
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.
The CLI will always return signed annotations to caller for successful verification, so it will need to loop through this.
Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>
Pending second review and related spec approval. |
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.
This is great to start testing. I do think we should land Verification Plugin and Signing Scheme spec #165 before merging this.
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>
Codecov Report
@@ Coverage Diff @@
## main #79 +/- ##
=======================================
Coverage ? 68.67%
=======================================
Files ? 32
Lines ? 2008
Branches ? 0
=======================================
Hits ? 1379
Misses ? 511
Partials ? 118 📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>
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.
LGTM
} else { | ||
// if X509 authenticity passes, then perform Trusted Identity based authenticity | ||
return v.verifyTrustedIdentities(trustPolicy, outcome) | ||
} |
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:
} else { | |
// if X509 authenticity passes, then perform Trusted Identity based authenticity | |
return v.verifyTrustedIdentities(trustPolicy, outcome) | |
} | |
} | |
// if X509 authenticity passes, then perform Trusted Identity based authenticity | |
return v.verifyTrustedIdentities(trustPolicy, outcome) |
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.
Will address this and the other nits in my next PR
} else { | ||
return &VerificationResult{ | ||
Success: true, | ||
Type: Expiry, | ||
Action: outcome.VerificationLevel.VerificationMap[Expiry], | ||
} | ||
} |
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:
} else { | |
return &VerificationResult{ | |
Success: true, | |
Type: Expiry, | |
Action: outcome.VerificationLevel.VerificationMap[Expiry], | |
} | |
} | |
} | |
return &VerificationResult{ | |
Success: true, | |
Type: Expiry, | |
Action: outcome.VerificationLevel.VerificationMap[Expiry], | |
} |
} else { | ||
return &VerificationResult{ | ||
Success: true, | ||
Type: Authenticity, | ||
Action: outcome.VerificationLevel.VerificationMap[Authenticity], | ||
} | ||
} |
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:
} else { | |
return &VerificationResult{ | |
Success: true, | |
Type: Authenticity, | |
Action: outcome.VerificationLevel.VerificationMap[Authenticity], | |
} | |
} | |
} | |
return &VerificationResult{ | |
Success: true, | |
Type: Authenticity, | |
Action: outcome.VerificationLevel.VerificationMap[Authenticity], | |
} |
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 for recent changes
* verifier changes * fix expiry data formatting * use RFC1123Z for human readable time format * support signingAuthority trust store Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com> Signed-off-by: Pritesh Bandi <pritesb@amazon.com>
signingAuthority
trust store as spec'd in https://github.com/notaryproject/notaryproject/pull/165/filesSigned-off-by: rgnote 5878554+rgnote@users.noreply.github.com