Add support for CWT Claims & Type in Protected Headers

OR13 commented 7 months ago

closes #173

closes #184

This PR should remain open until numbers are assigned to:

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.20%. Comparing base (2b6f94f) to head (0906ae5). Report is 1 commits behind head on main.

:exclamation: Current head 0906ae5 differs from pull request most recent head 1980457

Please upload reports for the commit 1980457 to get more accurate results.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #183 +/- ## ========================================== + Coverage 92.04% 92.20% +0.16% ========================================== Files 12 12 Lines 1973 1976 +3 ========================================== + Hits 1816 1822 +6 + Misses 108 105 -3 Partials 49 49 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

SteveLasker commented 6 months ago

@qmuntal, @shizhMSFT, @thomas-fossati, @henkbirkholz can you please take a look, and provide some feedback to @OR13

SteveLasker commented 5 months ago

go-cose call feedback: consensus was to do it in this package.

OR13 commented 5 months ago

SteveLasker commented 5 months ago

blocked, awaiting: https://datatracker.ietf.org/doc/draft-ietf-cose-cwt-claims-in-headers/

SteveLasker commented 4 months ago

https://datatracker.ietf.org/doc/draft-ietf-cose-cwt-claims-in-headers/ is progressing is progressing. Expectation a few more weeks.

OR13 commented 4 months ago

I added support for the typ header parameter:

18([
h'a30138230fa2016e6973737565722e6578616d706c65026f7375626a6563742e6578616d706c65106f6170706c69636174696f6e2f637774', 
{4: h'31'}, 
h'68656c6c6f20776f726c64', h'00b8f0fbee7d5ca003678a173a1f1cb5f6233e85f2dd421d4d56d13541a67e01fd0c7addabc2c3b07d5b08a027382629aac41dd3f62f69d6c9209e2ca99255bda78600f3ca500d482c9b3a220dfd14395c42d02e0fa423e9192deb83c27b41257bcb3e9162db9e3de0895a81fb51d2ae41e9f07d91146832cbe91fa85b48456aef8ebc82'
])

Decoded protected header:

{1: -36, 15: {1: "issuer.example", 2: "subject.example"}, 16: "application/cwt"}

OR13 commented 4 months ago

I suppose there is an API consideration here, perhaps the validation checks should only happen when serialization occurs, instead of causing an error to be thrown when for example invalid claims or invalid cose type is set.

SteveLasker commented 2 months ago

Sooooooo, close.

OR13 commented 2 months ago

@thomas-fossati regarding https://github.com/veraison/go-cose/pull/183#discussion_r1649136571

Its a fair point.

My lazy answer is, people can add pull requests for the IANA registry entries they wish the library supported, and they can use the registries without adding "developer experience for them", even if we don't support them here.

We could decide to not add any registry entries from the CWT registry, and only support the registered headers.

OR13 commented 2 months ago

The combination of a cross fork PR, and the DCO rebase + github suggestion merges... has made this more trouble than its worth.

@thomas-fossati If you think we should drop the CWT side of this, I will probably just do a fresh PR that only defines the header parameters, and provide some examples that show how to use them, without defining any CWT registry specific stuff.

thomas-fossati commented 2 months ago

The combination of a cross fork PR, and the DCO rebase + github suggestion merges... has made this more trouble than its worth.

@thomas-fossati If you think we should drop the CWT side of this, I will probably just do a fresh PR that only defines the header parameters, and provide some examples that show how to use them, without defining any CWT registry specific stuff.

I believe this PR is good and worth adding into the mainline as-is.

Let's raise a couple of tracking issues (one for the registration, one for the potential refactoring) and let's get it merged.

Thanks again for the great contribution.

SteveLasker commented 1 month ago

@OR13, did you want to re-submit this as a new/clean PR, or can we merge as-is?

With #187 and #188 queued up, we're ready for a new release.

SteveLasker commented 1 month ago

Per discussion with @OR13, it was easier to close and redo this PR than fix the DCO issues.
Closing as this with replaced PR #189

veraison / go-cose

Add support for CWT Claims & Type in Protected Headers #183

Codecov Report