Skip to content

Commit

Permalink
Connect: re-add common name on secondary CA intermediate certificate
Browse files Browse the repository at this point in the history
hashicorp#10424 introduced a regression,
because it removed common name also for secondary CA intermediate
certificate, which used the same code path. This violates RFC 5280,
which mandates that Issuer CN must not be empty and equal to Subject CN
of subordinate certificate. Partially revert this patch to fix consul
provider, and add a test case to prevent future regressions.
  • Loading branch information
dclaisse committed Oct 5, 2022
1 parent 5a09a98 commit 876d9f1
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 3 deletions.
8 changes: 7 additions & 1 deletion agent/connect/ca/provider_consul.go
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,13 @@ func (c *ConsulProvider) GenerateIntermediateCSR() (string, error) {
return "", err
}

csr, err := connect.CreateCACSR(c.spiffeID, signer)
uid, err := connect.CompactUID()
if err != nil {
return "", err
}
cn := connect.CACN("consul", uid, c.clusterID, c.isPrimary)

csr, err := connect.CreateCACSR(c.spiffeID, cn, signer)
if err != nil {
return "", err
}
Expand Down
5 changes: 5 additions & 0 deletions agent/connect/ca/provider_consul_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,9 @@ func testSignIntermediateCrossDC(t *testing.T, provider1, provider2 Provider) {
// Sign the CSR with provider1.
intermediatePEM, err := provider1.SignIntermediate(csr)
require.NoError(t, err)
intermediateCert, err := connect.ParseCert(intermediatePEM)
require.NoError(t, err)
require.NotEmpty(t, intermediateCert.Subject.CommonName)
root, err := provider1.GenerateRoot()
require.NoError(t, err)
rootPEM := root.PEM
Expand All @@ -451,6 +454,8 @@ func testSignIntermediateCrossDC(t *testing.T, provider1, provider2 Provider) {
require.NoError(t, err)
requireNotEncoded(t, cert.SubjectKeyId)
requireNotEncoded(t, cert.AuthorityKeyId)
require.NotEmpty(t, cert.Issuer.CommonName)
require.Equal(t, cert.Issuer.CommonName, intermediateCert.Subject.CommonName)

// Check that the leaf signed by the new cert can be verified using the
// returned cert chain (signed intermediate + remote root).
Expand Down
22 changes: 20 additions & 2 deletions agent/connect/csr.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,31 @@ func CreateCSR(uri CertURI, privateKey crypto.Signer,

// CreateCSR returns a CA CSR to sign the given service along with the PEM-encoded
// private key for this certificate.
func CreateCACSR(uri CertURI, privateKey crypto.Signer) (string, error) {
func CreateCACSR(uri CertURI, commonName string, privateKey crypto.Signer) (string, error) {
ext, err := CreateCAExtension()
if err != nil {
return "", err
}
template := &x509.CertificateRequest{
URIs: []*url.URL{uri.URI()},
SignatureAlgorithm: SigAlgoForKey(privateKey),
ExtraExtensions: []pkix.Extension{ext},
Subject: pkix.Name{CommonName: commonName},
}

return CreateCSR(uri, privateKey, nil, nil, ext)
// Create the CSR itself
var csrBuf bytes.Buffer
bs, err := x509.CreateCertificateRequest(rand.Reader, template, privateKey)
if err != nil {
return "", err
}

err = pem.Encode(&csrBuf, &pem.Block{Type: "CERTIFICATE REQUEST", Bytes: bs})
if err != nil {
return "", err
}

return csrBuf.String(), nil
}

// CreateCAExtension creates a pkix.Extension for the x509 Basic Constraints
Expand Down

0 comments on commit 876d9f1

Please sign in to comment.