diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 784d389a9..d9d6265f2 100644 --- a/builtin/logical/pki/backend_test.go +++ b/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] - resp, err = client.Logical().WriteWithContext(context.Background(), "pki/sign/"+role, map[string]interface{}{ - "common_name": "localhost", - "csr": csrPem, - }) + _, _, csrPem := generateCSR(t, &x509.CertificateRequest{ + Subject: pkix.Name{ + 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 { - 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) + 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) + } + + 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) diff --git a/changelog/14943.txt b/changelog/14943.txt new file mode 100644 index 000000000..1501c563d --- /dev/null +++ b/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 +``` diff --git a/sdk/helper/certutil/helpers.go b/sdk/helper/certutil/helpers.go index cb5c5b6ee..c26020f43 100644 --- a/sdk/helper/certutil/helpers.go +++ b/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 diff --git a/website/content/api-docs/secret/pki.mdx b/website/content/api-docs/secret/pki.mdx index 19196d62c..e4b466d92 100644 --- a/website/content/api-docs/secret/pki.mdx +++ b/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