Skip to content

feat(NODE-3777): set tls options per kms provider #235

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Jan 20, 2022
Merged

feat(NODE-3777): set tls options per kms provider #235

merged 12 commits into from
Jan 20, 2022

Conversation

@durran
Copy link
Member

@durran durran commented Jan 17, 2022 •
edited
Loading

  • Changes the TLS provider options to function per KMS provider. This now ensures each KMS provider can customise their TLS settings.
  • Adds validation on the TLS options not to support any insecure options.

Note Prose and spec tests are located in the main Node repo. Related PR mongodb/node-mongodb-native#3070

Node CSFLE run against this specific branch: https://spruce.mongodb.com/version/61e753873066153369a025de/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

addaleax
addaleax reviewed Jan 17, 2022
View reviewed changes
@durran durran marked this pull request as ready for review January 18, 2022 21:36
@durran durran requested a review from nbbeeken as a code owner January 18, 2022 21:36
durran
durran commented Jan 19, 2022
View reviewed changes
bindings/node/index.d.ts Outdated
* TLS options for kms providers to use.
*/
tlsOptions?: ClientEncryptionTLSOptions;
tlsOptions?: {
Copy link
Member Author

Choose a reason for hiding this comment

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

TLS options to KMS providers are configured on a per KMS provider basis for future flexibility.

durran
durran commented Jan 19, 2022
View reviewed changes
durran
durran commented Jan 19, 2022
View reviewed changes
durran
durran commented Jan 19, 2022
View reviewed changes
@durran durran mentioned this pull request Jan 19, 2022
Merged
5 tasks
addaleax
addaleax approved these changes Jan 19, 2022
View reviewed changes
baileympearson
baileympearson approved these changes Jan 19, 2022
View reviewed changes
Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

I left one minor (non-blcoking) comment. Otherwise the changes look good to me

@durran durran merged commit 5920d84 into master Jan 20, 2022
@durran durran deleted the NODE-3777 branch January 20, 2022 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes

@baileympearson baileympearson baileympearson approved these changes

@nbbeeken nbbeeken Awaiting requested review from nbbeeken

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@durran @addaleax @baileympearson