Skip to content
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

GODRIVER-3321 Fix CSE SetTLSConfig option. #1900

Merged
merged 4 commits into from
Dec 3, 2024

Conversation

matthewdale
Copy link
Collaborator

@matthewdale matthewdale commented Nov 25, 2024

GODRIVER-3321

Summary

Immediately make a copy of the TLS config map in ClientEncryptionOptionsBuilder.SetTLSConfig and AutoEncryptionOptionsBuilder.SetTLSConfig to prevent the map values from changing before the option function is called.

Background & Motivation

Currently, if you modify the TLS config map passed to ClientEncryptionOptionsBuilder.SetTLSConfig or AutoEncryptionOptionsBuilder.SetTLSConfig before passing the options to NewClientEncryption, the TLS config map passed to NewClientEncryption will unexpectedly change. That's because the option function is a closure over the original map. Instead, we should make a copy of the map in the setter body and pass that copy into the option function.

@mongodb-drivers-pr-bot mongodb-drivers-pr-bot bot added the priority-3-low Low Priority PR for Review label Nov 26, 2024
Copy link
Contributor

API Change Report

No changes found!

@matthewdale matthewdale marked this pull request as ready for review November 26, 2024 00:19
@matthewdale matthewdale added priority-1-high High Priority PR for Review and removed priority-3-low Low Priority PR for Review labels Nov 26, 2024
@matthewdale matthewdale force-pushed the godriver3321-fix-kmip branch from 6ade819 to b7a77c8 Compare November 26, 2024 03:23
Copy link
Collaborator

@qingyang-hu qingyang-hu left a comment

Choose a reason for hiding this comment

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

I doubt if we need to deep-copy the tlsOpts, which happens to be a map. The root cause of the test failure is that we lazy-apply (apply-by-need?) the option values in our new option pattern, while the maps (and slices) in the Golang are reference types. Therefore, when we reuse the map in the test cases, options are overwritten accidentally.
I think we only need to document the "SetOption" behavior because the de facto pass-by-reference is familiar to Go developers though it's not intuitive.
On the other hand, if we are going to deep-copy the tlsOpts, it would be better deep-copy other options in reference types as well to avoid any surprising behavior.

@matthewdale
Copy link
Collaborator Author

matthewdale commented Dec 3, 2024

@qingyang-hu you're correct, only SetTLSConfig makes a copy of the input map. I believe the goal was to enforce TLS 1.2 as the minimum as suggested by this code:

// Use TLS min version 1.2 to enforce more secure hash algorithms and
// advanced cipher suites.
if config.MinVersion == 0 {
	config.MinVersion = tls.VersionTLS12
}

I believe the copy was made to prevent modifying the input values, but it's not actually a deep copy because the tls.Config is passed as a pointer, so the above line actually modifies the input tls.Config. That bug was probably introduced as a side effect of my comment on PR #812.

In any case, Go 1.18 changed the default to TLS 1.2 (see here), so all of that additional logic is now unnecessary because it matches the new Go default.

TLDR: I will remove the ineffective and unnecessary copy and rewrite the test to use map literals.

Copy link
Collaborator

@qingyang-hu qingyang-hu left a comment

Choose a reason for hiding this comment

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

Good! Let's merge it.

@matthewdale matthewdale merged commit 6824503 into mongodb:master Dec 3, 2024
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-1-high High Priority PR for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants