cli: add --cert-principal-map to client commands

Add support for the `--cert-principal-map` flag to the certs and client commands. Anywhere we were accepting the `--certs-dir` flag, we now also accept the `--cert-principal-map` flag. Fixes cockroachdb#47300 Fixes cockroachdb#47754 Fixes cockroachdb#48116 Release note (cli change): Support the `--cert-principal-map` flag in the `cert *` and "client" commands such as `sql`, `init`, and `quit`.
petermattis · May 2, 2020 · b893b2c · b893b2c
1 parent 6140917
commit b893b2c
Show file tree

Hide file tree

Showing 6 changed files with 52 additions and 26 deletions.
diff --git a/pkg/cli/cert.go b/pkg/cli/cert.go
@@ -203,9 +203,6 @@ List certificates and keys found in the certificate directory.
 
 // runListCerts loads and lists all certs.
 func runListCerts(cmd *cobra.Command, args []string) error {
-	if err := security.SetCertPrincipalMap(certCtx.certPrincipalMap); err != nil {
-		return err
-	}
 	cm, err := security.NewCertificateManager(baseCfg.SSLCertsDir)
 	if err != nil {
 		return errors.Wrap(err, "cannot load certificates")

diff --git a/pkg/cli/context.go b/pkg/cli/context.go
@@ -77,6 +77,7 @@ func initCLIDefaults() {
 	cliCtx.cmdTimeout = 0 // no timeout
 	cliCtx.clientConnHost = ""
 	cliCtx.clientConnPort = base.DefaultPort
+	cliCtx.certPrincipalMap = nil
 	cliCtx.sqlConnURL = ""
 	cliCtx.sqlConnUser = ""
 	cliCtx.sqlConnPasswd = ""
@@ -171,8 +172,6 @@ func initCLIDefaults() {
 
 	authCtx.validityPeriod = 1 * time.Hour
 
-	certCtx.certPrincipalMap = nil
-
 	initPreFlagsDefaults()
 
 	// Clear the "Changed" state of all the registered command-line flags.
@@ -215,6 +214,9 @@ type cliContext struct {
 	// clientConnPort is the port name/number to use to connect to a server.
 	clientConnPort string
 
+	// certPrincipalMap is the cert-principal:db-principal map.
+	certPrincipalMap []string
+
 	// for CLI commands that use the SQL interface, these parameters
 	// determine how to connect to the server.
 	sqlConnURL, sqlConnUser, sqlConnDBName string
@@ -396,9 +398,3 @@ var demoCtx struct {
 	transientCluster          *transientCluster
 	insecure                  bool
 }
-
-// certCtx captures the command-line parameters of the `cert` command.
-// Defaults set by InitCLIDefaults() above.
-var certCtx struct {
-	certPrincipalMap []string
-}
diff --git a/pkg/cli/flags.go b/pkg/cli/flags.go
@@ -240,7 +240,9 @@ func init() {
 
 	// Every command but start will inherit the following setting.
 	AddPersistentPreRunE(cockroachCmd, func(cmd *cobra.Command, _ []string) error {
-		extraClientFlagInit()
+		if err := extraClientFlagInit(); err != nil {
+			return err
+		}
 		return setDefaultStderrVerbosity(cmd, log.Severity_WARNING)
 	})
 
@@ -440,12 +442,11 @@ func init() {
 		f := cmd.Flags()
 		// All certs commands need the certificate directory.
 		StringFlag(f, &baseCfg.SSLCertsDir, cliflags.CertsDir, baseCfg.SSLCertsDir)
+		// All certs commands get the certificate principal map.
+		StringSlice(f, &cliCtx.certPrincipalMap,
+			cliflags.CertPrincipalMap, cliCtx.certPrincipalMap)
 	}
 
-	// The list certs command needs the certificate principal map.
-	StringSlice(listCertsCmd.Flags(), &certCtx.certPrincipalMap,
-		cliflags.CertPrincipalMap, certCtx.certPrincipalMap)
-
 	for _, cmd := range []*cobra.Command{createCACertCmd, createClientCACertCmd} {
 		f := cmd.Flags()
 		// CA certificates have a longer expiration time.
@@ -494,6 +495,9 @@ func init() {
 
 		// Certificate flags.
 		StringFlag(f, &baseCfg.SSLCertsDir, cliflags.CertsDir, baseCfg.SSLCertsDir)
+		// Certificate principal map.
+		StringSlice(f, &cliCtx.certPrincipalMap,
+			cliflags.CertPrincipalMap, cliCtx.certPrincipalMap)
 	}
 
 	// Auth commands.
@@ -875,7 +879,10 @@ func extraServerFlagInit(cmd *cobra.Command) error {
 	return nil
 }
 
-func extraClientFlagInit() {
+func extraClientFlagInit() error {
+	if err := security.SetCertPrincipalMap(cliCtx.certPrincipalMap); err != nil {
+		return err
+	}
 	serverCfg.Addr = net.JoinHostPort(cliCtx.clientConnHost, cliCtx.clientConnPort)
 	serverCfg.AdvertiseAddr = serverCfg.Addr
 	serverCfg.SQLAddr = net.JoinHostPort(cliCtx.clientConnHost, cliCtx.clientConnPort)
@@ -891,6 +898,7 @@ func extraClientFlagInit() {
 	if sqlCtx.debugMode {
 		sqlCtx.echo = true
 	}
+	return nil
 }
 
 func setDefaultStderrVerbosity(cmd *cobra.Command, defaultSeverity log.Severity) error {

diff --git a/pkg/cli/flags_test.go b/pkg/cli/flags_test.go
@@ -799,7 +799,9 @@ func TestServerJoinSettings(t *testing.T) {
 			t.Fatalf("Parse(%#v) got unexpected error: %v", td.args, err)
 		}
 
-		extraClientFlagInit()
+		if err := extraClientFlagInit(); err != nil {
+			t.Fatal(err)
+		}
 
 		var actual []string
 		myHostname, _ := os.Hostname()
@@ -861,7 +863,9 @@ func TestClientConnSettings(t *testing.T) {
 			t.Fatalf("Parse(%#v) got unexpected error: %v", td.args, err)
 		}
 
-		extraClientFlagInit()
+		if err := extraClientFlagInit(); err != nil {
+			t.Fatal(err)
+		}
 		if td.expectedAddr != serverCfg.Addr {
 			t.Errorf("%d. serverCfg.Addr expected '%s', but got '%s'. td.args was '%#v'.",
 				i, td.expectedAddr, serverCfg.Addr, td.args)

diff --git a/pkg/cli/interactive_tests/test_cert_advisory_validation.tcl b/pkg/cli/interactive_tests/test_cert_advisory_validation.tcl
@@ -11,7 +11,8 @@ set prompt ":/# "
 eexpect $prompt
 
 # create some cert without an IP address in there.
-set certs_dir "./my-safe-directory"
+set db_dir "logs/db"
+set certs_dir "logs/my-safe-directory"
 send "mkdir -p $certs_dir\r"
 eexpect $prompt
 
@@ -21,7 +22,7 @@ send "$argv cert create-node localhost --certs-dir=$certs_dir --ca-key=$certs_di
 eexpect $prompt
 
 start_test "Check that the server reports a warning if attempting to advertise an IP address not in cert."
-send "$argv start-single-node --certs-dir=$certs_dir --advertise-addr=127.0.0.1\r"
+send "$argv start-single-node --store=$db_dir --certs-dir=$certs_dir --advertise-addr=127.0.0.1\r"
 eexpect "advertise address"
 eexpect "127.0.0.1"
 eexpect "not in node certificate"
@@ -32,7 +33,7 @@ eexpect $prompt
 end_test
 
 start_test "Check that the server reports no warning if the avertise addr is in the cert."
-send "$argv start-single-node --certs-dir=$certs_dir --advertise-addr=localhost\r"
+send "$argv start-single-node --store=$db_dir --certs-dir=$certs_dir --advertise-addr=localhost\r"
 expect {
   "not in node certificate" {
      report "unexpected warning"
@@ -51,21 +52,21 @@ send "COCKROACH_CERT_NODE_USER=foo.bar $argv cert create-node localhost --certs-
 eexpect $prompt
 
 start_test "Check that the server reports an error if the node cert does not contain a node principal."
-send "$argv start-single-node --certs-dir=$certs_dir --advertise-addr=localhost\r"
+send "$argv start-single-node --store=$db_dir --certs-dir=$certs_dir --advertise-addr=localhost\r"
 eexpect "cannot load certificates"
 expect $prompt
 end_test
 
 start_test "Check that the cert principal map can allow the use of non-standard cert principal."
-send "$argv start-single-node --certs-dir=$certs_dir --cert-principal-map=foo.bar:node --advertise-addr=localhost\r"
+send "$argv start-single-node --store=$db_dir --certs-dir=$certs_dir --cert-principal-map=foo.bar:node --advertise-addr=localhost\r"
 eexpect "node starting"
 interrupt
 eexpect "interrupted"
 expect $prompt
 end_test
 
 start_test "Check that the cert principal map can allow the use of a SAN principal."
-send "$argv start-single-node --certs-dir=$certs_dir --cert-principal-map=localhost:node --advertise-addr=localhost\r"
+send "$argv start-single-node --store=$db_dir --certs-dir=$certs_dir --cert-principal-map=localhost:node --advertise-addr=localhost\r"
 eexpect "node starting"
 interrupt
 eexpect "interrupted"
@@ -77,3 +78,23 @@ send "$argv cert list --certs-dir=$certs_dir --cert-principal-map=foo.bar:node\r
 eexpect "Certificate directory:"
 expect $prompt
 end_test
+
+start_test "Check that 'cert create-client' can utilize cert principal map."
+send "$argv cert create-client root.crdb.io --certs-dir=$certs_dir --ca-key=$certs_dir/ca.key --cert-principal-map=foo.bar:node\r"
+eexpect $prompt
+send "mv $certs_dir/client.root.crdb.io.crt $certs_dir/client.root.crt; mv $certs_dir/client.root.crdb.io.key $certs_dir/client.root.key\r"
+eexpect $prompt
+end_test
+
+start_test "Check that the client commands can use cert principal map."
+system "$argv start-single-node --store=$db_dir --certs-dir=$certs_dir --cert-principal-map=foo.bar:node,root.crdb.io:root --advertise-addr=localhost --background >>expect-cmd.log 2>&1"
+send "$argv sql --certs-dir=$certs_dir --cert-principal-map=foo.bar:node,root.crdb.io:root -e \"select 'hello'\"\r"
+eexpect "hello"
+expect $prompt
+send "$argv node ls --certs-dir=$certs_dir --cert-principal-map=foo.bar:node,root.crdb.io:root\r"
+eexpect "1 row"
+expect $prompt
+send "$argv quit --certs-dir=$certs_dir --cert-principal-map=foo.bar:node,root.crdb.io:root\r"
+eexpect "ok"
+expect $prompt
+end_test
diff --git a/pkg/server/server.go b/pkg/server/server.go
@@ -271,7 +271,7 @@ func NewServer(cfg Config, stopper *stop.Stopper) (*Server, error) {
 			return nil, err
 		}
 		cm.RegisterSignalHandler(stopper)
-		registry.AddMetricStruct(cm.Metrics())
+		s.registry.AddMetricStruct(cm.Metrics())
 	}
 
 	// Add a dynamic log tag value for the node ID.