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

feat(ext/crypto): implement importKey/exportKey formats (jwk, spki, pkcs8) for ECDSA/ECDH #13013

Closed
wants to merge 66 commits into from
Closed

feat(ext/crypto): implement importKey/exportKey formats (jwk, spki, pkcs8) for ECDSA/ECDH #13013

wants to merge 66 commits into from

Conversation

cryptographix
Copy link
Contributor

@cryptographix cryptographix commented Dec 7, 2021

towards #11690, implementing importKey(jwk, spki, pkcs8) and exportKey(jwk, spki, pkcs8) for ECDSA/ECDH*

Based on and requires refacs from PR#12022. When that lands, I'll rebase the PR.

Note: * Curve P-256 fully implemented, P-384 only partially with support for EC Public Keys only.

cryptographix and others added 30 commits September 12, 2021 22:36
@cryptographix cryptographix changed the title feat(ext/crypto): implement importKey/exportKey (jwk) for ECDSA/ECDH feat(ext/crypto): implement importKey/exportKey formats (jwk, spki, pkcs8) for ECDSA/ECDH Dec 7, 2021
@panva panva mentioned this pull request Dec 8, 2021
@oles
Copy link

oles commented Dec 8, 2021

I'll do some test with and without jose tomorrow and report my findings!

@oles
Copy link

oles commented Dec 9, 2021

Alright! I've built deno from your crypto-ec-import-export branch, @seanwykes, and tested with this code: https://gist.github.com/oles/7b1f4569805a7606056d0c867821d4c7

It no longer errors out with Uncaught (in promise) NotSupportedError: Unrecognized algorithm name, and the imported key looks like this:

CryptoKey {
  type: "private",
  extractable: false,
  algorithm: { name: "ECDSA", hash: undefined },
  usages: [ "sign" ]
}

Which seems promising, but it then errors out at the signing:

