connect: intermediate CA certs generated with the vault provider lack…

… URI SANs (#6491) This only affects vault versions >=1.1.1 because the prior code accidentally relied upon a bug that was fixed in hashicorp/vault#6505 The existing tests should have caught this, but they were using a vendored copy of vault version 0.10.3. This fixes the tests by running an actual copy of vault instead of an in-process copy. This has the added benefit of changing the dependency on vault to just vault/api. Also update VaultProvider to use similar SetIntermediate validation code as the ConsulProvider implementation.
hashicorp · Sep 23, 2019 · 39e4fee · 39e4fee
1 parent 4303f83
commit 39e4fee
Show file tree

Hide file tree

Showing 818 changed files with 9,719 additions and 218,595 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -22,6 +22,7 @@ references:
     GIT_COMMITTER_NAME: circleci-consul
     S3_ARTIFACT_BUCKET: consul-dev-artifacts
     BASH_ENV: .circleci/bash_env.sh
+    VAULT_BINARY_VERSION: 1.2.2
 
 jobs:
   # lint consul tests
@@ -101,6 +102,10 @@ jobs:
       - run: mkdir -p $TEST_RESULTS_DIR
       - run: sudo apt-get update && sudo apt-get install -y rsyslog
       - run: sudo service rsyslog start
+      - run: |
+          wget -q -O /tmp/vault.zip https://releases.hashicorp.com/vault/${VAULT_BINARY_VERSION}/vault_${VAULT_BINARY_VERSION}_linux_amd64.zip
+          sudo unzip -d /usr/local/bin /tmp/vault.zip
+          rm -rf /tmp/vault*
       - run: |
           PACKAGE_NAMES=$(go list ./... | circleci tests split --split-by=timings --timings-type=classname)
           gotestsum --format=short-verbose --junitfile $TEST_RESULTS_DIR/gotestsum-report.xml -- -tags=$GOTAGS -p 3 $PACKAGE_NAMES

diff --git a/agent/connect/ca/common.go b/agent/connect/ca/common.go
@@ -0,0 +1,65 @@
+package ca
+
+import (
+	"bytes"
+	"crypto/x509"
+	"fmt"
+
+	"github.com/hashicorp/consul/agent/connect"
+)
+
+func validateSetIntermediate(
+	intermediatePEM, rootPEM string,
+	currentPrivateKey string, // optional
+	spiffeID *connect.SpiffeIDSigning,
+) error {
+	// Get the key from the incoming intermediate cert so we can compare it
+	// to the currently stored key.
+	intermediate, err := connect.ParseCert(intermediatePEM)
+	if err != nil {
+		return fmt.Errorf("error parsing intermediate PEM: %v", err)
+	}
+
+	if currentPrivateKey != "" {
+		privKey, err := connect.ParseSigner(currentPrivateKey)
+		if err != nil {
+			return err
+		}
+
+		// Compare the two keys to make sure they match.
+		b1, err := x509.MarshalPKIXPublicKey(intermediate.PublicKey)
+		if err != nil {
+			return err
+		}
+		b2, err := x509.MarshalPKIXPublicKey(privKey.Public())
+		if err != nil {
+			return err
+		}
+		if !bytes.Equal(b1, b2) {
+			return fmt.Errorf("intermediate cert is for a different private key")
+		}
+	}
+
+	// Validate the remaining fields and make sure the intermediate validates against
+	// the given root cert.
+	if !intermediate.IsCA {
+		return fmt.Errorf("intermediate is not a CA certificate")
+	}
+	if uriCount := len(intermediate.URIs); uriCount != 1 {
+		return fmt.Errorf("incoming intermediate cert has unexpected number of URIs: %d", uriCount)
+	}
+	if got, want := intermediate.URIs[0].String(), spiffeID.URI().String(); got != want {
+		return fmt.Errorf("incoming cert URI %q does not match current URI: %q", got, want)
+	}
+
+	pool := x509.NewCertPool()
+	pool.AppendCertsFromPEM([]byte(rootPEM))
+	_, err = intermediate.Verify(x509.VerifyOptions{
+		Roots: pool,
+	})
+	if err != nil {
+		return fmt.Errorf("could not verify intermediate cert against root: %v", err)
+	}
+
+	return nil
+}
diff --git a/agent/connect/ca/provider_consul.go b/agent/connect/ca/provider_consul.go
@@ -220,50 +220,14 @@ func (c *ConsulProvider) SetIntermediate(intermediatePEM, rootPEM string) error
 		return fmt.Errorf("cannot set an intermediate using another root in the primary datacenter")
 	}
 
-	// Get the key from the incoming intermediate cert so we can compare it
-	// to the currently stored key.
-	intermediate, err := connect.ParseCert(intermediatePEM)
-	if err != nil {
-		return fmt.Errorf("error parsing intermediate PEM: %v", err)
-	}
-	privKey, err := connect.ParseSigner(providerState.PrivateKey)
-	if err != nil {
-		return err
-	}
-
-	// Compare the two keys to make sure they match.
-	b1, err := x509.MarshalPKIXPublicKey(intermediate.PublicKey)
-	if err != nil {
-		return err
-	}
-	b2, err := x509.MarshalPKIXPublicKey(privKey.Public())
+	err = validateSetIntermediate(
+		intermediatePEM, rootPEM,
+		providerState.PrivateKey,
+		c.spiffeID,
+	)
 	if err != nil {
 		return err
 	}
