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

Add support to COSE_Countersignature (RFC 9338) #172

Merged
merged 4 commits into from Sep 30, 2023

Conversation

balena
Copy link
Contributor

@balena balena commented Sep 11, 2023

Added support to countersignatures:

  • Added type Countersignature as an alias to Signature, and modified Sign and Verify functions to obtain ToBeSigned value as specified in RFC 9338;
  • Headers now marshal/unmarshal headers HeaderLabelCounterSignature or HeaderLabelCounterSignatureV2 as Countersignature or []Countersignature;
  • Added support to CounterSignature0 and CounterSignature0V2 using functions Countersign0 and VerifyCountersign0.

Whenever Sign and Verify are called, the parameter parent captures the parent structure being countersigned. It supports Sign1Message, SignMessage, Signature and Countersignature (itself).

@balena balena changed the title Added support to countersignatures Added support to countersignatures (RFC 9338) Sep 11, 2023
@balena balena changed the title Added support to countersignatures (RFC 9338) Add support to `COSE_Countersignatures (RFC 9338) Sep 12, 2023
@balena balena changed the title Add support to `COSE_Countersignatures (RFC 9338) Add support to COSE_Countersignature (RFC 9338) Sep 12, 2023
@balena balena force-pushed the rfc9338-countersignatures branch 2 times, most recently from e661db7 to 95e52e1 Compare September 18, 2023 01:24
Signed-off-by: Guilherme Balena Versiani <guibv@mailbox.org>
Copy link
Collaborator

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

overall this looks good to me.

I would prefer to see the test vectors imported from files (ideally ones that are shared cross implementations).

@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@da0f9a6). Click here to learn what that means.
Report is 1 commits behind head on main.
The diff coverage is 78.80%.

@@           Coverage Diff           @@
##             main     #172   +/-   ##
=======================================
  Coverage        ?   91.99%           
=======================================
  Files           ?       12           
  Lines           ?     1961           
  Branches        ?        0           
=======================================
  Hits            ?     1804           
  Misses          ?      108           
  Partials        ?       49           
Files Coverage Δ
headers.go 92.69% <93.93%> (ø)
countersign.go 73.36% <73.36%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

headers_test.go Show resolved Hide resolved
@OR13
Copy link
Collaborator

OR13 commented Sep 22, 2023

Perhaps we can submit fixture files back to the COSE WG based on this implementation, is case no fixtures exist yet for countersignatures.

@balena
Copy link
Contributor Author

balena commented Sep 22, 2023

Yes, I had to manually edit these vectors also to include references to the mocked algorithm that allows the execution of the unit tests. These may not be available at IETF test vectors anyway.

A second aspect to consider is that the examples available at the COSE WG at GitHub are outdated. They still refer to RFC 8152, whereas this code is based on RFC 9338 which supersedes it.

@SteveLasker
Copy link
Contributor

Thanks @balena for the great work and explanation and @OR13 for the review.
We'd like an additional set of 👀 from @shizhMSFT, @thomas-fossati and/or @qmuntal (others welcome as well) before merging.

Copy link
Contributor

@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.

The implementation looks solid and well tested. Could you add some usage examples to example_test.go and some minimal documentation into README.md? Thanks.

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 with suggestions

countersign.go Outdated Show resolved Hide resolved
countersign.go Outdated Show resolved Hide resolved
headers.go Outdated Show resolved Hide resolved
@OR13
Copy link
Collaborator

OR13 commented Sep 26, 2023

Good suggestions, they should be applied imo.

@thomas-fossati
Copy link
Contributor

Good suggestions, they should be applied imo.

+1

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.

Apologies for being late to the party...

First, this is really an excellent contribution, thanks a lot @balena !

I agree with the spirit of Orie's comment about pulling the test vectors from some shared source (gluecose being another good candidate), but I think that can be added at a subsequent point in time.

Same as for Quim's comment about improving documentation around this new feature. As long as we track it and make it nextrel-blocking I am fine.

Re: Shiwei's suggestions. Please take the time to apply them, they are sensible and simple :-)

@thomas-fossati
Copy link
Contributor

thomas-fossati commented Sep 26, 2023

Yes, I had to manually edit these vectors also to include references to the mocked algorithm that allows the execution of the unit tests. These may not be available at IETF test vectors anyway.

A second aspect to consider is that the examples available at the COSE WG at GitHub are outdated. They still refer to RFC 8152, whereas this code is based on RFC 9338 which supersedes it.

A while ago we started https://github.com/gluecose/test-vectors, which go-cose uses for the Sign1 conformance tests suite. That may be the place to add these too.

* Syntax sugar while passing parents as references at `Sign` and `Verify` functions;
* Added countersignatures usage examples to `example_test.go`;
* Applied code improvements as suggested by @shizhMSFT.

Signed-off-by: Guilherme Balena Versiani <guibv@mailbox.org>
@balena
Copy link
Contributor Author

balena commented Sep 28, 2023

The implementation looks solid and well tested. Could you add some usage examples to example_test.go and some minimal documentation into README.md? Thanks.

Did that, let me know if it's OK now.

@balena balena requested a review from qmuntal September 28, 2023 02:36
@thomas-fossati
Copy link
Contributor

@balena can you amend 7fa55c8 to add the missing signoff, please?

example_test.go Outdated Show resolved Hide resolved
Signed-off-by: Guilherme Balena Versiani <guibv@mailbox.org>
* Returning either reference to countersignatures or slice of countersignature references at `UnmarshalCBOR` header functions;
* Adjusted unit tests accordingly.

Signed-off-by: Guilherme Balena Versiani <guibv@mailbox.org>
@balena balena requested a review from qmuntal September 28, 2023 10:41
@balena
Copy link
Contributor Author

balena commented Sep 30, 2023

@qmuntal @thomas-fossati @OR13 @shizhMSFT please let me know if there is anything else I can help with.

@thomas-fossati
Copy link
Contributor

@qmuntal @thomas-fossati @OR13 @shizhMSFT please let me know if there is anything else I can help with.

Again, thank you for an awesome job. FMPOV this PR can be merged now.

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

@SteveLasker
Copy link
Contributor

@balena thank you for your patience and all the followup

@SteveLasker SteveLasker merged commit e7ac36d into veraison:main Sep 30, 2023
4 checks passed
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.

None yet

6 participants