error: Uncaught (in promise) TypeError: CryptoKey does not support this operation, its algorithm.namedCurve must be P-256
  return new TypeError(`CryptoKey does not support this operation, its ${prop} must be ${name}`)
         ^
    at unusable (https://deno.land/x/[email protected]/lib/crypto_key.ts:4:10)
    at checkSigCryptoKey (https://deno.land/x/[email protected]/lib/crypto_key.ts:89:38)
    at getCryptoKey (https://deno.land/x/[email protected]/runtime/get_sign_verify_key.ts:8:5)
    at sign (https://deno.land/x/[email protected]/runtime/sign.ts:8:27)
    at FlattenedSign.sign (https://deno.land/x/[email protected]/jws/flattened/sign.ts:134:29)
    at CompactSign.sign (https://deno.land/x/[email protected]/jws/compact/sign.ts:47:39)
    at SignJWT.sign (https://deno.land/x/[email protected]/jwt/sign.ts:54:16)
    at file:///home/oles/projects/deno-test/jwt-test.js:40:8

So I changed the

const cryptoKey = await jose.importPKCS8(privateKey, 'ES256')

to

let cryptoKey = await jose.importPKCS8(privateKey, 'ES256')

cryptoKey.algorithm.namedCurve = 'P-256'

AND IT WORKED 🥳

@panva
Copy link
Contributor

panva commented Dec 9, 2021

You shouldn't have to do that. The webcrypto implementation should.

@oles
Copy link

oles commented Dec 9, 2021

You shouldn't have to do that. The webcrypto implementation should.

As I thought, but glad it could be set manually to verify the rest!

@panva
Copy link
Contributor

panva commented Dec 9, 2021

The key should look like this:

CryptoKey {
  type: 'private',
  extractable: false,
  algorithm: { name: 'ECDSA', namedCurve: 'P-256' },
  usages: [ 'sign' ]
}

no hash and a namedCurve.

@cryptographix
Copy link
Contributor Author

@oles, @panva - quick fix done.

Current jose status - FAILED. 106 passed; 40 failed; 0 ignored; 0 measured; 0 filtered out (3s)

@panva
Copy link
Contributor

panva commented Dec 9, 2021

Going in the right direction.

@cryptographix
Copy link
Contributor Author

@panva - I'm having trouble debugging the jose test cases, for example, test-deno/jwe_asymmetric.test.ts.
Using VSCode or cmd-line/Chrome Dev Tools, the tests execute before the debugger attaches.

Have you seen this behaviour before? I'm on a MAC/M1.

@bartlomieju
Copy link
Member

@panva - I'm having trouble debugging the jose test cases, for example, test-deno/jwe_asymmetric.test.ts. Using VSCode or cmd-line/Chrome Dev Tools, the tests execute before the debugger attaches.

Have you seen this behaviour before? I'm on a MAC/M1.

@seanwykes try running with --inspect-brk it will break on first line of code and you'll be able to set a breakpoint where you want.

@cryptographix
Copy link
Contributor Author

@panva - I'm having trouble debugging the jose test cases, for example, test-deno/jwe_asymmetric.test.ts. Using VSCode or cmd-line/Chrome Dev Tools, the tests execute before the debugger attaches.
Have you seen this behaviour before? I'm on a MAC/M1.

@seanwykes try running with --inspect-brk it will break on first line of code and you'll be able to set a breakpoint where you want.

Done that. it just runs the tests without stopping.

Debugger listening on ws://127.0.0.1:9229/ws/289230fb-d902-4f52-a2e7-6c81c1da6dd2
running 7 tests from file:///Users/sean/Projects/deno/jose/test-deno/jwe_asymmetric.test.ts
test Encrypt/Decrypt ECDH-ES crv: P-256 ... FAILED (57ms)
test Encrypt/Decrypt ECDH-ES crv: P-384 ... FAILED (10ms)
test (expecting failure) Encrypt/Decrypt ECDH-ES crv: P-521 ... ok (6ms)
test Encrypt/Decrypt RSA-OAEP-256 ... ok (92ms)
test Encrypt/Decrypt RSA-OAEP-384 ... ok (236ms)
test Encrypt/Decrypt RSA-OAEP-512 ... ok (195ms)
test Encrypt/Decrypt RSA-OAEP ... ok (189ms)```

@panva
Copy link
Contributor

panva commented Dec 9, 2021

It's not expected that this PR makes everything pass. But to answer your question, no i have no idea how to debug a deno runtime.

@bartlomieju
Copy link
Member

Done that. it just runs the tests without stopping.

Debugger listening on ws://127.0.0.1:9229/ws/289230fb-d902-4f52-a2e7-6c81c1da6dd2
running 7 tests from file:///Users/sean/Projects/deno/jose/test-deno/jwe_asymmetric.test.ts
test Encrypt/Decrypt ECDH-ES crv: P-256 ... FAILED (57ms)
test Encrypt/Decrypt ECDH-ES crv: P-384 ... FAILED (10ms)
test (expecting failure) Encrypt/Decrypt ECDH-ES crv: P-521 ... ok (6ms)
test Encrypt/Decrypt RSA-OAEP-256 ... ok (92ms)
test Encrypt/Decrypt RSA-OAEP-384 ... ok (236ms)
test Encrypt/Decrypt RSA-OAEP-512 ... ok (195ms)
test Encrypt/Decrypt RSA-OAEP ... ok (189ms)```

Right... This is actually #12584

@cryptographix
Copy link
Contributor Author

It's not expected that this PR makes everything pass. But to answer your question, no i have no idea how to debug a deno runtime.

I am interested in two fails ..
test Encrypt/Decrypt ECDH-ES crv: P-256 ... FAILED (57ms)
test Encrypt/Decrypt ECDH-ES crv: P-384 ... FAILED (10ms)

I'll run as isolated code and not tests to see what's up.

@panva
Copy link
Contributor

panva commented Dec 10, 2021

It's not expected that this PR makes everything pass. But to answer your question, no i have no idea how to debug a deno runtime.

I am interested in two fails .. test Encrypt/Decrypt ECDH-ES crv: P-256 ... FAILED (57ms) test Encrypt/Decrypt ECDH-ES crv: P-384 ... FAILED (10ms)

I'll run as isolated code and not tests to see what's up.

Ye, the only thing missing in those two tests was jwk export (on encrypt), and import (on decrypt) of the ECDH key in dist/deno/lib/encrypt_key_management.ts:50 and src/lib/decrypt_key_management.ts:48. The algorithm.namedCurve attribute that was missing above in some case is also needed.

@cryptographix
Copy link
Contributor Author

PR superseded by #13088 and #13104. Closing.

@cryptographix cryptographix deleted the crypto-ec-import-export branch January 5, 2022 22:34
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.

5 participants