From d04b143bd5a4ee6be4931583e447fb5765ef91d6 Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Fri, 8 Jul 2022 10:56:15 -0400 Subject: [PATCH] pki: When a role sets key_type to any ignore key_bits value when signing a csr (#16246) * pki: When a role sets key_type to any ignore key_bits value when signing - Bypass the validation for the role's key_bits value when signing CSRs if the key_type is set to any. We still validate the key is at least 2048 for RSA backed CSRs as we did in 1.9.x and lower. --- builtin/logical/pki/backend_test.go | 34 +++++++++++++++++++++++++ builtin/logical/pki/cert_util.go | 24 +++++++++-------- builtin/logical/pki/path_roles.go | 2 +- changelog/16246.txt | 3 +++ website/content/api-docs/secret/pki.mdx | 5 ++-- 5 files changed, 54 insertions(+), 14 deletions(-) create mode 100644 changelog/16246.txt diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 12c904b01..bfe0f4999 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -2486,6 +2486,40 @@ func TestBackend_SignIntermediate_AllowedPastCA(t *testing.T) { } } +func TestBackend_ConsulSignLeafWithLegacyRole(t *testing.T) { + // create the backend + b, s := createBackendWithStorage(t) + + // generate root + data, err := CBWrite(b, s, "root/generate/internal", map[string]interface{}{ + "ttl": "40h", + "common_name": "myvault.com", + }) + require.NoError(t, err, "failed generating internal root cert") + rootCaPem := data.Data["certificate"].(string) + + // Create a signing role like Consul did with the default args prior to Vault 1.10 + _, err = CBWrite(b, s, "roles/test", map[string]interface{}{ + "allow_any_name": true, + "allowed_serial_numbers": []string{"MySerialNumber"}, + "key_type": "any", + "key_bits": "2048", + "signature_bits": "256", + }) + require.NoError(t, err, "failed creating legacy role") + + _, csrPem := generateTestCsr(t, certutil.ECPrivateKey, 256) + data, err = CBWrite(b, s, "sign/test", map[string]interface{}{ + "csr": csrPem, + }) + require.NoError(t, err, "failed signing csr") + certAsPem := data.Data["certificate"].(string) + + signedCert := parseCert(t, certAsPem) + rootCert := parseCert(t, rootCaPem) + requireSignedBy(t, signedCert, rootCert.PublicKey) +} + func TestBackend_SignSelfIssued(t *testing.T) { // create the backend b, storage := createBackendWithStorage(t) diff --git a/builtin/logical/pki/cert_util.go b/builtin/logical/pki/cert_util.go index aa561542e..867435ad3 100644 --- a/builtin/logical/pki/cert_util.go +++ b/builtin/logical/pki/cert_util.go @@ -836,22 +836,24 @@ func signCert(b *backend, // 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. + // meaningful 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 { + // We ignore the role's original KeyBits value if the KeyType is any + // as legacy (pre-1.10) roles had default values that made sense only + // for RSA keys (key_bits=2048) and the older code paths ignored the role value + // set for KeyBits when KeyType was set to any. This also enforces the + // docs saying when key_type=any, we only enforce our specified minimums + // for signing operations + if data.role.KeyBits, data.role.SignatureBits, err = certutil.ValidateDefaultOrValueKeyTypeSignatureLength( + actualKeyType, 0, 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 + // We're using the KeyBits field as a minimum value below, 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 { + // to P-256 as that's a saner default than P-224 (w.r.t. generation), so + // override it here to allow 224 as the smallest size we permit. + if actualKeyType == "ec" { data.role.KeyBits = 224 } } diff --git a/builtin/logical/pki/path_roles.go b/builtin/logical/pki/path_roles.go index 8ad7ed7eb..1aab44686 100644 --- a/builtin/logical/pki/path_roles.go +++ b/builtin/logical/pki/path_roles.go @@ -587,7 +587,7 @@ func (b *backend) getRole(ctx context.Context, s logical.Storage, n string) (*ro modified = true } - // Ensure the role is valida fter updating. + // Ensure the role is valid after updating. _, err = validateRole(b, &result, ctx, s) if err != nil { return nil, err diff --git a/changelog/16246.txt b/changelog/16246.txt new file mode 100644 index 000000000..8883de147 --- /dev/null +++ b/changelog/16246.txt @@ -0,0 +1,3 @@ +```release-note:bug +secret/pki: Do not fail validation with a legacy key_bits default value and key_type=any when signing CSRs +``` diff --git a/website/content/api-docs/secret/pki.mdx b/website/content/api-docs/secret/pki.mdx index bc43bb8a0..d090709e4 100644 --- a/website/content/api-docs/secret/pki.mdx +++ b/website/content/api-docs/secret/pki.mdx @@ -2338,7 +2338,7 @@ request is denied. generated private keys and the type of key expected for submitted CSRs. Currently, `rsa`, `ec`, and `ed25519` are supported, or when signing existing CSRs, `any` can be specified to allow keys of either type - and with any bit size (subject to >1024 bits for RSA keys). + and with any bit size (subject to >=2048 bits for RSA keys or >= 224 for EC keys). ~> **Note**: In FIPS 140-2 mode, the following algorithms are not certified and thus should not be used: `ed25519`. @@ -2347,7 +2347,8 @@ request is denied. generated keys. Allowed values are 0 (universal default); with `key_type=rsa`, allowed values are: 2048 (default), 3072, or 4096; with `key_type=ec`, allowed values are: 224, 256 (default), - 384, or 521; ignored with `key_type=ed25519`. + 384, or 521; ignored with `key_type=ed25519` or in signing operations + when `key_type=any`. - `signature_bits` `(int: 0)` - Specifies the number of bits to use in the signature algorithm; accepts 256 for SHA-2-256, 384 for SHA-2-384,