Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Evaluate ssh validprincipals user template before splitting #16622

Merged
merged 1 commit into from
Oct 13, 2022

Conversation

optiz0r
Copy link
Contributor

@optiz0r optiz0r commented Aug 7, 2022

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

@cipherboy
Copy link
Contributor

@optiz0r If you get a little bit, do you mind rebasing this? I think we merged some stuff that conflicted here...

@candlerb
Copy link
Contributor

candlerb commented Sep 2, 2022

Note: since #16350 / #16351, there is now a default_user_template setting which performs equivalent expansion on the default_user value. Despite the name of this setting, the value can also contain a comma-separated list.

Therefore, this needs checking for default_user as well: I don't know if it currently does split-before-expand or expand-before-split, but it needs to be consistent with allowed_users.

(See #10946. Ensuring allowed_users_template and default_user_template use the same expansion logic means you can automatically issue a certificate with all permitted principals, when none is requested).

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 hashicorp#11038
@optiz0r
Copy link
Contributor Author

optiz0r commented Oct 6, 2022

Could someone from Hashi check why the tests on this PR have not run? I was not able to get the full test suite running locally, and while I have been able to target the specific tests for the changes I've made, I would like to see that all tests are passing.

@rculpepper rculpepper closed this Oct 13, 2022
@rculpepper rculpepper reopened this Oct 13, 2022
@rculpepper
Copy link
Contributor

Hi @optiz0r ! I got the tests to rerun, and everything looks good, so I'll go ahead and merge it. Thanks for the contribution!

@rculpepper rculpepper merged commit 92b453d into hashicorp:main Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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