Allow old certs to be cross-signed (#16494)

* Allow old certs to be cross-signed In Vault 1.11, we introduced cross-signing support, but the earlier SKID field change in Vault 1.10 causes problems: notably, certs created on older versions of Vault (<=1.9) or outside of Vault (with a different SKID method) cannot be cross-signed and validated in OpenSSL. In particular, OpenSSL appears to be unique in requiring a SKID/AKID match for chain building. If AKID and SKID are present on an otherwise valid client/parent cert pair and the values are different, OpenSSL will not build a valid path over those two, whereas most other chain validation implementations will. Regardless, to have proper cross-signing support, we really aught to support copying an SKID. This adds such support to the sign-intermediate endpoint. Support for the /issue endpoint is not added, as cross-signing leaf certs isn't generally useful and can accept random SKIDs. Resolves: #16461 Signed-off-by: Alexander Scheel <[email protected]> * Add changelog Signed-off-by: Alexander Scheel <[email protected]> * Address review feedback, fix tests Also adds a known-answer test using LE R3 CA's SKID. Signed-off-by: Alexander Scheel <[email protected]> * Address review feedback regarding separators Signed-off-by: Alexander Scheel <[email protected]>
hashicorp · Aug 3, 2022 · 74d68e2 · 74d68e2
1 parent b4e8151
commit 74d68e2
Show file tree

Hide file tree

Showing 8 changed files with 102 additions and 4 deletions.
diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go
@@ -12,6 +12,7 @@ import (
 	"crypto/x509"
 	"crypto/x509/pkix"
 	"encoding/base64"
+	"encoding/hex"
 	"encoding/json"
 	"encoding/pem"
 	"fmt"
@@ -583,6 +584,8 @@ func generateURLSteps(t *testing.T, caCert, caKey string, intdata, reqdata map[s
 				"signature_bits":      512,
 				"format":              "der",
 				"not_before_duration": "2h",
+				// Let's Encrypt -- R3 SKID
+				"skid": "14:2E:B3:17:B7:58:56:CB:AE:50:09:40:E6:1F:AF:9D:8B:14:C2:C6",
 			},
 			Check: func(resp *logical.Response) error {
 				certString := resp.Data["certificate"].(string)
@@ -602,6 +605,8 @@ func generateURLSteps(t *testing.T, caCert, caKey string, intdata, reqdata map[s
 				}
 				cert := certs[0]
 
+				skid, _ := hex.DecodeString("142EB317B75856CBAE500940E61FAF9D8B14C2C6")
+
 				switch {
 				case !reflect.DeepEqual(expected.IssuingCertificates, cert.IssuingCertificateURL):
 					return fmt.Errorf("IssuingCertificateURL:\nexpected\n%#v\ngot\n%#v\n", expected.IssuingCertificates, cert.IssuingCertificateURL)
@@ -613,6 +618,8 @@ func generateURLSteps(t *testing.T, caCert, caKey string, intdata, reqdata map[s
 					return fmt.Errorf("DNSNames\nexpected\n%#v\ngot\n%#v\n", []string{"intermediate.cert.com"}, cert.DNSNames)
 				case !reflect.DeepEqual(x509.SHA512WithRSA, cert.SignatureAlgorithm):
 					return fmt.Errorf("Signature Algorithm:\nexpected\n%#v\ngot\n%#v\n", x509.SHA512WithRSA, cert.SignatureAlgorithm)
+				case !reflect.DeepEqual(skid, cert.SubjectKeyId):
+					return fmt.Errorf("SKID:\nexpected\n%#v\ngot\n%#v\n", skid, cert.SubjectKeyId)
 				}
 
 				if math.Abs(float64(time.Now().Add(-2*time.Hour).Unix()-cert.NotBefore.Unix())) > 10 {

diff --git a/builtin/logical/pki/cert_util.go b/builtin/logical/pki/cert_util.go
@@ -10,6 +10,7 @@ import (
 	"crypto/x509/pkix"
 	"encoding/asn1"
 	"encoding/base64"
+	"encoding/hex"
 	"encoding/pem"
 	"errors"
 	"fmt"
@@ -1340,6 +1341,27 @@ func generateCreationBundle(b *backend, data *inputBundle, caSign *certutil.CAIn
 		}
 	}
 
+	// Parse SKID from the request for cross-signing.
+	var skid []byte
+	{
+		if rawSKIDValue, ok := data.apiData.GetOk("skid"); ok {
+			// Handle removing common separators to make copy/paste from tool
+			// output easier. Chromium uses space, OpenSSL uses colons, and at
+			// one point, Vault had preferred dash as a separator for hex
+			// strings.
+			var err error
+			skidValue := rawSKIDValue.(string)
+			for _, separator := range []string{":", "-", " "} {
+				skidValue = strings.ReplaceAll(skidValue, separator, "")
+			}
+
+			skid, err = hex.DecodeString(skidValue)
+			if err != nil {
+				return nil, errutil.UserError{Err: fmt.Sprintf("cannot parse requested SKID value as hex: %v", err)}
+			}
+		}
+	}
+
 	creation := &certutil.CreationBundle{
 		Params: &certutil.CreationParameters{
 			Subject:                       subject,
@@ -1359,6 +1381,7 @@ func generateCreationBundle(b *backend, data *inputBundle, caSign *certutil.CAIn
 			BasicConstraintsValidForNonCA: data.role.BasicConstraintsValidForNonCA,
 			NotBeforeDuration:             data.role.NotBeforeDuration,
 			ForceAppendCaChain:            caSign != nil,
+			SKID:                          skid,
 		},
 		SigningBundle: caSign,
 		CSR:           csr,

diff --git a/builtin/logical/pki/chain_test.go b/builtin/logical/pki/chain_test.go
@@ -1,9 +1,11 @@
 package pki
 
 import (
+	"bytes"
 	"context"
 	"crypto/x509"
 	"crypto/x509/pkix"
+	"encoding/hex"
 	"encoding/pem"
 	"fmt"
 	"strconv"
@@ -113,6 +115,7 @@ type CBGenerateIntermediate struct {
 	Existing           bool
 	Name               string
 	CommonName         string
+	SKID               string
 	Parent             string
 	ImportErrorMessage string
 }
@@ -151,13 +154,32 @@ func (c CBGenerateIntermediate) Run(t testing.TB, b *backend, s logical.Storage,
 	if len(c.CommonName) > 0 {
 		data["common_name"] = c.CommonName
 	}
+	if len(c.SKID) > 0 {
+		// Copy the SKID from an existing, already-issued cert.
+		otherPEM := knownCerts[c.SKID]
+		otherCert := ToCertificate(t, otherPEM)
+
+		data["skid"] = hex.EncodeToString(otherCert.SubjectKeyId)
+	}
+
 	resp, err = CBWrite(b, s, url, data)
 	if err != nil {
 		t.Fatalf("failed to sign CSR for issuer (%v): %v / body: %v", c.Name, err, data)
 	}
 
 	knownCerts[c.Name] = strings.TrimSpace(resp.Data["certificate"].(string))
 
+	// Verify SKID if one was requested.
+	if len(c.SKID) > 0 {
+		otherPEM := knownCerts[c.SKID]
+		otherCert := ToCertificate(t, otherPEM)
+		ourCert := ToCertificate(t, knownCerts[c.Name])
+
+		if !bytes.Equal(otherCert.SubjectKeyId, ourCert.SubjectKeyId) {
+			t.Fatalf("Expected two certs to have equal SKIDs but differed: them: %v vs us: %v", otherCert.SubjectKeyId, ourCert.SubjectKeyId)
+		}
+	}
+
 	// Set the signed intermediate
 	url = "intermediate/set-signed"
 	data = make(map[string]interface{})
@@ -829,6 +851,7 @@ var chainBuildingTestCases = []CBTestScenario{
 				Existing:   true,
 				Name:       "cross-old-new",
 				CommonName: "root-new",
+				SKID:       "root-new-a",
 				// Which old issuer is used here doesn't matter as they have
 				// the same CN and key.
 				Parent: "root-old-a",
@@ -887,6 +910,7 @@ var chainBuildingTestCases = []CBTestScenario{
 				Existing:   true,
 				Name:       "cross-new-old",
 				CommonName: "root-old",
+				SKID:       "root-old-a",
 				// Which new issuer is used here doesn't matter as they have
 				// the same CN and key.
 				Parent: "root-new-a",

diff --git a/builtin/logical/pki/path_sign_issuers.go b/builtin/logical/pki/path_sign_issuers.go
@@ -7,15 +7,15 @@ import (
 
 func pathIssuerSignIntermediate(b *backend) *framework.Path {
 	pattern := "issuer/" + framework.GenericNameRegex(issuerRefParam) + "/sign-intermediate"
-	return pathIssuerSignIntermediateRaw(b, pattern)
+	return buildPathIssuerSignIntermediateRaw(b, pattern)
 }
 
 func pathSignIntermediate(b *backend) *framework.Path {
 	pattern := "root/sign-intermediate"
-	return pathIssuerSignIntermediateRaw(b, pattern)
+	return buildPathIssuerSignIntermediateRaw(b, pattern)
 }
 
-func pathIssuerSignIntermediateRaw(b *backend, pattern string) *framework.Path {
+func buildPathIssuerSignIntermediateRaw(b *backend, pattern string) *framework.Path {
 	fields := addIssuerRefField(map[string]*framework.FieldSchema{})
 	path := &framework.Path{
 		Pattern: pattern,
@@ -67,6 +67,24 @@ SHA-2-512. Defaults to 0 to automatically detect based on key length
 		},
 	}
 
+	fields["skid"] = &framework.FieldSchema{
+		Type:    framework.TypeString,
+		Default: "",
+		Description: `Value for the Subject Key Identifier field
+(RFC 5280 Section 4.2.1.2). This value should ONLY be used when
+cross-signing to mimic the existing certificate's SKID value; this
+is necessary to allow certain TLS implementations (such as OpenSSL)
+which use SKID/AKID matches in chain building to restrict possible
+valid chains.
+
+Specified as a string in hex format. Default is empty, allowing
+Vault to automatically calculate the SKID according to method one
+in the above RFC section.`,
+		DisplayAttrs: &framework.DisplayAttributes{
+			Value: "",
+		},
+	}
+
 	return path
 }
 

diff --git a/changelog/16494.txt b/changelog/16494.txt
@@ -0,0 +1,3 @@
+```release-note:improvement
+secret/pki: Allow specifying SKID for cross-signed issuance from older Vault versions.
+```
diff --git a/sdk/helper/certutil/helpers.go b/sdk/helper/certutil/helpers.go
@@ -87,6 +87,16 @@ func GetSubjKeyID(privateKey crypto.Signer) ([]byte, error) {
 	return getSubjectKeyID(privateKey.Public())
 }
 
+// Returns the explicit SKID when used for cross-signing, else computes a new
+// SKID from the key itself.
+func getSubjectKeyIDFromBundle(data *CreationBundle) ([]byte, error) {
+	if len(data.Params.SKID) > 0 {
+		return data.Params.SKID, nil
+	}
+
+	return getSubjectKeyID(data.CSR.PublicKey)
+}
+
 func getSubjectKeyID(pub interface{}) ([]byte, error) {
 	var publicKeyBytes []byte
 	switch pub := pub.(type) {
@@ -1066,7 +1076,7 @@ func signCertificate(data *CreationBundle, randReader io.Reader) (*ParsedCertBun
 		return nil, err
 	}
 
-	subjKeyID, err := getSubjectKeyID(data.CSR.PublicKey)
+	subjKeyID, err := getSubjectKeyIDFromBundle(data)
 	if err != nil {
 		return nil, err
 	}

diff --git a/sdk/helper/certutil/types.go b/sdk/helper/certutil/types.go
@@ -796,6 +796,9 @@ type CreationParameters struct {
 
 	// The duration the certificate will use NotBefore
 	NotBeforeDuration time.Duration
+
+	// The explicit SKID to use; especially useful for cross-signing.
+	SKID []byte
 }
 
 type CreationBundle struct {

diff --git a/website/content/api-docs/secret/pki.mdx b/website/content/api-docs/secret/pki.mdx
@@ -571,6 +571,16 @@ when signing an externally-owned intermediate.
    `signature_bits` value; only RSA issuers will change signature types
    based on this parameter.
 
+- `skid` `(string: "")` - Value for the Subject Key Identifier field
+  (RFC 5280 Section 4.2.1.2). Specified as a string in hex format. Default
+  is empty, allowing Vault to automatically calculate the SKID according
+  to method one in the above RFC section.
+
+~> **Note**: This value should ONLY be used when cross-signing to mimic
+   the existing certificate's SKID value; this is necessary to allow
+   certain TLS implementations (such as OpenSSL) which use SKID/AKID
+   matches in chain building to restrict possible valid chains.
+
 #### Sample Payload
 
 ```json