-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(NODE-5393): Migrate AWS signature v4 logic into driver #4824
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
base: main
Are you sure you want to change the base?
Conversation
…ompare the output with the old aws4 library
| const date = options.date || new Date(); | ||
| const requestDateTime = date.toISOString().replace(/[:-]|\.\d{3}/g, ''); | ||
| const requestDate = requestDateTime.substring(0, 8); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is sort of difficult to parse but seems like it's constructing an ISO 8601 date with no timezone or hyphens?
Maybe we could either comment this, or just move it into an appropriately named function to make it clear
(https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_sigv-signing-elements.html#date <- I found these docs)
| export type AwsSessionCredentials = { | ||
| accessKeyId: string; | ||
| secretAccessKey: string; | ||
| sessionToken: string; | ||
| }; | ||
|
|
||
| export type AwsLongtermCredentials = { | ||
| accessKeyId: string; | ||
| secretAccessKey: string; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we reuse our existing AWSCredentials type instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, removing these unnecessary types.
| }; | ||
| }; | ||
|
|
||
| const convertHeaderValue = (value: string | number) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this function URI encoding the header value? Could we just use encodeUriComponent instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, this is not URI encoding, this is replacing consecutive spaces with a single space.
I'll add comments for all of these methods.
| }; | ||
| service: string; | ||
| region: string; | ||
| date?: Date; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this only exposed so we can unit test sigv4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Do we have a different way to override new Date() in tests?
| }; | ||
| }; | ||
|
|
||
| export interface AWS4 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason you can think of to continue using this type? I'm not sure it's necessary anymore, now that we've taken on ownership of this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that all of this is internal and specific to just a single call, yeah, we can drop this interface.
| const requestDateTime = date.toISOString().replace(/[:-]|\.\d{3}/g, ''); | ||
| const requestDate = requestDateTime.substring(0, 8); | ||
|
|
||
| const headers: string[] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make this a bit simpler if we used Headers?
something like:
const headers = new Headers({
'content-length': ...,
'content-type': ...,
...
});That'd make things further down a bit nicer, ex:
const signedHeaders = Array.from(headers.keys()).join(';');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplifies things a bit, using it.
| return value.toString().trim().replace(/\s+/g, ' '); | ||
| }; | ||
|
|
||
| export function aws4Sign( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add some comments and/or docs about where the algorithm here is defined? Just to make it easier to maintain for future reviewers and devs.
| // This is done by calculating a signature, then using it to make a real request to the AWS STS service. | ||
| // To run this test, simply run `./etc/aws-test.sh`. | ||
|
|
||
| describe('AwsSigV4', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a separate test suite, what if we just included these tests in the existing MongoDB AWS test suite and then only ran them if we are running in AWS ECS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like a good approach, looking into it.
| @@ -0,0 +1,24 @@ | |||
| #!/usr/bin/env bash | |||
|
|
|||
| cd $DRIVERS_TOOLS/.evergreen/auth_aws | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do decide to leave these tests in their own test suite, can we break this into three separate tasks?
That is the approach we generally prefer because each test run produces an xunit file. This means only the last test run's results are actually uploaded to evergreen
| this: void, | ||
| options: Options, | ||
| credentials: AwsSessionCredentials | AwsLongtermCredentials | undefined | ||
| ): SignedHeaders { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ): SignedHeaders { | |
| export function aws4Sign( | |
| options: Options, | |
| credentials: AwsSessionCredentials | AwsLongtermCredentials | undefined | |
| ): SignedHeaders { |
this isn't necessary
| const getHash = (str: string): string => { | ||
| return crypto.createHash('sha256').update(str, 'utf8').digest('hex'); | ||
| }; | ||
| const getHmacArray = (key: string | Uint8Array, str: string): Uint8Array => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A name like getHmacArray would imply to me that this returns an actual Array, not a typed array – if you want to keep the brevity, I'd suggest something like getHmacBuffer, which, even if it doesn't return an actual Buffer, still communicates the return type better (or just have it be verbose and call it getHmacUint8Array)
| }; | ||
| const getHmacString = (key: Uint8Array, str: string): string => { | ||
| return crypto.createHmac('sha256', key).update(str, 'utf8').digest('hex'); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the webcrypto API for this? It's supported as a global in all versions of Node.js and browsers that we target, right?
| let aws4: AWS4 | { kModuleError: MongoMissingDependencyError }; | ||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| aws4 = require('aws4'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make sure to mark the ticket as having DevTools impact (we can now also remove this dependency)
Description
Summary of Changes
Replace optional dependency on aws4 package with a minimal equivalent implementation.
What is the motivation for this change?
This helps us reduce our runtime dependencies, as part of https://jira.mongodb.org/browse/NODE-6601
Release Highlight
Replace optional dependency on aws4 package with a minimal equivalent implementation
This reduces the number of optional dependencies.
Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript