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

Basic Signature Verification #79

Merged
merged 12 commits into from
Aug 2, 2022
Merged

Conversation

rgnote
Copy link
Contributor

@rgnote rgnote commented Jul 9, 2022

  1. Perform signature verification
  2. Support signingAuthority trust store as spec'd in https://github.com/notaryproject/notaryproject/pull/165/files

Signed-off-by: rgnote 5878554+rgnote@users.noreply.github.com

Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>
@rgnote rgnote changed the title verifier changes Basic Signature Verification Jul 9, 2022
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>
@gokarnm gokarnm requested a review from a team July 11, 2022 20:11
Copy link

@gokarnm gokarnm left a 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 !! 🚀

verification/helpers.go Show resolved Hide resolved
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) {
Copy link

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.

Copy link
Contributor Author

@rgnote rgnote Jul 11, 2022

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.

Copy link

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.

verification/verifier.go Outdated Show resolved Hide resolved
verification/verifier.go Outdated Show resolved Hide resolved
verification/verifier.go Outdated Show resolved Hide resolved
verification/verifier.go Outdated Show resolved Hide resolved
verification/policy.go Outdated Show resolved Hide resolved
verification/verifier_helpers.go Show resolved Hide resolved
verification/verifier_helpers.go Show resolved Hide resolved
Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>
@gokarnm
Copy link

gokarnm commented Jul 11, 2022

Pending second review and related spec approval.

Copy link
Contributor

@SteveLasker SteveLasker left a 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.

verification/verifier.go Outdated Show resolved Hide resolved
Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>
Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>
@iamsamirzon iamsamirzon added this to the RC-1 milestone Jul 14, 2022
Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>
internal/mock/mocks.go Outdated Show resolved Hide resolved
verification/helpers.go Show resolved Hide resolved
verification/policy.go Outdated Show resolved Hide resolved
verification/store.go Outdated Show resolved Hide resolved
verification/verifier.go Show resolved Hide resolved
verification/verifier.go Outdated Show resolved Hide resolved
verification/verifier.go Show resolved Hide resolved
verification/verifier_helpers.go Outdated Show resolved Hide resolved
verification/verifier_helpers.go Show resolved Hide resolved
@dtzar dtzar modified the milestones: RC-1, alpha-3 Jul 28, 2022
Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>
@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@3d22fbc). Click here to learn what that means.
The diff coverage is n/a.

@@           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>
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +98 to +101
} else {
// if X509 authenticity passes, then perform Trusted Identity based authenticity
return v.verifyTrustedIdentities(trustPolicy, outcome)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
} 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)

Copy link
Contributor Author

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

Comment on lines +112 to +118
} else {
return &VerificationResult{
Success: true,
Type: Expiry,
Action: outcome.VerificationLevel.VerificationMap[Expiry],
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
} else {
return &VerificationResult{
Success: true,
Type: Expiry,
Action: outcome.VerificationLevel.VerificationMap[Expiry],
}
}
}
return &VerificationResult{
Success: true,
Type: Expiry,
Action: outcome.VerificationLevel.VerificationMap[Expiry],
}

Comment on lines +131 to +137
} else {
return &VerificationResult{
Success: true,
Type: Authenticity,
Action: outcome.VerificationLevel.VerificationMap[Authenticity],
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
} else {
return &VerificationResult{
Success: true,
Type: Authenticity,
Action: outcome.VerificationLevel.VerificationMap[Authenticity],
}
}
}
return &VerificationResult{
Success: true,
Type: Authenticity,
Action: outcome.VerificationLevel.VerificationMap[Authenticity],
}

verification/verifier_helpers.go Show resolved Hide resolved
Copy link

@gokarnm gokarnm left a 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

@gokarnm gokarnm merged commit 6312370 into notaryproject:main Aug 2, 2022
priteshbandi pushed a commit to priteshbandi/notation-go that referenced this pull request Aug 9, 2022
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants