From 12d875c18876a18354666fe7a9dd9c9169ad9f32 Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Thu, 7 Apr 2022 10:52:59 -0500 Subject: [PATCH] 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 --- builtin/logical/pki/backend_test.go | 120 ++++++++++++++---------- changelog/14943.txt | 3 + sdk/helper/certutil/helpers.go | 48 +++++----- website/content/api-docs/secret/pki.mdx | 4 + 4 files changed, 105 insertions(+), 70 deletions(-) create mode 100644 changelog/14943.txt 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