-
Notifications
You must be signed in to change notification settings - Fork 100
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
Add JWKS data source for fetching public keys for JWT validation #447
Conversation
nomad/data_source_jwks.go
Outdated
|
||
func keyToPem(key Key) (string, error) { | ||
|
||
// FIXME does Nomad always use RSA keys for JWKS? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this, but I think Nomad only uses RSA for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory the root key can also use EdDSA
, but I think for OIDC RSA is required.
https://github.com/hashicorp/nomad/blob/78f9f178670097eac9036c4bf83f7e0ceb55e6f3/nomad/encrypter.go#L499-L505
But we can probably remove this check altogether? It would also help future-proof this data source in case new algorithms are added later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a bit of investigation on this: to support EdDSA it looks like we would need to add crv
and x
fields for Key
s, and then construct a ed25519
public key from them. We would still need the KeyType
check to see which type of key to construct ("OKP" for EdDSA).
Also, as you pointed out, OIDC doesn't support non-RSA keys, so Nomad doesn't have code to make JWKs out of them. We would therefore need to construct a mocked-up key to test with.
I'm happy to do this, just want to make sure this sounds like worthwhile effort at this point. An alternative approach for future-proofing might be to ask for Nomad's ListKeys
RPC to be make available via the HTTP API and fetch the keyset directly.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. I think we can just leave RSA support like it is then, maybe just update the comment to explain why only RSA is supported?
Supporting new key types seem like will require code changes here as well, so not a bit deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you and @wraithm already added a comment - thank you! I pushed a quick fix to change the indentation to match the surrounding code, I think it looks good to go now?
What do you think about this @lgfa29 ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the very comprehensive PR @clintonford-btnl!
I left some small suggestions, but things are looking great 🙂
nomad/data_source_jwks.go
Outdated
|
||
func keyToPem(key Key) (string, error) { | ||
|
||
// FIXME does Nomad always use RSA keys for JWKS? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory the root key can also use EdDSA
, but I think for OIDC RSA is required.
https://github.com/hashicorp/nomad/blob/78f9f178670097eac9036c4bf83f7e0ceb55e6f3/nomad/encrypter.go#L499-L505
But we can probably remove this check altogether? It would also help future-proof this data source in case new algorithms are added later on.
nomad/provider.go
Outdated
@@ -163,6 +163,7 @@ func Provider() *schema.Provider { | |||
"nomad_regions": dataSourceRegions(), | |||
"nomad_volumes": dataSourceVolumes(), | |||
"nomad_variable": dataSourceVariable(), | |||
"nomad_jwks": dataSourceJWKS(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep the keys sorted alphabetically so it's easier to read them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed - thank you!
Thanks @lgfa29 ! I'm on vacation this week but will be able to respond and implement your suggestions this coming Monday. Your comments all look good, no concerns from me at this point. |
Co-authored-by: Luiz Aoqui <luiz@hashicorp.com>
Co-authored-by: Luiz Aoqui <lgfa29@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the updates @clf-cklf.
I no longer work at HashiCorp, so I can't get this PR merged, but I'm leaving my approval here to signal to the rest of the team that these changes look good 😄
Thanks @lgfa29! Do you know who's taken over maintainership of this project? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @clf-cklf.
This adds a
data
provider for the JWKS public keys used for verifying workload identity JWTs. These are exposed at the/.well-known/jwks.json
HTTP API endpoint.One use for this would be to provide these keys to Vault via https://registry.terraform.io/providers/hashicorp/vault/latest/docs/resources/jwt_auth_backend#jwt_validation_pubkeys. This would be useful, because Vault cannot access the HTTPS API with mTLS enabled unless one turns off client verification: https://developer.hashicorp.com/nomad/docs/integrations/vault/acl#mutual-tls-in-nomad
The keys are returned as both a list of objects with human-readable field names, and as a list of PEM-encoded X.509 public keys; the latter is, I believe, the preferred way to pass PKI information in Terraform, and is the format that the Terraform Vault provider expects.