Fix handling of SignatureBits for ECDSA issuers

builtin/logical/pki/backend_test.go

@@ -4476,8 +4476,11 @@ func TestBackend_Roles_IssuanceRegression(t *testing.T) {
 }
 
 type KeySizeRegression struct {
-	RoleKeyType       string
-	RoleKeyBits       []int
+	// Values reused for both Role and CA configuration.
+	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 _, 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().WriteWithContext(context.Background(), "pki/roles/"+role, map[string]interface{}{
-				"key_type":       test.RoleKeyType,
-				"key_bits":       roleKeyBits,
-				"signature_bits": roleSignatureBits,
+	for _, caKeyType := range test.KeyTypeValues() {
+		for _, caKeyBits := range test.RoleKeyBits {
+			// Generate a new CA key.
+			resp, err := client.Logical().Write("pki/root/generate/exported", map[string]interface{}{
+				"common_name": "myvault.com",
+				"ttl":         "128h",
+				"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 {
-				keyBits := test.TestKeyBits[index]
+			for _, roleKeyBits := range test.RoleKeyBits {
+				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{
-					Subject: pkix.Name{
-						CommonName: "localhost",
-					},
-				}, keyType, keyBits)
+					for index, keyType := range test.TestKeyTypes {
+						keyBits := test.TestKeyBits[index]
+
+						_, _, csrPem := generateCSR(t, &x509.CertificateRequest{
+							Subject: pkix.Name{
+								CommonName: "localhost",
+							},
+						}, keyType, keyBits)
+
+						resp, err = client.Logical().Write("pki/sign/"+role, map[string]interface{}{
+							"common_name": "localhost",
+							"csr":         csrPem,
+						})
 
-				resp, err = client.Logical().WriteWithContext(context.Background(), "pki/sign/"+role, map[string]interface{}{
-					"common_name": "localhost",
-					"csr":         csrPem,
-				})
+						haveErr := err != nil || resp == nil
 
-				haveErr := err != nil || resp == nil
+						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)
+						}
 
-				if haveErr != test.ExpectError {
-					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)
+						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.
-		/*  3 */ {"ec", []int{0, 256}, []int{0, 256}, []string{"ec"}, []int{256}, false},
-		/*  4 */ {"ec", []int{384}, []int{0, 384}, []string{"ec"}, []int{384}, false},
-		/*  5 */ {"ec", []int{521}, []int{0, 512}, []string{"ec"}, []int{521}, false},
+		// 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},
 
 		// 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
-		// store policy section 5.1.2, enforce that NIST P-curves use a hash
-		// 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
-		}
+		// Enforcement of curve moved to selectSignatureAlgorithmForECDSA. See
+		// note there about why.
 	} 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.
-	switch signatureBits {
-	case 256:
-		return x509.ECDSAWithSHA256
-	case 384:
-		return x509.ECDSAWithSHA384
-	case 512:
-		return x509.ECDSAWithSHA512
-	}
-
+	// Previously we preferred the user-specified signature bits for ECDSA
+	// keys. However, this could result in using a longer hash function than
+	// the underlying NIST P-curve will encode (e.g., a SHA-512 hash with a
+	// P-256 key). This isn't ideal: the hash is implicitly truncated
+	// (effectively turning it into SHA-512/256) and we then need to rely
+	// on the prefix security of the hash. Since both NIST and Mozilla guidance
+	// suggest instead using the correct hash function, we should prefer that
+	// 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