cli/sql: new option autocerts for TLS client cert auto-discovery#101987
cli/sql: new option autocerts for TLS client cert auto-discovery#101987craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
|
This change is |
992f1e1 to
0932ac6
Compare
rafiss
left a comment
There was a problem hiding this comment.
no blocking comments from me. nice improvement!
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ecwall, @knz, and @stevendanna)
pkg/cli/clisqlshell/sql.go line 1699 at r2 (raw file):
// Parse the arguments to \connect: // it accepts newdb, user, host, port in that order.
nit: the comment here can mention options as well
pkg/cli/clisqlshell/sql.go line 2740 at r2 (raw file):
func autoFillClientCerts(newURL, currURL *pgurl.URL, extraCertsDir string) error { username := newURL.GetUsername() // We could use methods from package "certnames" here but we're
i don't feel strongly about using it here, but isn't certnames already an intentionally tiny package?
pkg/cli/interactive_tests/test_connect_cmd.tcl line 122 at r2 (raw file):
start_test "Check that the auto-cert feature properly fails if certs were not found" send "\\c - root - - autocerts\r" eexpect "unable to find TLS client cert and key"
i must be missing something obvious. why is it unable to find the certs here, when the previous test ran the same command and succeeded?
Prior to this patch, only the error message string was printed if `\c` fails. This patch ensures the hints/details are also printed. Release note: None
See the release note below. An additional benefit not mentioned in the release note is that it simplifies switching from one tenant to another when using shared-process multitenancy. For example, this becomes possible: ``` > CREATE TENANT foo; > ALTER TENANT foo START SERVICE SHARED; > \c cluster:foo root - - autocerts ``` Alternatively, this can also be used to quickly switch from a non-root user in an app tenant to the root user in the system tenant: ``` > \c cluster:system root - - autocerts ``` This works because (currently) all tenant servers running side-by-side use the same TLS CA to validate SQL client certs. Release note (cli change): The `\connect` client-side command for the SQL shell (included in `cockroach sql`, `cockroach demo`, `cockroach-sql`) now recognizes an option `autocerts` as last argument. When provided, `\c` will now try to discover a TLS client certificate and key in the same directory(ies) as used by the previous connection URL. This feature makes it easier to switch usernames when TLS client/key files are available for both the previous and the new username.
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/cli/clisqlshell/sql.go line 1699 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: the comment here can mention
optionsas well
Done.
pkg/cli/clisqlshell/sql.go line 2740 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
i don't feel strongly about using it here, but isn't
certnamesalready an intentionally tiny package?
certnames depends on pkg username which depends on lexbase, catid. Catid in turn depends on intsets, lib/pq/oid, sql/oidext. I think that's a lot for just constructing a string.
pkg/cli/interactive_tests/test_connect_cmd.tcl line 122 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
i must be missing something obvious. why is it unable to find the certs here, when the previous test ran the same command and succeeded?
lol, yes you're right (and CI is telling us the same).
I mistyped. Fixed.
|
bors r=rafiss |
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/cli/clisqlshell/sql.go line 2740 at r2 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
certnames depends on pkg username which depends on lexbase, catid. Catid in turn depends on intsets, lib/pq/oid, sql/oidext. I think that's a lot for just constructing a string.
i see, makes sense. i incorrectly thought we already depended on username
|
Build failed (retrying...): |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error setting reviewers, but backport branch blathers/backport-release-23.1-101987 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/103144/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. [] Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Fixes #101986.
See the release note below.
An additional benefit not mentioned in the release note is that
it simplifies switching from one tenant to another when using
shared-process multitenancy. For example, this becomes possible:
Alternatively, this can also be used to quickly switch from a non-root
user in an app tenant to the root user in the system tenant:
This works because (currently) all tenant servers running side-by-side
use the same TLS CA to validate SQL client certs.
Release note (cli change): The
\connectclient-side command for theSQL shell (included in
cockroach sql,cockroach demo,cockroach-sql) now recognizes an optionautocertsas lastargument.
When provided,
\cwill now try to discover a TLS clientcertificate and key in the same directory(ies) as used by the previous
connection URL.
This feature makes it easier to switch usernames when
TLS client/key files are available for both the previous and the new
username.