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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 7 commits into from

Conversation

OR13
Copy link
Collaborator

@OR13 OR13 commented Jan 26, 2024 •
edited

@codecov Codecov
Copy link

codecov bot commented Jan 26, 2024 •
edited

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 c7cee92. Consider uploading reports for the commit c7cee92 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
OR13
OR13 commented Jan 26, 2024
View reviewed changes
cwt_test.go Outdated Show resolved Hide resolved
OR13
OR13 commented Jan 26, 2024
View reviewed changes
headers.go Outdated Show resolved Hide resolved
OR13
OR13 commented Jan 26, 2024
View reviewed changes
cwt.go
CWTClaimConfirmation int64 = 8
CWTClaimScope int64 = 9

// TODO: the rest upon request
Copy link
Collaborator 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!

@OR13 OR13 mentioned this pull request Jan 26, 2024
Open
OR13
OR13 commented Jan 26, 2024
View reviewed changes
cwt_test.go Outdated Show resolved Hide resolved
OR13
OR13 commented Jan 26, 2024
View reviewed changes
cwt_test.go
fmt.Println("message verified")

// tamper the message and verification should fail
msgToVerify.Payload = []byte("foobar")
Copy link
Collaborator 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

shizhMSFT
shizhMSFT reviewed Feb 26, 2024
View reviewed changes
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 •
edited

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

@SteveLasker SteveLasker mentioned this pull request Mar 8, 2024
Open
@SteveLasker SteveLasker added the blocked blocked for action: see notes which should explain the status label Mar 8, 2024
@OR13
Copy link
Collaborator Author

OR13 commented Mar 8, 2024

related issue #184

@SteveLasker
Copy link
Contributor

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

@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
henkbirkholz
henkbirkholz approved these changes Apr 5, 2024
View reviewed changes
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.

correctly reflects:
https://datatracker.ietf.org/doc/draft-ietf-cose-cwt-claims-in-headers/10/
which will move with:
https://datatracker.ietf.org/doc/draft-ietf-cose-typ-header-parameter/05/

SteveLasker
SteveLasker requested changes Apr 5, 2024
View reviewed changes
Copy link
Contributor

@SteveLasker SteveLasker left a comment •
edited

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
Collaborator Author

OR13 commented Apr 7, 2024 •
edited

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 and others added 7 commits April 7, 2024 13:36
@OR13
add initial test
bc4610a 
Signed-off-by: Orie Steele <orie@transmute.industries>
@OR13
fix test
25991e9 
Signed-off-by: Orie Steele <orie@transmute.industries>
@OR13
add basic test for cwt
203b417 
Signed-off-by: Orie Steele <orie@transmute.industries>
@yogeshbdeshpande @OR13
Update cwt_test.go
adc1599 
Co-authored-by: Orie Steele <orie@or13.io>
Signed-off-by: Orie Steele <orie@transmute.industries>
@OR13
Update cwt_test.go
a379d94 
Signed-off-by: Orie Steele <orie@transmute.industries>
@SteveLasker @OR13
Update Readme for latest release (veraison#182)
1a0f2e1 
Signed-off-by: Orie Steele <orie@transmute.industries>
@OR13
add typ and validation
c7cee92 
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
Collaborator 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shizhMSFT shizhMSFT shizhMSFT left review comments

@SteveLasker SteveLasker SteveLasker requested changes

@henkbirkholz henkbirkholz henkbirkholz approved these changes

@qmuntal qmuntal Awaiting requested review from qmuntal qmuntal is a code owner

@roywill roywill Awaiting requested review from roywill roywill is a code owner

@setrofim setrofim Awaiting requested review from setrofim setrofim is a code owner

@SimonFrost-Arm SimonFrost-Arm Awaiting requested review from SimonFrost-Arm SimonFrost-Arm is a code owner

@thomas-fossati thomas-fossati Awaiting requested review from thomas-fossati thomas-fossati is a code owner

@yogeshbdeshpande yogeshbdeshpande Awaiting requested review from yogeshbdeshpande yogeshbdeshpande is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@OR13 OR13

Labels
blocked blocked for action: see notes which should explain the status
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for typ in protected header Support for CWT Claims in Headers
5 participants
@OR13 @SteveLasker @henkbirkholz @yogeshbdeshpande @shizhMSFT