SSH CA: allowed_users with template expansion fails when the templated item is a list

[candlerb]

Describe the bug
Given a template expansion in an SSH role such as

  "allowed_users": "root,{{identity.entity.metadata.ssh_username}}",
  "allowed_users_template": true,

This fails to match principals when the metadata item ssh_username is itself a comma-separated list.

To Reproduce

Create an SSH CA with allowed_users of the form shown above.

vault secrets enable -path=ssh-user-ca ssh
vault write ssh-user-ca/config/ca generate_signing_key=true
vault write ssh-user-ca/roles/ssh-admin - <<"EOH"
{
  "algorithm_signer": "rsa-sha2-256",
  "allow_user_certificates": true,
  "allowed_users": "root,{{identity.entity.metadata.ssh_username}}",
  "allowed_users_template": true,
  "allowed_extensions": "permit-pty,permit-agent-forwarding,permit-X11-forwarding,permit-port-forwarding",
  "default_extensions": {
     "permit-pty": "",
     "permit-agent-forwarding": ""
  },
  "key_type": "ca",
  "max_ttl": "12h",
  "ttl": "12h"
}
EOH

Case A: create an entity where the metadata is a single item

metadata               map[ssh_username:brian]

Case B: create an entity where the metadata is a comma-separated list

metadata               map[ssh_username:brian,ubuntu]

In both cases, login to vault as the appropriate entity and attempt to issue a certificate with one permitted principal, e.g.

vault login -method=oidc
vault write -field=signed_key ssh-user-ca/sign/ssh-admin public_key="@$HOME/.ssh/id_rsa.pub" valid_principals="brian" 

Expected behavior
I would expect in both case A and B, to be able to issue a certificate with principal "brian"

What happens is that case A works, but case B fails with

Error writing data to ssh-user-ca/sign/ssh-admin: Error making API request.

URL: PUT https://vault1.home.deploy2.net:8200/v1/ssh-user-ca/sign/ssh-admin
Code: 400. Errors:

* brian is not a valid value for valid_principals

It also fails if I pass valid_principals="brian,ubuntu" or valid_principals="root,brian,ubuntu". However it succeeds if I pass valid_principals="root" (since that value is statically present in allowed_principals)

Here is a copy of the entire entity for case B:

$ vault read identity/entity/id/11238cf6-074d-680a-3920-0f25d4c72670
Key                    Value
---                    -----
aliases                [map[canonical_id:11238cf6-074d-680a-3920-0f25d4c72670 creation_time:2021-02-23T16:19:28.658382282Z id:b2e97f47-bfaf-20ff-0f27-c6c8f84f2c1a last_update_time:2021-02-24T17:16:53.714546541Z merged_from_canonical_ids:[4254a4d7-1e74-fdbf-b44b-697c3383ff7a] metadata:map[email:XXXXXXXX@gmail.com name:Brian Candler] mount_accessor:auth_oidc_1b2fab39 mount_path:auth/google/ mount_type:oidc name:XXXXXXXX]]
creation_time          2021-02-22T17:57:16.318801957Z
direct_group_ids       []
disabled               false
group_ids              []
id                     11238cf6-074d-680a-3920-0f25d4c72670
inherited_group_ids    []
last_update_time       2021-03-03T18:27:48.27341616Z
merged_entity_ids      [4254a4d7-1e74-fdbf-b44b-697c3383ff7a]
metadata               map[ssh_username:brian,ubuntu]
name                   entity_a5aeeb8b
namespace_id           root
policies               [ssh-admin]

Environment:

• Vault Server Version (retrieve with vault status): 1.6.2
• Vault CLI Version (retrieve with vault version): Vault v1.6.3 ('b540be4b7ec48d0dd7512c8d8df9399d6bf84d76+CHANGES')
• Server Operating System/Architecture: Ubuntu 20.04 amd64

Vault server configuration file(s):

storage "file" {
  path = "/var/lib/vault"
}

listener "tcp" {
  address = "0.0.0.0:8200"
  tls_cert_file = "/etc/vault/vault-cert.pem"
  tls_key_file = "/etc/vault/vault-key.pem"
}

cluster_addr = "https://10.12.255.56:8201"
api_addr = "https://10.12.255.56:8200"
ui = "true"
disable_mlock="true"

Additional context
This behaviour is consistent with allowed_users being split into a list before template expansion is done, i.e. template expansion is done on each list item separately. Whether this is intentional or not is unclear. The syntax looks like it should work as simple inline string expansion, and I couldn't find documentation which says otherwise.

I checked SSH API, templated policies, and policy templating tutorial.

SSH API just says:

allowed_users_template (bool: false) - If set, allowed_users can be specified using identity template policies. Non-templated users are also permitted.

[candlerb]

Thinks: if the metadata item contained something that the user controlled (e.g. email??), and they could set themselves up with an E-mail address like root,user@somedomain.com, then I suppose expanding that might have unintended consequences. Allowing any user-controlled metadata as an ssh principal would be highly risky at best though.

I just found related issue #10388, where "allowed_users": "foo-{{ bar }}" is not allowed. It seems indeed that allowed_users is first split into a list, and only items in that list which match ^{{.+?}}$ are expanded.

[systemmonkey42]

Since templating is optional, some of the security around template formatting could be relaxed.

• Templating is part of role or policy and requires permissions to create.
• Individual roles have a flag which enables/disables templating
• It would be up to the admin to ensure uses cannot modify the data used in the template.

I would like to see one or more of the following;

1. Allow "{{identity.entity.groups.names}}" or equivalent so I can add users to "groups" named after principles they are allowed to use when getting an ssh certificate signed. (Ie, allow a template to expand to a list of items)
2. Allow metadata (in a group or user) which contains "," to be expanded into multiple principles.
3. Since {{template}}{{template}} works, I dont see why supporting {{template}}-{{template}} or prefix-{{template}} should not be handled, though I honestly see no use for this personally.

From reading the code its clear that;

• if I try and use {{identity.entity.groups.names}}, the result is a (string[]) and errors out with the misleading "value not found" error in aclTemplateHandler().
• if I add metadata containing 'name,name,name', then the principals string[] will contain the literal value, including commas, to the list of valid principals (which can never be matched).

I would suggest that updating the PopulateIdentityTemplate to return an array of strings, and appending the results to the list of allowedPrincipals would solve a number of problems, especially since I really doubt anyone is using the {{template}}{{template}} concatenation form.

Naturally you can't do everything. Eg, Would prefix-{{identity.entity.groups.names}} return a list of group names with prefix- prepended to each of them?, and prefix-{{identity.entity.groups.names}}{{identity.entity.metadata.ssh_username}} could return a long list if the number of groups were large, and the ssh_username metadata was a comma separated list..

Any thoughts?
I'm happy to start a PR on this, but it seems to be a discussion the dev don't want to have.