cli: allow SQL commands to use password authn in more cases #53994

knz · 2020-09-05T10:16:15Z

First two commits from #53991.

Previously, SQL password authn was only allowed over TLS connections.

With this change, password authn is allowed regardless of whether the
connection uses TLS.

This is implemented by also only asking for a password interactively
the first time that the server complains that pw auth has failed. This
way, no password is ever requested interactively if the server
"trusts" the connection (via HBA rules or --insecure).

Release justification: low risk, high benefit changes to existing functionality

cockroach-teamcity · 2020-09-05T10:16:25Z

This change is

aaron-crl

Looks good aside from potential nit.

aaron-crl · 2020-09-08T18:37:56Z

pkg/sql/pgwire/server.go

 			return
 		}

-		// Non-SSL in non-secure mode, all is well: no-op.
+		// Secure mode: disallow if TCP and the user did not opt into SQL
+		// conns.


Nit (I think): Should this comment read "[...] user did not opt into non-TLS conns" or something similar?

irfansharif

Reviewed 8 of 8 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @bdarnell, @irfansharif, and @knz)

pkg/cli/sql_util.go, line 67 at r3 (raw file):

	// This is set to false if not using the unix socket, the connection
	// is not encrypted, and --allow-unencrypted-password is not set.
	canPassword bool

s/canPassword/canUsePassword? Feel free to ignore this one.

pkg/cli/sql_util.go, line 779 at r3 (raw file):

	// Password can be safely encrypted, or the user opted in
	// manually to non-encryption. All good.

[nit] Add a new line here? It reads as if the comment is for the password prompt below, but it's not.

bdarnell

Reviewed 8 of 8 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @bdarnell and @knz)

pkg/cli/flags.go, line 696 at r3 (raw file):

			_ = f.MarkHidden(cliflags.ClientPort.Name)

			boolFlag(f, &cliCtx.allowUnencryptedClientPassword, cliflags.AllowUnencryptedClientPassword)

The server flag is experimental; shouldn't this one be too?

pkg/cli/flags_test.go, line 404 at r3 (raw file):

				// from discrete flags yield equivalent connection parameters.
				// (RPC commands never reconstruct a URL.)
				if cmdName != anySQL[0] && cmdName != anySQL[1] {

This looks like it should loop over anySQL instead of hard-coding the fact that it's length 2.

knz

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @bdarnell and @knz)

pkg/cli/flags.go, line 696 at r3 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

The server flag is experimental; shouldn't this one be too?

We can mark it as experimental in the doc perhaps? But I don't see a reason to hide it - it's generally useful, including when connecting to pre-20.2 servers and to postgresql servers with a crdb client.

bdarnell

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz)

pkg/cli/flags.go, line 696 at r3 (raw file):

Previously, knz (kena) wrote…

We can mark it as experimental in the doc perhaps? But I don't see a reason to hide it - it's generally useful, including when connecting to pre-20.2 servers and to postgresql servers with a crdb client.

We already envision future changes when we support scram. You suggest that we could do something where we allow the password by default if either the connection is TLS or the authentication protocol is scram, but that doesn't seem to be something that is supported today by the postgres drivers. In the future when scram is the default I'm not sure it's going to be worth the effort and maybe we remove the password-over-non-tls safeguard completely. Anyway, this seems like a stopgap that is just as experimental as the server-side counterpart.

pkg/cli/cliflags/flags.go, line 550 at r3 (raw file):

	}

	AllowUnencryptedClientPassword = FlagInfo{

This flag should also have an EnvVar alternative (for the same reasons as COCKROACH_INSECURE - you should be able to set up your env vars so that cockroach sql works without additional arguments).

knz

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @bdarnell and @irfansharif)

pkg/cli/flags.go, line 696 at r3 (raw file):

that doesn't seem to be something that is supported today by the postgres drivers

The drivers don't care about which configuration you give them. They are generally happy to pass passwords in-clear over unencrypted links. It's the user's responsibility to set sslmode to something else than disable. When sslmode is not set it is equivalent to auto and TLS is used if the server supports it. The driver in our SQL shell even probably knows how to do that, even though our command-line argument parsing does not recognize it yet.

My reason for the flag here is to prevent the user from shooting themselves in the foot; and also to satisfy our current users' expectation that CockroachDB will refuse to connect an insecure client to a secure server.

