luxolus
/
open-vault
2
0
Fork 0
Code Issues 1 Pull Requests Releases Activity

Fix handling of default zero SignatureBits value with Any key type in PKI Secrets Engine (#14875) 

* Correctly handle minimums, default SignatureBits

When using KeyType = "any" on a role (whether explicitly or implicitly
via a sign-verbatim like operation), we need to update the value of
SignatureBits from its new value 0 to a per-key-type default value. This
will allow sign operations on these paths to function correctly, having
the correctly inferred default signature bit length.

Additionally, this allows the computed default value for key type to be
used for minimum size validation in the RSA/ECDSA paths. We additionally
enforce the 2048-minimum in this case as well.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Fix defaults and validation of "any" KeyType

When certutil is given the placeholder any keytype, it attempts to
validate and update the default zero value. However, in lacking a
default value for SignatureBits, it cannot update the value from the
zero value, thus causing validation to fail.

Add more awareness to the placeholder "any" value to certutil.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add role-based regression tests for key bits

This adds regression tests for Key Type, Key Bits, and Signature Bits
parameters on the role. We test several values, including the "any"
value to ensure it correctly restricts key sizes.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add sign-verbatim test for key type

This ensures that we test sign-verbatim against a variety of key types.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add changelog entry

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

Co-authored-by: Steven Clark <steven.clark@hashicorp.com>
This commit is contained in:
Alexander Scheel 2022-04-04 14:26:54 -05:00 committed by GitHub
parent 1b62e17f89
commit 8904f2a55a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 313 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

215
builtin/logical/pki/backend_test.go
View File

@ -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 (

133
builtin/logical/pki/cert_util.go
View File

@ -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

3
changelog/14875.txt Normal file
View File

@ -0,0 +1,3 @@
```release-note:bug
secrets/pki: Fix handling of "any" key type with default zero signature bits value.
```

10
sdk/helper/certutil/helpers.go
View File

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