Address review feedback, fix tests

Also adds a known-answer test using LE R3 CA's SKID.

Signed-off-by: Alexander Scheel <[email protected]>

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"
@@ -577,6 +578,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)
@@ -596,6 +599,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)
@@ -607,6 +612,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 {

builtin/logical/pki/cert_util.go

@@ -1351,7 +1351,8 @@ func generateCreationBundle(b *backend, data *inputBundle, caSign *certutil.CAIn
 			// Handle removing common separators to make copy/paste from tool
 			// output easier.
 			skidNoColonsSeparators := strings.ReplaceAll(skidValue, ":", "")
-			skidNoSeparators := strings.ReplaceAll(skidNoColonsSeparators, "-", "")
+			skidNoSpaceSeparators := strings.ReplaceAll(skidNoColonsSeparators, " ", "")
+			skidNoSeparators := strings.ReplaceAll(skidNoSpaceSeparators, "-", "")
 			skid, err = hex.DecodeString(skidNoSeparators)
 			if err != nil {
 				return nil, errutil.UserError{Err: fmt.Sprintf("cannot parse requested SKID value as hex: %v", err)}

builtin/logical/pki/chain_test.go

@@ -1,6 +1,7 @@
 package pki
 
 import (
+	"bytes"
 	"context"
 	"crypto/x509"
 	"crypto/x509/pkix"
@@ -168,6 +169,17 @@ func (c CBGenerateIntermediate) Run(t testing.TB, b *backend, s logical.Storage,
 
 	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{})

builtin/logical/pki/path_sign_issuers.go

@@ -69,7 +69,7 @@ SHA-2-512. Defaults to 0 to automatically detect based on key length
 
 	fields["skid"] = &framework.FieldSchema{
 		Type:    framework.TypeString,
-		Default: 0,
+		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