From f73c63beead26ed34468cf1d75f7b622c8a77a0f Mon Sep 17 00:00:00 2001 From: Steve Clark Date: Fri, 15 Oct 2021 14:17:35 -0400 Subject: [PATCH] Forbid ssh key signing with specified extensions when role allowed_extensions is not set - This is a behaviour change on how we process the allowed_extensions role parameter when it does not contain a value. The previous handling allowed a client to override and specify any extension they requested. - We now require a role to explicitly set this behaviour by setting the parameter to a '*' value which matches the behaviour of other keys such as allowed_users within the role. - No migration of existing roles is provided either, so operators if they truly want this behaviour will need to update existing roles appropriately. --- builtin/logical/ssh/backend_test.go | 55 ++++++++++++++++++++++++++--- builtin/logical/ssh/path_roles.go | 3 +- builtin/logical/ssh/path_sign.go | 15 ++++++-- changelog/12847.log | 5 +++ 4 files changed, 71 insertions(+), 7 deletions(-) create mode 100644 changelog/12847.log diff --git a/builtin/logical/ssh/backend_test.go b/builtin/logical/ssh/backend_test.go index b4253ba1ce0bf..31c350266d033 100644 --- a/builtin/logical/ssh/backend_test.go +++ b/builtin/logical/ssh/backend_test.go @@ -124,7 +124,6 @@ SjOQL/GkH1nkRcDS9++aAAAAAmNhAQID dockerImageTagSupportsRSA1 = "8.1_p1-r0-ls20" dockerImageTagSupportsNoRSA1 = "8.4_p1-r3-ls48" - ) func prepareTestContainer(t *testing.T, tag, caPublicKeyPEM string) (func(), string) { @@ -1349,7 +1348,7 @@ func TestBackend_DefExtTemplatingEnabled(t *testing.T) { t.Fatal(err) } - sshKeyID := "vault-userpass-"+testUserName+"-9bd0f01b7dfc50a13aa5e5cd11aea19276968755c8f1f9c98965d04147f30ed0" + sshKeyID := "vault-userpass-" + testUserName + "-9bd0f01b7dfc50a13aa5e5cd11aea19276968755c8f1f9c98965d04147f30ed0" // Issue SSH certificate with default extensions templating enabled, and no user-provided extensions client.SetToken(userpassToken) @@ -1414,6 +1413,53 @@ func TestBackend_DefExtTemplatingEnabled(t *testing.T) { } } +func TestBackend_EmptyAllowedExtensionFailsClosed(t *testing.T) { + cluster, userpassToken := getSshCaTestCluster(t, testUserName) + defer cluster.Cleanup() + client := cluster.Cores[0].Client + + // Get auth accessor for identity template. + auths, err := client.Sys().ListAuth() + if err != nil { + t.Fatal(err) + } + userpassAccessor := auths["userpass/"].Accessor + + // Write SSH role to test with any extension. We also provide a templated default extension, + // to verify that it's not actually being evaluated + _, err = client.Logical().Write("ssh/roles/test_allow_all_extensions", map[string]interface{}{ + "key_type": "ca", + "allow_user_certificates": true, + "allowed_users": "tuber", + "default_user": "tuber", + "allowed_extensions": "", + "default_extensions_template": false, + "default_extensions": map[string]interface{}{ + "login@foobar.com": "{{identity.entity.aliases." + userpassAccessor + ".name}}", + }, + }) + if err != nil { + t.Fatal(err) + } + + // Issue SSH certificate with default extensions templating disabled, and no user-provided extensions + client.SetToken(userpassToken) + userProvidedAnyExtensionPermissions := map[string]string{ + "login@foobar.com": "not_userpassname", + } + _, err = client.Logical().Write("ssh/sign/test_allow_all_extensions", map[string]interface{}{ + "public_key": publicKey4096, + "extensions": userProvidedAnyExtensionPermissions, + }) + if err == nil { + t.Fatal("Expected failure we should not have allowed specifying custom extensions") + } + + if !strings.Contains(err.Error(), "are not on allowed list") { + t.Fatalf("Expected failure to contain 'are not on allowed list' but was %s", err) + } +} + func TestBackend_DefExtTemplatingDisabled(t *testing.T) { cluster, userpassToken := getSshCaTestCluster(t, testUserName) defer cluster.Cleanup() @@ -1433,6 +1479,7 @@ func TestBackend_DefExtTemplatingDisabled(t *testing.T) { "allow_user_certificates": true, "allowed_users": "tuber", "default_user": "tuber", + "allowed_extensions": "*", "default_extensions_template": false, "default_extensions": map[string]interface{}{ "login@foobar.com": "{{identity.entity.aliases." + userpassAccessor + ".name}}", @@ -1442,9 +1489,9 @@ func TestBackend_DefExtTemplatingDisabled(t *testing.T) { t.Fatal(err) } - sshKeyID := "vault-userpass-"+testUserName+"-9bd0f01b7dfc50a13aa5e5cd11aea19276968755c8f1f9c98965d04147f30ed0" + sshKeyID := "vault-userpass-" + testUserName + "-9bd0f01b7dfc50a13aa5e5cd11aea19276968755c8f1f9c98965d04147f30ed0" -// Issue SSH certificate with default extensions templating disabled, and no user-provided extensions + // Issue SSH certificate with default extensions templating disabled, and no user-provided extensions client.SetToken(userpassToken) defaultExtensionPermissions := map[string]string{ "login@foobar.com": "{{identity.entity.aliases." + userpassAccessor + ".name}}", diff --git a/builtin/logical/ssh/path_roles.go b/builtin/logical/ssh/path_roles.go index ac20d06b20405..f8cd2ded206fb 100644 --- a/builtin/logical/ssh/path_roles.go +++ b/builtin/logical/ssh/path_roles.go @@ -245,7 +245,8 @@ func pathRoles(b *backend) *framework.Path { Description: ` [Not applicable for Dynamic type] [Not applicable for OTP type] [Optional for CA type] A comma-separated list of extensions that certificates can have when signed. - To allow any extensions, set this to an empty string. + An empty list means that no extension overrides are allowed by an end-user; explicitly + specify '*' to allow any extensions to be set. `, }, "default_critical_options": { diff --git a/builtin/logical/ssh/path_sign.go b/builtin/logical/ssh/path_sign.go index 166beac769e02..0d3278da0a0d8 100644 --- a/builtin/logical/ssh/path_sign.go +++ b/builtin/logical/ssh/path_sign.go @@ -362,11 +362,22 @@ func (b *backend) calculateExtensions(data *framework.FieldData, req *logical.Re if len(unparsedExtensions) > 0 { extensions := convertMapToStringValue(unparsedExtensions) + if role.AllowedExtensions == "*" { + // Allowed extensions was configured to allow all + return extensions, nil + } + notAllowed := []string{} + if role.AllowedExtensions == "" { + // If we don't have any explicit allowed_extensions configured fail closed if the user provided us any. + for k := range extensions { + notAllowed = append(notAllowed, k) + } + return nil, fmt.Errorf("extensions %v are not on allowed list", notAllowed) + } if role.AllowedExtensions != "" { - notAllowed := []string{} allowedExtensions := strings.Split(role.AllowedExtensions, ",") - for extensionKey, _ := range extensions { + for extensionKey := range extensions { if !strutil.StrListContains(allowedExtensions, extensionKey) { notAllowed = append(notAllowed, extensionKey) } diff --git a/changelog/12847.log b/changelog/12847.log new file mode 100644 index 0000000000000..a3648aeeb7089 --- /dev/null +++ b/changelog/12847.log @@ -0,0 +1,5 @@ +```release-note:breaking-change +secrets/ssh: Roles with empty allowed_extensions will now forbid end-users +specifying extensions when requesting ssh key signing. Update roles setting +allowed_extensions to '*' to permit any extension to be specified by an end-user. +``` \ No newline at end of file