Connect: Check db cert before using it for local proxy#18740
Merged
Connect: Check db cert before using it for local proxy#18740
Conversation
ravicious
force-pushed
the
ravicious/check-cert-subject
branch
from
100740b to
7279a2b
Compare
Those tests are too fast and simple to run them in parallel.
Those helpers will be useful in lib/teleterm tests where we need to create a gateway which spawns an alpnproxy.LocalProxy underneath.
We're going to need a standalone version of this function in the next commit.
This function builds on top of alpnproxytest.MustGenCertSignedWithCA. In alpnproxy, LocalProxy operates on certs solely in memory through NewLocalProxy and LocalProxy.SetCerts. Connect on the other hand assumes that the key pair can be loaded from disk, so we have to provide a function which generates the certs and then saves them to file. In the next commit, we're going to check the subject of the cert, so Connect tests need a way of generating valid certs.
ravicious
force-pushed
the
ravicious/check-cert-subject
branch
from
7279a2b to
00dc268
Compare
smallinsky
reviewed
Nov 28, 2022
smallinsky
approved these changes
Nov 28, 2022
Co-authored-by: Marek Smoliński <marek@goteleport.com>
GavinFrazar
approved these changes
Nov 28, 2022
Contributor
GavinFrazar
left a comment
GavinFrazar
left a comment
There was a problem hiding this comment.
approved with some nits
Contributor
|
@ravicious See the table below for backport results.
|
ravicious
added a commit
that referenced
this pull request
Dec 1, 2022
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
3 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 PR addresses an issue that was found during the review of another PR (#18259 (comment)). Best reviewed commit by commit.
At the moment, when Connect creates a local db proxy, it obtains a cert for the database, saves it to disk, then loads it from disk and provides it to
alpnproxy.LocalProxy. Between being saved and loaded, the cert can be modified by another process, such as the user executingtsh db loginwith different--db-useror--db-namethan we issued the cert for. It's unlikely to happen but still possible.Why not just avoid saving the cert to disk? This is a form of tech debt that stems from Connect using
TeleportClient.ReissueUserCertsto obtain db certs. That method doesn't return the cert, it saves it directly to disk.Once we add per-session MFA support to Connect, we'll be able to address this. But for now, we have to check the cert after loading it from disk. In case the check fails, the user will see the error message and will be able to recreate the local proxy with the same params (thus effectively reissuing the cert).
Since we now check the cert when initializing
gateway.Gateway, I had to update the tests to use valid certs since in the past I provided some bogus ones – they wouldn't be checked until something established an actual connection through the proxy.