Evaluate ssh validprincipals user template before splitting (#16622)

The SSH secrets engine previously split the `validPrincipals` field
on comma, then if user templating is enabled, evaluated the
templates on each substring. This meant the identity template was only
ever allowed to return a single principal. There are use cases
where it would be helpful for identity metadata to contain a list
of valid principals and for the identity template to be able to inject
all of those as valid principals.

This change inverts the order of processing. First the template
is evaluated, and then the resulting string is split on commas.
This allows the identity template to return a single comma-separated
string with multiple permitted principals.

There is a potential security implication here, that if a user is
allowed to update their own identity metadata, they may be able to
elevate privileges where previously this was not possible.

Fixes #11038

builtin/logical/ssh/backend_test.go

@@ -28,6 +28,7 @@ import (
 const (
 	testIP              = "127.0.0.1"
 	testUserName        = "vaultssh"
+	testMultiUserName   = "vaultssh,otherssh"
 	testAdminUser       = "vaultssh"
 	testCaKeyType       = "ca"
 	testOTPKeyType      = "otp"
@@ -356,6 +357,15 @@ func TestBackend_AllowedUsersTemplate(t *testing.T) {
 	)
 }
 
+func TestBackend_MultipleAllowedUsersTemplate(t *testing.T) {
+	testAllowedUsersTemplate(t,
+		"{{ identity.entity.metadata.ssh_username }}",
+		testUserName, map[string]string{
+			"ssh_username": testMultiUserName,
+		},
+	)
+}
+
 func TestBackend_AllowedUsersTemplate_WithStaticPrefix(t *testing.T) {
 	testAllowedUsersTemplate(t,
 		"ssh-{{ identity.entity.metadata.ssh_username }}",

builtin/logical/ssh/path_issue_sign.go

@@ -176,18 +176,14 @@ func (b *backend) calculateValidPrincipals(data *framework.FieldData, req *logic
 	parsedPrincipals := strutil.RemoveDuplicates(strutil.ParseStringSlice(validPrincipals, ","), false)
 	// Build list of allowed Principals from template and static principalsAllowedByRole
 	var allowedPrincipals []string
-	for _, principal := range strutil.RemoveDuplicates(strutil.ParseStringSlice(principalsAllowedByRole, ","), false) {
-		if enableTemplating {
-			rendered, err := b.renderPrincipal(principal, req)
-			if err != nil {
-				return nil, err
-			}
-			// Template returned a principal
-			allowedPrincipals = append(allowedPrincipals, rendered)
-		} else {
-			// Static principal
-			allowedPrincipals = append(allowedPrincipals, principal)
+	if enableTemplating {
+		rendered, err := b.renderPrincipal(principalsAllowedByRole, req)
+		if err != nil {
+			return nil, err
 		}
+		allowedPrincipals = strutil.RemoveDuplicates(strutil.ParseStringSlice(rendered, ","), false)
+	} else {
+		allowedPrincipals = strutil.RemoveDuplicates(strutil.ParseStringSlice(principalsAllowedByRole, ","), false)
 	}
 
 	switch {

changelog/16622.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+secrets/ssh: Evaluate ssh validprincipals user template before splitting
+```