If you think we can do without the flag right now already (i.e. the user knows best) I'd be happy to remove it.
Maybe we don't even need this flag: when all clusters are secure by default, it won't matter any more.
If/when we remove it, we can make it a no-op in a deprecation cycle (i.e. it doesn't hurt).

But point granted. Marked the flag as experimental in its help text.

pkg/cli/flags_test.go, line 404 at r3 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

This looks like it should loop over anySQL instead of hard-coding the fact that it's length 2.

Done.

pkg/cli/sql_util.go, line 67 at r3 (raw file):

Previously, irfansharif (irfan sharif) wrote…

s/canPassword/canUsePassword? Feel free to ignore this one.

Done.

pkg/cli/sql_util.go, line 779 at r3 (raw file):

Previously, irfansharif (irfan sharif) wrote…

[nit] Add a new line here? It reads as if the comment is for the password prompt below, but it's not.

Done.

pkg/cli/cliflags/flags.go, line 550 at r3 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

This flag should also have an EnvVar alternative (for the same reasons as COCKROACH_INSECURE - you should be able to set up your env vars so that cockroach sql works without additional arguments).

Good point. Done.

pkg/sql/pgwire/server.go, line 702 at r3 (raw file):

Previously, aaron-crl wrote…

Nit (I think): Should this comment read "[...] user did not opt into non-TLS conns" or something similar?

Done.

53991: pgwire: accept non-TLS client conns safely in secure mode r=aaron-crl,irfansharif,bdarnell a=knz Fixes #44842. Informs #49532. Informs #53404. This change makes it possible for a DBA / system administrator to reconfigure individual nodes *in a secure cluster* to accept SQL client sessions over TCP without mandating a TLS handshake. Authentication remains mandatory as per the HBA rules. Motivation: we have at least two high-profile customers who keep their nodes and client apps in a private secure network (with network-level encryption / privacy) and who experience client-side TLS as unnecessary and expensive friction. Additionally, **this feature is a prerequisite to upgrade an insecure cluster to secure mode without downtime.** Why this does not impair security: - authentication remains mandatory (as per the HBA rules -- [1] [2]). - the feature is opt-in: the operator must set a command-line flag (`--accept-sql-without-tls`), which is not enabled by default. - there is an interlock: the user must both set up the flag and set log-in passwords for their SQL users (by default, users get created without a password and thus cannot log in without client certs). - for now, node-node connections still require TLS. [1]: https://www.postgresql.org/docs/12/auth-pg-hba-conf.html [2]: https://dr-knz.net/authentication-in-postgresql-and-cockroachdb.html For context, the default HBA configuration is the following: ``` host all root all cert-password # fixed rule host all all all cert-password # built-in CockroachDB default local all all password # built-in CockroachDB default ``` The directive `host` covers both TLS and non-TLS incoming TCP connections (`local` is for the unix socket). The method `cert-password` means "client cert or password": without a cert, the password is mandatory. As previously, the user can further secure the configuration by restricting non-TLS connections to just a subnetwork, for example: ``` host all all 10.0.0.0/8 password # accept conns on the 10/8 network. host all all all reject # refuse conns from other nets. local all all password ``` Note that this change is limited to the server side: CockroachDB's own `cockroach` CLI commands do not yet know how to connect to a CockroachDB server without TLS; such connections are only supported from `psql` or SQL client drivers in apps. See #53994 for a follow-up. Release justification: fixes for high-priority or high-severity bugs in existing functionality 54019: roachtest: de-flake 'inconsistent' r=knz a=tbg This test sets up an intentionally corrupted replica and wants its node to shut down as a result of its detection. When only two of the three nodes were included in the consistency check, either one of them could end up terminating (as no obvious majority of healthy replicas can be determined). Change the test so that we wait for the cluster to come fully together before setting a low consistency check interval. Closes #54005. Release justification: testing Release note: None Co-authored-by: Raphael 'kena' Poss <[email protected]> Co-authored-by: Tobias Grieger <[email protected]>

knz · 2020-09-10T13:48:10Z

There's a contingent of folk (@BramGruneir @dt @dbist) who convinced me that:

if the server operator chooses to not allow TLS we should not yell at the client user for not using it
if the server operator chooses to not allow TLS, they already made a choice, we don't need to ask them to choose a second time

so there is no flag necessary. I'll tweak the PR accordingly.

knz · 2020-09-10T13:55:39Z

Done, PTAL

Previously, SQL password authn was only allowed over TLS connections. With this change, password authn is allowed regardless of whether the connection uses TLS. This is implemented by also only asking for a password interactively the first time that the server complains that pw auth has failed. This way, no password is ever requested interactively if the server "trusts" the connection (via HBA rules or `--insecure`). Release justification: low risk, high benefit changes to existing functionality Release note (cli change): It is now possible to use password authentication over non-TLS connections with `cockroach` client CLI commands that use only SQL, e.g. `cockroach sql` or `cockroach node ls`.

knz · 2020-09-10T16:38:22Z

I'm going to go ahead and merge this as-is. We can polish further in a follow-up if needed.

bors r=bdarnell,irfansharif,aaron-crl

bdarnell

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @bdarnell, @irfansharif, and @knz)

pkg/cli/flags.go, line 696 at r3 (raw file):

Previously, knz (kena) wrote…

that doesn't seem to be something that is supported today by the postgres drivers

