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

cli/sql: new option autocerts for TLS client cert auto-discovery #101987

Merged
merged 2 commits into from
May 11, 2023

Conversation

knz
Copy link
Contributor

@knz knz commented Apr 21, 2023

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:

> 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 knz added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Apr 21, 2023
@knz knz requested review from stevendanna, rafiss, ecwall and a team April 21, 2023 10:02
@knz knz requested review from a team as code owners April 21, 2023 10:02
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz force-pushed the 20230421-cli-connect branch 2 times, most recently from 992f1e1 to 0932ac6 Compare April 21, 2023 10:15
@knz knz added the A-multitenancy Related to multi-tenancy label Apr 21, 2023
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

no blocking comments from me. nice improvement!

Reviewable status: :shipit: 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?

knz added 2 commits May 11, 2023 19:18
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 knz force-pushed the 20230421-cli-connect branch from 0932ac6 to dedeacf Compare May 11, 2023 17:28
Copy link
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ecwall, @rafiss, and @stevendanna)


pkg/cli/clisqlshell/sql.go line 1699 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: the comment here can mention options as 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 certnames already 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.

@knz
Copy link
Contributor Author

knz commented May 11, 2023

bors r=rafiss

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ecwall, @knz, and @stevendanna)


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

@craig
Copy link
Contributor

craig bot commented May 11, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented May 11, 2023

Build succeeded:

@blathers-crl
Copy link

blathers-crl bot commented May 11, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-multitenancy Related to multi-tenancy backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cli/sql: make it easier to switch users/tenants when TLS client cert authn is used
3 participants