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

Allow system:serviceaccounts to read the SA discovery endpoints #88344

Merged
merged 1 commit into from Mar 17, 2020

Conversation

enj
Copy link
Member

@enj enj commented Feb 19, 2020

This change allows all service accounts to read the service account issuer discovery endpoints.

This guarantees that in-cluster services can rely on this info being available to them.

Signed-off-by: Monis Khan mok@vmware.com

/kind bug

/assign @mtaufen @liggitt @mikedanese
@kubernetes/sig-auth-pr-reviews

/sig auth
/milestone v1.18
/priority important-soon

NONE

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Feb 19, 2020
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 19, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Feb 19, 2020
@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 19, 2020
@liggitt
Copy link
Member

liggitt commented Feb 19, 2020

This needs discussion before being exposed. The original proposal explicitly said we wouldn't start by doing this:

We will provide a default RBAC ClusterRole called service-account-issuer-discovery that provides GET access to these nonResourceURLs, but will not provide a default ClusterRoleBinding. This leaves the decision of who is allowed to access these endpoints up to cluster admins. A default binding requires further discussion, including ongoing efforts to harden the unauthenticated API surface area.

@enj
Copy link
Member Author

enj commented Feb 19, 2020

This needs discussion before being exposed.

Yup, and I figured this would help move the discussion along 😄

A default binding requires further discussion, including ongoing efforts to harden the unauthenticated API surface area.

I do not see us adding anything new to the unauthenticated group regardless of how much we harden Kube.

With that in mind, the question becomes "is this safe to bind to system:authenticated?" I do not see how this is any conceptually different than system:discovery in regards to implementation complexity and it does not leak sensitive information. Since the binding is distinct, even when the SA issuer discovery endpoints are enabled by default, the cluster-admin could still opt to disable the feature by removing access to it. I cannot really think of why they would want to do that though.

At the very least I think workloads on the cluster should be able to have some reasonable guarantee that this information will be available to them.

@enj enj removed request for ncdc and sttts February 21, 2020 21:14
@smourapina
Copy link

Hello @enj, @liggitt !
This is Bug Triage for release 1.18 checking in, with a friendly reminder that code freeze is coming in about 1 week (5 March). Is this PR still intended for milestone 1.18?

@enj
Copy link
Member Author

enj commented Feb 27, 2020

Hello @enj, @liggitt !
This is Bug Triage for release 1.18 checking in, with a friendly reminder that code freeze is coming in about 1 week (5 March). Is this PR still intended for milestone 1.18?

Yes, though I do not know if folks have the bandwidth to review/discuss it. I opened kubernetes/enhancements#1567 to make the KEP match this change so things stay in sync if we move forward.

@mikedanese
Copy link
Member

Three things to consider:

  • When a cluster is configured to use a large, flat, public IDP, system:authenticated is not much different than system:unauthenticated.
  • Many users deploy kube-apiservers with public endpoints.
  • The OIDC configuration document will often contain de-anonymizing information in the issuer URL.

By adding permission to read this document to system:authenticated, our info leak to scanners goes from "this is a Kubernetes cluster" to "this is X's Kubernetes cluster" when the above conditions are met. This is not something we could turn on in GKE. system:discovery was something we tried to walk back and it's still problematic for us.

@enj
Copy link
Member Author

enj commented Mar 2, 2020

@mikedanese

When a cluster is configured to use a large, flat, public IDP, system:authenticated is not much different than system:unauthenticated.

Could you be specific with an example? If you are using say, Google, it would make sense to restrict access via hosted domain. Similarly, if you are using GitHub, one would limit by organization. But I would say its on the cluster admin to place restrictions on who can authenticate.

The OIDC configuration document will often contain de-anonymizing information in the issuer URL

You would learn that same information during the authentication process?

By adding permission to read this document to system:authenticated, our info leak to scanners goes from "this is a Kubernetes cluster" to "this is X's Kubernetes cluster" when the above conditions are met

Why are scanners able to authenticate to the API?

This is not something we could turn on in GKE

Does GKE let anyone authenticate?

system:discovery was something we tried to walk back and it's still problematic for us

But you need (as system:authenticated) access to discovery so that things like kubectl can resolve where APIs live. Yes it does let anyone who can authenticate know what APIs exist on the cluster, but that is kind of the point.


Overall I do not see how system:authenticated and system:unauthenticated are the same thing. If your IDP lets anyone authenticate to the cluster, I think it is okay for anyone to be able to read API discovery and OIDC discovery.


