From 57d264f569c69bd4eb97d8406ac7d9d48f15e4a3 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Sat, 5 Sep 2020 12:10:04 +0200 Subject: [PATCH] cli: allow SQL commands to use password authn in more cases 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`. --- pkg/cli/client_url.go | 9 +- pkg/cli/context.go | 9 ++ pkg/cli/flags.go | 1 - pkg/cli/flags_test.go | 35 +++++-- .../test_url_db_override.tcl | 2 - pkg/cli/interactive_tests/test_url_login.tcl | 2 - pkg/cli/sql_util.go | 93 +++++++++++++++---- pkg/rpc/pg.go | 10 +- pkg/sql/pgwire/server.go | 4 +- 9 files changed, 130 insertions(+), 35 deletions(-) diff --git a/pkg/cli/client_url.go b/pkg/cli/client_url.go index 1d91f3d45b2d..cd63a1e1c99c 100644 --- a/pkg/cli/client_url.go +++ b/pkg/cli/client_url.go @@ -190,8 +190,13 @@ func (u urlParser) setInternal(v string, warn bool) error { switch sslMode := options.Get("sslmode"); sslMode { case "", "disable": - if err := fl.Set(cliflags.ClientInsecure.Name, "true"); err != nil { - return errors.Wrapf(err, "setting insecure connection based on --url") + if u.sslStrict { + // For "strict" mode (RPC client commands) we don't support non-TLS + // yet. See https://github.com/cockroachdb/cockroach/issues/54007 + // Instead, we see a request for no TLS to imply insecure mode. + if err := fl.Set(cliflags.ClientInsecure.Name, "true"); err != nil { + return errors.Wrapf(err, "setting secure connection based on --url") + } } case "require", "verify-ca", "verify-full": if sslMode != "verify-full" && u.sslStrict { diff --git a/pkg/cli/context.go b/pkg/cli/context.go index c5f33aca5b65..9a8078cb062f 100644 --- a/pkg/cli/context.go +++ b/pkg/cli/context.go @@ -154,6 +154,14 @@ type cliContext struct { // extraConnURLOptions contains any additional query URL options // specified in --url that do not have discrete equivalents. extraConnURLOptions url.Values + + // allowUnencryptedClientPassword enables the CLI commands to use + // password authentication over non-TLS TCP connections. This is + // disallowed by default: the user must opt-in and understand that + // CockroachDB does not guarantee confidentiality of a password + // provided this way. + // TODO(knz): Relax this when SCRAM is implemented. + allowUnencryptedClientPassword bool } // cliCtx captures the command-line parameters common to most CLI utilities. @@ -184,6 +192,7 @@ func setCliContextDefaults() { cliCtx.sqlConnPasswd = "" cliCtx.sqlConnDBName = "" cliCtx.extraConnURLOptions = nil + cliCtx.allowUnencryptedClientPassword = false } // sqlCtx captures the command-line parameters of the `sql` command. diff --git a/pkg/cli/flags.go b/pkg/cli/flags.go index eae7b48702bb..17e816866291 100644 --- a/pkg/cli/flags.go +++ b/pkg/cli/flags.go @@ -692,7 +692,6 @@ func init() { _ = f.MarkHidden(cliflags.ClientHost.Name) stringFlag(f, &cliCtx.clientConnPort, cliflags.ClientPort) _ = f.MarkHidden(cliflags.ClientPort.Name) - } if cmd == sqlShellCmd { diff --git a/pkg/cli/flags_test.go b/pkg/cli/flags_test.go index 5d1483ab3e60..cedde7add8c2 100644 --- a/pkg/cli/flags_test.go +++ b/pkg/cli/flags_test.go @@ -274,8 +274,12 @@ func TestClientURLFlagEquivalence(t *testing.T) { {anyNonSQL, []string{"--url=postgresql://b:12345"}, []string{"--host=b", "--port=12345"}, "", ""}, {anyNonSQL, []string{"--url=postgresql://b:c"}, nil, `invalid port ":c" after host`, ""}, - {anyCmd, []string{"--url=postgresql://foo?application_name=abc"}, []string{"--host=foo", "--insecure"}, "", ""}, - {anyCmd, []string{"--url=postgresql://foo?sslmode=disable"}, []string{"--host=foo", "--insecure"}, "", ""}, + {anyNonSQL, []string{"--url=postgresql://foo?application_name=abc"}, []string{"--host=foo", "--insecure"}, "", ""}, + {anySQL, []string{"--url=postgresql://foo?application_name=abc"}, []string{"--host=foo"}, "", ""}, + + {anyNonSQL, []string{"--url=postgresql://foo?sslmode=disable"}, []string{"--host=foo", "--insecure"}, "", ""}, + {anySQL, []string{"--url=postgresql://foo?sslmode=disable"}, []string{"--host=foo"}, "", ""}, + {anySQL, []string{"--url=postgresql://foo?sslmode=require"}, []string{"--host=foo", "--insecure=false"}, "", ""}, {anyNonSQL, []string{"--url=postgresql://foo?sslmode=require"}, nil, "command .* only supports sslmode=disable or sslmode=verify-full", ""}, {anyCmd, []string{"--url=postgresql://foo?sslmode=verify-full"}, []string{"--host=foo", "--insecure=false"}, "", ""}, @@ -287,8 +291,10 @@ func TestClientURLFlagEquivalence(t *testing.T) { {anyCmd, []string{"--port=baz", "--url=postgresql://foo"}, []string{"--host=foo", "--port=baz"}, "", `invalid port ":baz" after host`}, {sqlShell, []string{"--database=baz", "--url=postgresql://foo"}, []string{"--host=foo", "--database=baz"}, "", ""}, {anySQL, []string{"--user=baz", "--url=postgresql://foo"}, []string{"--host=foo", "--user=baz"}, "", ""}, + {anyCmd, []string{"--insecure=false", "--url=postgresql://foo"}, []string{"--host=foo", "--insecure=false"}, "", ""}, - {anyCmd, []string{"--insecure", "--url=postgresql://foo"}, []string{"--host=foo", "--insecure"}, "", ""}, + // Only non-SQL lets --insecure bleed into a URL that does not specify sslmode. + {anyNonSQL, []string{"--insecure", "--url=postgresql://foo"}, []string{"--host=foo", "--insecure"}, "", ""}, // URL overrides previous flags if component specified. {anyCmd, []string{"--host=baz", "--url=postgresql://bar"}, []string{"--host=bar"}, "", ""}, @@ -296,8 +302,11 @@ func TestClientURLFlagEquivalence(t *testing.T) { {anyCmd, []string{"--port=baz", "--url=postgresql://foo:bar"}, nil, `invalid port ":bar" after host`, ""}, {sqlShell, []string{"--database=baz", "--url=postgresql://foo/bar"}, []string{"--host=foo", "--database=bar"}, "", ""}, {anySQL, []string{"--user=baz", "--url=postgresql://bar@foo"}, []string{"--host=foo", "--user=bar"}, "", ""}, - {anyCmd, []string{"--insecure=false", "--url=postgresql://foo?sslmode=disable"}, []string{"--host=foo", "--insecure"}, "", ""}, + + {anyNonSQL, []string{"--insecure=false", "--url=postgresql://foo?sslmode=disable"}, []string{"--host=foo", "--insecure"}, "", ""}, {anyCmd, []string{"--insecure", "--url=postgresql://foo?sslmode=verify-full"}, []string{"--host=foo", "--insecure=false"}, "", ""}, + // SQL is special case: specifying sslmode= does not imply insecure mode. So the insecure bit does not get reset. + {anySQL, []string{"--insecure=false", "--url=postgresql://foo?sslmode=disable"}, []string{"--host=foo"}, "", ""}, // Discrete flag overrides URL if specified afterwards. {anyCmd, []string{"--url=postgresql://bar", "--host=baz"}, []string{"--host=baz"}, "", ""}, @@ -306,8 +315,9 @@ func TestClientURLFlagEquivalence(t *testing.T) { {anyCmd, []string{"--url=postgresql://foo:bar", "--port=baz"}, nil, `invalid port ":bar" after host`, ""}, {sqlShell, []string{"--url=postgresql://foo/bar", "--database=baz"}, []string{"--host=foo", "--database=baz"}, "", ""}, {anySQL, []string{"--url=postgresql://bar@foo", "--user=baz"}, []string{"--host=foo", "--user=baz"}, "", ""}, - {anyCmd, []string{"--url=postgresql://foo?sslmode=disable", "--insecure=false"}, []string{"--host=foo", "--insecure=false"}, "", ""}, - {anyCmd, []string{"--url=postgresql://foo?sslmode=verify-full", "--insecure"}, []string{"--host=foo", "--insecure"}, "", ""}, + + {anyNonSQL, []string{"--url=postgresql://foo?sslmode=disable", "--insecure=false"}, []string{"--host=foo", "--insecure=false"}, "", ""}, + {anyNonSQL, []string{"--url=postgresql://foo?sslmode=verify-full", "--insecure"}, []string{"--host=foo", "--insecure"}, "", ""}, // Check that the certs dir is extracted properly. {anyNonSQL, []string{"--url=postgresql://foo?sslmode=verify-full&sslrootcert=" + testCertsDirPath + "/ca.crt"}, []string{"--host=foo", "--certs-dir=" + testCertsDirPath}, "", ""}, @@ -346,9 +356,9 @@ func TestClientURLFlagEquivalence(t *testing.T) { } } - for _, test := range testData { + for testNum, test := range testData { for _, cmdName := range test.cmds { - t.Run(fmt.Sprintf("%s/%s", cmdName, strings.Join(test.flags, " ")), func(t *testing.T) { + t.Run(fmt.Sprintf("%d/%s/%s", testNum+1, cmdName, strings.Join(test.flags, " ")), func(t *testing.T) { cmd, _, _ := cockroachCmd.Find([]string{cmdName}) // Parse using the URL. @@ -388,6 +398,15 @@ func TestClientURLFlagEquivalence(t *testing.T) { t.Fatalf("mismatch: URL %q parses\n%+v,\ndiscrete parses\n%+v", resultURL, urlParams, discreteParams) } + // For SQL commands only, test that reconstructing the URL + // from discrete flags yield equivalent connection parameters. + // (RPC commands never reconstruct a URL.) + for _, s := range anyNonSQL { + if cmdName == s { + return + } + } + // Re-parse using the derived URL. // We'll want to ensure below that the derived URL specifies the same parameters // (i.e. check makeClientConnURL does its work properly). diff --git a/pkg/cli/interactive_tests/test_url_db_override.tcl b/pkg/cli/interactive_tests/test_url_db_override.tcl index e77b7772a1df..70699f5e5cae 100644 --- a/pkg/cli/interactive_tests/test_url_db_override.tcl +++ b/pkg/cli/interactive_tests/test_url_db_override.tcl @@ -32,8 +32,6 @@ start_test "Check that the insecure flag overrides the sslmode if URL is already set ::env(COCKROACH_INSECURE) "false" spawn $argv sql --url "postgresql://test@localhost:26257?sslmode=verify-full" --certs-dir=$certs_dir -e "select 1" -eexpect "password:" -send "\r" eexpect "SSL is not enabled on the server" eexpect eof diff --git a/pkg/cli/interactive_tests/test_url_login.tcl b/pkg/cli/interactive_tests/test_url_login.tcl index 18c54393d547..6dee23587ff1 100644 --- a/pkg/cli/interactive_tests/test_url_login.tcl +++ b/pkg/cli/interactive_tests/test_url_login.tcl @@ -26,8 +26,6 @@ system "$argv start-single-node --insecure --pid-file=server_pid --socket-dir=. $argv sql --insecure -e 'select 1'" spawn $argv sql --url "postgresql://?host=$mywd&port=26257" -eexpect "Enter password" -send "insecure\r" eexpect root@ send_eof eexpect eof diff --git a/pkg/cli/sql_util.go b/pkg/cli/sql_util.go index 5529b73c6e37..d700763dc3dc 100644 --- a/pkg/cli/sql_util.go +++ b/pkg/cli/sql_util.go @@ -58,6 +58,9 @@ type sqlConn struct { conn sqlConnI reconnecting bool + // passwordMissing is true iff the url is missing a password. + passwordMissing bool + pendingNotices []*pq.Error // delayNotices, if set, makes notices accumulate for printing @@ -152,6 +155,26 @@ func (c *sqlConn) ensureConn() error { // connections only once instead. The context is only used for dialing. conn, err := connector.Connect(context.TODO()) if err != nil { + // Connection failed: if the failure is due to a mispresented + // password, we're going to fill the password here. + // + // TODO(knz): CockroachDB servers do not properly fill SQLSTATE + // (28P01) for password auth errors, so we have to "make do" + // with a string match. This should be cleaned up by adding + // the missing code server-side. + errStr := strings.TrimPrefix(err.Error(), "pq: ") + if strings.HasPrefix(errStr, "password authentication failed") && c.passwordMissing { + if pErr := c.fillPassword(); pErr != nil { + return errors.CombineErrors(err, pErr) + } + // Recurse, once. We recurse to ensure that pq.NewConnector + // and ConnectorWithNoticeHandler get called with the new URL. + // The recursion only occurs once because fillPassword() + // resets c.passwordMissing, so we cannot get into this + // conditional a second time. + return c.ensureConn() + } + // Not a password auth error, or password already set. Simply fail. return wrapConnError(err) } if c.reconnecting && c.dbName != "" { @@ -665,23 +688,33 @@ func makeSQLClient(appName string, defaultMode defaultSQLDb) (*sqlConn, error) { return nil, err } - // Insecure connections are insecure and should never see a password. Reject - // one that may be present in the URL already. - if options.Get("sslmode") == "disable" { - if _, pwdSet := baseURL.User.Password(); pwdSet { - return nil, errors.Errorf("cannot specify a password in URL with an insecure connection") + // tcpConn is true iff the connection is going over the network. + tcpConn := baseURL.Host != "" + + // If there is no TLS mode yet, conjure one based on defaults. + if options.Get("sslmode") == "" { + if cliCtx.Insecure { + options.Set("sslmode", "disable") + } else if tcpConn { + options.Set("sslmode", "verify-full") } - } else { - if options.Get("sslcert") == "" || options.Get("sslkey") == "" { - // If there's no password in the URL yet and we don't have a client - // certificate, ask for it and populate it in the URL. - if _, pwdSet := baseURL.User.Password(); !pwdSet { - pwd, err := security.PromptForPassword() - if err != nil { - return nil, err - } - baseURL.User = url.UserPassword(baseURL.User.Username(), pwd) - } + // (We don't use TLS over unix socket conns.) + } + + // Prevent explicit TLS request in insecure mode. + if cliCtx.Insecure && options.Get("sslmode") != "disable" { + return nil, errors.Errorf("cannot use TLS connections in insecure mode") + } + + // How we're going to authenticate. + _, pwdSet := baseURL.User.Password() + if pwdSet { + // There's a password already configured. + + // In insecure mode, we don't want the user to get the mistaken + // idea that a password is worth anything. + if cliCtx.Insecure { + return nil, errors.Errorf("password authentication not enabled in insecure mode") } } @@ -707,7 +740,33 @@ func makeSQLClient(appName string, defaultMode defaultSQLDb) (*sqlConn, error) { log.Infof(context.Background(), "connecting with URL: %s", sqlURL) } - return makeSQLConn(sqlURL), nil + conn := makeSQLConn(sqlURL) + + conn.passwordMissing = !pwdSet + + return conn, nil +} + +// fillPassword is called the first time the server complains that the +// password authentication has failed, if no password was supplied to +// start with. It asks the user for a password interactively. +func (c *sqlConn) fillPassword() error { + connURL, err := url.Parse(c.url) + if err != nil { + return err + } + + // Password can be safely encrypted, or the user opted in + // manually to non-encryption. All good. + + pwd, err := security.PromptForPassword() + if err != nil { + return err + } + connURL.User = url.UserPassword(connURL.User.Username(), pwd) + c.url = connURL.String() + c.passwordMissing = false + return nil } type queryFunc func(conn *sqlConn) (rows *sqlRows, isMultiStatementQuery bool, err error) diff --git a/pkg/rpc/pg.go b/pkg/rpc/pg.go index 4e3503731c3c..e16f0c9cbde2 100644 --- a/pkg/rpc/pg.go +++ b/pkg/rpc/pg.go @@ -26,7 +26,15 @@ func (ctx *SecurityContext) LoadSecurityOptions(options url.Values, username str options.Del("sslkey") } else { sslMode := options.Get("sslmode") - if sslMode == "" || sslMode == "disable" { + if sslMode == "disable" { + // TLS explicitly disabled by client. Nothing to do here. + options.Del("sslrootcert") + options.Del("sslcert") + options.Del("sslkey") + return nil + } + // Default is to verify the server's identity. + if sslMode == "" { options.Set("sslmode", "verify-full") } diff --git a/pkg/sql/pgwire/server.go b/pkg/sql/pgwire/server.go index f0cc26c730c2..e361b963b447 100644 --- a/pkg/sql/pgwire/server.go +++ b/pkg/sql/pgwire/server.go @@ -698,8 +698,8 @@ func (s *Server) maybeUpgradeToSecureConn( return } - // Secure mode: disallow if TCP and the user did not opt into SQL - // conns. + // Secure mode: disallow if TCP and the user did not opt into + // non-TLS SQL conns. if !s.cfg.AcceptSQLWithoutTLS && connType != hba.ConnLocal { clientErr = pgerror.New(pgcode.ProtocolViolation, ErrSSLRequired) }