diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index f04d9fad9..785b3e285 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -702,6 +702,46 @@ func generateURLSteps(t *testing.T, caCert, caKey string, intdata, reqdata map[s return ret } +func generateCSR(t *testing.T, csrTemplate *x509.CertificateRequest, keyType string, keyBits int) (interface{}, []byte, string) { + var priv interface{} + var err error + switch keyType { + case "rsa": + priv, err = rsa.GenerateKey(rand.Reader, keyBits) + case "ec": + switch keyBits { + case 224: + priv, err = ecdsa.GenerateKey(elliptic.P224(), rand.Reader) + case 256: + priv, err = ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + case 384: + priv, err = ecdsa.GenerateKey(elliptic.P384(), rand.Reader) + case 521: + priv, err = ecdsa.GenerateKey(elliptic.P521(), rand.Reader) + default: + t.Fatalf("Got unknown ec< key bits: %v", keyBits) + } + case "ed25519": + _, priv, err = ed25519.GenerateKey(rand.Reader) + } + + if err != nil { + t.Fatalf("Got error generating private key for CSR: %v", err) + } + + csr, err := x509.CreateCertificateRequest(rand.Reader, csrTemplate, priv) + if err != nil { + t.Fatalf("Got error generating CSR: %v", err) + } + + csrPem := strings.TrimSpace(string(pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE REQUEST", + Bytes: csr, + }))) + + return priv, csr, csrPem +} + func generateCSRSteps(t *testing.T, caCert, caKey string, intdata, reqdata map[string]interface{}) []logicaltest.TestStep { csrTemplate := x509.CertificateRequest{ Subject: pkix.Name{ @@ -726,12 +766,7 @@ func generateCSRSteps(t *testing.T, caCert, caKey string, intdata, reqdata map[s }, } - priv, _ := rsa.GenerateKey(rand.Reader, 2048) - csr, _ := x509.CreateCertificateRequest(rand.Reader, &csrTemplate, priv) - csrPem := strings.TrimSpace(string(pem.EncodeToMemory(&pem.Block{ - Type: "CERTIFICATE REQUEST", - Bytes: csr, - }))) + _, _, csrPem := generateCSR(t, &csrTemplate, "rsa", 2048) ret := []logicaltest.TestStep{ { @@ -1260,7 +1295,7 @@ func generateRoleSteps(t *testing.T, useCSRs bool) []logicaltest.TestStep { for name, allowedInt := range cnMap { roleVals.KeyType = "rsa" roleVals.KeyBits = 2048 - if mathRand.Int()%2 == 1 { + if mathRand.Int()%3 == 1 { roleVals.KeyType = "ec" roleVals.KeyBits = 224 } @@ -1918,6 +1953,23 @@ func TestBackend_PathFetchCertList(t *testing.T) { } func TestBackend_SignVerbatim(t *testing.T) { + testCases := []struct { + testName string + keyType string + }{ + {testName: "RSA", keyType: "rsa"}, + {testName: "ED25519", keyType: "ed25519"}, + {testName: "EC", keyType: "ec"}, + {testName: "Any", keyType: "any"}, + } + for _, tc := range testCases { + t.Run(tc.testName, func(t *testing.T) { + runTestSignVerbatim(t, tc.keyType) + }) + } +} + +func runTestSignVerbatim(t *testing.T, keyType string) { // create the backend config := logical.TestBackendConfig() storage := &logical.InmemStorage{} @@ -1995,8 +2047,9 @@ func TestBackend_SignVerbatim(t *testing.T) { // create a role entry; we use this to check that sign-verbatim when used with a role is still honoring TTLs roleData := map[string]interface{}{ - "ttl": "4h", - "max_ttl": "8h", + "ttl": "4h", + "max_ttl": "8h", + "key_type": keyType, } resp, err = b.HandleRequest(context.Background(), &logical.Request{ Operation: logical.UpdateOperation, @@ -2109,6 +2162,7 @@ func TestBackend_SignVerbatim(t *testing.T) { "ttl": "4h", "max_ttl": "8h", "generate_lease": true, + "key_type": keyType, } resp, err = b.HandleRequest(context.Background(), &logical.Request{ Operation: logical.UpdateOperation, @@ -4216,6 +4270,145 @@ func TestBackend_Roles_IssuanceRegression(t *testing.T) { t.Fatal(err) } + // We need a RSA key so all signature sizes are valid with it. + resp, err := client.Logical().WriteWithContext(context.Background(), "pki/root/generate/exported", map[string]interface{}{ + "common_name": "myvault.com", + "ttl": "128h", + "key_type": "rsa", + "key_bits": 2048, + }) + if err != nil { + t.Fatal(err) + } + if resp == nil { + t.Fatal("expected ca info") + } + + tested := 0 + for index, test := range testCases { + tested += RoleIssuanceRegressionHelper(t, client, index, test) + } + + t.Log(fmt.Sprintf("Issuance regression expanded matrix test scenarios: %d", tested)) +} + +type KeySizeRegression struct { + RoleKeyType string + RoleKeyBits []int + RoleSignatureBits []int + + // These are tuples; must be of the same length. + TestKeyTypes []string + TestKeyBits []int + + // All of the above key types/sizes must pass or fail together. + ExpectError bool +} + +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, + }) + if err != nil { + t.Fatal(err) + } + + 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().WriteWithContext(context.Background(), "pki/sign/"+role, map[string]interface{}{ + "common_name": "localhost", + "csr": csrPem, + }) + + 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, role: %v, keyType: %v, keyBits: %v", index, haveErr, test.ExpectError, err, resp, test, role, keyType, keyBits) + } + + tested += 1 + } + } + } + + return tested +} + +func TestBackend_Roles_KeySizeRegression(t *testing.T) { + // Regression testing of role's issuance policy. + 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", "rsa", "ec", "ec", "ec", "ec", "ed25519"}, []int{512, 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", "rsa"}, []int{2048, 3072, 4906}, 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", "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}, + + // 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}, + // But it should work to issue Ed25519 keys. + /* 7 */ {"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}, + // 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}, + + // 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}, + } + + if len(testCases) != 11 { + t.Fatalf("misnumbered test case entries will make it hard to find bugs: %v", len(testCases)) + } + + coreConfig := &vault.CoreConfig{ + LogicalBackends: map[string]logical.Factory{ + "pki": Factory, + }, + } + cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{ + HandlerFunc: vaulthttp.Handler, + }) + cluster.Start() + defer cluster.Cleanup() + + client := cluster.Cores[0].Client + var err error + + // Generate a root CA at /pki to use for our tests + err = client.Sys().MountWithContext(context.Background(), "pki", &api.MountInput{ + Type: "pki", + Config: api.MountConfigInput{ + DefaultLeaseTTL: "12h", + MaxLeaseTTL: "128h", + }, + }) + if err != nil { + t.Fatal(err) + } + resp, err := client.Logical().WriteWithContext(context.Background(), "pki/root/generate/exported", map[string]interface{}{ "common_name": "myvault.com", "ttl": "128h", @@ -4231,10 +4424,10 @@ func TestBackend_Roles_IssuanceRegression(t *testing.T) { tested := 0 for index, test := range testCases { - tested += RoleIssuanceRegressionHelper(t, client, index, test) + tested += RoleKeySizeRegressionHelper(t, client, index, test) } - t.Log(fmt.Sprintf("Issuance regression expanded matrix test scenarios: %d", tested)) + t.Log(fmt.Sprintf("Key size regression expanded matrix test scenarios: %d", tested)) } var ( diff --git a/builtin/logical/pki/cert_util.go b/builtin/logical/pki/cert_util.go index 301f4534d..11ac905e0 100644 --- a/builtin/logical/pki/cert_util.go +++ b/builtin/logical/pki/cert_util.go @@ -673,6 +673,11 @@ func signCert(b *backend, return nil, errutil.UserError{Err: fmt.Sprintf("certificate request could not be parsed: %v", err)} } + // This switch validates that the CSR key type matches the role and sets + // the value in the actualKeyType/actualKeyBits values. + actualKeyType := "" + actualKeyBits := 0 + switch data.role.KeyType { case "rsa": // Verify that the key matches the role type @@ -681,24 +686,14 @@ func signCert(b *backend, "role requires keys of type %s", data.role.KeyType)} } + pubKey, ok := csr.PublicKey.(*rsa.PublicKey) if !ok { return nil, errutil.UserError{Err: "could not parse CSR's public key"} } - // Verify that the key is at least 2048 bits - if pubKey.N.BitLen() < 2048 { - return nil, errutil.UserError{Err: "RSA keys < 2048 bits are unsafe and not supported"} - } - - // Verify that the bit size is at least the size specified in the role - if pubKey.N.BitLen() < data.role.KeyBits { - return nil, errutil.UserError{Err: fmt.Sprintf( - "role requires a minimum of a %d-bit key, but CSR's key is %d bits", - data.role.KeyBits, - pubKey.N.BitLen())} - } - + actualKeyType = "rsa" + actualKeyBits = pubKey.N.BitLen() case "ec": // Verify that the key matches the role type if csr.PublicKeyAlgorithm != x509.ECDSA { @@ -711,14 +706,8 @@ func signCert(b *backend, return nil, errutil.UserError{Err: "could not parse CSR's public key"} } - // Verify that the bit size is at least the size specified in the role - if pubKey.Params().BitSize < data.role.KeyBits { - return nil, errutil.UserError{Err: fmt.Sprintf( - "role requires a minimum of a %d-bit key, but CSR's key is %d bits", - data.role.KeyBits, - pubKey.Params().BitSize)} - } - + actualKeyType = "ec" + actualKeyBits = pubKey.Params().BitSize case "ed25519": // Verify that the key matches the role type if csr.PublicKeyAlgorithm != x509.Ed25519 { @@ -726,31 +715,107 @@ func signCert(b *backend, "role requires keys of type %s", data.role.KeyType)} } + _, ok := csr.PublicKey.(ed25519.PublicKey) if !ok { return nil, errutil.UserError{Err: "could not parse CSR's public key"} } + actualKeyType = "ed25519" + actualKeyBits = 0 case "any": - // We only care about running RSA < 2048 bit checks, so if not RSA - // break out - if csr.PublicKeyAlgorithm != x509.RSA { - break - } + // We need to compute the actual key type and key bits, to correctly + // validate minimums and SignatureBits below. + switch csr.PublicKeyAlgorithm { + case x509.RSA: + pubKey, ok := csr.PublicKey.(*rsa.PublicKey) + if !ok { + return nil, errutil.UserError{Err: "could not parse CSR's public key"} + } + if pubKey.N.BitLen() < 2048 { + return nil, errutil.UserError{Err: "RSA keys < 2048 bits are unsafe and not supported"} + } - // Run RSA < 2048 bit checks - pubKey, ok := csr.PublicKey.(*rsa.PublicKey) - if !ok { - return nil, errutil.UserError{Err: "could not parse CSR's public key"} - } - if pubKey.N.BitLen() < 2048 { - return nil, errutil.UserError{Err: "RSA keys < 2048 bits are unsafe and not supported"} - } + actualKeyType = "rsa" + actualKeyBits = pubKey.N.BitLen() + case x509.ECDSA: + pubKey, ok := csr.PublicKey.(*ecdsa.PublicKey) + if !ok { + return nil, errutil.UserError{Err: "could not parse CSR's public key"} + } + actualKeyType = "ec" + actualKeyBits = pubKey.Params().BitSize + case x509.Ed25519: + _, ok := csr.PublicKey.(ed25519.PublicKey) + if !ok { + return nil, errutil.UserError{Err: "could not parse CSR's public key"} + } + + actualKeyType = "ed25519" + actualKeyBits = 0 + default: + return nil, errutil.UserError{Err: "Unknown key type in CSR: " + csr.PublicKeyAlgorithm.String()} + } default: return nil, errutil.InternalError{Err: fmt.Sprintf("unsupported key type value: %s", data.role.KeyType)} } + // Before validating key lengths, update our KeyBits/SignatureBits based + // on the actual CSR key type. + if data.role.KeyType == "any" { + // We update the value of KeyBits and SignatureBits here (from the + // role), using the specified key type. This allows us to convert + // the default value (0) for SignatureBits and KeyBits to a + // meaningful value. In the event KeyBits takes a zero value, we also + // update that to a new value. + // + // This is mandatory because on some roles, with KeyType any, we'll + // set a default SignatureBits to 0, but this will need to be updated + // in order to behave correctly during signing. + roleBitsWasZero := data.role.KeyBits == 0 + if data.role.KeyBits, data.role.SignatureBits, err = certutil.ValidateDefaultOrValueKeyTypeSignatureLength(actualKeyType, data.role.KeyBits, data.role.SignatureBits); err != nil { + return nil, errutil.InternalError{Err: fmt.Sprintf("unknown internal error updating default values: %v", err)} + } + + // We're using the KeyBits field as a minimum value, and P-224 is safe + // and a previously allowed value. However, the above call defaults + // to P-256 as that's a saner default than P-224 (w.r.t. generation). + // So, override our fake Role value if it was previously zero. + if actualKeyType == "ec" && roleBitsWasZero { + data.role.KeyBits = 224 + } + } + + // At this point, data.role.KeyBits and data.role.SignatureBits should both + // be non-zero, for RSA and ECDSA keys. Validate the actualKeyBits based on + // the role's values. If the KeyType was any, and KeyBits was set to 0, + // KeyBits should be updated to 2048 unless some other value was chosen + // explicitly. + // + // This validation needs to occur regardless of the role's key type, so + // that we always validate both RSA and ECDSA key sizes. + if actualKeyType == "rsa" { + if actualKeyBits < data.role.KeyBits { + return nil, errutil.UserError{Err: fmt.Sprintf( + "role requires a minimum of a %d-bit key, but CSR's key is %d bits", + data.role.KeyBits, actualKeyBits)} + } + + if actualKeyBits < 2048 { + return nil, errutil.UserError{Err: fmt.Sprintf( + "Vault requires a minimum of a 2048-bit key, but CSR's key is %d bits", + actualKeyBits)} + } + } else if actualKeyType == "ec" { + if actualKeyBits < data.role.KeyBits { + return nil, errutil.UserError{Err: fmt.Sprintf( + "role requires a minimum of a %d-bit key, but CSR's key is %d bits", + data.role.KeyBits, + actualKeyBits)} + } + } + creation, err := generateCreationBundle(b, data, caSign, csr) if err != nil { return nil, err diff --git a/changelog/14875.txt b/changelog/14875.txt new file mode 100644 index 000000000..ef4622d2e --- /dev/null +++ b/changelog/14875.txt @@ -0,0 +1,3 @@ +```release-note:bug +secrets/pki: Fix handling of "any" key type with default zero signature bits value. +``` diff --git a/sdk/helper/certutil/helpers.go b/sdk/helper/certutil/helpers.go index b6bee2e34..cb5c5b6ee 100644 --- a/sdk/helper/certutil/helpers.go +++ b/sdk/helper/certutil/helpers.go @@ -600,10 +600,11 @@ func DefaultOrValueHashBits(keyType string, keyBits int, hashBits int) (int, err // To match previous behavior (and ignoring NIST's recommendations for // hash size to align with RSA key sizes), default to SHA-2-256. hashBits = 256 - } else if keyType == "ed25519" || keyType == "ed448" { + } else if keyType == "ed25519" || keyType == "ed448" || keyType == "any" { // No-op; ed25519 and ed448 internally specify their own hash and // we do not need to select one. Double hashing isn't supported in - // certificate signing and we must + // certificate signing. Additionally, the any key type can't know + // what hash algorithm to use yet, so default to zero. return 0, nil } @@ -642,7 +643,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 == "ed25519" || keyType == "ed448" { + if keyType == "any" || 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: // @@ -654,6 +655,9 @@ func ValidateSignatureLength(keyType string, hashBits int) error { // // In all cases, we won't have a hash algorithm to validate here, so // return nil. + // + // Additionally, when KeyType is any, we can't yet validate the + // signature algorithm size, so it takes the default zero value. return nil }