Adds OIDC namespace_in_state option #140
Conversation
path_config.go
Outdated
| @@ -94,6 +94,11 @@ func pathConfig(b *jwtAuthBackend) *framework.Path { | |||
| }, | |||
| }, | |||
| }, | |||
| "pass_namespace_in_state": { | |||
There was a problem hiding this comment.
Nit you can probably drop "pass" and have it be namespace_in_state.
There was a problem hiding this comment.
Yep, that's better. Changed in bbfa423.
| return resp, nil | ||
| } | ||
| qParam := inputURI.Query() | ||
| namespace = qParam.Get("namespace") |
There was a problem hiding this comment.
Should we error here if it does contain a namespace (as opposed to ignoring/removing it)?
There was a problem hiding this comment.
So in this block if namespace is a query parameter, it does need to be removed from the redirectURI, so it can be appended to state later on.
Though I did see that this was re-encoding the query parameters for redirectURI needlessly in the case where there are no namespaces to worry about. Updated in b021102.
There was a problem hiding this comment.
I see, thanks for the explanation. Is the namespace always going to be passed as a query parameter regardless of NamespaceInState? How is the namespace determined if it's coming from a request header or request path?
There was a problem hiding this comment.
So as things stand today with the redirect_uri, I think the namespace is always passed as a query parameter, since that's how the UI sends it to this auth_url endpoint. (The cli login for OIDC doesn't need to set a namespace parameter since it just opens a local listener.)
I did a little testing with the UI, and I don't see any headers coming through in req.Headers in the auth_url endpoint handler. I think it would be possible to infer the namespace from the mountpoint, or add another field to the auth_url API payload. Maybe that would be a cleaner way to specify the namespace? I'm not sure what else that would gain us, unless there's another UI-like system we're trying to integrate with.
Checks whether there actually was a namespace query parameter before removing it and re-encoding the remaining query parameters.
tvoran
changed the title
|
Using state seems like the better option. Is there any reason users wouldn’t prefer this to be true, aside from backwards compatibility? If “no” then a default of |
|
I think we landed on defaulting to false to give people the opportunity to enable this explicitly without breaking their setup right out of an update since they have to update their list of allowed callback URLs on the provider side as well. We can flip the default to true in a future release. @tvoran correct me if this assumption is wrong. If we do want to enable this by default, we might want to rename it to |
|
I was think of something like:
I think this gives us the preferred default without breaking existing configs? |
|
@tvoran There are quite a few examples of upgrades and versions in Vault, and a few in plugins I think. Simple vault example: https://github.com/hashicorp/vault/blob/86209a769a254dfaa0b21cf8b3412c1450b6609d/vault/expiration.go#L2016-L2018. Much more elaborate: https://github.com/hashicorp/vault/blob/0c9f8a377cf11f71fe0d6f5458f91706038762a8/builtin/credential/aws/path_role.go#L449 I was thinking about this more and searching for |
|
I think the concern lies partially on the out-of-band update that is required on the auth provider side to update the set of valid callback URLs. Since |
|
I do like the idea of defaulting to namespace in state, since that encourages a simpler provider config. And you're right, @calvn, if we keep If we want to go with Jim's idea of |
|
I agree that using |
path_config.go
Outdated
| Type: framework.TypeBool, | ||
| Description: "Pass namespace in the OIDC state parameter instead of as a separate query parameter. With this setting, the allowed redirect URL(s) in Vault and on the provider side should not contain a namespace query parameter. This means only one redirect URL entry needs to be maintained on the provider side for all vault namespaces that will be authenticating against it. Defaults to true for new configs.", | ||
| DisplayAttrs: &framework.DisplayAttributes{ | ||
| Name: "Namespace in OIDC state", |
There was a problem hiding this comment.
Should we keep the value in DisplayAttrs for UI purposes? I am not sure if Default and DisplayAttrs.Value is required to match.
There was a problem hiding this comment.
Yep, I'll add the DisplayAttrs value back in. Added in 58a9841.
From my brief testing without the DisplayAttrs value, it shows up on the config edit page as unchecked. And if the user doesn't click on the field, it isn't sent when the config is submitted, which ends up defaulting to true on the backend. But it looks odd to submit a config with that value unchecked, only to find that the backend defaulted to having it checked.
path_config.go
Outdated
| @@ -189,6 +197,22 @@ func (b *jwtAuthBackend) pathConfigWrite(ctx context.Context, req *logical.Reque | |||
| ProviderConfig: d.Get("provider_config").(map[string]interface{}), | |||
| } | |||
|
|
|||
| // Check if the config already exists, to determine if this is a create or | |||
| // an update | |||
There was a problem hiding this comment.
Might want to add a note on why we're doing this vs having an existence check.
There was a problem hiding this comment.
Adding something in 58a9841.
namespace_in_state defaults to true only on new config write
Overview
Add an option to the OIDC config to pass namespace in the state parameter instead of as a separate query parameter. With this setting the allowed redirect URL in Vault and on the provider side should not contain a namespace query parameter. This means only one redirect URL entry needs to be maintained on the provider side for all vault namespaces that will be authenticating against it.
To test this, I've been doing something like this along with the vault PR hashicorp/vault#10171:
yarn startin the vault/ui directory to start the UI in live-reloading/dev mode./scripts/local_dev.shin this vault-plugin-auth-jwt repo, with a vault enterprise binary in your pathDesign of Change
A config option in the OIDC config
Related Issues/Pull Requests
Contributor Checklist