From 3428de017acd3cfd5d91c92a211ebfe377fea44f Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Fri, 15 Oct 2021 17:55:18 -0400 Subject: [PATCH] Forbid ssh key signing with specified extensions when role allowed_extensions is not set (#12847) * 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 | 51 +++++++++++++++++++++++++++-- builtin/logical/ssh/path_roles.go | 3 +- builtin/logical/ssh/path_sign.go | 22 +++++++------ changelog/12847.txt | 5 +++ 4 files changed, 68 insertions(+), 13 deletions(-) create mode 100644 changelog/12847.txt diff --git a/builtin/logical/ssh/backend_test.go b/builtin/logical/ssh/backend_test.go index b4253ba1c..55ba9f386 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) { @@ -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 no allowed 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 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}}", @@ -1444,7 +1491,7 @@ func TestBackend_DefExtTemplatingDisabled(t *testing.T) { 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 ac20d06b2..265581ec0 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 166beac76..459f88589 100644 --- a/builtin/logical/ssh/path_sign.go +++ b/builtin/logical/ssh/path_sign.go @@ -362,19 +362,21 @@ func (b *backend) calculateExtensions(data *framework.FieldData, req *logical.Re if len(unparsedExtensions) > 0 { extensions := convertMapToStringValue(unparsedExtensions) - if role.AllowedExtensions != "" { - notAllowed := []string{} - allowedExtensions := strings.Split(role.AllowedExtensions, ",") + if role.AllowedExtensions == "*" { + // Allowed extensions was configured to allow all + return extensions, nil + } - for extensionKey, _ := range extensions { - if !strutil.StrListContains(allowedExtensions, extensionKey) { - notAllowed = append(notAllowed, extensionKey) - } + notAllowed := []string{} + allowedExtensions := strings.Split(role.AllowedExtensions, ",") + for extensionKey, _ := range extensions { + if !strutil.StrListContains(allowedExtensions, extensionKey) { + notAllowed = append(notAllowed, extensionKey) } + } - if len(notAllowed) != 0 { - return nil, fmt.Errorf("extensions %v are not on allowed list", notAllowed) - } + if len(notAllowed) != 0 { + return nil, fmt.Errorf("extensions %v are not on allowed list", notAllowed) } return extensions, nil } diff --git a/changelog/12847.txt b/changelog/12847.txt new file mode 100644 index 000000000..30c92d0ee --- /dev/null +++ b/changelog/12847.txt @@ -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. +```