[core-crypto] initial core-crypto with sha256 hmac/hash functions #13232
Conversation
chradek
requested review from
bterlson,
mikeharder,
praveenkuttappan,
ramya-rao-a,
xirzec and
mitchdenny
as code owners
sdk/core/core-crypto/README.md
Outdated
|
|
||
| ## Key concepts | ||
|
|
||
| TODO: ??? |
There was a problem hiding this comment.
for now a list of supported protocols might be sufficient here
| @@ -0,0 +1,118 @@ | |||
| import path from "path"; | |||
There was a problem hiding this comment.
can you use the common rollup config that @witemple-msft set up inside of devtool? Look at Text Analytics for an example
There was a problem hiding this comment.
I updated the browser/nodejs builds to use the common rollup from devtool. I have a separate rollup config for creating the webworker script that runs in our tests.
| return subtleCrypto; | ||
| } | ||
|
|
||
| if (!self.crypto || !self.crypto.subtle) { |
There was a problem hiding this comment.
good using self here, we should maybe add a webworker test
There was a problem hiding this comment.
Webworker tests added!
| * @hidden | ||
| * @internal | ||
| */ | ||
| export function bufferToBase64(buffer: ArrayBuffer): string { |
There was a problem hiding this comment.
do we need to provide these helpers publicly too? I'm pretty sure there are some credential types out there that need base64 support
There was a problem hiding this comment.
Also, perhaps consider using this buffer library: https://www.npmjs.com/package/buffer
I am using it in attestation tests:
https://github.com/Azure/azure-sdk-for-js/blob/master/sdk/attestation/attestation/test/utils/Buffer.browser.ts
There was a problem hiding this comment.
I was debating exposing these helpers. Would we need these outside of crypto? e.g. do we need like a core-encoder package?
I thought it was interesting that every usage of the sha256 hmac function today passed the key as a base64 string already...does this feel acceptable to continue doing this here or should I accept something like a byte array/buffer?
Also, perhaps consider using this buffer
I did consider this. I actually wanted to stay away from Buffer since it can be a fairly large package and increases browser bundle sizes. Also I'd expect the built in atob/btoa functions in browsers to perform better than a JS Buffer implementation. Now, I think we depend on it in core-amqp and core-http but wasn't sure if we were going to continue that with corev2. If we are then it won't be a big deal to include it in this package.
There was a problem hiding this comment.
Thanks Chris. Perhaps we can discuss in our next weekly meeting whether having a core-utils/core-encoding is a good idea. I am personally onboard with that.
There was a problem hiding this comment.
I'd like to avoid exporting these helpers until we know we need them to avoid adding to the public API prematurely. I do think a core-encoding package may be a better place for these to live long-term.
| "packageName": "@azure/core-crypto", | ||
| "projectFolder": "sdk/core/core-crypto", | ||
| "versionPolicyName": "core" | ||
| }, |
|
This pull request is protected by Check Enforcer. What is Check Enforcer?Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass. Why am I getting this message?You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged. What should I do now?If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows: What if I am onboarding a new service?Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment: |
chradek
force-pushed
the
rn-crypto-helpers
branch
from
a412d94
to
2c5f13a
Compare
…ure#13232) * [core-crypto] initial core-crypto with sha256 hmac/hash functions * [core-crypto] add entry in ci.yml * fix api review quotes * add webworker tests * [core-crypto] add supported protocols to README key concepts section * [core-crypto] use rollup config from dev-tool for node and browser builds * [core-crypto] update typedoc dep * update pnpm-lock.yaml * [core-crypto] fix README validation
## What - Adds a rule to prevent usage of `window` and prefer `self` instead ## Why - This came up recently in a few cases: #11067 and in this discussion: #13232 (comment) - window is not always the global object, but self's reference changes depending on the context and will point to the right global object - Adding a linter rule makes this easy to remember instead of relying on diligence Resolves #13472
Resolves #10204
Description
This PR adds a core-crypto package that our SDK can use to access shared crypto logic.
Motivations
DRY
We have multiple packages that need to compute SHA256 hashes and HMACs at runtime. Thus far, each package has rolled their own support for this as needed. This means if we want to fix a bug or add functionality (e.g. react-native support), we currently need to update each package's implementation. A developer may not even be aware of all the packages that need updating.
Support multiple environments
I had considered using a common folder for this functionality instead of a package, but then the burden is put on each consuming package to properly map node.js to browser/react-native versions of files. Using a separate package instead let's us define these mappings in one place.
Future plans
This PR only supports SHA256 hash/hmac algorithms in node.js and browsers. After this is merged, I'll explore adding react-native support as well.