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

Handling COSE EC keys encoded without leading 0 bytes in coordinates. #64

Conversation

waltercacau
Copy link
Contributor

@waltercacau waltercacau commented Jul 10, 2024

Should address #63

Added a unit test to cover it and ran them locally to verify it was working:

% bundle exec rspec spec

Randomized with seed 61233
.......................................................................................

Finished in 0.3211 seconds (files took 0.12736 seconds to load)
87 examples, 0 failures

Randomized with seed 61233

@waltercacau waltercacau force-pushed the fix-ec-keys-with-omitted-leading-zeros branch from da14320 to 00f45e9 Compare August 1, 2024 12:36
@santiagorodriguez96
Copy link
Contributor

Hi @waltercacau! Thank you so much for the report and also taking the time to propose a fix to it!

The changes look good to me, although I wonder what make you go for a different approach that the one taken in ruby-jwt? Would it be possible for a coordinate to have more than one leading zero and thus both coordinates differing in more than one byte?

@waltercacau
Copy link
Contributor Author

waltercacau commented Aug 10, 2024

Hi @waltercacau! Thank you so much for the report and also taking the time to propose a fix to it!

The changes look good to me, although I wonder what make you go for a different approach that the one taken in ruby-jwt? Would it be possible for a coordinate to have more than one leading zero and thus both coordinates differing in more than one byte?

I am not expert on it by any means but my understanding is that it is possible though might be unlikely.

The approach I took here was inspired by the approach done in this java library:
https://github.com/felx/nimbus-jose-jwt/blob/47d66f2775c392964788aa6389a46fac84f976cd/src/main/java/com/nimbusds/jose/jwk/ECKey.java#L595-L612

That code seemed to handle these cases more generally.

Copy link
Contributor

@santiagorodriguez96 santiagorodriguez96 left a comment

Choose a reason for hiding this comment

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

Makes sense! I think it's okay to go with this approach 😄

Just one minor comment before merging 🙂

spec/cose/key/ec2_spec.rb Outdated Show resolved Hide resolved
@waltercacau
Copy link
Contributor Author

Alright, deleted the extra test/debug code I had and reran the tests:

% bundle exec rspec spec

Randomized with seed 4620
......................................................................................

Finished in 0.21978 seconds (files took 0.3266 seconds to load)
86 examples, 0 failures

I think this PR is now good to go

Copy link
Contributor

@santiagorodriguez96 santiagorodriguez96 left a comment

Choose a reason for hiding this comment

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

Great work! Thank you so much!!

@santiagorodriguez96 santiagorodriguez96 merged commit b997db1 into cedarcode:master Aug 12, 2024
@waltercacau waltercacau deleted the fix-ec-keys-with-omitted-leading-zeros branch August 12, 2024 14:12
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.

2 participants