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

Add a CryptoProvider interface and NodeCryptoProvider implementation. #1237

Merged
merged 1 commit into from
Sep 13, 2021

Conversation

dcr-stripe
Copy link
Contributor

@dcr-stripe dcr-stripe commented Sep 10, 2021

Notify

r? @richardm-stripe

Summary

Adds an CryptoProvider interface and moves out the Node crypto logic into a NodeCryptoProvider implementation.

This initially only supports computing HMAC signatures, but lets us support this in a pluggable way. The Webhooks functions are updated to support passing in a custom provider. This can be used in environments where no 'crypto' package is available (removing another Node dependency for #997).

Test plan

Added a small common set of test cases which can be re-used by any implementation to ensure they meet the contract.

Looking forwards

Some crypto APIs (eg. the Web Crypto API) only provide async functions for computing HMACs (and other primitives). In the future, we may want to extend this interface with an asyncComputeHMACSignature function that returns a promise alongside computeHMACSignature. We would need to update the Webhooks code to support both sync and async computations (eg. providing asyncConstructEvent and asyncVerifyHeader).

@dcr-stripe dcr-stripe changed the title [WIP] Add a CryptoProvider interface and NodeCryptoProvider implementation. Add a CryptoProvider interface and NodeCryptoProvider implementation. Sep 10, 2021
@@ -104,7 +99,8 @@ const signature = {
});
}

const expectedSignature = this._computeSignature(
cryptoProvider = cryptoProvider || getNodeCryptoProvider();
const expectedSignature = cryptoProvider.computeHMACSignature(
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be evil, but for async support:

- const expectedSignature = cryptoProvider.computeHMACSignature(
-    ...
- );
- // Do a bunch of stuff with expectedSignature
+ const compare = (expectedSignature) => {
+   // Do a bunch of stuff with expectedSignature
+ };
+ const expectedSignature = cryptoProvider.computeHMACSignature(
+   ...
+ );
+ if (expectedSignature && typeof expectedSignature.then === 'function') {
+   return expectedSignature.then(compare);
+ } else {
+   return compare(expectedSignature);
+ }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. I think this could potentially simplify the implementation over having two separate methods... let's think about it and see how the need comes up? Either change would require updating the types though I imagine changing constructEvent's return type from Stripe.Event to Stripe.Event | Promise<Stripe.Event> wouldn't technically be backwards compatible?

Think we can merge this and handle async in a separate PR.

Copy link
Contributor

@richardm-stripe richardm-stripe 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 to me!

@dcr-stripe dcr-stripe merged commit a5b4f77 into master Sep 13, 2021
@dcr-stripe dcr-stripe deleted the dcr-crypto-interface branch September 13, 2021 16:17
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