Add mutex for certs in local proxy#18278
Merged
GavinFrazar merged 17 commits intomasterfrom Nov 18, 2022
Merged
Conversation
ryanclark
reviewed
Nov 8, 2022
Member
ryanclark
left a comment
ryanclark
left a comment
There was a problem hiding this comment.
This could be an RWMutex instead
Co-authored-by: Ryan Clark <ryan.clark@goteleport.com>
Co-authored-by: Ryan Clark <ryan.clark@goteleport.com>
Co-authored-by: Ryan Clark <ryan.clark@goteleport.com>
Contributor
Author
|
I think you're right, the certs will mostly be read not written to, so RWMutex makes more sense |
rosstimothy
approved these changes
Nov 8, 2022
Contributor
Author
|
@ryanclark I used your suggestions, are there any other concerns? |
zmb3
reviewed
Nov 8, 2022
* test for data race * test for cert checking
Member
zmb3
reviewed
Nov 9, 2022
ryanclark
approved these changes
Nov 14, 2022
rosstimothy
reviewed
Nov 17, 2022
Contributor
|
@GavinFrazar See the table below for backport results.
|
GavinFrazar
added a commit
that referenced
this pull request
Nov 18, 2022
* Add mutex for certs in local proxy * Update lib/srv/alpnproxy/local_proxy.go Co-authored-by: Ryan Clark <ryan.clark@goteleport.com> * Update lib/srv/alpnproxy/local_proxy.go Co-authored-by: Ryan Clark <ryan.clark@goteleport.com> * Update lib/srv/alpnproxy/local_proxy.go Co-authored-by: Ryan Clark <ryan.clark@goteleport.com> * Move cert checking out of middleware into local proxy * Configure a logger for local proxy * Fixup imports * Add tests for local proxy * test for data race * test for cert checking * Update integration test for local proxy * Mark err assert fns as helpers Co-authored-by: Ryan Clark <ryan.clark@goteleport.com>
GavinFrazar
added a commit
that referenced
this pull request
Nov 22, 2022
* Add mutex for certs in local proxy * Update lib/srv/alpnproxy/local_proxy.go Co-authored-by: Ryan Clark <ryan.clark@goteleport.com> * Update lib/srv/alpnproxy/local_proxy.go Co-authored-by: Ryan Clark <ryan.clark@goteleport.com> * Update lib/srv/alpnproxy/local_proxy.go Co-authored-by: Ryan Clark <ryan.clark@goteleport.com> * Move cert checking out of middleware into local proxy * Configure a logger for local proxy * Fixup imports * Add tests for local proxy * test for data race * test for cert checking * Update integration test for local proxy * Mark err assert fns as helpers Co-authored-by: Ryan Clark <ryan.clark@goteleport.com> Co-authored-by: Ryan Clark <ryan.clark@goteleport.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a small PR that adds a mutex to guard get/set certs in the alpn local proxy.The purpose of this PR is to eliminate the possibility of a data race by using a mutex and providing a safe API for packages outside of alpnproxy. The PR was small, but @zmb3 correctly pointed out that
GetCertswas basically a bug waiting to happen, since the caller must know to not mutate the slice returned. Therefore I refactored and eliminated the publicGetCertsmethod in favor of having the local proxy check its own certs.This PR moves database cert checking out of the local proxy middleware and into the local proxy itself. The middleware can instead call the local proxy's method to check for cert validity, while still handling cert renewal if needed.
In #16958 I had added a mutex and the get/set methods on local proxy for the purpose of protecting reads/writes to the TLS certs, but I removed the mutex when I moved the middleware out the goroutine spawned in
lib/srv/alpnproxy/LocalProxy.Start. What I failed to account for was that the certs are read once and copied inside the function that handles the downstream connection in that goroutine. I noticed this when reviewing this code today. I was unable to trip the go race detector in tests unless I modified the local proxy code to call GetCerts in a tight loop. I think it's because there is a very small window where the race can actually happen, but regardless we should guard the certs with a mutex.Tests