-
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?
Changes from all commits
7cc1156
811d453
a0ba1ec
449d677
a44f3b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,5 @@ cd $DRIVERS_TOOLS/.evergreen/auth_aws | |
|
|
||
| cd $BEFORE | ||
|
|
||
| npm install --no-save aws4 | ||
|
|
||
| # revert to show test output | ||
| set -x | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| cd $DRIVERS_TOOLS/.evergreen/auth_aws | ||
|
|
||
| . ./activate-authawsvenv.sh | ||
|
|
||
| # Test with permanent credentials | ||
| . aws_setup.sh env-creds | ||
| unset MONGODB_URI | ||
| echo "AWS_SESSION_TOKEN is set to '${AWS_SESSION_TOKEN-NOT SET}'" | ||
| npm run check:test -- --grep "AwsSigV4" | ||
|
|
||
| # Test with session credentials | ||
| . aws_setup.sh session-creds | ||
| unset MONGODB_URI | ||
| echo "AWS_SESSION_TOKEN is set to '${AWS_SESSION_TOKEN-NOT SET}'" | ||
| npm run check:test -- --grep "AwsSigV4" | ||
|
|
||
| # Test with missing credentials | ||
| unset MONGODB_URI | ||
| unset AWS_ACCESS_KEY_ID | ||
| unset AWS_SECRET_ACCESS_KEY | ||
| unset AWS_SESSION_TOKEN | ||
| npm run check:test -- --grep "AwsSigV4" | ||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,141 @@ | ||||||||||||
| import * as crypto from 'node:crypto'; | ||||||||||||
|
|
||||||||||||
| export type Options = { | ||||||||||||
| path: '/'; | ||||||||||||
| body: string; | ||||||||||||
| host: string; | ||||||||||||
| method: 'POST'; | ||||||||||||
| headers: { | ||||||||||||
| 'Content-Type': 'application/x-www-form-urlencoded'; | ||||||||||||
| 'Content-Length': number; | ||||||||||||
| 'X-MongoDB-Server-Nonce': string; | ||||||||||||
| 'X-MongoDB-GS2-CB-Flag': 'n'; | ||||||||||||
| }; | ||||||||||||
| service: string; | ||||||||||||
| region: string; | ||||||||||||
| date?: Date; | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this only exposed so we can unit test sigv4?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup. Do we have a different way to override |
||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| export type AwsSessionCredentials = { | ||||||||||||
| accessKeyId: string; | ||||||||||||
| secretAccessKey: string; | ||||||||||||
| sessionToken: string; | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| export type AwsLongtermCredentials = { | ||||||||||||
| accessKeyId: string; | ||||||||||||
| secretAccessKey: string; | ||||||||||||
| }; | ||||||||||||
|
Comment on lines
+19
to
+28
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we reuse our existing
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Absolutely, removing these unnecessary types. |
||||||||||||
|
|
||||||||||||
| export type SignedHeaders = { | ||||||||||||
| headers: { | ||||||||||||
| Authorization: string; | ||||||||||||
| 'X-Amz-Date': string; | ||||||||||||
| }; | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| export interface AWS4 { | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||||||||
| /** | ||||||||||||
| * Created these inline types to better assert future usage of this API | ||||||||||||
| * @param options - options for request | ||||||||||||
| * @param credentials - AWS credential details, sessionToken should be omitted entirely if its false-y | ||||||||||||
| */ | ||||||||||||
| sign( | ||||||||||||
| this: void, | ||||||||||||
| options: Options, | ||||||||||||
| credentials: AwsSessionCredentials | AwsLongtermCredentials | undefined | ||||||||||||
| ): SignedHeaders; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| const getHash = (str: string): string => { | ||||||||||||
| return crypto.createHash('sha256').update(str, 'utf8').digest('hex'); | ||||||||||||
| }; | ||||||||||||
| const getHmacArray = (key: string | Uint8Array, str: string): Uint8Array => { | ||||||||||||
| return crypto.createHmac('sha256', key).update(str, 'utf8').digest(); | ||||||||||||
| }; | ||||||||||||
| const getHmacString = (key: Uint8Array, str: string): string => { | ||||||||||||
| return crypto.createHmac('sha256', key).update(str, 'utf8').digest('hex'); | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| const getEnvCredentials = () => { | ||||||||||||
| const env = process.env; | ||||||||||||
| return { | ||||||||||||
| accessKeyId: env.AWS_ACCESS_KEY_ID || env.AWS_ACCESS_KEY, | ||||||||||||
| secretAccessKey: env.AWS_SECRET_ACCESS_KEY || env.AWS_SECRET_KEY, | ||||||||||||
| sessionToken: env.AWS_SESSION_TOKEN | ||||||||||||
| }; | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| const convertHeaderValue = (value: string | number) => { | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this function URI encoding the header value? Could we just use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||||||||
| return value.toString().trim().replace(/\s+/g, ' '); | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| export function aws4Sign( | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: void, | ||||||||||||
| options: Options, | ||||||||||||
| credentials: AwsSessionCredentials | AwsLongtermCredentials | undefined | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We only ever call this with credentials already fetched - could we make this explicitly required? The benefit of that is that we then don't need logic to fetch credentials from the environment, if unset. Right now, we rely on the AWS SDK to fetch these credentials for us and it would be nice not to add this logic back into the driver 🙂
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Making creds explicitly required. |
||||||||||||
| ): SignedHeaders { | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
| const method = options.method; | ||||||||||||
| const canonicalUri = options.path; | ||||||||||||
| const canonicalQuerystring = ''; | ||||||||||||
| const creds = credentials || getEnvCredentials(); | ||||||||||||
|
|
||||||||||||
| const date = options.date || new Date(); | ||||||||||||
| const requestDateTime = date.toISOString().replace(/[:-]|\.\d{3}/g, ''); | ||||||||||||
| const requestDate = requestDateTime.substring(0, 8); | ||||||||||||
|
Comment on lines
+83
to
+85
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) |
||||||||||||
|
|
||||||||||||
| const headers: string[] = [ | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Simplifies things a bit, using it. |
||||||||||||
| `content-length:${convertHeaderValue(options.headers['Content-Length'])}`, | ||||||||||||
| `content-type:${convertHeaderValue(options.headers['Content-Type'])}`, | ||||||||||||
| `host:${convertHeaderValue(options.host)}`, | ||||||||||||
| `x-amz-date:${convertHeaderValue(requestDateTime)}`, | ||||||||||||
| `x-mongodb-gs2-cb-flag:${convertHeaderValue(options.headers['X-MongoDB-GS2-CB-Flag'])}`, | ||||||||||||
| `x-mongodb-server-nonce:${convertHeaderValue(options.headers['X-MongoDB-Server-Nonce'])}` | ||||||||||||
| ]; | ||||||||||||
| if ('sessionToken' in creds && creds.sessionToken) { | ||||||||||||
| headers.push(`x-amz-security-token:${convertHeaderValue(creds.sessionToken)}`); | ||||||||||||
| } | ||||||||||||
| const canonicalHeaders = headers.sort().join('\n'); | ||||||||||||
| const canonicalHeaderNames = headers.map(header => header.split(':', 2)[0].toLowerCase()); | ||||||||||||
| const signedHeaders = canonicalHeaderNames.sort().join(';'); | ||||||||||||
|
|
||||||||||||
| const hashedPayload = getHash(options.body || ''); | ||||||||||||
|
|
||||||||||||
| const canonicalRequest = [ | ||||||||||||
| method, | ||||||||||||
| canonicalUri, | ||||||||||||
| canonicalQuerystring, | ||||||||||||
| canonicalHeaders + '\n', | ||||||||||||
| signedHeaders, | ||||||||||||
| hashedPayload | ||||||||||||
| ].join('\n'); | ||||||||||||
|
|
||||||||||||
| const canonicalRequestHash = getHash(canonicalRequest); | ||||||||||||
| const credentialScope = `${requestDate}/${options.region}/${options.service}/aws4_request`; | ||||||||||||
|
|
||||||||||||
| const stringToSign = [ | ||||||||||||
| 'AWS4-HMAC-SHA256', | ||||||||||||
| requestDateTime, | ||||||||||||
| credentialScope, | ||||||||||||
| canonicalRequestHash | ||||||||||||
| ].join('\n'); | ||||||||||||
|
|
||||||||||||
| const dateKey = getHmacArray('AWS4' + creds.secretAccessKey, requestDate); | ||||||||||||
| const dateRegionKey = getHmacArray(dateKey, options.region); | ||||||||||||
| const dateRegionServiceKey = getHmacArray(dateRegionKey, options.service); | ||||||||||||
| const signingKey = getHmacArray(dateRegionServiceKey, 'aws4_request'); | ||||||||||||
| const signature = getHmacString(signingKey, stringToSign); | ||||||||||||
|
|
||||||||||||
| const authorizationHeader = [ | ||||||||||||
| 'AWS4-HMAC-SHA256 Credential=' + creds.accessKeyId + '/' + credentialScope, | ||||||||||||
| 'SignedHeaders=' + signedHeaders, | ||||||||||||
| 'Signature=' + signature | ||||||||||||
| ].join(', '); | ||||||||||||
|
|
||||||||||||
| return { | ||||||||||||
| headers: { | ||||||||||||
| Authorization: authorizationHeader, | ||||||||||||
| 'X-Amz-Date': requestDateTime | ||||||||||||
| } | ||||||||||||
| }; | ||||||||||||
| } | ||||||||||||
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