-	if !bytes.Equal(b1, b2) {
-		return fmt.Errorf("intermediate cert is for a different private key")
-	}
-
-	// Validate the remaining fields and make sure the intermediate validates against
-	// the given root cert.
-	if !intermediate.IsCA {
-		return fmt.Errorf("intermediate is not a CA certificate")
-	}
-	if uriCount := len(intermediate.URIs); uriCount != 1 {
-		return fmt.Errorf("incoming intermediate cert has unexpected number of URIs: %d", uriCount)
-	}
-	if got, want := intermediate.URIs[0].String(), c.spiffeID.URI().String(); got != want {
-		return fmt.Errorf("incoming cert URI %q does not match current URI: %q", got, want)
-	}
-
-	pool := x509.NewCertPool()
-	pool.AppendCertsFromPEM([]byte(rootPEM))
-	_, err = intermediate.Verify(x509.VerifyOptions{
-		Roots: pool,
-	})
-	if err != nil {
-		return fmt.Errorf("could not verify intermediate cert against root: %v", err)
-	}
 
 	// Update the state
 	newState := *providerState

diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go
@@ -26,6 +26,7 @@ type VaultProvider struct {
 	client    *vaultapi.Client
 	isRoot    bool
 	clusterId string
+	spiffeID  *connect.SpiffeIDSigning
 }
 
 func vaultTLSConfig(config *structs.VaultCAProviderConfig) *vaultapi.TLSConfig {
@@ -63,6 +64,7 @@ func (v *VaultProvider) Configure(clusterId string, isRoot bool, rawConfig map[s
 	v.client = client
 	v.isRoot = isRoot
 	v.clusterId = clusterId
+	v.spiffeID = connect.SpiffeIDSigningForCluster(&structs.CAConfiguration{ClusterID: clusterId})
 
 	return nil
 }
@@ -96,14 +98,13 @@ func (v *VaultProvider) GenerateRoot() error {
 
 		fallthrough
 	case ErrBackendNotInitialized:
-		spiffeID := connect.SpiffeIDSigning{ClusterID: v.clusterId, Domain: "consul"}
 		uuid, err := uuid.GenerateUUID()
 		if err != nil {
 			return err
 		}
 		_, err = v.client.Logical().Write(v.config.RootPKIPath+"root/generate/internal", map[string]interface{}{
 			"common_name": fmt.Sprintf("Vault CA Root Authority %s", uuid),
-			"uri_sans":    spiffeID.URI().String(),
+			"uri_sans":    v.spiffeID.URI().String(),
 			"key_type":    v.config.PrivateKeyType,
 			"key_bits":    v.config.PrivateKeyBits,
 		})
@@ -158,7 +159,6 @@ func (v *VaultProvider) generateIntermediateCSR() (string, error) {
 	if err != nil {
 		return "", err
 	}
-	spiffeID := connect.SpiffeIDSigning{ClusterID: v.clusterId, Domain: "consul"}
 	if role == nil {
 		_, err := v.client.Logical().Write(rolePath, map[string]interface{}{
 			"allow_any_name":   true,
@@ -178,7 +178,7 @@ func (v *VaultProvider) generateIntermediateCSR() (string, error) {
 		"common_name": "Vault CA Intermediate Authority",
 		"key_type":    v.config.PrivateKeyType,
 		"key_bits":    v.config.PrivateKeyBits,
-		"uri_sans":    spiffeID.URI().String(),
+		"uri_sans":    v.spiffeID.URI().String(),
 	})
 	if err != nil {
 		return "", err
@@ -201,7 +201,16 @@ func (v *VaultProvider) SetIntermediate(intermediatePEM, rootPEM string) error {
 		return fmt.Errorf("cannot set an intermediate using another root in the primary datacenter")
 	}
 
-	_, err := v.client.Logical().Write(v.config.IntermediatePKIPath+"intermediate/set-signed", map[string]interface{}{
+	err := validateSetIntermediate(
+		intermediatePEM, rootPEM,
+		"", // we don't have access to the private key directly
+		v.spiffeID,
+	)
+	if err != nil {
+		return err
+	}
+
+	_, err = v.client.Logical().Write(v.config.IntermediatePKIPath+"intermediate/set-signed", map[string]interface{}{
 		"certificate": fmt.Sprintf("%s\n%s", intermediatePEM, rootPEM),
 	})
 	if err != nil {
@@ -257,8 +266,9 @@ func (v *VaultProvider) GenerateIntermediate() (string, error) {
 
 	// Sign the CSR with the root backend.
 	intermediate, err := v.client.Logical().Write(v.config.RootPKIPath+"root/sign-intermediate", map[string]interface{}{
-		"csr":    csr,
-		"format": "pem_bundle",
+		"csr":            csr,
+		"use_csr_values": true,
+		"format":         "pem_bundle",
 	})
 	if err != nil {
 		return "", err
@@ -323,6 +333,7 @@ func (v *VaultProvider) SignIntermediate(csr *x509.CertificateRequest) (string,
 	// Sign the CSR with the root backend.
 	data, err := v.client.Logical().Write(v.config.RootPKIPath+"root/sign-intermediate", map[string]interface{}{
 		"csr":             pemBuf.String(),
+		"use_csr_values":  true,
 		"format":          "pem_bundle",
 		"max_path_length": 0,
 	})