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 for CWT Claims & Type in Protected Headers #183

Closed
wants to merge 18 commits into from

Conversation

OR13
Copy link
Contributor

@OR13 OR13 commented Jan 26, 2024

Copy link

codecov bot commented Jan 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

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

@OR13 OR13 force-pushed the feat/cwt-claims-in-headers branch from 83f2079 to bd3d70d Compare January 26, 2024 17:25
cwt_test.go Outdated Show resolved Hide resolved
headers.go Outdated Show resolved Hide resolved
CWTClaimConfirmation int64 = 8
CWTClaimScope int64 = 9

// TODO: the rest upon request
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it is smart to add the whole registry in the first PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the above, it looks like we have the minimal code needed to proceed.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should add what is bare minimum required, and then extend it when a new point needs to be registered!

Copy link
Contributor

@thomas-fossati thomas-fossati Jun 21, 2024

Choose a reason for hiding this comment

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

[...] and then extend it when a new point needs to be registered!

Sorry, I have probably missed the right conversations, but I am not sure I get what the strategy is. There are 10's of claims already registered. What is the criterion for promoting them from the CWT Claims registry to consts in the cose package?

I think we should explicitly document how to add new claims here.

cwt_test.go Outdated Show resolved Hide resolved
fmt.Println("message verified")

// tamper the message and verification should fail
msgToVerify.Payload = []byte("foobar")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for this test, it might be better to tamper with the protected header.

@SteveLasker
Copy link
Contributor

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

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.

Does CWT deserve its own package package cwt under package cose?

@SteveLasker
Copy link
Contributor

SteveLasker commented Mar 8, 2024

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

@SteveLasker SteveLasker added the blocked blocked for action: see notes which should explain the status label Mar 8, 2024
@OR13
Copy link
Contributor Author

OR13 commented Mar 8, 2024

related issue #184

@SteveLasker
Copy link
Contributor

@SteveLasker
Copy link
Contributor

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

@OR13 OR13 force-pushed the feat/cwt-claims-in-headers branch from e23425d to 0906ae5 Compare April 5, 2024 15:12
Copy link
Member

@henkbirkholz henkbirkholz left a comment

Choose a reason for hiding this comment

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

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.

Approve of the changes, however we decided to wait for https://datatracker.ietf.org/doc/draft-ietf-cose-cwt-claims-in-headers/ to be completed, with a comment added referencing the RFC #.

@OR13, please add the RFC # as part of this pending PR/Issue as well: #184

@OR13
Copy link
Contributor Author

OR13 commented Apr 7, 2024

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"}

SteveLasker and others added 2 commits April 7, 2024 13:36
Signed-off-by: Orie Steele <orie@transmute.industries>
Signed-off-by: Orie Steele <orie@transmute.industries>
@OR13 OR13 force-pushed the feat/cwt-claims-in-headers branch from 5ee4b28 to c7cee92 Compare April 7, 2024 18:36
@OR13
Copy link
Contributor Author

OR13 commented Apr 7, 2024

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.

@OR13 OR13 changed the title Add support for CWT Claims in Protected Headers Add support for CWT Claims & Type in Protected Headers Apr 7, 2024
@SteveLasker
Copy link
Contributor

Sooooooo, close.

cwt.go Outdated Show resolved Hide resolved
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.

marvellous, thanks!

I have left a few (mostly silly) comments for your consideration.

CWTClaimConfirmation int64 = 8
CWTClaimScope int64 = 9

// TODO: the rest upon request
Copy link
Contributor

@thomas-fossati thomas-fossati Jun 21, 2024

Choose a reason for hiding this comment

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

[...] and then extend it when a new point needs to be registered!

Sorry, I have probably missed the right conversations, but I am not sure I get what the strategy is. There are 10's of claims already registered. What is the criterion for promoting them from the CWT Claims registry to consts in the cose package?

I think we should explicitly document how to add new claims here.

cwt_test.go Outdated Show resolved Hide resolved
cwt_test.go Show resolved Hide resolved
headers.go Show resolved Hide resolved
cwt_test.go Outdated Show resolved Hide resolved
OR13 and others added 9 commits June 21, 2024 10:46
Signed-off-by: Orie Steele <orie@transmute.industries>
Signed-off-by: steve lasker <stevenlasker@hotmail.com>
Signed-off-by: Orie Steele <orie@transmute.industries>
Signed-off-by: steve lasker <stevenlasker@hotmail.com>
Signed-off-by: Orie Steele <orie@transmute.industries>
Signed-off-by: steve lasker <stevenlasker@hotmail.com>
Co-authored-by: Orie Steele <orie@or13.io>
Signed-off-by: Orie Steele <orie@transmute.industries>
Signed-off-by: steve lasker <stevenlasker@hotmail.com>
Signed-off-by: Orie Steele <orie@transmute.industries>
Signed-off-by: steve lasker <stevenlasker@hotmail.com>
Signed-off-by: Orie Steele <orie@transmute.industries>
Signed-off-by: steve lasker <stevenlasker@hotmail.com>
Signed-off-by: Orie Steele <orie@transmute.industries>
Signed-off-by: steve lasker <stevenlasker@hotmail.com>
Co-authored-by: Thomas Fossati <tho.ietf@gmail.com>
Signed-off-by: steve lasker <stevenlasker@hotmail.com>
Co-authored-by: Thomas Fossati <tho.ietf@gmail.com>
Signed-off-by: steve lasker <stevenlasker@hotmail.com>
@SteveLasker SteveLasker force-pushed the feat/cwt-claims-in-headers branch from dd69daa to c9f2ab1 Compare June 21, 2024 17:47
Co-authored-by: Thomas Fossati <tho.ietf@gmail.com>
@OR13
Copy link
Contributor Author

OR13 commented Jun 21, 2024

@thomas-fossati regarding #183 (comment)

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
Copy link
Contributor Author

OR13 commented Jun 21, 2024

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
Copy link
Contributor

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.

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
Copy link
Contributor

@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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked blocked for action: see notes which should explain the status
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for typ in protected header Support for CWT Claims in Headers
6 participants