nomad_acl_auth_method: add pkce and client assertions #523
Conversation
|
|
| RequiredWith: []string{ | ||
| "config.0.oidc_discovery_url", // if this is set, | ||
| "config.0.oidc_client_id", // client id must also be set | ||
| }, |
There was a problem hiding this comment.
Not an issue, but when I went to look this validator up, it's... weird?
// RequiredWith is a set of attribute paths, including this attribute,
// that must be set simultaneously.
The docstring almost implies that we'd end up always needing oidc_discovery_url and oidc_client_id and oidc_client_secret and oidc_client_assertion if any of them are set. But obviously that's not how it works (fortunately).
There was a problem hiding this comment.
A lot of the go docs on these fields have strained my brain cells a bit too, yeah. I took it to mean (and experimentation bears out) that "if I, myself, <my name here, explicitly> am set, then this other thing(s) must also be set."
So in total in this PR, from RequiredWiths:
- if
oidc_discovery_urlis set,oidc_client_idis required, too (defined here inoidc_discovery_url) - if
oidc_client_secretis set,oidc_client_idis required, too (defined below inoidc_client_secret) - if
oidc_client_assertionis set,oidc_client_idis required, too (defined below inoidc_client_assertion)
oidc_discovery_url is required by virtue of ExactlyOneOf up in jwt_validation_pub_keys (unless one of the jwt/jwks options are set).
I could not for the life of me find a way to say "one of: client secret, or assertion must be provided if client id (or discovery url?) is set, otherwise we must be in jwt-land, so don't worry about it." And I think technically, one can have an unsecured OIDC setup without any secret material, so I opted to stop banging my head against that problem, and let reality sort it out.
There was a problem hiding this comment.
Yeah, it's nice to give as much validation as we can but at some point you have to worry about drift anyways. LGTM
| RequiredWith: []string{ | ||
| "config.0.oidc_client_secret", | ||
| "config.0.oidc_client_id", | ||
| }, |
There was a problem hiding this comment.
Out of scope of this PR obviously, but I wanted to check the assertions here against the API docs and we don't have these requirements documented. Might not be a bad idea to follow-up with that.
nomad/resource_acl_auth_method.go
Outdated
| if originalAuthMethod.Config.OIDCClientAssertion != nil && | ||
| originalAuthMethod.Config.OIDCClientAssertion.PrivateKey != nil && | ||
| originalAuthMethod.Config.OIDCClientAssertion.PrivateKey.PemKey != "" { | ||
| fetchedAuthMethod.Config.OIDCClientAssertion.PrivateKey.PemKey = originalAuthMethod.Config.OIDCClientAssertion.PrivateKey.PemKey |
There was a problem hiding this comment.
is fetchedAuthMethod.Config.OIDCClientAssertion guaranteed to be non-nil if fetchedAuthMethod.Config is? It doesn't look like it based on the nil check we do further down.
There was a problem hiding this comment.
anecdotally, it doesn't get tripped in any tests, but I still like to guard against nil, so I'll do some extra checkin 👍
There was a problem hiding this comment.
the pile of conditions was feeling like a jenga tower, so I made a (pretty precious) wrapper to make this stretch of code easier to read (and safe). I could be convinced to pull back from that, but it does the thing. whatcha think?
There was a problem hiding this comment.
I'd have probably made those free functions but here in Terraform provider code it's not like we're instantiating this extra object a million times in a server or something, so that's just a matter of taste, not function. LGTM
Add support for enabling PKCE and using client assertions (private key JWT) instead of client secrets.
Also scope creeped a bit for a couple little things that were bothering me.