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

svcacct token issuer should support oidc discovery #2314

Closed
wants to merge 1 commit into from

Conversation

mikedanese
Copy link
Member

@mikedanese mikedanese commented Jun 26, 2018

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

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 26, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: liggitt

Assign the PR to them by writing /assign @liggitt in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/design Categorizes issue or PR as related to design. sig/auth Categorizes an issue or PR as relevant to SIG Auth. labels Jun 26, 2018
Copy link
Member

@micahhausler micahhausler left a 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",
Copy link
Member

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?

Copy link
Member

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

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

@micahhausler micahhausler Jul 3, 2018

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",
Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

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

That should work.

Copy link
Member

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",
Copy link
Member

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)

Copy link
Member

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"
]
Copy link
Member

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

Copy link
Member

@liggitt liggitt Jul 3, 2018

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.

Copy link
Member Author

@mikedanese mikedanese Jul 3, 2018

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.

Copy link
Member Author

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?

@mikedanese
Copy link
Member Author

@mdietz

@enj
Copy link
Member

enj commented Aug 28, 2018

While discovery in general sounds like a good idea, especially to allow distributed auth models, the OpenID Provider Metadata is clearly focused on Open ID flows.

If we take OpenShift as an example, today you can do:

$ oc get --raw '/.well-known/oauth-authorization-server'

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 jwks_uri?

@mikedanese
Copy link
Member Author

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.

@enj
Copy link
Member

enj commented Aug 29, 2018

OIDC discovery would be exposed because it is widely adopted and supported by target integrations (e.g. AWS, vault, dex etc...)

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 jwks_uri. @liggitt's concerns in #2314 (comment) basically point out that this proposed endpoint simply fails to meet the OIDC contract. Having custom grant types just further couples any integration with Kube specifics. So if integrations already need to know that "this is a Kube SA token and all you can do is verify it - you cannot do any other OIDC things with it," than at best you have allowed them to reuse some existing structs for decoding.

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.

@liggitt
Copy link
Member

liggitt commented Aug 29, 2018

I do not think this actually succeeds in making any of those integrations easier.

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)

@enj
Copy link
Member

enj commented Aug 29, 2018

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.

@mikedanese
Copy link
Member Author

I am mostly referring to the fact that integrators still have Kube specific knowledge in their codebase

I think this is not true. This doc would be compatible with AWS and vault without any changes on their side.

@enj
Copy link
Member

enj commented Aug 29, 2018

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 jwks_uri). As a user I would find it very confusing to be presented an OIDC discovery doc from a server that cannot perform any OIDC flow.

@micahhausler
Copy link
Member

micahhausler commented Aug 30, 2018

@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",
Copy link
Member

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",
Copy link
Member

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. 

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 20, 2019
@mikedanese
Copy link
Member Author

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 22, 2019
@mikedanese
Copy link
Member Author

mikedanese commented Jan 22, 2019

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.

@liggitt
Copy link
Member

liggitt commented Jan 22, 2019

How do people feel about only exposing a JWKs URL and proceeding from there?

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

@cceckman
Copy link

If a relying party wants to validate the token (including the iss claim), they'd still need a provider-specific discovery mechanism to find that issuer.

Could we offer a subset of the OIDC metadata (issuer, jwks_uri, anything else necessary) at a non-standard endpoint? e.g. /serviceaccountissuer/{keys,metadata} rather than /.well-known/openid-configuration?

@cceckman
Copy link

cceckman commented Mar 6, 2019

Some notes from this week's sig-auth meeting- I'm probably misrepresenting these a bit, apoligies:

  • @liggitt: "We're using this for validation, not for request flows. Not going to be helpful to offer at a nonstandard path" (as I suggested above; fair.) "Work out the minimal spec-compliant thing that won't cause validators to crash."
  • @mikedanese: "Authz endpoint is an actual URL, but always returns 403 / 404. Nothing that says grant types have to be well-known."

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 4, 2019
@cceckman
Copy link

cceckman commented Jun 5, 2019

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 5, 2019
@liggitt
Copy link
Member

liggitt commented Jul 10, 2019

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

@mikedanese
Copy link
Member Author

Ya, was planning on doing that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/design Categorizes issue or PR as related to design. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants