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

Signatures 13: Problem with test-key-rsa-pss in Browser JS #2290

Closed
bblfish opened this issue Nov 4, 2022 · 5 comments · Fixed by #2301
Closed

Signatures 13: Problem with test-key-rsa-pss in Browser JS #2290

bblfish opened this issue Nov 4, 2022 · 5 comments · Fixed by #2301

Comments

@bblfish
Copy link

bblfish commented Nov 4, 2022

I am having difficulty getting the test-key-rsa-pss private key being accepted by the web browsers.

This is a bit of a weird problem that would benefit from help from some JS crypto experts. But it is not a new problem: we had a similar problem a year ago in draft 07, which was the last time I worked on an implementation of the spec. See #1867

I am developing a library called bobcats in Scala that compiles both to JS and to Java and runs the same tests. I have been testing bobcats by developing tests take from the Message Signing spec.

Using the same data, the Java tests pass, but the JS tests don't when using the new keys placed in object test-key-rsa-pss extends TestKeyPair.

When that code is commented out, and instead we use the test-key-rsa-pss from draft 07, as shown in the code above, then all the tests pass.

So there is something in the given PEM keys that the JS Web Crypto APIs don't like. I am sure it would help implementations in JS browser land if it was not so difficult to get the PEM key working. There must be some explanation to what the problem is.

Browsers tested are Chrome and Firefox using Selenium. See for example Chrome log in PR bblfish/bobcats#7

@bblfish bblfish changed the title Signatures 13: Problem with in test-key-rsa-pss in Browser JS Signatures 13: Problem with test-key-rsa-pss in Browser JS Nov 4, 2022
bblfish added a commit to bblfish/bobcats that referenced this issue Nov 5, 2022
@bblfish
Copy link
Author

bblfish commented Nov 5, 2022

I tried to just use the private pk8 key from draft 07 with the new public key in commit bblfish/bobcats@f5c5ef3 but that failed the tests. Going back to fully using the private and public key from draft 07, with commit bblfish/bobcats@8f3e75f
and all the tests are passing again https://github.com/bblfish/bobcats/actions/runs/3399167351/jobs/5652745295

@bblfish
Copy link
Author

bblfish commented Nov 6, 2022

The scala bobcats library referenced above has the advantage that as it compiles to JS and Java bytecode that it can help find inconsistencies as we have in this bug report. The disadvantage is that there are a few layers between the code and the result in JS. So I worked on a pure JS import-key.zip example, adapted a little from MDN dom examples so that it can parse multiple PEMs and remove all the white spaces so that comparison with the keys in the spec are more easy to make.

  1. The first key '0' is the one from the MDN example,
  2. the second '1' is the one that I got to work in v07 and that correctly signs the examples from v13 spec,
  3. the third one is taken directly from the v013 spec. This one throws an exception.

@bblfish
Copy link
Author

bblfish commented Nov 6, 2022

In response to my question on the webcrypto repository @panva gave some very valuable feedback:

The keys in appendix-B.1.1 are in PKCS1, which isn't accepted by webcrypto at all. Recommend using rsaEncryption OID PKCS8 and SPKI PEM or JWK if they ought to be imported as CryptoKey reliably.

The private key in appendix-B.1.2 is 1.2.840.113549.1.1.10 (id-RSASSA-PSS). WebCryptoAPI implementations only generally accept 1.2.840.113549.1.1.1 (rsaEncryption) keys. Recommend using rsaEncryption OID PKCS8 PEM or JWK if they ought to be imported as CryptoKey reliably.

The private key in appendix-B.1.3 is in SEC1 format, which isn't accepted by webcrypto at all. Recommend using id-ecPublicKey OID PKCS8 PEM or JWK if they ought to be imported as CryptoKey reliably.

The keys in appendix-B.1.4 are fine but currently only Node.js and Deno runtimes implement Ed25519 as per Secure Curves in the Web Cryptography API.

Hope this helps inform the WG. I would propose to keep the PEM keys as is and add their JWK representation.

@bblfish
Copy link
Author

bblfish commented Nov 6, 2022

I updated the bobcats cryptography library for Scala and ScalaJS to verify

  1. that the keys in the spec can be loaded in JS and Java
  2. that they can sign the text and verify the signatures

The PR 7 now passes all tests.

But as explained by @panva in the message I quoted above, the tests pass but not because the keys were in the right format. What happened is that I was able to get the keys to work correctly early this year somehow (I can't quite remember how) by converting them and trying out various options, and finally publishing those that did work. So a trial and error process.

The PEM files that work are listed in HttpMessageSignaturesV07. The updated versions that are used by scalaJS are those named by the functions privatePk8Key and publicPk8Key. Where those are specified it is because the key in the spec 07 were not in the right format.

As is visible from the HttpMessageSignaturesV13.scala all the keys just link to the ones from the 07 spec.

So using those keys I think would cover all of Java and the WebCrypto API.

@jricher
Copy link
Contributor

jricher commented Nov 7, 2022

All the keys listed in the test vectors section are used by the test generation script at : https://github.com/bspk/sig-example-scripts/blob/main/http_sig_examples.py

The key formats in the draft are generated using openssl, as is mentioned at the head of the section. PEM formats like this seem to be not universally supported across different libraries, and even the underlying Python code that runs the examples does need to unwrap the PKCS8 values for some of these, but not others. Adding in the JWK equivalents of these keys is not a bad idea, per @panva 's suggestions.

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

Successfully merging a pull request may close this issue.

2 participants