Skip to content

Conversation

@shizhMSFT
Copy link
Contributor

@shizhMSFT shizhMSFT commented Mar 17, 2025

Important

This PR implements an RFC draft, which is "work in progress".

This PR implements the RFC draft COSE Hash Envelope (draft-05).

@codecov
Copy link

codecov bot commented Mar 17, 2025

Codecov Report

Attention: Patch coverage is 93.18182% with 9 lines in your changes missing coverage. Please review.

Project coverage is 91.37%. Comparing base (cfe4231) to head (87052d1).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
hash_envelope.go 91.08% 6 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #212      +/-   ##
==========================================
+ Coverage   91.27%   91.37%   +0.09%     
==========================================
  Files          12       13       +1     
  Lines        2006     2133     +127     
==========================================
+ Hits         1831     1949     +118     
- Misses        119      125       +6     
- Partials       56       59       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@shizhMSFT shizhMSFT requested a review from Copilot March 17, 2025 09:20
Copy link

Copilot AI left a 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]>
Signed-off-by: Shiwei Zhang <[email protected]>
Signed-off-by: Shiwei Zhang <[email protected]>
Signed-off-by: Shiwei Zhang <[email protected]>
@shizhMSFT shizhMSFT requested a review from Copilot March 18, 2025 05:14
Copy link

Copilot AI left a 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.

Copy link

Copilot AI left a 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)

@SteveLasker
Copy link
Contributor

SteveLasker commented Mar 18, 2025

Thanks, @shizhMSFT
We pushed version 04 to DataTracker which adds the pre-allocation COSE headers, so you're not dependent on a github PR:
https://www.ietf.org/archive/id/draft-ietf-cose-hash-envelope-04.html#name-iana-considerations

We'll update the iana reference as well

@shizhMSFT
Copy link
Contributor Author

@SteveLasker I've updated the docs in the code to be draft-04.

Copy link
Member

@qmuntal qmuntal left a comment

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: Shiwei Zhang <[email protected]>
Copy link
Member

@qmuntal qmuntal left a comment

Choose a reason for hiding this comment

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

Still LGTM.

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.

LGTM

@SteveLasker
Copy link
Contributor

@thomas-fossati, any additional input from your perspective, including any IETF draft thoughts?
With the latest version, we'll be requesting WGLC.

Copy link

@JeyJeyGao JeyJeyGao left a comment

Choose a reason for hiding this comment

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

LGTM, but IANAM

@thomas-fossati
Copy link
Contributor

thomas-fossati commented Mar 31, 2025

@thomas-fossati, any additional input from your perspective, including any IETF draft thoughts? With the latest version, we'll be requesting WGLC.

hi @SteveLasker, thanks for flagging this. I thought 258 and 259 were already taken by COSE TSA-TST: see my exchange with Francesca, IANA DE for the registry, for more context. I didn’t see the early allocation request for hash envelopes otherwise, I’d have raised my hand :-) Is there a way to rectify this?

@SteveLasker
Copy link
Contributor

@thomas-fossati

I didn’t see the early allocation request for hash envelopes otherwise, I’d have raised my hand :-) Is there a way to rectify this?

Looking at the thread, it looks like it was a discussion as codepoints in flight however, these have been assigned and published:
https://www.iana.org/assignments/cose/cose.xhtml#header-parameters

This just affirms the process of pre-allocation, as I was on the receiving side of "assumed" codepoints that we had to change.

@thomas-fossati
Copy link
Contributor

@thomas-fossati

I didn’t see the early allocation request for hash envelopes otherwise, I’d have raised my hand :-) Is there a way to rectify this?

Looking at the thread, it looks like it was a discussion as codepoints in flight however, these have been assigned and published: https://www.iana.org/assignments/cose/cose.xhtml#header-parameters

This just affirms the process of pre-allocation, as I was on the receiving side of "assumed" codepoints that we had to change.

Yeah. I guess @henkbirkholz, who’s on both docs, could have spotted the clash. As I said, I didn’t see the early allocation request on the COSE list or I could've chimed in. The implication is that TSA-TST we’ll have to modify all the examples. The document is scheduled for one of the upcoming IESG telechats (early May, IIRC). So, I think we’ll do the tweaks with the RFC editor, after IANA has given us the new numbers 😅

@SteveLasker
Copy link
Contributor

@thomas-fossati are you ok with the rest of the PR? If so, can you LGTM?

@thomas-fossati
Copy link
Contributor

@thomas-fossati are you ok with the rest of the PR? If so, can you LGTM?

Yes, the implementation looks very good and has excellent testing.

Copy link
Contributor

@thomas-fossati thomas-fossati left a comment

Choose a reason for hiding this comment

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

🚢 it!

@SteveLasker SteveLasker merged commit a633822 into veraison:main Mar 31, 2025
4 checks passed
@shizhMSFT shizhMSFT deleted the hash_envelope branch April 8, 2025 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants