Add support for templated values in SSH CA DefaultExtensions. #11495
Conversation
|
|
builtin/logical/ssh/path_sign.go
Outdated
| return nil, fmt.Errorf("extensions %v are not on allowed list", notAllowed) | ||
| if role.DefaultExtensionsTemplate { | ||
| templatedExtensions := make(map[string]string) | ||
| for extensionKey, extensionValue := range extensions { |
There was a problem hiding this comment.
At this point in time the extensions map could contain user provided extensions (from the data.Get("extensions") call above). The boolean you added only (by name) covers the DefaultExtensions. It may be safer to only allow default extensions to be templated so users cannot query unintended identity data. We could return early above if user provided extensions are passed.
There was a problem hiding this comment.
Ah, excllent point. Let me re-think this portion; originally I had struggled a bit with working out all of the possible unintended consequences (like this).
There was a problem hiding this comment.
So, I'm going to send an additional commit here, to demonstrate my thought process. Ideally, I think we'd want to have Default Extensions that are configured by the role (and templated, if the flag is enabled), but also if there are Allowed Extensions configured, then take user input and override the Default Extensions (sans templating). Is that a more reasonable approach?
There was a problem hiding this comment.
I think that sounds right. The existing logic prioritizes user input and does not try to merge it with the default extensions. I think we should continue to have the same semantics. So if user supplied extensions exist process those (without templating), else use the defaults with optional templating.
There was a problem hiding this comment.
Thanks for the feedback - I've sent a commit to rework the logic, and also add a basic test - can extend this test structure, if it looks acceptable.
robison
force-pushed
the
robison/add-ssh-ca-extension-templating
branch
from
3e86485
to
80133b8
Compare
robison
force-pushed
the
robison/add-ssh-ca-extension-templating
branch
from
80133b8
to
9733066
Compare
|
@briankassouf Is there anything else I can do on this PR? |
robison
force-pushed
the
robison/add-ssh-ca-extension-templating
branch
from
bd85e05
to
b3958b3
Compare
robison
force-pushed
the
robison/add-ssh-ca-extension-templating
branch
from
b3958b3
to
54b6591
Compare
robison
force-pushed
the
robison/add-ssh-ca-extension-templating
branch
from
54b6591
to
3d72d77
Compare
robison
force-pushed
the
robison/add-ssh-ca-extension-templating
branch
from
3d72d77
to
b3cccd7
Compare
builtin/logical/ssh/path_sign.go
Outdated
| extensions := make(map[string]string) | ||
|
|
||
| if len(unparsedExtensions) > 0 { | ||
| parsedExtensions := convertMapToStringValue(unparsedExtensions) |
There was a problem hiding this comment.
I believe there is a behavior change here. If the role has no AllowedExtensions the below branch will be skipped and nothing will be added to the extensions map. Should we assign extensions here like we were previously?
The docs for AllowedExtensions state:
Specifies a comma-separated list of extensions that certificates can have when signed. To allow any extensions, set this to an empty string. Will default to allowing any extensions.
There was a problem hiding this comment.
Got it - added a commit to fix up this inadvertent change in behavior, and added a test case to cover it.
robison
force-pushed
the
robison/add-ssh-ca-extension-templating
branch
from
b3cccd7
to
61dff11
Compare
|
As an aside: any thoughts/preferences on cleaning up the tests I've introduced here? I think they've gotten to the point of needing to be broken out into separate cases, but also don't want to start crowding |
robison
force-pushed
the
robison/add-ssh-ca-extension-templating
branch
from
61dff11
to
80ffa2d
Compare
|
Thank you for the latest commit. My only feedback on the test would be the user supplied tests could be broken into a separate test from the templating tests |
|
I'll do that right now, and send an additional commit shortly. |
… default when user-provided extensions are present.
robison
force-pushed
the
robison/add-ssh-ca-extension-templating
branch
from
80ffa2d
to
1d37190
Compare
robison
force-pushed
the
robison/add-ssh-ca-extension-templating
branch
from
1d37190
to
f112d34
Compare
…orp#11495) * Add support for templated values in SSH CA DefaultExtensions. * Reworking the logic per feedback, adding basic test. * Adding test, so we cover both default extension templating & ignoring default when user-provided extensions are present. * Fixed up an unintentional extension handling defect, added test to cover the case. * Refactor Default Extension tests into `enabled` and `disabled`.
…orp#11495) * Add support for templated values in SSH CA DefaultExtensions. * Reworking the logic per feedback, adding basic test. * Adding test, so we cover both default extension templating & ignoring default when user-provided extensions are present. * Fixed up an unintentional extension handling defect, added test to cover the case. * Refactor Default Extension tests into `enabled` and `disabled`.
This PR adds support for using identity templates in values used in Default Extensions. The primary focus here is to allow the Vault SSH CA backend to function as a point of identity/authentication for GitHub Enterprise (and Github.com) users, by supporting their identity semantics, as documented here.
I've based a large chunk of this on #7548 - happy to make changes as requested/needed to get this into the tree.