Another approach would be to assign this permission to the system:serviceaccounts group. That would require a user to have edit-ish levels of access to see the OIDC discovery information (but any workload running on the cluster could see it so integrations would work OOTB).

@mikedanese
Copy link
Member

If you are using say, Google, it would make sense to restrict access via hosted domain. Similarly, if you are using GitHub, one would limit by organization.

Agree that these kinds of restrictions are ideal from a defense-in-depth perspective although we need to consider where the community is. Looking at search results, hits for "oidc-required-claim" are significantly lower than hits for "oidc-client-id" if that's any indication.

You would learn that same information during the authentication process?

Please elaborate.

Does GKE let anyone authenticate?

Anyone with a gmail account can authenticate. To GKE kube-apiservers. They use the same authentication stack as the rest of GCP.

@enj
Copy link
Member Author

enj commented Mar 4, 2020

Will respond to comments shortly but moving to next milestone based on last SIG Auth discussion.

/milestone v1.19

@k8s-ci-robot k8s-ci-robot modified the milestones: v1.18, v1.19 Mar 4, 2020
@enj enj force-pushed the enj/i/sa_oidc_all_authenticated branch from 50133c0 to db95586 Compare March 5, 2020 15:33
@enj enj changed the title Allow system:authenticated to read the SA issuer discovery endpoints Allow system:serviceaccounts to read the SA discovery endpoints Mar 5, 2020
@enj
Copy link
Member Author

enj commented Mar 5, 2020

If you are using say, Google, it would make sense to restrict access via hosted domain. Similarly, if you are using GitHub, one would limit by organization.

Agree that these kinds of restrictions are ideal from a defense-in-depth perspective although we need to consider where the community is. Looking at search results, hits for "oidc-required-claim" are significantly lower than hits for "oidc-client-id" if that's any indication.

Some examples:

  1. Dex supports org restriction for GitHub, hosted domain for Google, tenant for Azure
  2. OpenShift support org restriction for GitHub and hosted name restriction for Google (as well as a general whitelist only approach across all IDPs)

You would learn that same information during the authentication process?

Please elaborate.

I think I misunderstood the authentication flow. I assumed there would be an OIDC flow per cluster which would involve some form of redirection to the issuer URL in the process. This comment can be ignored.

Does GKE let anyone authenticate?

Anyone with a gmail account can authenticate. To GKE kube-apiservers. They use the same authentication stack as the rest of GCP.

Per the discussion on the last SIG Auth meeting, it seemed unlikely that GKE would be able to lock down who can authenticate to the GKE clusters (from what I understand, anyone can authenticate but authorization is limited to the appropriate subjects). To make this change reasonable in GKE environments while also usable by default by in-cluster workloads (without cluster admin interaction), I have scoped the binding down to all service accounts via the system:serviceaccounts group. A user would need edit-ish levels of write access to gain this information, which should not be an issue with GKE (or any other environment that uses a global public IDP given a sane authorization config).

@enj
Copy link
Member Author

enj commented Mar 5, 2020

/retest

1 similar comment
@enj
Copy link
Member Author

enj commented Mar 5, 2020

/retest

@mikedanese
Copy link
Member

/lgtm
/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 6, 2020
This change allows all service accounts to read the service account
issuer discovery endpoints.

This guarantees that in-cluster services can rely on this info being
available to them.

Signed-off-by: Monis Khan <mok@vmware.com>
@enj enj force-pushed the enj/i/sa_oidc_all_authenticated branch from db95586 to a38071c Compare March 9, 2020 17:40
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2020
@enj
Copy link
Member Author

enj commented Mar 9, 2020

Comments addressed.

@mikedanese
Copy link
Member

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 9, 2020
@enj
Copy link
Member Author

enj commented Mar 9, 2020

/retest

2 similar comments
@enj
Copy link
Member Author

enj commented Mar 9, 2020

/retest

@enj
Copy link
Member Author

enj commented Mar 10, 2020

/retest

@liggitt
Copy link
Member

liggitt commented Mar 16, 2020

/remove-kind bug
/kind feature
/lgtm

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed kind/bug Categorizes issue or PR as related to a bug. labels Mar 16, 2020
@liggitt
Copy link
Member

liggitt commented Mar 17, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enj, liggitt

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 17, 2020
@k8s-ci-robot k8s-ci-robot merged commit 50d574b into kubernetes:master Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants