-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(NODE-7046)!: remove AWS uri/options support #4689
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
|
||
// Get the URI for the cluster then set AWS_ACCESS_KEY_ID as the username in the | ||
// URI and AWS_SECRET_ACCESS_KEY as the password, then set the appropriate auth | ||
// options. Note that MongoClient now auto-connects so no need to store the connect() |
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.
I think we need to update the text in the comments to talk about doing this via env vars instead
const { username, password } = mongoOptions.credentials; | ||
if (username || password) { | ||
throw new MongoAPIError( | ||
'username and password cannot be provided when using MONGODB-AWS. Credentials must be read via the AWS SDK' |
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.
'username and password cannot be provided when using MONGODB-AWS. Credentials must be read via the AWS SDK' | |
'username and password cannot be provided directly when using MONGODB-AWS. Credentials must be read via the AWS SDK' |
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.
I'm hesitant to accept this change as the addition of the word "directly" to me indicates that a username and password could be provided in another fashion. In the case of AWS authentication username and password do not really apply in any form. Same with the comment on the other error message.
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.
Open to different phrasing, but I wanted to make sure that we differentiate between the valid and invalid manner of providing the credentials, "via the sdk" isn't really actionable for users, so perhaps something along the lines "in a manner that can be read by the AWS SDK" and/or "via environment variables or a custom credential provider". Does that make sense?
} | ||
if (mongoOptions.credentials.mechanismProperties.AWS_SESSION_TOKEN) { | ||
throw new MongoAPIError( | ||
'AWS_SESSION_TOKEN cannot be provided when using MONGODB-AWS. Credentials must be read via the AWS SDK' |
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.
'AWS_SESSION_TOKEN cannot be provided when using MONGODB-AWS. Credentials must be read via the AWS SDK' | |
'AWS_SESSION_TOKEN cannot be provided directly when using MONGODB-AWS. Credentials must be read via the AWS SDK' |
. ./activate-authawsvenv.sh | ||
# Source the environment variables. Configure the environment and the server. | ||
. aws_setup.sh $AWS_CREDENTIAL_TYPE | ||
. aws_setup.sh --nouri $AWS_CREDENTIAL_TYPE |
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.
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.
We could file a ticket to update the tools script to log this, but it would be obvious if the option was set or not given the tests. In this PR if the option wasn't set the driver would be throwing an error since MONGODB_URI would have the credentials set. (But we don't want to log what our URIs are in CI or we redact them)
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.
I think it would be useful to do this anyway for debugging purposes in the future - can you go ahead and file the corresponding ticket?
import { loadSpecTests } from '../../spec'; | ||
import { executeUriValidationTest } from '../../tools/uri_spec_runner'; | ||
|
||
const SKIP = [ |
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.
I don't have a good file to comment on, but I was looking for changes to the prose tests, and noticed that our prose tests don't follow the convention - the relevant tests for this are in mongodb_aws.test.ts, where I think we can delete the Case 1 block. Can we move the prose tests from that file to mongodb_aws.prose.test
?
Description
Removes the ability to provide credentials when using MONGODB-AWS authentication.
Summary of Changes
Notes for Reviewers
I made AWS_SESSION_TOKEN internal instead of removing it completely as we still use that object and property internally when fetching credentials.
What is the motivation for this change?
NODE-7046/DRIVERS-3131
Release Highlight
Explicitly Provided Credentials No Longer Accepted With MONGODB-AWS Authentication
AWS environments (such as AWS Lambda) do not have credentials that are permanent and expire within a set amount of time. Providing credentials in the URI or options would mandate that those credentials would be valid for the life of the
MongoClient
, which is problematic. With this change, the installed required AWS SDK will now fetch credentials using the environment, endpoints, or a custom credential provider.This means that for AWS authentication, all client URIs MUST now be specified as:
Double check the following
npm run check:lint
)type(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript