Address review comments

pkg/cli/cert.go

@@ -75,7 +75,7 @@ var createClientCACertCmd = &cobra.Command{
 	Use:   "create-client-ca --certs-dir=<path to cockroach certs dir> --ca-key=<path-to-client-ca-key>",
 	Short: "create client CA certificate and key",
 	Long: `
-Generate a client CA certificate "<certs-dir>/ca.client.crt" and CA key "<client-ca-key>".
+Generate a client CA certificate "<certs-dir>/ca-client.crt" and CA key "<client-ca-key>".
 The certs directory is created if it does not exist.
 
 If the CA key exists and --allow-ca-key-reuse is true, the key is used.

pkg/security/certificate_loader.go

@@ -171,21 +171,16 @@ func CertInfoFromFilename(filename string) (*CertInfo, error) {
 	prefix := parts[0]
 	switch parts[0] {
 	case `ca`:
-		if numParts == 2 {
-			// This is main CA certificate.
-			fileUsage = CAPem
-		} else if numParts == 3 {
-			switch parts[1] {
-			case `client`:
-				// CA for client certificates.
-				fileUsage = ClientCAPem
-			default:
-				return nil, errors.Errorf("CA certificate filename must be one of ca%s, ca.client%s",
-					certExtension, certExtension)
-			}
-		} else {
-			return nil, errors.Errorf("CA certificate filename must be one of ca%s, ca.client%s",
-				certExtension, certExtension)
+		// This is main CA certificate.
+		fileUsage = CAPem
+		if numParts != 2 {
+			return nil, errors.Errorf("CA certificate filename should match ca%s", certExtension)
+		}
+	case `ca-client`:
+		// This is client CA certificate.
+		fileUsage = ClientCAPem
+		if numParts != 2 {
+			return nil, errors.Errorf("client CA certificate filename should match ca-client%s", certExtension)
 		}
 	case `node`:
 		fileUsage = NodePem

pkg/security/certificate_loader_test.go

@@ -50,7 +50,7 @@ func TestCertNomenclature(t *testing.T) {
 	}{
 		// Test valid names.
 		{"ca.crt", "", security.CAPem, ""},
-		{"ca.client.crt", "", security.ClientCAPem, ""},
+		{"ca-client.crt", "", security.ClientCAPem, ""},
 		{"node.crt", "", security.NodePem, ""},
 		{"client.root.crt", "", security.ClientPem, "root"},
 		{"client.foo-bar.crt", "", security.ClientPem, "foo-bar"},
@@ -60,9 +60,7 @@ func TestCertNomenclature(t *testing.T) {
 		{"crt", "not enough parts found", 0, ""},
 		{".crt", "unknown prefix", 0, ""},
 		{"ca2.crt", "unknown prefix \"ca2\"", 0, ""},
-		{"ca.foo.crt", "CA certificate filename must be one of ca.crt, ca.client.crt", 0, ""},
-		{"ca.clients.crt", "CA certificate filename must be one of ca.crt, ca.client.crt", 0, ""},
-		{"ca.ui.crt", "CA certificate filename must be one of ca.crt, ca.client.crt", 0, ""},
+		{"ca.client.crt", "CA certificate filename should match ca.crt", 0, ""},
 		{"node2.crt", "unknown prefix \"node2\"", 0, ""},
 		{"node.foo.crt", "node certificate filename should match node.crt", 0, ""},
 		{"client2.crt", "unknown prefix \"client2\"", 0, ""},

pkg/security/certificate_manager.go

@@ -61,15 +61,15 @@ var (
 // The nomenclature for certificates is as follows, all within the certs-dir.
 // - ca.crt             main CA certificate.
 //                      Used to verify everything unless overridden by more specifica CAs.
-// - ca.client.crt      CA certificate to verify client certificates.
+// - ca-client.crt      CA certificate to verify client certificates.
 //                      Used **IF AND ONLY IF** client.node.crt exists.
 // - node.crt           node certificate.
 //                      Server-side certificate (always) and client-side certificate unless
 //                      client.node.crt is found.
 //                      Verified using 'ca.crt'.
-// - client.<user>.crt  client certificate for 'user'. Verified using 'ca.crt', or 'ca.client.crt'.
+// - client.<user>.crt  client certificate for 'user'. Verified using 'ca.crt', or 'ca-client.crt'.
 // - client.node.crt    client certificate for the 'node' user. If this file exists,
-//                      we always use 'ca.client.crt' to verify client certificates.
+//                      we always use 'ca-client.crt' to verify client certificates.
 type CertificateManager struct {
 	// Certificate directory is not modified after initialization.
 	certsDir string
@@ -171,7 +171,7 @@ func (cm *CertificateManager) CACertPath() string {
 // ClientCACertPath returns the expected file path for the CA certificate
 // used to verify client certificates.
 func (cm *CertificateManager) ClientCACertPath() string {
-	return filepath.Join(cm.certsDir, "ca.client"+certExtension)
+	return filepath.Join(cm.certsDir, "ca-client"+certExtension)
 }
 
 // NodeCertPath returns the expected file path for the node certificate.

pkg/security/certs.go

@@ -100,7 +100,7 @@ func CreateClientCAPair(
 // The filename of the certificate file must be specified.
 // It should be one of:
 // - ca.crt: the general CA certificate
-// - ca.client.crt: the CA certificate to verify client certificates
+// - ca-client.crt: the CA certificate to verify client certificates
 func createCACertAndKey(
 	certsDir, caKeyPath string,
 	caType PemUsage,

pkg/security/certs_test.go

@@ -12,8 +12,6 @@
 // implied. See the License for the specific language governing
 // permissions and limitations under the License.
 
-// +build !windows
-
 package security_test
 
 import (
@@ -33,7 +31,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/pkg/errors"
-	"golang.org/x/sys/unix"
 )
 
 func TestGenerateCACert(t *testing.T) {
@@ -183,10 +180,10 @@ func generateBaseCerts(certsDir string) error {
 
 // Generate certificates with separate CAs:
 // ca.crt: CA certificate
-// ca.client.crt: CA certificate to verify client certs
+// ca-client.crt: CA certificate to verify client certs
 // node.crt: node server cert: signed by ca.crt
-// client.node.crt: node client cert: signed by ca.client.crt
-// client.root.crt: root client cert: signed by ca.client.crt
+// client.node.crt: node client cert: signed by ca-client.crt
+// client.root.crt: root client cert: signed by ca-client.crt
 func generateSplitCACerts(certsDir string) error {
 	if err := security.CreateCAPair(
 		certsDir, filepath.Join(certsDir, security.EmbeddedCAKey),
@@ -406,7 +403,7 @@ func TestUseSplitCACerts(t *testing.T) {
 		t.Error(err)
 	}
 
-	// Test a SQL client with correct certs usage.
+	// Test a SQL client with various certificates.
 	testCases := []struct {
 		user, caName, certPrefix string
 		expectedError            string
@@ -434,37 +431,94 @@ func TestUseSplitCACerts(t *testing.T) {
 			t.Errorf("#%d: expected error %v, got %v", i, tc.expectedError, err)
 		}
 	}
+}
+
+// This is a fairly high-level test of CA and node certificates.
+// We construct SSL server and clients and use the generated certs.
+func TestUseWrongSplitCACerts(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	// Do not mock cert access for this test.
+	security.ResetAssetLoader()
+	defer ResetTest()
+	certsDir, err := ioutil.TempDir("", "certs_test")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() {
+		if err := os.RemoveAll(certsDir); err != nil {
+			t.Fatal(err)
+		}
+	}()
+
+	if err := generateSplitCACerts(certsDir); err != nil {
+		t.Fatal(err)
+	}
 
+	// Delete the ca-client.crt before starting the node.
+	// This will make the server fall back on using ca.crt.
 	// Now remove the node client cert which forces the server to use the general certs.
 	if err := os.Remove(filepath.Join(certsDir, "client.node.crt")); err != nil {
 		t.Fatal(err)
 	}
 
-	// Trigger certificate rotation.
-	t.Log("issuing SIGHUP")
-	if err := unix.Kill(unix.Getpid(), unix.SIGHUP); err != nil {
+	// Start a test server and override certs.
+	// We use a real context since we want generated certs.
+	// Web session authentication is disabled in order to avoid the need to
+	// authenticate the individual clients being instantiated (session auth has
+	// no effect on what is being tested here).
+	params := base.TestServerArgs{
+		SSLCertsDir:                     certsDir,
+		DisableWebSessionAuthentication: true,
+	}
+	s, _, db := serverutils.StartServer(t, params)
+	defer s.Stopper().Stop(context.TODO())
+
+	// Insecure mode.
+	clientContext := testutils.NewNodeTestBaseContext()
+	clientContext.Insecure = true
+	httpClient, err := clientContext.GetHTTPClient()
+	if err != nil {
 		t.Fatal(err)
 	}
+	req, err := http.NewRequest("GET", s.AdminURL()+"/_status/metrics/local", nil)
+	if err != nil {
+		t.Fatalf("could not create request: %v", err)
+	}
+	resp, err := httpClient.Do(req)
+	if err == nil {
+		defer resp.Body.Close()
+		body, _ := ioutil.ReadAll(resp.Body)
+		t.Fatalf("Expected SSL error, got success: %s", body)
+	}
+
+	// New client. With certs this time.
+	clientContext = testutils.NewNodeTestBaseContext()
+	clientContext.SSLCertsDir = certsDir
+	httpClient, err = clientContext.GetHTTPClient()
+	if err != nil {
+		t.Fatalf("Expected success, got %v", err)
+	}
+	req, err = http.NewRequest("GET", s.AdminURL()+"/_status/metrics/local", nil)
+	if err != nil {
+		t.Fatalf("could not create request: %v", err)
+	}
+	resp, err = httpClient.Do(req)
+	if err != nil {
+		t.Fatalf("Expected success, got %v", err)
+	}
+	defer resp.Body.Close()
+	if resp.StatusCode != http.StatusOK {
+		body, _ := ioutil.ReadAll(resp.Body)
+		t.Fatalf("Expected OK, got %q with body: %s", resp.Status, body)
+	}
 
-	// Try again, the first client should now fail, the second should succeed.
-	testutils.SucceedsSoon(t,
-		func() error {
-			pgUrl := makeSecurePGUrl(s.ServingAddr(), "root", certsDir, "ca.crt", "client.root.crt", "client.root.key")
-			goDB, err := gosql.Open("postgres", pgUrl)
-			if err != nil {
-				t.Fatal(err)
-			}
-			defer goDB.Close()
-
-			_, err = goDB.Exec("SELECT 1")
-			if err == nil {
-				return errors.New("waiting for failure")
-			}
-			return nil
-		})
-
-	// Try again.
-	testCases = []struct {
+	// Check KV connection.
+	if err := db.Put(context.Background(), "foo", "bar"); err != nil {
+		t.Error(err)
+	}
+
+	// Try with various certificates.
+	testCases := []struct {
 		user, caName, certPrefix string
 		expectedError            string
 	}{

pkg/security/tls.go

@@ -30,8 +30,8 @@ const (
 	EmbeddedCertsDir     = "test_certs"
 	EmbeddedCACert       = "ca.crt"
 	EmbeddedCAKey        = "ca.key"
-	EmbeddedClientCACert = "ca.client.crt"
-	EmbeddedClientCAKey  = "ca.client.key"
+	EmbeddedClientCACert = "ca-client.crt"
+	EmbeddedClientCAKey  = "ca-client.key"
 	EmbeddedNodeCert     = "node.crt"
 	EmbeddedNodeKey      = "node.key"
 	EmbeddedRootCert     = "client.root.crt"