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

OpenID Configuration Discovery Endpoint (just the endpoint) #18691

Merged
merged 13 commits into from
Oct 21, 2023
Merged

Conversation

schmichael
Copy link
Member

@schmichael schmichael commented Oct 7, 2023

Added the OIDC Discovery /.well-known/openid-configuration endpoint to Nomad, but it is only enabled if the server.oidc_issuer parameter is set. Documented the parameter, but without a tutorial trying to actually use this will be very hard.

I intentionally did not use https://github.com/hashicorp/cap for the OIDC configuration struct because it's built to be a compliant OIDC provider. Nomad is not trying to be compliant initially because compliance to the spec does not guarantee it will actually satisfy the requirements of third parties. I want to avoid the problem where in an attempt to be standards compliant we ship configuration parameters that lock us in to a certain behavior that we end up regretting. I want to add parameters and behaviors as there's a demonstrable need.

Users always have the escape hatch of providing their own OIDC configuration endpoint. Nomad just needs to know the Issuer so that the JWTs match the OIDC configuration. There's no reason the actual OIDC configuration JSON couldn't live in S3 and get served directly from there. Unlike JWKS the OIDC configuration should be static, or at least change very rarely.

This PR is just the endpoint extracted from #18535. The RS256 algorithm still needs to be added in hopes of supporting third parties such as AWS IAM OIDC Provider.

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

website/content/docs/configuration/server.mdx Show resolved Hide resolved
@tgross tgross added this to the 1.7.0 milestone Oct 19, 2023
Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor ramblings, but LGTM 😄

command/agent/config.go Show resolved Hide resolved
command/agent/keyring_endpoint.go Outdated Show resolved Hide resolved
command/agent/keyring_endpoint_test.go Outdated Show resolved Hide resolved
- `oidc_issuer` `(string: "")` - Specifies the Issuer URL for [Workload
Identity][wi] JWTs. If set the `/.well-known/openid-configuration` HTTP
endpoint is enabled for third parties to discover Nomad's OIDC configuration.
Once set `oidc_issuer` *cannot be changed* without invalidating Workload
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How bad would it be for this config to drift across servers, like a typo or an old machine coming back up for some reason?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very bad. All JWTs would randomly fail (except when used with Nomad because at least for now we don't bother checking the issuer: if the signature is valid and the alloc is live, we're happy).

Moving this state to Raft prevents that issue nicely, and I suppose we could refuse to overwrite existing non-empty values unless a -force or -i-know-what-im-doing flag is set... 🤔

I assumed the config file approach might be easier to synchronize across the multiply places that need to agree on this value (Nomad server configuration, the proxy you're putting in front of the OIDC Discovery endpoint, and the consumers of the discovery endpoint such as AWS IAM OIDC Provider). I'm not sure though. Opinions welcome! 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was wondering about storing this in state. I don't have any strong opinions, and I don't know if we have any prescriptive guidance of what should go where 🤔

Some aspects I just pulled out of thin air:

Config file State
Required on agent start Not required on agent start
Specific to each node Must be the same in all nodes
Requires restart to apply change Can be changed live

So I feel like this falls into the "state" category? But I also don't feel strongly about my table either 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of the WI related configuration falls onto that right-hand side of the table (ex. all the identity blocks for Consul and Vault configs). I'd love for us to ship some kind of comprehensive server (and client!) configuration management tooling. But I also think that design is something we should work out for a post-1.7 release.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, a more holistic strategy for configuration management would be great!

I'm good with the changes here as they are, I just smelled a potentially bad footgun and wanted to confirm it was indeed a bad problem 😅

schmichael and others added 2 commits October 19, 2023 10:30
Co-authored-by: Luiz Aoqui <[email protected]>
Co-authored-by: Luiz Aoqui <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants