Add PSS support to PKI Secrets Engine (#16519)

* Add PSS signature support to Vault PKI engine Signed-off-by: Alexander Scheel <[email protected]> * Use issuer's RevocationSigAlg for CRL signing We introduce a new parameter on issuers, revocation_signature_algorithm to control the signature algorithm used during CRL signing. This is because the SignatureAlgorithm value from the certificate itself is incorrect for this purpose: a RSA root could sign an ECDSA intermediate with say, SHA256WithRSA, but when the intermediate goes to sign a CRL, it must use ECDSAWithSHA256 or equivalent instead of SHA256WithRSA. When coupled with support for PSS-only keys, allowing the user to set the signature algorithm value as desired seems like the best approach. Signed-off-by: Alexander Scheel <[email protected]> * Add use_pss, revocation_signature_algorithm docs Signed-off-by: Alexander Scheel <[email protected]> * Add PSS to signature role issuance test matrix Signed-off-by: Alexander Scheel <[email protected]> * Add changelog Signed-off-by: Alexander Scheel <[email protected]> * Allow roots to self-identify revocation alg When using PSS support with a managed key, sometimes the underlying device will not support PKCS#1v1.5 signatures. This results in CRL building failing, unless we update the entry's signature algorithm prior to building the CRL for the new root. With a RSA-type key and use_pss=true, we use the signature bits value to decide which hash function to use for PSS support. Signed-off-by: Alexander Scheel <[email protected]> * Add clearer error message on failed import When CRL building fails during cert/key import, due to PSS failures, give a better indication to the user that import succeeded its just CRL building that failed. This tells them the parameter to adjust on the issuer and warns that CRL building will fail until this is fixed. Signed-off-by: Alexander Scheel <[email protected]> * Add case insensitive SigAlgo matching Signed-off-by: Alexander Scheel <[email protected]> * Convert UsePSS back to regular bool Signed-off-by: Alexander Scheel <[email protected]> * Refactor PSS->certTemplate into helper function Signed-off-by: Alexander Scheel <[email protected]> * Proper string output on rev_sig_alg display Signed-off-by: Alexander Scheel <[email protected]> * Copy root's SignatureAlgorithm for CRL building Signed-off-by: Alexander Scheel <[email protected]>
hashicorp · Aug 3, 2022 · ce7f0ff · ce7f0ff
1 parent ee28326
commit ce7f0ff
Show file tree

Hide file tree

Showing 19 changed files with 351 additions and 46 deletions.
diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go
@@ -3552,6 +3552,7 @@ func TestReadWriteDeleteRoles(t *testing.T) {
 		"allowed_serial_numbers":             []interface{}{},
 		"generate_lease":                     false,
 		"signature_bits":                     json.Number("256"),
+		"use_pss":                            false,
 		"allowed_domains":                    []interface{}{},
 		"allowed_uri_sans_template":          false,
 		"enforce_hostnames":                  true,
@@ -4459,6 +4460,7 @@ type KeySizeRegression struct {
 
 	// Signature Bits presently is only specified on the role.
 	RoleSignatureBits []int
+	RoleUsePSS        bool
 
 	// These are tuples; must be of the same length.
 	TestKeyTypes []string
@@ -4502,6 +4504,7 @@ func RoleKeySizeRegressionHelper(t *testing.T, b *backend, s logical.Storage, in
 						"key_type":       test.RoleKeyType,
 						"key_bits":       roleKeyBits,
 						"signature_bits": roleSignatureBits,
+						"use_pss":        test.RoleUsePSS,
 					})
 					if err != nil {
 						t.Fatal(err)
@@ -4527,6 +4530,15 @@ func RoleKeySizeRegressionHelper(t *testing.T, b *backend, s logical.Storage, in
 							t.Fatalf("key size regression test [%d] failed: haveErr: %v, expectErr: %v, err: %v, resp: %v, test case: %v, caKeyType: %v, caKeyBits: %v, role: %v, keyType: %v, keyBits: %v", index, haveErr, test.ExpectError, err, resp, test, caKeyType, caKeyBits, role, keyType, keyBits)
 						}
 
+						if resp != nil && test.RoleUsePSS && caKeyType == "rsa" {
+							leafCert := parseCert(t, resp.Data["certificate"].(string))
+							switch leafCert.SignatureAlgorithm {
+							case x509.SHA256WithRSAPSS, x509.SHA384WithRSAPSS, x509.SHA512WithRSAPSS:
+							default:
+								t.Fatalf("key size regression test [%d] failed on role %v: unexpected signature algorithm; expected RSA-type CA to sign a leaf cert with PSS algorithm; got %v", index, role, leafCert.SignatureAlgorithm.String())
+							}
+						}
+
 						tested += 1
 					}
 				}
@@ -4548,36 +4560,41 @@ func TestBackend_Roles_KeySizeRegression(t *testing.T) {
 	testCases := []KeySizeRegression{
 		// RSA with default parameters should fail to issue smaller RSA keys
 		// and any size ECDSA/Ed25519 keys.
-		/*  0 */ {"rsa", []int{0, 2048}, []int{0, 256, 384, 512}, []string{"rsa", "ec", "ec", "ec", "ec", "ed25519"}, []int{1024, 224, 256, 384, 521, 0}, true},
+		/*  0 */ {"rsa", []int{0, 2048}, []int{0, 256, 384, 512}, false, []string{"rsa", "ec", "ec", "ec", "ec", "ed25519"}, []int{1024, 224, 256, 384, 521, 0}, true},
 		// But it should work to issue larger RSA keys.
-		/*  1 */ {"rsa", []int{0, 2048}, []int{0, 256, 384, 512}, []string{"rsa", "rsa"}, []int{2048, 3072}, false},
+		/*  1 */ {"rsa", []int{0, 2048}, []int{0, 256, 384, 512}, false, []string{"rsa", "rsa"}, []int{2048, 3072}, false},
 
 		// EC with default parameters should fail to issue smaller EC keys
 		// and any size RSA/Ed25519 keys.
-		/*  2 */ {"ec", []int{0}, []int{0}, []string{"rsa", "ec", "ed25519"}, []int{2048, 224, 0}, true},
+		/*  2 */ {"ec", []int{0}, []int{0}, false, []string{"rsa", "ec", "ed25519"}, []int{2048, 224, 0}, true},
 		// But it should work to issue larger EC keys. Note that we should be
 		// independent of signature bits as that's computed from the issuer
 		// type (for EC based issuers).
-		/*  3 */ {"ec", []int{224}, []int{0, 256, 384, 521}, []string{"ec", "ec", "ec", "ec"}, []int{224, 256, 384, 521}, false},
-		/*  4 */ {"ec", []int{0, 256}, []int{0, 256, 384, 521}, []string{"ec", "ec", "ec"}, []int{256, 384, 521}, false},
-		/*  5 */ {"ec", []int{384}, []int{0, 256, 384, 521}, []string{"ec", "ec"}, []int{384, 521}, false},
-		/*  6 */ {"ec", []int{521}, []int{0, 256, 384, 512}, []string{"ec"}, []int{521}, false},
+		/*  3 */ {"ec", []int{224}, []int{0, 256, 384, 521}, false, []string{"ec", "ec", "ec", "ec"}, []int{224, 256, 384, 521}, false},
+		/*  4 */ {"ec", []int{0, 256}, []int{0, 256, 384, 521}, false, []string{"ec", "ec", "ec"}, []int{256, 384, 521}, false},
+		/*  5 */ {"ec", []int{384}, []int{0, 256, 384, 521}, false, []string{"ec", "ec"}, []int{384, 521}, false},
+		/*  6 */ {"ec", []int{521}, []int{0, 256, 384, 512}, false, []string{"ec"}, []int{521}, false},
 
 		// Ed25519 should reject RSA and EC keys.
-		/*  7 */ {"ed25519", []int{0}, []int{0}, []string{"rsa", "ec", "ec"}, []int{2048, 256, 521}, true},
+		/*  7 */ {"ed25519", []int{0}, []int{0}, false, []string{"rsa", "ec", "ec"}, []int{2048, 256, 521}, true},
 		// But it should work to issue Ed25519 keys.
-		/*  8 */ {"ed25519", []int{0}, []int{0}, []string{"ed25519"}, []int{0}, false},
+		/*  8 */ {"ed25519", []int{0}, []int{0}, false, []string{"ed25519"}, []int{0}, false},
 
 		// Any key type should reject insecure RSA key sizes.
-		/*  9 */ {"any", []int{0}, []int{0, 256, 384, 512}, []string{"rsa", "rsa"}, []int{512, 1024}, true},
+		/*  9 */ {"any", []int{0}, []int{0, 256, 384, 512}, false, []string{"rsa", "rsa"}, []int{512, 1024}, true},
 		// But work for everything else.
-		/* 10 */ {"any", []int{0}, []int{0, 256, 384, 512}, []string{"rsa", "rsa", "ec", "ec", "ec", "ec", "ed25519"}, []int{2048, 3072, 224, 256, 384, 521, 0}, false},
+		/* 10 */ {"any", []int{0}, []int{0, 256, 384, 512}, false, []string{"rsa", "rsa", "ec", "ec", "ec", "ec", "ed25519"}, []int{2048, 3072, 224, 256, 384, 521, 0}, false},
 
 		// RSA with larger than default key size should reject smaller ones.
-		/* 11 */ {"rsa", []int{3072}, []int{0, 256, 384, 512}, []string{"rsa"}, []int{2048}, true},
+		/* 11 */ {"rsa", []int{3072}, []int{0, 256, 384, 512}, false, []string{"rsa"}, []int{2048}, true},
+
+		// We should be able to sign with PSS with any CA key type.
+		/* 12 */ {"rsa", []int{0}, []int{0, 256, 384, 512}, true, []string{"rsa"}, []int{2048}, false},
+		/* 13 */ {"ec", []int{0}, []int{0}, true, []string{"ec"}, []int{256}, false},
+		/* 14 */ {"ed25519", []int{0}, []int{0}, true, []string{"ed25519"}, []int{0}, false},
 	}
 
-	if len(testCases) != 12 {
+	if len(testCases) != 15 {
 		t.Fatalf("misnumbered test case entries will make it hard to find bugs: %v", len(testCases))
 	}
 

diff --git a/builtin/logical/pki/ca_util.go b/builtin/logical/pki/ca_util.go
@@ -49,6 +49,7 @@ func getGenerationParams(sc *storageContext, data *framework.FieldData) (exporte
 		KeyType:                   keyType,
 		KeyBits:                   keyBits,
 		SignatureBits:             data.Get("signature_bits").(int),
+		UsePSS:                    data.Get("use_pss").(bool),
 		AllowLocalhost:            true,
 		AllowAnyName:              true,
 		AllowIPSANs:               true,

diff --git a/builtin/logical/pki/cert_util.go b/builtin/logical/pki/cert_util.go
@@ -138,6 +138,7 @@ func (sc *storageContext) fetchCAInfoByIssuerId(issuerId issuerID, usage issuerU
 		ParsedCertBundle:     *parsedBundle,
 		URLs:                 nil,
 		LeafNotAfterBehavior: entry.LeafNotAfterBehavior,
+		RevocationSigAlg:     entry.RevocationSigAlg,
 	}
 
 	entries, err := getURLs(sc.Context, sc.Storage)
@@ -1373,6 +1374,7 @@ func generateCreationBundle(b *backend, data *inputBundle, caSign *certutil.CAIn
 			KeyType:                       data.role.KeyType,
 			KeyBits:                       data.role.KeyBits,
 			SignatureBits:                 data.role.SignatureBits,
+			UsePSS:                        data.role.UsePSS,
 			NotAfter:                      notAfter,
 			KeyUsage:                      x509.KeyUsage(parseKeyUsages(data.role.KeyUsage)),
 			ExtKeyUsage:                   parseExtKeyUsages(data.role),

diff --git a/builtin/logical/pki/crl_test.go b/builtin/logical/pki/crl_test.go
@@ -2,6 +2,7 @@ package pki
 
 import (
 	"context"
+	"encoding/asn1"
 	"strings"
 	"testing"
 	"time"
@@ -25,6 +26,61 @@ func TestBackend_CRL_EnableDisableRoot(t *testing.T) {
 	crlEnableDisableTestForBackend(t, b, s, []string{caSerial})
 }
 
+func TestBackend_CRL_AllKeyTypeSigAlgos(t *testing.T) {
+	type testCase struct {
+		KeyType string
+		KeyBits int
+		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"},
+	}
+
+	for index, tc := range testCases {
+		t.Logf("tv %v", index)
+		b, s := createBackendWithStorage(t)
+
+		resp, err := CBWrite(b, s, "root/generate/internal", map[string]interface{}{
+			"ttl":         "40h",
+			"common_name": "myvault.com",
+			"key_type":    tc.KeyType,
+			"key_bits":    tc.KeyBits,
+		})
+		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)
+		}
+
+		crlEnableDisableTestForBackend(t, b, s, []string{caSerial})
+
+		crl := getParsedCrlFromBackend(t, b, s, "crl")
+		if strings.HasSuffix(tc.SigAlgo, "PSS") {
+			algo := crl.SignatureAlgorithm
+			pssOid := asn1.ObjectIdentifier{1, 2, 840, 113549, 1, 1, 10}
+			if !algo.Algorithm.Equal(pssOid) {
+				t.Fatalf("tc %v failed: expected sig-alg to be %v / got %v", index, pssOid, algo)
+			}
+		}
+	}
+}
+
 func TestBackend_CRL_EnableDisableIntermediateWithRoot(t *testing.T) {
 	crlEnableDisableIntermediateTestForBackend(t, true)
 }

diff --git a/builtin/logical/pki/crl_util.go b/builtin/logical/pki/crl_util.go
@@ -622,6 +622,7 @@ WRITE:
 		Number:              big.NewInt(crlNumber),
 		ThisUpdate:          time.Now(),
 		NextUpdate:          time.Now().Add(crlLifetime),
+		SignatureAlgorithm:  signingBundle.RevocationSigAlg,
 	}
 
 	crlBytes, err := x509.CreateRevocationList(rand.Reader, revocationListTemplate, signingBundle.Certificate, signingBundle.PrivateKey)

diff --git a/builtin/logical/pki/fields.go b/builtin/logical/pki/fields.go
@@ -319,6 +319,13 @@ SHA-2-512. Defaults to 0 to automatically detect based on key length
 		},
 	}
 
+	fields["use_pss"] = &framework.FieldSchema{
+		Type:    framework.TypeBool,
+		Default: false,
+		Description: `Whether or not to use PSS signatures when using a
+RSA key-type issuer. Defaults to false.`,
+	}
+
 	fields["key_type"] = &framework.FieldSchema{
 		Type:    framework.TypeString,
 		Default: "rsa",

diff --git a/builtin/logical/pki/path_fetch_issuers.go b/builtin/logical/pki/path_fetch_issuers.go
@@ -2,6 +2,7 @@ package pki
 
 import (
 	"context"
+	"crypto/x509"
 	"encoding/pem"
 	"fmt"
 	"strings"
@@ -106,6 +107,17 @@ this issuer; valid values are "read-only", "issuing-certificates", and
 and always set.`,
 		Default: []string{"read-only", "issuing-certificates", "crl-signing"},
 	}
+	fields["revocation_signature_algorithm"] = &framework.FieldSchema{
+		Type: framework.TypeString,
+		Description: `Which x509.SignatureAlgorithm name to use for
+signing CRLs. This parameter allows differentiation between PKCS#1v1.5
+and PSS keys and choice of signature hash algorithm. The default (empty
+string) value is for Go to select the signature algorithm. This can fail
+if the underlying key does not support the requested signature algorithm,
+which may not be known at modification time (such as with PKCS#11 managed
+RSA keys).`,
+		Default: "",
+	}
 
 	return &framework.Path{
 		// Returns a JSON entry.
@@ -179,16 +191,22 @@ func respondReadIssuer(issuer *issuerEntry) (*logical.Response, error) {
 		respManualChain = append(respManualChain, string(entity))
 	}
 
+	revSigAlgStr := issuer.RevocationSigAlg.String()
+	if issuer.RevocationSigAlg == x509.UnknownSignatureAlgorithm {
+		revSigAlgStr = ""
+	}
+
 	return &logical.Response{
 		Data: map[string]interface{}{
-			"issuer_id":               issuer.ID,
-			"issuer_name":             issuer.Name,
-			"key_id":                  issuer.KeyID,
-			"certificate":             issuer.Certificate,
-			"manual_chain":            respManualChain,
-			"ca_chain":                issuer.CAChain,
-			"leaf_not_after_behavior": issuer.LeafNotAfterBehavior.String(),
-			"usage":                   issuer.Usage.Names(),
+			"issuer_id":                      issuer.ID,
+			"issuer_name":                    issuer.Name,
+			"key_id":                         issuer.KeyID,
+			"certificate":                    issuer.Certificate,
+			"manual_chain":                   respManualChain,
+			"ca_chain":                       issuer.CAChain,
+			"leaf_not_after_behavior":        issuer.LeafNotAfterBehavior.String(),
+			"usage":                          issuer.Usage.Names(),
+			"revocation_signature_algorithm": revSigAlgStr,
 		},
 	}, nil
 }
@@ -258,6 +276,23 @@ func (b *backend) pathUpdateIssuer(ctx context.Context, req *logical.Request, da
 		return logical.ErrorResponse(fmt.Sprintf("Unable to parse specified usages: %v - valid values are %v", rawUsage, AllIssuerUsages.Names())), nil
 	}
 
+	// Revocation signature algorithm changes
+	revSigAlgStr := data.Get("revocation_signature_algorithm").(string)
+	revSigAlg, present := certutil.SignatureAlgorithmNames[strings.ToLower(revSigAlgStr)]
+	if !present && revSigAlgStr != "" {
+		var knownAlgos []string
+		for algoName := range certutil.SignatureAlgorithmNames {
+			knownAlgos = append(knownAlgos, algoName)
+		}
+
+		return logical.ErrorResponse(fmt.Sprintf("Unknown signature algorithm value: %v - valid values are %v", revSigAlg, strings.Join(knownAlgos, ", "))), nil
+	} else if revSigAlgStr == "" {
+		revSigAlg = x509.UnknownSignatureAlgorithm
+	}
+	if err := issuer.CanMaybeSignWithAlgo(revSigAlg); err != nil {
+		return nil, err
+	}
+
 	modified := false
 
 	var oldName string
@@ -277,6 +312,11 @@ func (b *backend) pathUpdateIssuer(ctx context.Context, req *logical.Request, da
 		modified = true
 	}
 
+	if revSigAlg != issuer.RevocationSigAlg {
+		issuer.RevocationSigAlg = revSigAlg
+		modified = true
+	}
+
 	var updateChain bool
 	var constructedChain []issuerID
 	for index, newPathRef := range newPath {
@@ -426,6 +466,32 @@ func (b *backend) pathPatchIssuer(ctx context.Context, req *logical.Request, dat
 		}
 	}
 
+	// Revocation signature algorithm changes
+	rawRevSigAlg, ok := data.GetOk("revocation_signature_algorithm")
+	if ok {
+		revSigAlgStr := rawRevSigAlg.(string)
+		revSigAlg, present := certutil.SignatureAlgorithmNames[strings.ToLower(revSigAlgStr)]
+		if !present && revSigAlgStr != "" {
+			var knownAlgos []string
+			for algoName := range certutil.SignatureAlgorithmNames {
+				knownAlgos = append(knownAlgos, algoName)
+			}
+
+			return logical.ErrorResponse(fmt.Sprintf("Unknown signature algorithm value: %v - valid values are %v", revSigAlg, strings.Join(knownAlgos, ", "))), nil
+		} else if revSigAlgStr == "" {
+			revSigAlg = x509.UnknownSignatureAlgorithm
+		}
+
+		if err := issuer.CanMaybeSignWithAlgo(revSigAlg); err != nil {
+			return nil, err
+		}
+
+		if revSigAlg != issuer.RevocationSigAlg {
+			issuer.RevocationSigAlg = revSigAlg
+			modified = true
+		}
+	}
+
 	// Manual Chain Changes
 	newPathData, ok := data.GetOk("manual_chain")
 	if ok {

diff --git a/builtin/logical/pki/path_intermediate.go b/builtin/logical/pki/path_intermediate.go
@@ -66,13 +66,18 @@ func (b *backend) pathGenerateIntermediate(ctx context.Context, req *logical.Req
 	// Nasty hack part two. :-) For generation of CSRs, certutil presently doesn't
 	// support configuration of this. However, because we need generation parameters,
 	// which create a role and attempt to read this parameter, we need to provide
-	// a value (which will be ignored). Hence, we stub in the missing parameter here,
+	// a value (which will be ignored). Hence, we stub in the missing parameters here,
 	// including its schema, just enough for it to work..
 	data.Schema["signature_bits"] = &framework.FieldSchema{
 		Type:    framework.TypeInt,
 		Default: 0,
 	}
 	data.Raw["signature_bits"] = 0
+	data.Schema["use_pss"] = &framework.FieldSchema{
+		Type:    framework.TypeBool,
+		Default: false,
+	}
+	data.Raw["use_pss"] = false
 
 	sc := b.makeStorageContext(ctx, req.Storage)
 	exported, format, role, errorResp := getGenerationParams(sc, data)

diff --git a/builtin/logical/pki/path_issue_sign.go b/builtin/logical/pki/path_issue_sign.go
@@ -151,6 +151,13 @@ SHA-2-512. Defaults to 0 to automatically detect based on key length
 		},
 	}
 
+	ret.Fields["use_pss"] = &framework.FieldSchema{
+		Type:    framework.TypeBool,
+		Default: false,
+		Description: `Whether or not to use PSS signatures when using a
+RSA key-type issuer. Defaults to false.`,
+	}
+
 	return ret
 }
 
@@ -211,6 +218,7 @@ func (b *backend) pathSignVerbatim(ctx context.Context, req *logical.Request, da
 		ExtKeyUsage:               data.Get("ext_key_usage").([]string),
 		ExtKeyUsageOIDs:           data.Get("ext_key_usage_oids").([]string),
 		SignatureBits:             data.Get("signature_bits").(int),
+		UsePSS:                    data.Get("use_pss").(bool),
 	}
 	*entry.AllowWildcardCertificates = true