-
Notifications
You must be signed in to change notification settings - Fork 28
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
feat: cose hash envelope #212
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #212 +/- ##
==========================================
+ Coverage 91.27% 91.38% +0.10%
==========================================
Files 12 13 +1
Lines 2006 2135 +129
==========================================
+ Hits 1831 1951 +120
- Misses 119 125 +6
- Partials 56 59 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull Request Overview
This PR implements the experimental COSE Hash Envelope feature as defined in the RFC draft, including new APIs for signing and verifying hash envelopes, new header parameters, and supporting changes to algorithm handling.
- Added an example demonstrating signing and verification of hash envelopes in example_test.go.
- Introduced SignHashEnvelope and VerifyHashEnvelope functions along with helper functions in hash_envelope.go.
- Updated header handling and algorithm constants in headers.go and algorithm.go, and adjusted example naming in cwt_test.go.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
example_test.go | Added a new example for COSE Hash Envelope signing and verification. |
hash_envelope.go | Introduced new functions and header settings for hash envelope support. |
headers.go | Added new header setters/getters for hash envelope parameters. |
algorithm.go | Updated algorithm constants and hash function support for SHA variants. |
cwt_test.go | Renamed an example function to reflect the new API usage. |
Comments suppressed due to low confidence (3)
cwt_test.go:13
- [nitpick] The example function name 'Example_cWTMessage' includes an underscore and does not follow Go naming conventions. Consider renaming it to 'ExampleCWTMessage'.
func Example_cWTMessage() {
example_test.go:490
- [nitpick] The example function name 'Example_hashEnvelope' uses underscores which are unconventional in Go example naming. Consider renaming it to 'ExampleHashEnvelope' for consistency.
func Example_hashEnvelope() {
hash_envelope.go:123
- The use of 'maps.Clone' requires Go 1.21 or later. Please ensure that the project documentation specifies the minimum supported Go version.
header := maps.Clone(base)
Signed-off-by: Shiwei Zhang <[email protected]>
db2bd89
to
f6f0572
Compare
Signed-off-by: Shiwei Zhang <[email protected]>
Signed-off-by: Shiwei Zhang <[email protected]>
Signed-off-by: Shiwei Zhang <[email protected]>
Signed-off-by: Shiwei Zhang <[email protected]>
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Signed-off-by: Shiwei Zhang <[email protected]>
Signed-off-by: Shiwei Zhang <[email protected]>
Signed-off-by: Shiwei Zhang <[email protected]>
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.
Pull Request Overview
This PR implements the experimental COSE Hash Envelope API, following the draft specification, and includes new signing and verification functions along with supporting tests and updated header and algorithm handling.
- Updated error messages for algorithm handling in headers.
- Added tests for payload hash algorithm conversion across various numeric types.
- Introduced examples and new constants for SHA-based algorithms.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
headers_test.go | Updated tests for error messages and payload hash algorithm tests. |
example_test.go | Added an example demonstrating signing and verifying a hash envelope. |
hash_envelope.go | Implemented signing and verifying functions for the hash envelope. |
headers.go | Updated header constants and error handling in algorithm extraction. |
algorithm_test.go | Extended tests to cover new SHA algorithms and hash function behaviors. |
algorithm.go | Added SHA algorithm support and adjusted constant definitions. |
Comments suppressed due to low confidence (1)
headers.go:163
- [nitpick] Consider aligning the error messaging for unsupported algorithm types between the Algorithm() and PayloadHashAlgorithm() functions to maintain a consistent behavior across header extraction methods.
return AlgorithmReserved, fmt.Errorf("Algorithm(%q): %w", alg, ErrAlgorithmNotSupported)
Thanks, @shizhMSFT We'll update the iana reference as well |
Signed-off-by: Shiwei Zhang <[email protected]>
@SteveLasker I've updated the docs in the code to be draft-04. |
return errors.New("protected header parameter: payload hash alg: require int type") | ||
} | ||
foundPayloadHashAlgorithm = true | ||
case HeaderLabelPayloadPreimageContentType: |
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.
What happens if PreimageContentType
exists in both the protected and unprotected headers? Is it a valid case? If it is valid, what happens if they have different values? This scenario is not defined in the draft.
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.
Thanks @JeyJeyGao,
This is a good question that I've opened cose-wg/draft-ietf-cose-hash-envelope#49
For future reference, you can always ask the question to the associated IETF discussion group. In this case:
[email protected] with a subject that contains: draft-ietf-cose-hash-envelope
The mail archive can be viewed here: https://mailarchive.ietf.org/arch/browse/cose?q=draft-ietf-cose-hash-envelope
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.
I've double checked the RFC 9052 section 3, it states
Applications SHOULD verify that the same label does not occur in both the protected and unprotected header parameters. If the message is not rejected as malformed, attributes MUST be obtained from the protected bucket, and only if an attribute is not found in the protected bucket can that attribute be obtained from the unprotected bucket.
In other words, the same label can exist in both headers although it is not suggested. It really depends on the application. Therefore, I would keep the current implementation and leave the discussion to the hash envelope draft.
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.
While the end result of above is the same, I'd suggest holding to minimize churn while we sort out cose-wg/draft-ietf-cose-hash-envelope#50
We're all here in Bangkok this week for IETF 122, so I'm sure we can solve this quickly.
This shouldn't be left ambiguous.
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.
I'd limit this library to simple validation of int / tstr for the labels, and leave use of these parameters to applications that build on this library.
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.
We've updated the hash-envelope draft to clarify the protected/unprotected headers: https://datatracker.ietf.org/doc/draft-ietf-cose-hash-envelope/05/
Signed-off-by: Shiwei Zhang <[email protected]>
Important
This PR implements an RFC draft, which is "work in progress".
This PR implements the RFC draft COSE Hash Envelope (draft-04).