Fix handling of SignatureBits for ECDSA issuers (#14943)

When adding SignatureBits control logic, we incorrectly allowed
specification of SignatureBits in the case of an ECDSA issuer. As noted
in the original request, NIST and Mozilla (and others) are fairly
prescriptive in the choice of signatures (matching the size of the
NIST P-curve), and we shouldn't usually use a smaller (or worse, larger
and truncate!) hash.

Ignore the configuration of signature bits and always use autodetection
for ECDSA like ed25519.

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

builtin/logical/pki/backend_test.go

@@ -4476,8 +4476,11 @@ func TestBackend_Roles_IssuanceRegression(t *testing.T) {
 }
 
 type KeySizeRegression struct {
-	RoleKeyType       string
+	// Values reused for both Role and CA configuration.
-	RoleKeyBits       []int
+	RoleKeyType string
+	RoleKeyBits []int
+
+	// Signature Bits presently is only specified on the role.
 	RoleSignatureBits []int
 
 	// These are tuples; must be of the same length.
@@ -4488,42 +4491,73 @@ type KeySizeRegression struct {
 	ExpectError bool
 }
 
+func (k KeySizeRegression) KeyTypeValues() []string {
+	if k.RoleKeyType == "any" {
+		return []string{"rsa", "ec", "ed25519"}
+	}
+
+	return []string{k.RoleKeyType}
+}
+
 func RoleKeySizeRegressionHelper(t *testing.T, client *api.Client, index int, test KeySizeRegression) int {
 	tested := 0
 
-	for _, roleKeyBits := range test.RoleKeyBits {
+	for _, caKeyType := range test.KeyTypeValues() {
-		for _, roleSignatureBits := range test.RoleSignatureBits {
+		for _, caKeyBits := range test.RoleKeyBits {
-			role := fmt.Sprintf("key-size-regression-%d-keytype-%v-keybits-%d-signature-bits-%d", index, test.RoleKeyType, roleKeyBits, roleSignatureBits)
+			// Generate a new CA key.
-			resp, err := client.Logical().WriteWithContext(context.Background(), "pki/roles/"+role, map[string]interface{}{
+			resp, err := client.Logical().Write("pki/root/generate/exported", map[string]interface{}{
-				"key_type":       test.RoleKeyType,
+				"common_name": "myvault.com",
-				"key_bits":       roleKeyBits,
+				"ttl":         "128h",
-				"signature_bits": roleSignatureBits,
+				"key_type":    caKeyType,
+				"key_bits":    caKeyBits,
 			})
 			if err != nil {
 				t.Fatal(err)
 			}
+			if resp == nil {
+				t.Fatal("expected ca info")
+			}
 
-			for index, keyType := range test.TestKeyTypes {
+			for _, roleKeyBits := range test.RoleKeyBits {
-				keyBits := test.TestKeyBits[index]
+				for _, roleSignatureBits := range test.RoleSignatureBits {
+					role := fmt.Sprintf("key-size-regression-%d-keytype-%v-keybits-%d-signature-bits-%d", index, test.RoleKeyType, roleKeyBits, roleSignatureBits)
+					resp, err := client.Logical().Write("pki/roles/"+role, map[string]interface{}{
+						"key_type":       test.RoleKeyType,
+						"key_bits":       roleKeyBits,
+						"signature_bits": roleSignatureBits,
+					})
+					if err != nil {
+						t.Fatal(err)
+					}
 
-				_, _, csrPem := generateCSR(t, &x509.CertificateRequest{
+					for index, keyType := range test.TestKeyTypes {
-					Subject: pkix.Name{
+						keyBits := test.TestKeyBits[index]
-						CommonName: "localhost",
-					},
-				}, keyType, keyBits)
 
-				resp, err = client.Logical().WriteWithContext(context.Background(), "pki/sign/"+role, map[string]interface{}{
+						_, _, csrPem := generateCSR(t, &x509.CertificateRequest{
-					"common_name": "localhost",
+							Subject: pkix.Name{
-					"csr":         csrPem,
+								CommonName: "localhost",
-				})
+							},
+						}, keyType, keyBits)
 
-				haveErr := err != nil || resp == nil
+						resp, err = client.Logical().Write("pki/sign/"+role, map[string]interface{}{
+							"common_name": "localhost",
+							"csr":         csrPem,
+						})
 
-				if haveErr != test.ExpectError {
+						haveErr := err != nil || resp == nil
-					t.Fatalf("key size regression test [%d] failed: haveErr: %v, expectErr: %v, err: %v, resp: %v, test case: %v, role: %v, keyType: %v, keyBits: %v", index, haveErr, test.ExpectError, err, resp, test, role, keyType, keyBits)
+
+						if haveErr != test.ExpectError {
+							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)
+						}
+
+						tested += 1
+					}
 				}
+			}
 
-				tested += 1
+			_, err = client.Logical().Delete("pki/root")
+			if err != nil {
+				t.Fatal(err)
 			}
 		}
 	}
@@ -4543,26 +4577,29 @@ func TestBackend_Roles_KeySizeRegression(t *testing.T) {
 		// 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", "rsa", "rsa", "ec", "ed25519"}, []int{1024, 2048, 4096, 224, 0}, true},
-		// But it should work to issue larger EC keys.
+		// But it should work to issue larger EC keys. Note that we should be
-		/*  3 */ {"ec", []int{0, 256}, []int{0, 256}, []string{"ec"}, []int{256}, false},
+		// independent of signature bits as that's computed from the issuer
-		/*  4 */ {"ec", []int{384}, []int{0, 384}, []string{"ec"}, []int{384}, false},
+		// type (for EC based issuers).
-		/*  5 */ {"ec", []int{521}, []int{0, 512}, []string{"ec"}, []int{521}, false},
+		/*  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},
 
 		// Ed25519 should reject RSA and EC keys.
-		/*  6 */ {"ed25519", []int{0}, []int{0}, []string{"rsa", "rsa", "rsa", "ec", "ec"}, []int{1024, 2048, 4096, 256, 521}, true},
+		/*  7 */ {"ed25519", []int{0}, []int{0}, []string{"rsa", "rsa", "rsa", "ec", "ec"}, []int{1024, 2048, 4096, 256, 521}, true},
 		// But it should work to issue Ed25519 keys.
-		/*  7 */ {"ed25519", []int{0}, []int{0}, []string{"ed25519"}, []int{0}, false},
+		/*  8 */ {"ed25519", []int{0}, []int{0}, []string{"ed25519"}, []int{0}, false},
 
 		// Any key type should reject insecure RSA key sizes.
-		/*  8 */ {"any", []int{0}, []int{0, 256, 384, 512}, []string{"rsa", "rsa"}, []int{512, 1024}, true},
+		/*  9 */ {"any", []int{0}, []int{0, 256, 384, 512}, []string{"rsa", "rsa"}, []int{512, 1024}, true},
 		// But work for everything else.
-		/*  9 */ {"any", []int{0}, []int{0, 256, 384, 512}, []string{"rsa", "rsa", "rsa", "ec", "ec", "ec", "ec", "ed25519"}, []int{2048, 3072, 4906, 224, 256, 384, 521, 0}, false},
+		/* 10 */ {"any", []int{0}, []int{0, 256, 384, 512}, []string{"rsa", "rsa", "rsa", "ec", "ec", "ec", "ec", "ed25519"}, []int{2048, 3072, 4906, 224, 256, 384, 521, 0}, false},
 
 		// RSA with larger than default key size should reject smaller ones.
-		/* 10 */ {"rsa", []int{3072}, []int{0, 256, 384, 512}, []string{"rsa", "rsa", "rsa"}, []int{512, 1024, 2048}, true},
+		/* 11 */ {"rsa", []int{3072}, []int{0, 256, 384, 512}, []string{"rsa", "rsa", "rsa"}, []int{512, 1024, 2048}, true},
 	}
 
-	if len(testCases) != 11 {
+	if len(testCases) != 12 {
 		t.Fatalf("misnumbered test case entries will make it hard to find bugs: %v", len(testCases))
 	}
 
@@ -4581,7 +4618,7 @@ func TestBackend_Roles_KeySizeRegression(t *testing.T) {
 	var err error
 
 	// Generate a root CA at /pki to use for our tests
-	err = client.Sys().MountWithContext(context.Background(), "pki", &api.MountInput{
+	err = client.Sys().Mount("pki", &api.MountInput{
 		Type: "pki",
 		Config: api.MountConfigInput{
 			DefaultLeaseTTL: "12h",
@@ -4592,19 +4629,6 @@ func TestBackend_Roles_KeySizeRegression(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	resp, err := client.Logical().WriteWithContext(context.Background(), "pki/root/generate/exported", map[string]interface{}{
-		"common_name": "myvault.com",
-		"ttl":         "128h",
-		"key_type":    "ec",
-		"key_bits":    256,
-	})
-	if err != nil {
-		t.Fatal(err)
-	}
-	if resp == nil {
-		t.Fatal("expected ca info")
-	}
-
 	tested := 0
 	for index, test := range testCases {
 		tested += RoleKeySizeRegressionHelper(t, client, index, test)

changelog/14943.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+secrets/pki: Fixed bug where larger SHA-2 hashes were truncated with shorter ECDSA CA certificates
+```

sdk/helper/certutil/helpers.go

@@ -585,17 +585,8 @@ func DefaultOrValueKeyBits(keyType string, keyBits int) (int, error) {
 // certain internal circumstances.
 func DefaultOrValueHashBits(keyType string, keyBits int, hashBits int) (int, error) {
 	if keyType == "ec" {
-		// To comply with BSI recommendations Section 4.2 and Mozilla root
+		// Enforcement of curve moved to selectSignatureAlgorithmForECDSA. See
-		// store policy section 5.1.2, enforce that NIST P-curves use a hash
+		// note there about why.
-		// length corresponding to curve length. Note that ed25519 does not
-		// the "ec" key type.
-		expectedHashBits := expectedNISTPCurveHashBits[keyBits]
-
-		if expectedHashBits != hashBits && hashBits != 0 {
-			return hashBits, fmt.Errorf("unsupported signature hash algorithm length (%d) for NIST P-%d", hashBits, keyBits)
-		} else if hashBits == 0 {
-			hashBits = expectedHashBits
-		}
 	} else if keyType == "rsa" && hashBits == 0 {
 		// To match previous behavior (and ignoring NIST's recommendations for
 		// hash size to align with RSA key sizes), default to SHA-2-256.
@@ -643,7 +634,7 @@ func ValidateDefaultOrValueKeyTypeSignatureLength(keyType string, keyBits int, h
 // Validates that the length of the hash (in bits) used in the signature
 // calculation is a known, approved value.
 func ValidateSignatureLength(keyType string, hashBits int) error {
-	if keyType == "any" || keyType == "ed25519" || keyType == "ed448" {
+	if keyType == "any" || keyType == "ec" || keyType == "ed25519" || keyType == "ed448" {
 		// ed25519 and ed448 include built-in hashing and is not externally
 		// configurable. There are three modes for each of these schemes:
 		//
@@ -658,6 +649,10 @@ func ValidateSignatureLength(keyType string, hashBits int) error {
 		//
 		// Additionally, when KeyType is any, we can't yet validate the
 		// signature algorithm size, so it takes the default zero value.
+		//
+		// When KeyType is ec, we also can't validate this value as we're
+		// forcefully ignoring the users' choice and specifying a value based
+		// on issuer type.
 		return nil
 	}
 
@@ -863,16 +858,25 @@ func createCertificate(data *CreationBundle, randReader io.Reader, privateKeyGen
 }
 
 func selectSignatureAlgorithmForECDSA(pub crypto.PublicKey, signatureBits int) x509.SignatureAlgorithm {
-	// If signature bits are configured, prefer them to the default choice.
+	// Previously we preferred the user-specified signature bits for ECDSA
-	switch signatureBits {
+	// keys. However, this could result in using a longer hash function than
-	case 256:
+	// the underlying NIST P-curve will encode (e.g., a SHA-512 hash with a
-		return x509.ECDSAWithSHA256
+	// P-256 key). This isn't ideal: the hash is implicitly truncated
-	case 384:
+	// (effectively turning it into SHA-512/256) and we then need to rely
-		return x509.ECDSAWithSHA384
+	// on the prefix security of the hash. Since both NIST and Mozilla guidance
-	case 512:
+	// suggest instead using the correct hash function, we should prefer that
-		return x509.ECDSAWithSHA512
+	// over the operator-specified signatureBits.
-	}
+	//
-
+	// Lastly, note that pub above needs to be the _signer's_ public key;
+	// the issue with DefaultOrValueHashBits is that it is called at role
+	// configuration time, which might _precede_ issuer generation. Thus
+	// it only has access to the desired key type and not the actual issuer.
+	// The reference from that function is reproduced below:
+	//
+	// > To comply with BSI recommendations Section 4.2 and Mozilla root
+	// > store policy section 5.1.2, enforce that NIST P-curves use a hash
+	// > length corresponding to curve length. Note that ed25519 does not
+	// > implement the "ec" key type.
 	key, ok := pub.(*ecdsa.PublicKey)
 	if !ok {
 		return x509.ECDSAWithSHA256

website/content/api-docs/secret/pki.mdx

@@ -973,6 +973,10 @@ request is denied.
   on key length (SHA-2-256 for RSA keys, and matching the curve size
   for NIST P-Curves).
 
+~> **Note**: ECDSA and Ed25519 issuers do not follow configuration of the
+   `signature_bits` value; only RSA issuers will change signature types
+   based on this parameter.
+
 - `key_usage` `(list: ["DigitalSignature", "KeyAgreement", "KeyEncipherment"])` –
   Specifies the allowed key usage constraint on issued certificates. Valid
   values can be found at https://golang.org/pkg/crypto/x509/#KeyUsage - simply