Allow identity templates in ssh backend default_user field
#16351
Conversation
ianferguson
force-pushed
the
ianferguson/default_user_template
branch
from
b81ed7f to
3aa20e0
Compare
There was a problem hiding this comment.
Thanks @ianferguson for the submission!
Overall this looks great! I do have a few little comments to bring this across the line, see individual other comments.
Additionally an update to the docs for the new field/behavior within ssh.mdx would be needed if you don't mind?
builtin/logical/ssh/backend_test.go
Outdated
| t.Errorf("signing request should fail when default_user is not in the allowed_users list, because allowed_users_template is true and default_user_template is not") | ||
| } | ||
|
|
||
| expectedErrStr := ":TKTKT" |
There was a problem hiding this comment.
Could we fix this test as it's failing now. I get the impression this expected error string was meant to be updated but was missed?
There was a problem hiding this comment.
it was, sorry about that! I've committed the expected value in 6ecf401
builtin/logical/ssh/backend_test.go
Outdated
| @@ -1893,6 +2034,66 @@ func getSshCaTestCluster(t *testing.T, userIdentity string) (*vault.TestCluster, | |||
| return cluster, userpassToken | |||
| } | |||
|
|
|||
| // TKTK | |||
There was a problem hiding this comment.
Could this comment be removed please?
There was a problem hiding this comment.
Sorry about that! I've removed it in 6ecf401
|
@stevendpclark Thanks for the review - I've addressed the comments (sorry about both of those!) in 6ecf401 and added the requested docs in 44ad5d4 |
| allowedPrincipals = append(allowedPrincipals, principal) | ||
| rendered, err := b.renderPrincipal(principal, req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("template '%s' could not be rendered -> %s", principal, err) |
There was a problem hiding this comment.
This is inconsistent with above (lines 77) -- I believe the err should be returned here and this message should be preserved as-is as we already contain a copy of the error message prefix you have here (so it'll be duplicated).
IOW, I think 77 is wrong too (return nil, err) to preserve existing behavior-ish.
There was a problem hiding this comment.
Oops good catch @cipherboy. I'll merge this PR and address this in another myself.
There was a problem hiding this comment.
previously (when only the allowed_users field could be templated) the template rendering happened in calculateValidPrincipals and the error on this line (return nil, fmt.Errorf("template '%s' could not be rendered -> %s", principal, err), on line 163 in the previous version) could be returned by that func,
That return value would then be caught on the previous version's line 74 which contained the logical.ErrorResponse wrapping, i.e. lines 73-76 prior to my changes were:
parsedPrincipals, err = b.calculateValidPrincipals(data, req, role, role.DefaultUser, role.AllowedUsers, strutil.StrListContains)
if err != nil {
return logical.ErrorResponse(err.Error()), nil
}
I was aiming to match the failure behaviors for templating errors in default_user to the existing behavior for allowed_users -- that all said, 👍 with any changes to it, y'all know the details on that better than me!
|
Thanks @ianferguson for the contribution! |
Fixes #16350 by enabling Identity attribute templating support for the
default_userfield on the SSH backend roles