Add universal default key_bits value for PKI endpoints (hashicorp#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 <[email protected]>

* 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 <[email protected]>

* Add missing RSA key sizes to pki/backend_test.go

Signed-off-by: Alexander Scheel <[email protected]>

* 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 <[email protected]>

* Add changelog entry

Signed-off-by: Alexander Scheel <[email protected]>

* Re-add fix for ed25519 from hashicorp#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 <[email protected]>

builtin/logical/pki/backend_test.go

@@ -1183,36 +1183,36 @@ 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 {
 				plan.errorOk = true
 			}
 
 		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 {

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())
 	}
 

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,
 		},
 	}
 

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
 	}
 

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
+```

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,63 +541,118 @@ 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
 		// length corresponding to curve length. Note that ed25519 does not
 		// 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:
 	case 512:
 	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: