From 31ff2be589fd360888d98971eff19dd2a9017dab Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Mon, 13 Dec 2021 15:26:42 -0500 Subject: [PATCH] Add universal default key_bits value for PKI endpoints (#13080) * Allow universal default for key_bits This allows the key_bits field to take a universal default value, 0, which, depending on key_type, gets adjusted appropriately into a specific default value (rsa->2048, ec->256, ignored under ed25519). Signed-off-by: Alexander Scheel * Handle universal default key size in certutil Also move RSA < 2048 error message into certutil directly, instead of in ca_util/path_roles. Signed-off-by: Alexander Scheel * Add missing RSA key sizes to pki/backend_test.go Signed-off-by: Alexander Scheel * Switch to returning updated values When determining the default, don't pass in pointer types, but instead return the newly updated value. Signed-off-by: Alexander Scheel * Add changelog entry Signed-off-by: Alexander Scheel * Re-add fix for ed25519 from #13254 Ed25519 internally specifies a hash length; by changing the default from 256 to 0, we fail validation in ValidateSignatureLength(...) unless we specify the key algorithm. Signed-off-by: Alexander Scheel --- builtin/logical/pki/backend_test.go | 10 +-- builtin/logical/pki/ca_util.go | 8 +-- builtin/logical/pki/fields.go | 11 +-- builtin/logical/pki/path_roles.go | 15 ++-- changelog/13080.txt | 3 + sdk/helper/certutil/helpers.go | 105 ++++++++++++++++++++++------ 6 files changed, 106 insertions(+), 46 deletions(-) create mode 100644 changelog/13080.txt diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 17dd6c11b..995bea598 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -1183,21 +1183,21 @@ func generateRoleSteps(t *testing.T, useCSRs bool) []logicaltest.TestStep { } getRandCsr := func(keyType string, errorOk bool, csrTemplate *x509.CertificateRequest) csrPlan { - rsaKeyBits := []int{2048, 4096} + rsaKeyBits := []int{2048, 3072, 4096} ecKeyBits := []int{224, 256, 384, 521} plan := csrPlan{errorOk: errorOk} var testBitSize int switch keyType { case "rsa": - plan.roleKeyBits = rsaKeyBits[mathRand.Int()%2] + plan.roleKeyBits = rsaKeyBits[mathRand.Int()%len(rsaKeyBits)] testBitSize = plan.roleKeyBits // If we don't expect an error already, randomly choose a // key size and expect an error if it's less than the role // setting if !keybitSizeRandOff && !errorOk { - testBitSize = rsaKeyBits[mathRand.Int()%2] + testBitSize = rsaKeyBits[mathRand.Int()%len(rsaKeyBits)] } if testBitSize < plan.roleKeyBits { @@ -1205,14 +1205,14 @@ func generateRoleSteps(t *testing.T, useCSRs bool) []logicaltest.TestStep { } case "ec": - plan.roleKeyBits = ecKeyBits[mathRand.Int()%4] + plan.roleKeyBits = ecKeyBits[mathRand.Int()%len(ecKeyBits)] testBitSize = plan.roleKeyBits // If we don't expect an error already, randomly choose a // key size and expect an error if it's less than the role // setting if !keybitSizeRandOff && !errorOk { - testBitSize = ecKeyBits[mathRand.Int()%4] + testBitSize = ecKeyBits[mathRand.Int()%len(ecKeyBits)] } if testBitSize < plan.roleKeyBits { diff --git a/builtin/logical/pki/ca_util.go b/builtin/logical/pki/ca_util.go index e5009386f..1e3da0333 100644 --- a/builtin/logical/pki/ca_util.go +++ b/builtin/logical/pki/ca_util.go @@ -50,12 +50,8 @@ func (b *backend) getGenerationParams( PostalCode: data.Get("postal_code").([]string), } - if role.KeyType == "rsa" && role.KeyBits < 2048 { - errorResp = logical.ErrorResponse("RSA keys < 2048 bits are unsafe and not supported") - return - } - - if err := certutil.ValidateKeyTypeSignatureLength(role.KeyType, role.KeyBits, &role.SignatureBits); err != nil { + var err error + if role.KeyBits, role.SignatureBits, err = certutil.ValidateDefaultOrValueKeyTypeSignatureLength(role.KeyType, role.KeyBits, role.SignatureBits); err != nil { errorResp = logical.ErrorResponse(err.Error()) } diff --git a/builtin/logical/pki/fields.go b/builtin/logical/pki/fields.go index fcb01d07b..67d705a50 100644 --- a/builtin/logical/pki/fields.go +++ b/builtin/logical/pki/fields.go @@ -250,12 +250,13 @@ the private key!`, fields["key_bits"] = &framework.FieldSchema{ Type: framework.TypeInt, - Default: 2048, - Description: `The number of bits to use. You will almost -certainly want to change this if you adjust -the key_type.`, + Default: 0, + Description: `The number of bits to use. Allowed values are +0 (universal default); with rsa key_type: 2048 (default), 3072, or +4096; with ec key_type: 224, 256 (default), 384, or 521; ignored with +ed25519.`, DisplayAttrs: &framework.DisplayAttributes{ - Value: 2048, + Value: 0, }, } diff --git a/builtin/logical/pki/path_roles.go b/builtin/logical/pki/path_roles.go index aa77dcf57..444e8071f 100644 --- a/builtin/logical/pki/path_roles.go +++ b/builtin/logical/pki/path_roles.go @@ -199,10 +199,11 @@ protection use. Defaults to false.`, "key_bits": { Type: framework.TypeInt, - Default: 2048, - Description: `The number of bits to use. You will almost -certainly want to change this if you adjust -the key_type.`, + Default: 0, + Description: `The number of bits to use. Allowed values are +0 (universal default); with rsa key_type: 2048 (default), 3072, or +4096; with ec key_type: 224, 256 (default), 384, or 521; ignored with +ed25519.`, }, "signature_bits": &framework.FieldSchema{ @@ -615,17 +616,13 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data *entry.GenerateLease = data.Get("generate_lease").(bool) } - if entry.KeyType == "rsa" && entry.KeyBits < 2048 { - return logical.ErrorResponse("RSA keys < 2048 bits are unsafe and not supported"), nil - } - if entry.MaxTTL > 0 && entry.TTL > entry.MaxTTL { return logical.ErrorResponse( `"ttl" value must be less than "max_ttl" value`, ), nil } - if err := certutil.ValidateKeyTypeSignatureLength(entry.KeyType, entry.KeyBits, &entry.SignatureBits); err != nil { + if entry.KeyBits, entry.SignatureBits, err = certutil.ValidateDefaultOrValueKeyTypeSignatureLength(entry.KeyType, entry.KeyBits, entry.SignatureBits); err != nil { return logical.ErrorResponse(err.Error()), nil } diff --git a/changelog/13080.txt b/changelog/13080.txt new file mode 100644 index 000000000..9c3ed52dc --- /dev/null +++ b/changelog/13080.txt @@ -0,0 +1,3 @@ +```release-note:bug +secrets/pki: Default value for key_bits changed to 0, enabling key_type=ec key generation with default value +``` diff --git a/sdk/helper/certutil/helpers.go b/sdk/helper/certutil/helpers.go index 8d47d619d..f7bd782a2 100644 --- a/sdk/helper/certutil/helpers.go +++ b/sdk/helper/certutil/helpers.go @@ -33,6 +33,14 @@ import ( cbasn1 "golang.org/x/crypto/cryptobyte/asn1" ) +const rsaMinimumSecureKeySize = 2048 + +// Mapping of key types to default key lengths +var defaultAlgorithmKeyBits = map[string]int { + "rsa": 2048, + "ec": 256, +} + // Mapping of NIST P-Curve's key length to expected signature bits. var expectedNISTPCurveHashBits = map[int]int{ 224: 256, @@ -533,14 +541,27 @@ func StringToOid(in string) (asn1.ObjectIdentifier, error) { return asn1.ObjectIdentifier(ret), nil } -// Validates that the combination of keyType, keyBits, and hashBits are -// valid together; replaces individual calls to ValidateSignatureLength and -// ValidateKeyTypeLength. -func ValidateKeyTypeSignatureLength(keyType string, keyBits int, hashBits *int) error { - if err := ValidateKeyTypeLength(keyType, keyBits); err != nil { - return err +// Returns default key bits for the specified key type, or the present value +// if keyBits is non-zero. +func DefaultOrValueKeyBits(keyType string, keyBits int) (int, error) { + if keyBits == 0 { + newValue, present := defaultAlgorithmKeyBits[keyType] + if present { + keyBits = newValue + } /* else { + // We cannot return an error here as ed25519 (and potentially ed448 + // in the future) aren't in defaultAlgorithmKeyBits -- the value of + // the keyBits parameter is ignored under that algorithm. + } */ } + return keyBits, nil +} + +// Returns default signature hash bit length for the specified key type and +// bits, or the present value if hashBits is non-zero. Returns an error under +// 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 @@ -548,35 +569,72 @@ func ValidateKeyTypeSignatureLength(keyType string, keyBits int, hashBits *int) // the "ec" key type. expectedHashBits := expectedNISTPCurveHashBits[keyBits] - if expectedHashBits != *hashBits && *hashBits != 0 { - return fmt.Errorf("unsupported signature hash algorithm length (%d) for NIST P-%d", *hashBits, keyBits) - } else if *hashBits == 0 { - *hashBits = expectedHashBits + 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 } - } else if keyType == "rsa" && *hashBits == 0 { - // To match previous behavior (and ignoring recommendations of hash - // size to match RSA key sizes), default to SHA-2-256. - *hashBits = 256 - } else if keyType == "ed25519" { + } 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. + hashBits = 256 + } else if keyType == "ed25519" || keyType == "ed448" { // 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. - return nil + // certificate signing and we must + return 0, nil + } + + return hashBits, nil +} + +// Validates that the combination of keyType, keyBits, and hashBits are +// valid together; replaces individual calls to ValidateSignatureLength and +// ValidateKeyTypeLength. Also updates the value of keyBits and hashBits on +// return. +func ValidateDefaultOrValueKeyTypeSignatureLength(keyType string, keyBits int, hashBits int) (int, int, error) { + var err error + + if keyBits, err = DefaultOrValueKeyBits(keyType, keyBits); err != nil { + return keyBits, hashBits, err + } + + if err = ValidateKeyTypeLength(keyType, keyBits); err != nil { + return keyBits, hashBits, err + } + + if hashBits, err = DefaultOrValueHashBits(keyType, keyBits, hashBits); err != nil { + return keyBits, hashBits, err } // Note that this check must come after we've selected a value for // hashBits above, in the event it was left as the default, but we // were allowed to update it. - if err := ValidateSignatureLength(*hashBits); err != nil || *hashBits == 0 { - return err + if err = ValidateSignatureLength(keyType, hashBits); err != nil { + return keyBits, hashBits, err } - return nil + return keyBits, hashBits, nil } // Validates that the length of the hash (in bits) used in the signature // calculation is a known, approved value. -func ValidateSignatureLength(hashBits int) error { +func ValidateSignatureLength(keyType string, hashBits int) error { + if 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: + // + // 1. Built-in hash (default, used in TLS, x509). + // 2. Double hash (notably used in some block-chain implementations, + // but largely regarded as a specialized use case with security + // concerns). + // 3. No hash (bring your own hash function, less commonly used). + // + // In all cases, we won't have a hash algorithm to validate here, so + // return nil. + return nil + } + switch hashBits { case 256: case 384: @@ -584,12 +642,17 @@ func ValidateSignatureLength(hashBits int) error { default: return fmt.Errorf("unsupported hash signature algorithm: %d", hashBits) } + return nil } func ValidateKeyTypeLength(keyType string, keyBits int) error { switch keyType { case "rsa": + if keyBits < rsaMinimumSecureKeySize { + return fmt.Errorf("RSA keys < %d bits are unsafe and not supported: got %d", rsaMinimumSecureKeySize, keyBits) + } + switch keyBits { case 2048: case 3072: