Skip to content

Conversation

@pranjalkole
Copy link
Contributor

Before merging, we need to change corim/testcases/signed-good-corim.cbor to include the kid parameter.

Copy link
Contributor

@setrofim setrofim left a comment

Choose a reason for hiding this comment

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

we need to change corim/testcases/signed-good-corim.cbor to include the kid parameter.

The test cases should be amended as part of the same commit commit that changes the code to make it necessary. (Doing this requires first updaing https://github.com/veraison/gen-testcases to include the key ID on signing).

@pranjalkole pranjalkole force-pushed the signedcorim branch 3 times, most recently from 1e94de3 to b16dda1 Compare January 20, 2025 18:52
* Added the kid parameter to the failing tests
* Added getKidFromJWK in corim/signer.go
* Regenerated all testcases
* Made regen-from-src.sh executable

Signed-off-by: Pranjal Kole <[email protected]>

o.message.Headers.Protected.SetAlgorithm(alg)
o.message.Headers.Protected[cose.HeaderLabelContentType] = ContentType
o.message.Headers.Protected[cose.HeaderLabelKeyID] = kid
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 we ensure that kid != nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kid is supposed to never be nil. I was thinking we should store kid as part of the cose.Signer while creating it from the JWK (like we do for alg). getKidFromJWK can get a kid from every JWK.

I'll wait for the meeting on kid being mandatory before making any more changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thomas-fossati Please ping me after the final decision on whether kid should be mandatory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants