From 92b453d3a8985571aa8d0769cc0245de47cba247 Mon Sep 17 00:00:00 2001
From: Ben Roberts <me@benroberts.net>
Date: Thu, 13 Oct 2022 23:34:36 +0100
Subject: [PATCH] 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    | 10 ++++++++++
 builtin/logical/ssh/path_issue_sign.go | 18 +++++++-----------
 changelog/16622.txt                    |  3 +++
 3 files changed, 20 insertions(+), 11 deletions(-)
 create mode 100644 changelog/16622.txt

diff --git a/builtin/logical/ssh/backend_test.go b/builtin/logical/ssh/backend_test.go
index 27934d42af..e6dc9aee17 100644
--- a/builtin/logical/ssh/backend_test.go
+++ b/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 }}",
diff --git a/builtin/logical/ssh/path_issue_sign.go b/builtin/logical/ssh/path_issue_sign.go
index 8e6056c46a..0ce45d5189 100644
--- a/builtin/logical/ssh/path_issue_sign.go
+++ b/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 {
diff --git a/changelog/16622.txt b/changelog/16622.txt
new file mode 100644
index 0000000000..37ae5abb5a
--- /dev/null
+++ b/changelog/16622.txt
@@ -0,0 +1,3 @@
+```release-note:improvement
+secrets/ssh: Evaluate ssh validprincipals user template before splitting
+```