The drivers don't care about which configuration you give them. They are generally happy to pass passwords in-clear over unencrypted links. It's the user's responsibility to set sslmode to something else than disable. When sslmode is not set it is equivalent to auto and TLS is used if the server supports it. The driver in our SQL shell even probably knows how to do that, even though our command-line argument parsing does not recognize it yet.

My reason for the flag here is to prevent the user from shooting themselves in the foot; and also to satisfy our current users' expectation that CockroachDB will refuse to connect an insecure client to a secure server.

If you think we can do without the flag right now already (i.e. the user knows best) I'd be happy to remove it.
Maybe we don't even need this flag: when all clusters are secure by default, it won't matter any more.
If/when we remove it, we can make it a no-op in a deprecation cycle (i.e. it doesn't hurt).

But point granted. Marked the flag as experimental in its help text.

On further reflection, I think I'd get rid of the flag - if the user constructs a connection string with a password and disables SSL, that's enough of an indication of their intentions. Scram is not ubiquitous enough in the postgres world that people would assume non-cleartext passwords. (although maybe they'd at least expect md5 password exchange?)

knz · 2020-09-10T17:08:38Z

On further reflection, I think I'd get rid of the flag - if the user constructs a connection string with a password and disables SSL, that's enough of an indication of their intentions.

I got rid of the flag.

craig · 2020-09-10T18:28:38Z

Build succeeded:

GitHub CI (Cockroach)

knz requested review from bdarnell, irfansharif and aaron-crl September 5, 2020 10:16

knz requested a review from a team as a code owner September 5, 2020 10:16

knz mentioned this pull request Sep 5, 2020

pgwire: accept non-TLS client conns safely in secure mode #53991

Merged

knz force-pushed the 20200905-cli-pw branch 3 times, most recently from dafee96 to eca91d3 Compare September 7, 2020 14:09

knz changed the title ~~cli: allow commands to use password authn in more cases~~ cli: allow SQL commands to use password authn in more cases Sep 7, 2020

knz force-pushed the 20200905-cli-pw branch 2 times, most recently from 9418858 to 031ac82 Compare September 8, 2020 10:56

aaron-crl approved these changes Sep 8, 2020

View reviewed changes

irfansharif approved these changes Sep 8, 2020

View reviewed changes

bdarnell approved these changes Sep 8, 2020

View reviewed changes

knz commented Sep 8, 2020

View reviewed changes

bdarnell reviewed Sep 9, 2020

View reviewed changes

knz force-pushed the 20200905-cli-pw branch from 031ac82 to 1bd86b9 Compare September 10, 2020 12:54

knz commented Sep 10, 2020

View reviewed changes

knz force-pushed the 20200905-cli-pw branch from 1bd86b9 to 8d8b95d Compare September 10, 2020 13:00

knz force-pushed the 20200905-cli-pw branch from 8d8b95d to 62d7eec Compare September 10, 2020 13:55

knz force-pushed the 20200905-cli-pw branch from 62d7eec to 2a698b9 Compare September 10, 2020 15:06

knz force-pushed the 20200905-cli-pw branch from 2a698b9 to 57d264f Compare September 10, 2020 15:09

knz mentioned this pull request Sep 10, 2020

release-20.2: support opt-in non-TLS SQL client conns in secure clusters #54198

Merged

bdarnell reviewed Sep 10, 2020

View reviewed changes

craig bot merged commit 6227db0 into cockroachdb:master Sep 10, 2020

jseldess mentioned this pull request Sep 26, 2020

Support opt-in non-TLS SQL client conns in secure clusters cockroachdb/docs#8473

Closed

thtruo mentioned this pull request Sep 30, 2020

Enable an online transition of insecure clusters to secure mode #55074

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: allow SQL commands to use password authn in more cases #53994

cli: allow SQL commands to use password authn in more cases #53994

knz commented Sep 5, 2020 •

edited

Loading

cockroach-teamcity commented Sep 5, 2020

aaron-crl left a comment

aaron-crl Sep 8, 2020

irfansharif left a comment

bdarnell left a comment

knz left a comment

bdarnell left a comment

knz left a comment

knz commented Sep 10, 2020

knz commented Sep 10, 2020

knz commented Sep 10, 2020

bdarnell left a comment

knz commented Sep 10, 2020

craig bot commented Sep 10, 2020

cli: allow SQL commands to use password authn in more cases #53994

cli: allow SQL commands to use password authn in more cases #53994

Conversation

knz commented Sep 5, 2020 • edited Loading

cockroach-teamcity commented Sep 5, 2020

aaron-crl left a comment

Choose a reason for hiding this comment

aaron-crl Sep 8, 2020

Choose a reason for hiding this comment

irfansharif left a comment

Choose a reason for hiding this comment

bdarnell left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

bdarnell left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

knz commented Sep 10, 2020

knz commented Sep 10, 2020

knz commented Sep 10, 2020

bdarnell left a comment

Choose a reason for hiding this comment

knz commented Sep 10, 2020

craig bot commented Sep 10, 2020

knz commented Sep 5, 2020 •

edited

Loading