veraison / go-cose

go library for CBOR Object Signing and Encryption (COSE)
Mozilla Public License 2.0
49 stars 26 forks source link

Add support to `COSE_Countersignature` (RFC 9338) #172

Closed balena closed 11 months ago

balena commented 11 months ago

Added support to countersignatures:

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

codecov[bot] commented 11 months ago

Codecov Report

:exclamation: 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%> (ø)

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

OR13 commented 11 months ago

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

balena commented 11 months ago

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 commented 11 months ago

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.

OR13 commented 11 months ago

Good suggestions, they should be applied imo.

thomas-fossati commented 11 months ago

Good suggestions, they should be applied imo.

+1

thomas-fossati commented 11 months ago

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.

balena commented 11 months ago

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.

thomas-fossati commented 11 months ago

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

balena commented 11 months ago

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

thomas-fossati commented 11 months ago

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

SteveLasker commented 11 months ago

@balena thank you for your patience and all the followup