Skip to content

Conversation

@igneel64
Copy link
Contributor

Additions

  • feat(backend-core): Add injectable loadCryptoKeyFunction
  • fix(clerk-sdk-node): Add jwksClient as the loadCryptoKey capability
  • fix(clerk-sdk-node): Clerk Base is part of the singleton
  • fix(clerk-sdk-node): Revisit middleware and instance tests
  • fix(clerk-sdk-node): deepmerge fetcher options

Notes

As discussed with @chanioxaris . We chose here to pass the public key as a CryptoKey from any external function like loadCryptoKey.
That is because there were a few issues with public key parsing etc.

As a next step or an alternative, we can use the loadCryptoKey as loadPublicKey and just return a public key as a string.

fix: Revisit middleware and instance tests, deepmerge fetcher options
@igneel64 igneel64 force-pushed the load_pk_injected_method branch from d1ea736 to 637b854 Compare January 13, 2022 14:15
"@peculiar/webcrypto": "^1.2.3",
"camelcase-keys": "^6.2.2",
"cookies": "^0.8.0",
"deepmerge": "^4.2.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was introduced as a fix for merging supplied httpOptions.

expect(allowlistIdentifiers2).toBe(allowlistIdentifiers);
});

test('clients getter returns a Client API instance', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests about instance types seemed superficial. If anyone has an objection we can restore them.

importKeyFunction: ImportKeyFunction;
verifySignatureFunction: VerifySignatureFunction;
decodeBase64Function: DecodeBase64Function;
loadCryptoKeyFunction?: LoadCryptoKeyFunction;
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ What's the difference between the loadCryptoKeyFunction and the importKeyFunction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Their docs:

   @param {ImportKeyFunction} importKeyFunction Function to import a PEM. Should have a similar result to crypto.subtle.importKey
   @param {LoadCryptoKeyFunction} loadCryptoKeyFunction Function load a PK CryptoKey from the host environment. Used for JWK clients etc.

Their difference is not so easily discernable for readers not familiar with the crypto operations we need to use for our jwt verification.

import is a reserved term coined as input a key in an external, portable format and take back a CryptoKey.

load does not have any special terminology like that and would allow injecting any kind of process that the client (of @clerk/backend-core) needs to do to provide a CryptoKey from his PK.

status: AuthStatus;
session?: Session;
interstitial?: string;
sessionClaims?: JWTPayload;
Copy link
Member

Choose a reason for hiding this comment

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

🔧 I will suggest to create a custom type for this, which will include the session ID and user ID for now and not depend on the JWT payload

'raw',
encoder.encode(
(
await jwksClient.getSigningKey(decoded.header.kid)
Copy link
Member

Choose a reason for hiding this comment

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

❓ What will happen if we can't find a key with the provided kid? Will it throw a descriptive error?

@igneel64 igneel64 force-pushed the load_pk_injected_method branch from b7d555d to 7699fa9 Compare January 18, 2022 09:10
@igneel64 igneel64 merged commit 54aaed3 into main Jan 18, 2022
@SokratisVidros SokratisVidros deleted the load_pk_injected_method branch February 3, 2022 12:53
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants