Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions .evergreen/run-mongodb-aws-ecs-test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,3 @@ source ./.evergreen/prepare-shell.sh # should not run git clone

# load node.js
source $DRIVERS_TOOLS/.evergreen/init-node-and-npm-env.sh

# run the tests
npm install aws4
2 changes: 0 additions & 2 deletions .evergreen/setup-mongodb-aws-auth-tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,5 @@ cd $DRIVERS_TOOLS/.evergreen/auth_aws

cd $BEFORE

npm install --no-save aws4

# revert to show test output
set -x
24 changes: 24 additions & 0 deletions etc/aws-test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#!/usr/bin/env bash

cd $DRIVERS_TOOLS/.evergreen/auth_aws
Copy link
Contributor

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


. ./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"
141 changes: 141 additions & 0 deletions src/aws4.ts
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;
Copy link
Contributor

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?

Copy link
Contributor Author

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 type AwsSessionCredentials = {
accessKeyId: string;
secretAccessKey: string;
sessionToken: string;
};

export type AwsLongtermCredentials = {
accessKeyId: string;
secretAccessKey: string;
};
Comment on lines +19 to +28
Copy link
Contributor

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?

Copy link
Contributor Author

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.


export type SignedHeaders = {
headers: {
Authorization: string;
'X-Amz-Date': string;
};
};

export interface AWS4 {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

/**
* 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) => {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

return value.toString().trim().replace(/\s+/g, ' ');
};

export function aws4Sign(
Copy link
Contributor

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: void,
options: Options,
credentials: AwsSessionCredentials | AwsLongtermCredentials | undefined
Copy link
Contributor

Choose a reason for hiding this comment

The 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 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making creds explicitly required.

): SignedHeaders {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
): SignedHeaders {
export function aws4Sign(
options: Options,
credentials: AwsSessionCredentials | AwsLongtermCredentials | undefined
): SignedHeaders {

this isn't necessary

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
Copy link
Contributor

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)


const headers: string[] = [
Copy link
Contributor

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(';');

Copy link
Contributor Author

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.

`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
}
};
}
13 changes: 4 additions & 9 deletions src/cmap/auth/mongodb_aws.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { aws4Sign } from '../../aws4';
import type { Binary, BSONSerializeOptions } from '../../bson';
import * as BSON from '../../bson';
import { aws4 } from '../../deps';
import {
MongoCompatibilityError,
MongoMissingCredentialsError,
Expand Down Expand Up @@ -45,11 +45,6 @@ export class MongoDBAWS extends AuthProvider {
throw new MongoMissingCredentialsError('AuthContext must provide credentials.');
}

if ('kModuleError' in aws4) {
throw aws4['kModuleError'];
}
const { sign } = aws4;

if (maxWireVersion(connection) < 9) {
throw new MongoCompatibilityError(
'MONGODB-AWS authentication requires MongoDB version 4.4 or later'
Expand Down Expand Up @@ -114,7 +109,7 @@ export class MongoDBAWS extends AuthProvider {
}

const body = 'Action=GetCallerIdentity&Version=2011-06-15';
const options = sign(
const signed = aws4Sign(
{
method: 'POST',
host,
Expand All @@ -133,8 +128,8 @@ export class MongoDBAWS extends AuthProvider {
);

const payload: AWSSaslContinuePayload = {
a: options.headers.Authorization,
d: options.headers['X-Amz-Date']
a: signed.headers.Authorization,
d: signed.headers['X-Amz-Date']
};

if (sessionToken) {
Expand Down
60 changes: 0 additions & 60 deletions src/deps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -203,66 +203,6 @@ export function getSocks(): SocksLib | { kModuleError: MongoMissingDependencyErr
}
}

interface AWS4 {
/**
* 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: {
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;
},
credentials:
| {
accessKeyId: string;
secretAccessKey: string;
sessionToken: string;
}
| {
accessKeyId: string;
secretAccessKey: string;
}
| undefined
): {
headers: {
Authorization: string;
'X-Amz-Date': string;
};
};
}

export const aws4: AWS4 | { kModuleError: MongoMissingDependencyError } = loadAws4();

function loadAws4() {
let aws4: AWS4 | { kModuleError: MongoMissingDependencyError };
try {
// eslint-disable-next-line @typescript-eslint/no-require-imports
aws4 = require('aws4');
} catch (error) {
aws4 = makeErrorModule(
new MongoMissingDependencyError(
'Optional module `aws4` not found. Please install it to enable AWS authentication',
{ cause: error, dependencyName: 'aws4' }
)
);
}

return aws4;
}

/** A utility function to get the instance of mongodb-client-encryption, if it exists. */
export function getMongoDBClientEncryption():
| typeof import('mongodb-client-encryption')
Expand Down
Loading