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

fix/ecdsa verification to use raw signature format per jwa spec #179

Merged
merged 1 commit into from
Jan 2, 2025

Conversation

ottokruse
Copy link
Contributor

Issue #, if available: N/A

Description of changes: Fixed ES256, ES384 and ES512 implementation: these should use raw signature format (r || s, as per JWA spec) but Node.js uses ASN.1 DER format by default. This can be switched to raw format by providing dsaEncoding: "ieee-p1363" which I added (thanks for the tip @NicolasViaud).

Also discovered that the browser implementation did not support the ECDSA formats at all yet, so added that.

And as icing on the cake, I finally fixed the browser test scrips so it doesn't orphan Vite processes anymore as it sometimes did! 🎉

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ottokruse ottokruse requested a review from hakanson December 27, 2024 11:15
@ottokruse ottokruse force-pushed the fix-ecdsa branch 3 times, most recently from 6aca094 to d1ce6e3 Compare December 30, 2024 07:58
Copy link
Contributor

@hakanson hakanson left a comment

Choose a reason for hiding this comment

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

Looks good - reviewed code, GitHub action output, and did local build/test.

Note: I have a local environmental issue that npm run test:browser fails with a different message using same Vite, Cypress, and Browser as CI.

VITE v5.4.11

  │ Cypress:        13.17.0
  │ Browser:        Electron 118 (headless)
  │ Node Version:   v18.20.2 (/usr/local/bin/node)
  1) click Verify RSA
       invalid JWKS Uri:

      Timed out retrying after 4000ms
      + expected - actual

      -'Error: Failed to fetch /notexample-JWKS.json: Content-type is "text/html", expected "application/json"'
      +'JWKS could not be parsed as JSON'
      
      at Context.eval (webpack://vite-app/./cypress/e2e/forminputs.cy.ts:63:0)

A local cd tests/vite-app/, npm run preview and npm run cypress:open:preview to test via Cypress UI worked fine - so not sure what changes could be made

@ottokruse
Copy link
Contributor Author

Thanks for reviewing!

Mmmm running that locally works for me. That message about the wrong content type is an old message, so there must be an old version of the lib somewhere locally for you.

You did run "npm run pack-for-tests" first right? And tried nuking all node_modules yet?

@hakanson
Copy link
Contributor

Got it working - removed both node_modules and dist under tests/vite-app the ran a npm run build there.

After that npm run test:browser had no errors.

@ottokruse ottokruse merged commit 46be74c into awslabs:main Jan 2, 2025
1 check passed
@ottokruse ottokruse deleted the fix-ecdsa branch January 2, 2025 07:23
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