Add support to `COSE_Countersignature` (RFC 9338) by balena · Pull Request #172 · veraison/go-cose

balena · 2023-09-11T21:22:14Z

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

Signed-off-by: Guilherme Balena Versiani <guibv@mailbox.org>

OR13

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 · 2023-09-22T15:38:10Z

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

OR13 · 2023-09-22T15:41:08Z

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

balena · 2023-09-22T20:32:46Z

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 · 2023-09-22T23:05:10Z

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.

qmuntal

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.

shizhMSFT

LGTM with suggestions

countersign.go

headers.go

OR13 · 2023-09-26T13:05:27Z

Good suggestions, they should be applied imo.

thomas-fossati · 2023-09-26T14:30:56Z

Good suggestions, they should be applied imo.

+1

thomas-fossati

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 · 2023-09-26T14:41:29Z

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.

@shizhMSFT

* 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 · 2023-09-28T02:36:29Z

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 · 2023-09-28T09:28:32Z

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

example_test.go

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 · 2023-09-30T03:27:35Z

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

thomas-fossati · 2023-09-30T03:40:38Z

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

shizhMSFT

LGTM

SteveLasker · 2023-09-30T17:31:54Z

@balena thank you for your patience and all the followup

balena force-pushed the rfc9338-countersignatures branch from f1a1e41 to 61d8f4a Compare September 11, 2023 21:22

balena mentioned this pull request Sep 11, 2023

Generate counter signatures #170

Closed

balena changed the title ~~Added support to countersignatures~~ Added support to countersignatures (RFC 9338) Sep 11, 2023

balena changed the title ~~Added support to countersignatures (RFC 9338)~~ Add support to `COSE_Countersignatures (RFC 9338) Sep 12, 2023

balena changed the title ~~Add support to `COSE_Countersignatures (RFC 9338)~~ Add support to COSE_Countersignature (RFC 9338) Sep 12, 2023

balena force-pushed the rfc9338-countersignatures branch 2 times, most recently from e661db7 to 95e52e1 Compare September 18, 2023 01:24

Added support to countersignatures as specified by RFC 9338.

22faed2

Signed-off-by: Guilherme Balena Versiani <guibv@mailbox.org>

balena force-pushed the rfc9338-countersignatures branch from ac7310b to 22faed2 Compare September 18, 2023 02:54

SteveLasker requested review from OR13 and shizhMSFT September 22, 2023 14:55

OR13 approved these changes Sep 22, 2023

View reviewed changes

SteveLasker requested a review from qmuntal September 22, 2023 15:39

OR13 reviewed Sep 22, 2023

View reviewed changes

headers_test.go Show resolved Hide resolved

SteveLasker requested a review from thomas-fossati September 22, 2023 15:39

qmuntal requested changes Sep 26, 2023

View reviewed changes

shizhMSFT approved these changes Sep 26, 2023

View reviewed changes

countersign.go Outdated Show resolved Hide resolved

countersign.go Outdated Show resolved Hide resolved

shizhMSFT reviewed Sep 26, 2023

View reviewed changes

headers.go Outdated Show resolved Hide resolved

thomas-fossati approved these changes Sep 26, 2023

View reviewed changes

balena requested a review from qmuntal September 28, 2023 02:36

qmuntal reviewed Sep 28, 2023

View reviewed changes

example_test.go Outdated Show resolved Hide resolved

Added short comment.

b287c6a

Signed-off-by: Guilherme Balena Versiani <guibv@mailbox.org>

balena force-pushed the rfc9338-countersignatures branch from 7fa55c8 to b287c6a Compare September 28, 2023 10:26

balena requested a review from qmuntal September 28, 2023 10:41

qmuntal approved these changes Sep 28, 2023

View reviewed changes

shizhMSFT approved these changes Sep 30, 2023

View reviewed changes

SteveLasker merged commit e7ac36d into veraison:main Sep 30, 2023

Conversation

balena commented Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OR13 left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

OR13 commented Sep 22, 2023

Uh oh!

balena commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveLasker commented Sep 22, 2023

Uh oh!

qmuntal left a comment

Choose a reason for hiding this comment

Uh oh!

shizhMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

OR13 commented Sep 26, 2023

Uh oh!

thomas-fossati commented Sep 26, 2023

Uh oh!

thomas-fossati left a comment

Choose a reason for hiding this comment

Uh oh!

thomas-fossati commented Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

balena commented Sep 28, 2023

Uh oh!

thomas-fossati commented Sep 28, 2023

Uh oh!

Uh oh!

balena commented Sep 30, 2023

Uh oh!

thomas-fossati commented Sep 30, 2023

Uh oh!

shizhMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

SteveLasker commented Sep 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

balena commented Sep 11, 2023 •

edited

Loading

codecov bot commented Sep 22, 2023 •

edited

Loading

balena commented Sep 22, 2023 •

edited

Loading

thomas-fossati commented Sep 26, 2023 •

edited

Loading