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

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 {

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.

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 {

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,17 +291,22 @@ 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"}, "", ""},
 		{anyCmd, []string{"--port=baz", "--url=postgresql://foo:12345"}, []string{"--host=foo", "--port=12345"}, "", ""},
 		{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).

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
 

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

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)

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")
 		}
 

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)
 		}