From d72fb088849c5a90a5e9d3fb3605a62310ab642a Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Fri, 18 Feb 2022 16:48:16 -0600 Subject: [PATCH] Allow OpenSSH-style key type identifiers (#14143) * Allow OpenSSH-style key type identifiers To bring better parity with the changes of #14008, wherein we allowed OpenSSH-style key identifiers during generation. When specifying a list of allowed keys, validate against both OpenSSH-style key identifiers and the usual simplified names as well ("rsa" or "ecdsa"). Notably, the PKI secrets engine prefers "ec" over "ecdsa", so we permit both as well. Signed-off-by: Alexander Scheel * Fix missing quote in docs --- builtin/logical/ssh/backend_test.go | 57 ++++++++++++ builtin/logical/ssh/path_sign.go | 115 ++++++++++++++++-------- website/content/api-docs/secret/ssh.mdx | 16 +++- 3 files changed, 148 insertions(+), 40 deletions(-) diff --git a/builtin/logical/ssh/backend_test.go b/builtin/logical/ssh/backend_test.go index 1287dceda..adee82aa1 100644 --- a/builtin/logical/ssh/backend_test.go +++ b/builtin/logical/ssh/backend_test.go @@ -119,6 +119,9 @@ SjOQL/GkH1nkRcDS9++aAAAAAmNhAQID -----END OPENSSH PRIVATE KEY----- ` + publicKeyECDSA256 = `ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBJsfOouYIjJNI23QJqaDsFTGukm21fRAMeGvKZDB59i5jnX1EubMH1AEjjzz4fgySUlyWKo+TS31rxU8kX3DDM4= demo@example.com` + publicKeyECDSA521 = `ecdsa-sha2-nistp521 AAAAE2VjZHNhLXNoYTItbmlzdHA1MjEAAAAIbmlzdHA1MjEAAACFBAEg73ORD4J3FV2CrL01gLSKREO2EHrZPlJCOeDL5OKD3M1GCHv3q8O452RW49Aw+8zFFFU5u6d1Ys3Qsj05zdaQwQDt/D3ceWLGVkWiKyLPQStfn0GGOZh3YFKEw5XmeW9jh6xudEHlKs4Pfv2FrroaUKZvM2SlxR/feOK0tCQyq3MN/g== demo@example.com` + // testPublicKeyInstall is the public key that is installed in the // admin account's authorized_keys testPublicKeyInstall = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQC9i+hFxZHGo6KblVme4zrAcJstR6I0PTJozW286X4WyvPnkMYDQ5mnhEYC7UWCvjoTWbPEXPX7NjhRtwQTGD67bV+lrxgfyzK1JZbUXK4PwgKJvQD+XyyWYMzDgGSQY61KUSqCxymSm/9NZkPU3ElaQ9xQuTzPpztM4ROfb8f2Yv6/ZESZsTo0MTAkp8Pcy+WkioI/uJ1H7zqs0EA4OMY4aDJRu0UtP4rTVeYNEAuRXdX+eH4aW3KMvhzpFTjMbaJHJXlEeUm2SaX5TNQyTOvghCeQILfYIL/Ca2ij8iwCmulwdV6eQGfd4VDu40PvSnmfoaE38o6HaPnX0kUcnKiT" @@ -1307,6 +1310,60 @@ func TestBackend_AllowedUserKeyLengths(t *testing.T) { return nil }, }, + // Fail with ECDSA key + { + Operation: logical.UpdateOperation, + Path: "sign/multikey", + Data: map[string]interface{}{ + "public_key": publicKeyECDSA256, + }, + ErrorOk: true, + Check: func(resp *logical.Response) error { + if resp.Data["error"] != "public_key failed to meet the key requirements: key of type ecdsa is not allowed" { + return errors.New("an ECDSA key was allowed under RSA-only policy") + } + return nil + }, + }, + createRoleStep("ectypes", map[string]interface{}{ + "key_type": "ca", + "allow_user_certificates": true, + "allowed_user_key_lengths": map[string]interface{}{ + "ec": []int{256}, + "ecdsa-sha2-nistp521": 0, + }, + }), + // Pass with ECDSA P-256 + { + Operation: logical.UpdateOperation, + Path: "sign/ectypes", + Data: map[string]interface{}{ + "public_key": publicKeyECDSA256, + }, + }, + // Pass with ECDSA P-521 + { + Operation: logical.UpdateOperation, + Path: "sign/ectypes", + Data: map[string]interface{}{ + "public_key": publicKeyECDSA521, + }, + }, + // Fail with RSA key + { + Operation: logical.UpdateOperation, + Path: "sign/ectypes", + Data: map[string]interface{}{ + "public_key": publicKey3072, + }, + ErrorOk: true, + Check: func(resp *logical.Response) error { + if resp.Data["error"] != "public_key failed to meet the key requirements: key of type rsa is not allowed" { + return errors.New("an RSA key was allowed under ECDSA-only policy") + } + return nil + }, + }, }, } diff --git a/builtin/logical/ssh/path_sign.go b/builtin/logical/ssh/path_sign.go index 0f150710d..df6923feb 100644 --- a/builtin/logical/ssh/path_sign.go +++ b/builtin/logical/ssh/path_sign.go @@ -448,63 +448,100 @@ func (b *backend) calculateTTL(data *framework.FieldData, role *sshRole) (time.D func (b *backend) validateSignedKeyRequirements(publickey ssh.PublicKey, role *sshRole) error { if len(role.AllowedUserKeyTypesLengths) != 0 { - var kstr string - var kbits int + var keyType string + var keyBits int switch k := publickey.(type) { case ssh.CryptoPublicKey: ff := k.CryptoPublicKey() switch k := ff.(type) { case *rsa.PublicKey: - kstr = "rsa" - kbits = k.N.BitLen() + keyType = "rsa" + keyBits = k.N.BitLen() case *dsa.PublicKey: - kstr = "dsa" - kbits = k.Parameters.P.BitLen() + keyType = "dsa" + keyBits = k.Parameters.P.BitLen() case *ecdsa.PublicKey: - kstr = "ecdsa" - kbits = k.Curve.Params().BitSize + keyType = "ecdsa" + keyBits = k.Curve.Params().BitSize case ed25519.PublicKey: - kstr = "ed25519" + keyType = "ed25519" default: - return fmt.Errorf("public key type of %s is not allowed", kstr) + return fmt.Errorf("public key type of %s is not allowed", keyType) } default: return fmt.Errorf("pubkey not suitable for crypto (expected ssh.CryptoPublicKey but found %T)", k) } - if allowed_values, ok := role.AllowedUserKeyTypesLengths[kstr]; ok { - var pass bool - for _, value := range allowed_values { - switch kstr { - case "rsa": - if kbits == value { - pass = true - break - } - case "dsa": - if kbits == value { - pass = true - break - } - case "ecdsa": - if kbits == value { - pass = true - break - } - case "ed25519": - // ed25519 public keys are always 256 bits in length, - // so there is no need to inspect their value - pass = true - break - } + keyTypeToMapKey := map[string][]string{ + "rsa": {"rsa", ssh.KeyAlgoRSA}, + "dsa": {"dsa", ssh.KeyAlgoDSA}, + "ecdsa": {"ecdsa", "ec"}, + "ed25519": {"ed25519", ssh.KeyAlgoED25519}, + } + + if keyType == "ecdsa" { + ecCurveBitsToAlgoName := map[int]string{ + 256: ssh.KeyAlgoECDSA256, + 384: ssh.KeyAlgoECDSA384, + 521: ssh.KeyAlgoECDSA521, } - if !pass { - return fmt.Errorf("key is of an invalid size: %v", kbits) + if algo, ok := ecCurveBitsToAlgoName[keyBits]; ok { + keyTypeToMapKey[keyType] = append(keyTypeToMapKey[keyType], algo) } - } else { - return fmt.Errorf("key type of %s is not allowed", kstr) + + // If the algorithm is not found, it could be that we have a curve + // that we haven't added a constant for yet. But they could allow it + // (assuming x/crypto/ssh can parse it) via setting a ec: + // mapping rather than using a named SSH key type, so erring out here + // isn't advisable. + } + + var present bool + var pass bool + for _, kstr := range keyTypeToMapKey[keyType] { + allowed_values, ok := role.AllowedUserKeyTypesLengths[kstr] + if !ok { + continue + } + + present = true + + for _, value := range allowed_values { + if keyType == "rsa" || keyType == "dsa" { + // Regardless of map naming, we always need to validate the + // bit length of RSA and DSA keys. Use the keyType flag to + if keyBits == value { + pass = true + } + } else if kstr == "ec" || kstr == "ecdsa" { + // If the map string is "ecdsa", we have to validate the keyBits + // are a match for an allowed value, meaning that our curve + // is allowed. This isn't necessary when a named curve (e.g. + // ssh.KeyAlgoECDSA256) is allowed (and hence kstr is that), + // because keyBits is already specified in the kstr. Thus, + // we have conditioned around kstr and not keyType (like with + // rsa or dsa). + if keyBits == value { + pass = true + } + } else { + // We get here in two cases: we have a algo-named EC key + // matching a format specifier in the key map (e.g., a P-256 + // key with a KeyAlgoECDSA256 entry in the map) or we have a + // ed25519 key (which is always allowed). + pass = true + } + } + } + + if !present { + return fmt.Errorf("key of type %s is not allowed", keyType) + } + + if !pass { + return fmt.Errorf("key is of an invalid size: %v", keyBits) } } return nil diff --git a/website/content/api-docs/secret/ssh.mdx b/website/content/api-docs/secret/ssh.mdx index e58c742d0..a76a97bf1 100644 --- a/website/content/api-docs/secret/ssh.mdx +++ b/website/content/api-docs/secret/ssh.mdx @@ -213,7 +213,21 @@ This endpoint creates or updates a named role. - `allowed_user_key_lengths` `(map: "")` – Specifies a map of ssh key types and their expected sizes which are allowed to be signed by the CA type. To specify multiple sizes, either use a comma-separated list or an - array of allowed key widths. + array of allowed key widths. We support both OpenSSH-style key identifiers and + short names (`rsa`, `ecdsa`, `dsa`, or `ed25519`) as keys. For example, a valid + policy to allow common RSA and ECDSA key lengths might be: + + ``` + { + "rsa": [2048, 3072, 4096], + "ec": 256, + "ecdsa-sha2-nistp521": 0 + } + ``` + + Note that when an algorithm identifier uniquely specifies a key length (such as + with `ecdsa-sha2-nistp256` or `ed25519`), the value of the length is ignored (and + can be zero). - `algorithm_signer` `(string: "default")` - Algorithm to sign keys with. Valid values are `ssh-rsa`, `rsa-sha2-256`, `rsa-sha2-512`, or `default`. This