Fix RevocationSigAlg provisioning in GCP (#17449)

* Fix RevocationSigAlg provisioning in GCP

GCP restricts keys to a certain type of signature, including hash
algorithm, so we must provision our RevocationSigAlg from the root
itself unconditionally in order for GCP to work.

This does change the default, but only for newly created certificates.

Additionally, we clarify that CRL building is not fatal to the import
process.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add inverse mapping for SignatureAlgorithm

By default we'd use .String() on x509.SignatureAlgorithm, but this
doesn't round-trip. Switch to a custom map that is round-trippable
and matches the constant name as there is no other way to get this info
presently.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add test to ensure root creation sets rev_sig_alg

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Test round-tripping of SigAlgoNames, InvSigAlgoNames

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Fix failing Default Update test

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

builtin/logical/pki/backend_test.go

@@ -5618,6 +5618,10 @@ func TestBackend_VerifyIssuerUpdateDefaultsMatchCreation(t *testing.T) {
 	requireSuccessNonNilResponse(t, resp, err, "failed reading default issuer")
 	preUpdateValues := resp.Data
 
+	// This field gets reset during issuer update to the empty string
+	// (meaning Go will auto-detect the rev-sig-algo).
+	preUpdateValues["revocation_signature_algorithm"] = ""
+
 	resp, err = CBWrite(b, s, "issuer/default", map[string]interface{}{})
 	requireSuccessNonNilResponse(t, resp, err, "failed updating default issuer with no values")
 

builtin/logical/pki/crl_test.go

@@ -144,20 +144,22 @@ func TestBackend_CRL_AllKeyTypeSigAlgos(t *testing.T) {
 	type testCase struct {
 		KeyType string
 		KeyBits int
+		SigBits int
+		UsePSS  bool
 		SigAlgo string
 	}
 
 	testCases := []testCase{
-		{"rsa", 2048, "SHA256WithRSA"},
-		{"rsa", 2048, "SHA384WithRSA"},
-		{"rsa", 2048, "SHA512WithRSA"},
-		{"rsa", 2048, "SHA256WithRSAPSS"},
-		{"rsa", 2048, "SHA384WithRSAPSS"},
-		{"rsa", 2048, "SHA512WithRSAPSS"},
-		{"ec", 256, "ECDSAWithSHA256"},
-		{"ec", 384, "ECDSAWithSHA384"},
-		{"ec", 521, "ECDSAWithSHA512"},
-		{"ed25519", 0, "PureEd25519"},
+		{"rsa", 2048, 256, false, "SHA256WithRSA"},
+		{"rsa", 2048, 384, false, "SHA384WithRSA"},
+		{"rsa", 2048, 512, false, "SHA512WithRSA"},
+		{"rsa", 2048, 256, true, "SHA256WithRSAPSS"},
+		{"rsa", 2048, 384, true, "SHA384WithRSAPSS"},
+		{"rsa", 2048, 512, true, "SHA512WithRSAPSS"},
+		{"ec", 256, 256, false, "ECDSAWithSHA256"},
+		{"ec", 384, 384, false, "ECDSAWithSHA384"},
+		{"ec", 521, 521, false, "ECDSAWithSHA512"},
+		{"ed25519", 0, 0, false, "Ed25519"},
 	}
 
 	for index, tc := range testCases {
@@ -169,18 +171,17 @@ func TestBackend_CRL_AllKeyTypeSigAlgos(t *testing.T) {
 			"common_name":    "myvault.com",
 			"key_type":       tc.KeyType,
 			"key_bits":       tc.KeyBits,
+			"signature_bits": tc.SigBits,
+			"use_pss":        tc.UsePSS,
 		})
 		if err != nil {
 			t.Fatalf("tc %v: %v", index, err)
 		}
 		caSerial := resp.Data["serial_number"].(string)
 
-		_, err = CBPatch(b, s, "issuer/default", map[string]interface{}{
-			"revocation_signature_algorithm": tc.SigAlgo,
-		})
-		if err != nil {
-			t.Fatalf("tc %v: %v", index, err)
-		}
+		resp, err = CBRead(b, s, "issuer/default")
+		requireSuccessNonNilResponse(t, resp, err, "fetching issuer should return data")
+		require.Equal(t, tc.SigAlgo, resp.Data["revocation_signature_algorithm"])
 
 		crlEnableDisableTestForBackend(t, b, s, []string{caSerial})
 

builtin/logical/pki/path_fetch_issuers.go

@@ -207,10 +207,13 @@ func respondReadIssuer(issuer *issuerEntry) (*logical.Response, error) {
 		respManualChain = append(respManualChain, string(entity))
 	}
 
-	revSigAlgStr := issuer.RevocationSigAlg.String()
+	revSigAlgStr, present := certutil.InvSignatureAlgorithmNames[issuer.RevocationSigAlg]
+	if !present {
+		revSigAlgStr = issuer.RevocationSigAlg.String()
 		if issuer.RevocationSigAlg == x509.UnknownSignatureAlgorithm {
 			revSigAlgStr = ""
 		}
+	}
 
 	data := map[string]interface{}{
 		"issuer_id":                      issuer.ID,

builtin/logical/pki/path_manage_issuers.go

@@ -250,6 +250,10 @@ func (b *backend) pathImportIssuers(ctx context.Context, req *logical.Request, d
 			// this issuer, so convert the error to a warning.
 			if strings.Contains(err.Error(), "PSS") || strings.Contains(err.Error(), "pss") {
 				err = fmt.Errorf("Rebuilding the CRL failed with a message relating to the PSS signature algorithm. This likely means the revocation_signature_algorithm needs to be set on the newly imported issuer(s) because a managed key supports only the PSS algorithm; by default PKCS#1v1.5 was used to build the CRLs. CRLs will not be generated until this has been addressed, however the import was successful. The original error is reproduced below:\n\n\t%v", err)
+			} else {
+				// Note to the caller that while this is an error, we did
+				// successfully import the issuers.
+				err = fmt.Errorf("Rebuilding the CRL failed. While this is indicative of a problem with the imported issuers (perhaps because of their revocation_signature_algorithm), they did import successfully and are now usable. It is strongly suggested to fix the CRL building errors before continuing. The original error is reproduced below:\n\n\t%v", err)
 			}
 
 			return nil, err

builtin/logical/pki/path_root.go

@@ -236,10 +236,6 @@ func (b *backend) pathCAGenerateRoot(ctx context.Context, req *logical.Request,
 	resp.Data["key_id"] = myKey.ID
 	resp.Data["key_name"] = myKey.Name
 
-	// Update the issuer to reflect the PSS status here for revocation; this
-	// allows CRL building to succeed if the root is using a managed key with
-	// only PSS support.
-	if input.role.KeyType == "rsa" && input.role.UsePSS {
 	// The one time that it is safe (and good) to copy the
 	// SignatureAlgorithm field off the certificate (for the purposes of
 	// detecting PSS support) is when we've freshly generated it AND it
@@ -249,11 +245,16 @@ func (b *backend) pathCAGenerateRoot(ctx context.Context, req *logical.Request,
 	// reflects the parent key's preferences. For imports, this doesn't
 	// hold as the old system might've allowed other signature types that
 	// the new system (whether Vault or a managed key) doesn't.
+	//
+	// Previously we did this conditionally on whether or not PSS was in
+	// use. This is insufficient as some cloud KMS providers (namely, GCP)
+	// restrict the key to a single signature algorithm! So e.g., a RSA 3072
+	// key MUST use SHA-384 as the hash algorithm. Thus we pull in the
+	// RevocationSigAlg unconditionally on roots now.
 	myIssuer.RevocationSigAlg = parsedBundle.Certificate.SignatureAlgorithm
 	if err := sc.writeIssuer(myIssuer); err != nil {
 		return nil, fmt.Errorf("unable to store PSS-updated issuer: %v", err)
 	}
-	}
 
 	// Also store it as just the certificate identified by serial number, so it
 	// can be revoked

sdk/helper/certutil/certutil_test.go

@@ -910,6 +910,34 @@ func TestNotAfterValues(t *testing.T) {
 	}
 }
 
+func TestSignatureAlgorithmRoundTripping(t *testing.T) {
+	for leftName, value := range SignatureAlgorithmNames {
+		if leftName == "pureed25519" && value == x509.PureEd25519 {
+			continue
+		}
+
+		rightName, present := InvSignatureAlgorithmNames[value]
+		if !present {
+			t.Fatalf("%v=%v is present in SignatureAlgorithmNames but not in InvSignatureAlgorithmNames", leftName, value)
+		}
+
+		if strings.ToLower(rightName) != leftName {
+			t.Fatalf("%v=%v is present in SignatureAlgorithmNames but inverse for %v has different name: %v", leftName, value, value, rightName)
+		}
+	}
+
+	for leftValue, name := range InvSignatureAlgorithmNames {
+		rightValue, present := SignatureAlgorithmNames[strings.ToLower(name)]
+		if !present {
+			t.Fatalf("%v=%v is present in InvSignatureAlgorithmNames but not in SignatureAlgorithmNames", leftValue, name)
+		}
+
+		if rightValue != leftValue {
+			t.Fatalf("%v=%v is present in InvSignatureAlgorithmNames but forwards for %v has different value: %v", leftValue, name, name, rightValue)
+		}
+	}
+}
+
 func genRsaKey(t *testing.T) *rsa.PrivateKey {
 	key, err := rsa.GenerateKey(rand.Reader, 2048)
 	if err != nil {

sdk/helper/certutil/helpers.go

@@ -64,6 +64,20 @@ var SignatureAlgorithmNames = map[string]x509.SignatureAlgorithm{
 	"ed25519":          x509.PureEd25519, // Duplicated for clarity; most won't expect the "Pure" prefix.
 }
 
+// Mapping of constant values<->constant names for SignatureAlgorithm
+var InvSignatureAlgorithmNames = map[x509.SignatureAlgorithm]string{
+	x509.SHA256WithRSA:    "SHA256WithRSA",
+	x509.SHA384WithRSA:    "SHA384WithRSA",
+	x509.SHA512WithRSA:    "SHA512WithRSA",
+	x509.ECDSAWithSHA256:  "ECDSAWithSHA256",
+	x509.ECDSAWithSHA384:  "ECDSAWithSHA384",
+	x509.ECDSAWithSHA512:  "ECDSAWithSHA512",
+	x509.SHA256WithRSAPSS: "SHA256WithRSAPSS",
+	x509.SHA384WithRSAPSS: "SHA384WithRSAPSS",
+	x509.SHA512WithRSAPSS: "SHA512WithRSAPSS",
+	x509.PureEd25519:      "Ed25519",
+}
+
 // OID for RFC 5280 Delta CRL Indicator CRL extension.
 //
 // > id-ce-deltaCRLIndicator OBJECT IDENTIFIER ::= { id-ce 27 }