From 181e590c9671c06cc2c25a7f23d46d3dad93c907 Mon Sep 17 00:00:00 2001 From: Steve Clark Date: Fri, 15 Oct 2021 14:17:35 -0400 Subject: [PATCH 1/3] 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 | 13 +++++++- changelog/12847.txt | 5 +++ 4 files changed, 68 insertions(+), 4 deletions(-) create mode 100644 changelog/12847.txt diff --git a/builtin/logical/ssh/backend_test.go b/builtin/logical/ssh/backend_test.go index b4253ba1ce0b..88cbff74d681 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 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}}", @@ -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 ac20d06b2040..265581ec0584 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 166beac769e0..4c648a561a70 100644 --- a/builtin/logical/ssh/path_sign.go +++ b/builtin/logical/ssh/path_sign.go @@ -362,8 +362,19 @@ 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 { diff --git a/changelog/12847.txt b/changelog/12847.txt new file mode 100644 index 000000000000..30c92d0ee0a1 --- /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. +``` From 674e663252b97aa456ea3ff5949e3a3bec93afaa Mon Sep 17 00:00:00 2001 From: Steve Clark Date: Fri, 15 Oct 2021 16:46:09 -0400 Subject: [PATCH 2/3] Simplify code, removing unneeded if statements --- builtin/logical/ssh/path_sign.go | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/builtin/logical/ssh/path_sign.go b/builtin/logical/ssh/path_sign.go index 4c648a561a70..459f885896cc 100644 --- a/builtin/logical/ssh/path_sign.go +++ b/builtin/logical/ssh/path_sign.go @@ -366,26 +366,17 @@ func (b *backend) calculateExtensions(data *framework.FieldData, req *logical.Re // 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) + allowedExtensions := strings.Split(role.AllowedExtensions, ",") + for extensionKey, _ := range extensions { + if !strutil.StrListContains(allowedExtensions, extensionKey) { + notAllowed = append(notAllowed, extensionKey) } - return nil, fmt.Errorf("extensions %v are not on allowed list", notAllowed) } - if role.AllowedExtensions != "" { - 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 } From 0e974b2c740db722e6298b4b23ed167dd1d2de5e Mon Sep 17 00:00:00 2001 From: Steve Clark Date: Fri, 15 Oct 2021 16:49:25 -0400 Subject: [PATCH 3/3] Fix incorrect unit test comments --- builtin/logical/ssh/backend_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/logical/ssh/backend_test.go b/builtin/logical/ssh/backend_test.go index 88cbff74d681..55ba9f386e3c 100644 --- a/builtin/logical/ssh/backend_test.go +++ b/builtin/logical/ssh/backend_test.go @@ -1425,7 +1425,7 @@ func TestBackend_EmptyAllowedExtensionFailsClosed(t *testing.T) { } userpassAccessor := auths["userpass/"].Accessor - // Write SSH role to test with any extension. We also provide a templated default extension, + // 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", @@ -1442,7 +1442,7 @@ func TestBackend_EmptyAllowedExtensionFailsClosed(t *testing.T) { t.Fatal(err) } - // Issue SSH certificate with default extensions templating disabled, and no user-provided extensions + // Issue SSH certificate with default extensions templating disabled, and user-provided extensions client.SetToken(userpassToken) userProvidedAnyExtensionPermissions := map[string]string{ "login@foobar.com": "not_userpassname",