Forbid ssh key signing with specified extensions when role allowed_ex…

…tensions 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

@@ -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{}{
+			"[email protected]": "{{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{
+		"[email protected]": "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{}{
 			"[email protected]": "{{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{
 		"[email protected]": "{{identity.entity.aliases." + userpassAccessor + ".name}}",

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": {

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
 	}

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.
+```