-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
svcacct token issuer should support oidc discovery #2314
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
This is really great stuff! Can the kid
for a given signing key be included in the JWT header? The existing proposal didn't specify, and that would be something I'd like to see. The existing proposal's decoded header comes out to
{
"alg": "RS256",
"typ": "JWT"
}
One other question about this, would the existing SA signing key in the Controller Manager remain or would it be deprecated?
"kty": "RSA", | ||
"alg": "RS256", | ||
"use": "sig", | ||
"kid": "ccab4acb107920dc284c96c6205b313270672039", |
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 see multiple keys here, would the --service-account-signing-key:
file contain multiple keys? Would the flag be specified multiple times? Would a apiserver restart be required to trigger a new list of keys (for that specific server)? How do you determine the kid
for a given key? How would the apiserver determine which key is used for a TokenRequest
response?
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 assume these would be populated from the --service-account-key-file
flag, which is used for verifying presented tokens, and does take multiple files (or files with multiple key PEM blocks):
File containing PEM-encoded x509 RSA or ECDSA private or public keys, used to verify ServiceAccount tokens. The specified file can contain multiple keys, and the flag can be specified multiple times with different files.
a server restart is required to pick up new public keys today
How would the apiserver determine which key is used for a TokenRequest response?
the signing key specified by --service-account-signing-key-file
is used
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.
the signing key specified by --service-account-signing-key-file is used
Sorry I wasn't clear on the last question. When given a list of keys, which one is used to sign a TokenRequest
? Ex: I want to rotate out a certain key and stop signing new tokens with it, but still keep it around for verification of existing tokens.
Also, is kid
derived from the key? I'm just fuzzy on where that comes from, and how it is coordinated across API servers.
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.
When given a list of keys, which one is used to sign a TokenRequest
--service-account-signing-key-file
takes a single key, and is used to sign TokenRequests.
--service-account-key-file
takes multiple keys for verification purposes.
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 that part makes sense. I wasn't paying close enough attention and was merging --service-account-key-file
and --service-account-signing-key-file
.
> GET /.well-known/openid-configuration | ||
{ | ||
"issuer": "https://dev.cluster.internal", | ||
"jwks_uri": "https://dev.cluster.internal/serviceaccountkeys/v1", |
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.
For this non-resource URL, would a RBAC policy/binding like below be required for applications utilizing this K8s IDP? I'm guessing this would require --anonymous-auth=true
with something similar to the below policy/binding
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
name: system:oidc
rules:
- nonResourceURLs:
- /.well-known/openid-configuration
- /serviceaccountkeys/v1
verbs:
- get
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
name: system:oidc
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: system:oidc
subjects:
- apiGroup: rbac.authorization.k8s.io
kind: Group
name: system:unauthenticated
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.
That should work.
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.
You want both system:unauthenticated
and system:authenticated
.
{ | ||
"issuer": "https://dev.cluster.internal", | ||
"jwks_uri": "https://dev.cluster.internal/serviceaccountkeys/v1", | ||
"authorization_endpoint": "urn:kubernetes:programmatic_authorization", |
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.
is this compatible with https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata:
authorization_endpoint
REQUIRED. URL of the OP's OAuth 2.0 Authorization Endpoint
that specifies a URL, not a URI (which would allow a URN)
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.
Does authorization_endpoint
even make sense? There is no way to actually do an OAuth flow with the API master.
"claims_supported": [ | ||
"sub", | ||
"iss" | ||
] |
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.
if grant_types
is omitted, the default value is ["authorization_code", "implicit"]
, which means that token_endpoint
is also required
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.
using an openid discovery doc as a vehicle for publishing links to public keysets gets weird when the tokens we're wanting to allow someone to validate can't actually be obtained via an openid flow... we'd have to do things like advertise grant_types_supported:[]
and make our authorization_endpoint
point to a URL that rejected all requests. that seems pretty weird.
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.
However, grant types are extensible:
https://tools.ietf.org/html/rfc6749#section-8.3
An alternative is to make
"grant_types":["urn:kubernetes:grant_type:programmatic_authorization"]
and make authorization_endpoint
point somewhere bogus.
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.
@BrenoDeMedeiros do you have any thoughts on how we should express the Kubernetes id_token flow in a openid-configuration to support oidc discovery?
While discovery in general sounds like a good idea, especially to allow distributed auth models, the If we take OpenShift as an example, today you can do:
Which returns: {
"issuer": "https://<master>",
"authorization_endpoint": "https://<master>/oauth/authorize",
"token_endpoint": "https://<master>/oauth/token",
"scopes_supported": [
"user:check-access",
"user:full",
"user:info",
"user:list-projects",
"user:list-scoped-projects"
],
"response_types_supported": [
"code",
"token"
],
"grant_types_supported": [
"authorization_code",
"implicit"
],
"code_challenge_methods_supported": [
"plain",
"S256"
]
} But that makes sense because a user can actually go through an OAuth flow with the server and have an OAuth token returned to them. No such flows exists for SA tokens, and I do not think anyone actually wants such a flow to exist. Basically, Kube SA tokens are JWTs, but that does not have any specific relationship to OIDC. Perhaps there is a different way to discover a |
OIDC discovery would be exposed because it is widely adopted and supported by target integrations (e.g. AWS, vault, dex etc...). I think we need to meet integrations where they are rather then force them to implement a new spec to validate k8s service account identities. I'm open to considering other discovery mechanisms but level of support is my top consideration. |
I do not think this actually succeeds in making any of those integrations easier. This still have to know that this is a Kube OIDC discovery doc, and that only meaningful field is I guess with this doc a generic verify method could be written for OIDC like endpoints: func Verify(token, issuer string) bool {
// get discovery doc
// use it to get all info needed to verify token
} I am not sure how critical reuse of any such logic would be to any integrator. |
It would support discovery/loading of the info needed to verify a token obtained out of band (this is the same level of info the kube OIDC token verifier uses from the issuer discovery docs) |
Right, no disagreement there. I am mostly referring to the fact that integrators still have Kube specific knowledge in their codebase, so I am not seeing how this is different than a custom Kube specific endpoint (non-resource or resource) that does not pretend to be OIDC. |
I think this is not true. This doc would be compatible with AWS and vault without any changes on their side. |
Can you describe their use case? Or point me to the docs or code? I can see how they could use this endpoint (because it is close enough to everything else and they only care about the |
@enj A use case we're exploring based on this proposal is outlined here https://docs.google.com/document/d/1V_B24zNLInLPRIPRXQeKj4JAJDUvKe-hZPJvrgx80yQ/edit We would like for Kubernetes would implement enough of OIDC to act as a federated identity provider in AWS. With the current proposal, there wouldn't be anything specific to Kubernetes we'd have to do on our side. |
``` | ||
> GET /.well-known/openid-configuration | ||
{ | ||
"issuer": "https://dev.cluster.internal", |
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.
This would match the issuer in the SA JWT I assume. Presumably that cannot change since that would invalidate the JWTs.
{ | ||
"issuer": "https://dev.cluster.internal", | ||
"jwks_uri": "https://dev.cluster.internal/serviceaccountkeys/v1", | ||
"authorization_endpoint": "urn:kubernetes:programmatic_authorization", |
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 feel like having a dummy authorization_endpoint
does not really honor the OIDC spec.
Pretty sure token endpoint is also required:
token_endpoint
URL of the OP's OAuth 2.0 Token Endpoint [OpenID.Core]. This is REQUIRED unless only the Implicit Flow is used.
scopes_supported
also weird:
scopes_supported
RECOMMENDED. JSON array containing a list of the OAuth 2.0 [RFC6749] scope values that this server supports. The server MUST support the openid scope value. Servers MAY choose not to advertise some supported scope values even when this parameter is used, although those defined in [OpenID.Core] SHOULD be listed, if supported.
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/remove-lifecycle stale |
How do people feel about only exposing a JWKs URL and proceeding from there? That solves half of the problem. The other 49% is discovering the issuer URL. |
I think that's a much easier step to reason about, and makes sense to me as a starting point. That would be necessary as a prereq for inclusion in an oidc discovery doc as well |
If a relying party wants to validate the token (including the Could we offer a subset of the OIDC metadata ( |
Some notes from this week's sig-auth meeting- I'm probably misrepresenting these a bit, apoligies:
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/remove-lifecycle stale |
can we move this into a KEP in enhancements? would be good to reconcile this with the external signing proposal to make sure whatever we end up with there supports this goal as well |
Ya, was planning on doing that. |
### Add Service Account signing key retrieval API | ||
|
||
A non-resource API will be added to the apiserver that will expose an [OIDC | ||
discovery](https://openid.net/specs/openid-connect-discovery-1_0.html) |
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.
This was omitted from the original document for the purpose of expediting it's merge. Reintroducing now that we've made progress.
This change allows clients with knowledge of oidc discovery to validate service account tokens. Simpler clients can choose to ignore the discovery document and validate tokens against the keys directly by fetching the JWKs URL.
@kubernetes/sig-auth-feature-requests @kubernetes/sig-auth-api-reviews