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

harden the default RBAC discovery clusterrolebindings #73807

Merged
merged 1 commit into from Mar 2, 2019

Conversation

@dekkagaijin
Copy link
Contributor

dekkagaijin commented Feb 7, 2019

Implements kubernetes/enhancements#789

/kind feature

Default RBAC policy no longer grants access to discovery and permission-checking APIs (used by `kubectl auth can-i`) to *unauthenticated* users. Upgraded clusters preserve prior behavior, but cluster administrators wishing to grant unauthenticated users access in new clusters will need to explicitly opt-in to expose the discovery and/or permission-checking APIs:
* `kubectl create clusterrolebinding anonymous-discovery --clusterrole=system:discovery --group=system:unauthenticated`
* `kubectl create clusterrolebinding anonymous-access-review --clusterrole=system:basic-user --group=system:unauthenticated`
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 7, 2019

Hi @dekkagaijin. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@dekkagaijin

This comment has been minimized.

Copy link
Contributor Author

dekkagaijin commented Feb 7, 2019

/assign @liggitt

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 7, 2019

/ok-to-test

@dekkagaijin dekkagaijin force-pushed the dekkagaijin:discovery-hardening branch from 4d023bd to 7ca8c84 Feb 7, 2019

@dekkagaijin dekkagaijin changed the title [WIP] harden the default RBAC discovery clusterrolebindings harden the default RBAC discovery clusterrolebindings Feb 8, 2019

@dekkagaijin

This comment has been minimized.

Copy link
Contributor Author

dekkagaijin commented Feb 8, 2019

@liggitt OK to review what I have now, don't really know the most sane way to test the storage_rbac change. I'll flesh out the release note to include the possible pitfalls and the sample kubectl escape hatches

@dekkagaijin dekkagaijin force-pushed the dekkagaijin:discovery-hardening branch from 7ca8c84 to c757cae Feb 8, 2019

Show resolved Hide resolved pkg/registry/rbac/rest/storage_rbac.go Outdated
const (
PostStartHookName = "rbac/bootstrap-roles"
discoveryClusterRoleName = "system:discovery"
publicInfoViewerClusterRoleName = "system:public-info-viewer"

This comment has been minimized.

@liggitt

liggitt Feb 23, 2019

Member

instead of hard-coding role names here, get a set of map[string]ClusterRoleBinding (source name -> destination clusterrolebinding template) from the bootstrap policy data (e.g. SplitClusterRoleBindings()), then iterate over them in primeSplitClusterRoleBindings

inside that method, make a DeepCopy() of the destination clusterrolebinding template and actually copy over the labels/annotations/subjects from the source clusterrolebinding if present

This comment has been minimized.

@liggitt

liggitt Feb 28, 2019

Member

still outstanding

This comment has been minimized.

@dekkagaijin

dekkagaijin Feb 28, 2019

Author Contributor

done

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 23, 2019

don't really know the most sane way to test the storage_rbac change

@jennybuckley has a great example of an integration test that:

  1. starts a shared etcd
  2. starts an apiserver and lets it come up
  3. monkeys with stuff
  4. shuts down the apiserver and starts a new one against the same etcd data
  5. verifies stuff

that sounds like exactly what you want

@dekkagaijin

This comment has been minimized.

Copy link
Contributor Author

dekkagaijin commented Feb 25, 2019

@liggitt thanks, I'll finish this up tomorrow (crosses fingers) after perf/promo is done

@dekkagaijin dekkagaijin force-pushed the dekkagaijin:discovery-hardening branch from c757cae to 8d51dd0 Feb 27, 2019

@dekkagaijin dekkagaijin force-pushed the dekkagaijin:discovery-hardening branch from 8d51dd0 to b88d83d Feb 27, 2019

Show resolved Hide resolved test/integration/auth/rbac_test.go Outdated
Show resolved Hide resolved test/integration/auth/rbac_test.go Outdated
@dekkagaijin

This comment has been minimized.

Copy link
Contributor Author

dekkagaijin commented Feb 28, 2019

Tests, logic, and documentation are 99% ready for review. PTAL!

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 28, 2019

a couple comments on the test, and one outstanding comment on where the old binding -> new binding data should live. lgtm otherwise

@dekkagaijin dekkagaijin force-pushed the dekkagaijin:discovery-hardening branch from 9308067 to 770c554 Feb 28, 2019

@dekkagaijin

This comment has been minimized.

Copy link
Contributor Author

dekkagaijin commented Feb 28, 2019

done, PTAL @liggitt

@dekkagaijin dekkagaijin force-pushed the dekkagaijin:discovery-hardening branch from 770c554 to 02b0f7b Feb 28, 2019

@dekkagaijin dekkagaijin force-pushed the dekkagaijin:discovery-hardening branch 2 times, most recently from 88ef13a to f1e6e9e Mar 1, 2019

@dekkagaijin

This comment has been minimized.

Copy link
Contributor Author

dekkagaijin commented Mar 1, 2019

/test pull-kubernetes-verify

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Mar 1, 2019

an attempt was made to switch to go1.12 last night... govet failure is spurious, retest should resolve it

@dekkagaijin

This comment has been minimized.

Copy link
Contributor Author

dekkagaijin commented Mar 1, 2019

@liggitt that was the trick, PTAL :)

// If source ClusterRoleBinding does not exist, do nothing.
existingRoleBinding, err := clusterRoleBindingClient.ClusterRoleBindings().Get(existingBindingName, metav1.GetOptions{})
if apierrors.IsNotFound(err) {
return nil

This comment has been minimized.

@liggitt

liggitt Mar 2, 2019

Member

the return nil lines inside the loop should be continue statements

This comment has been minimized.

@dekkagaijin

dekkagaijin Mar 2, 2019

Author Contributor

good catch

@dekkagaijin dekkagaijin force-pushed the dekkagaijin:discovery-hardening branch from f1e6e9e to 9c7d319 Mar 2, 2019

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Mar 2, 2019

/lgtm
/approve
/milestone v1.14

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 2, 2019

@k8s-ci-robot k8s-ci-robot added this to the v1.14 milestone Mar 2, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 2, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dekkagaijin, 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 merged commit f160356 into kubernetes:master Mar 2, 2019

17 checks passed

cla/linuxfoundation dekkagaijin authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@dekkagaijin dekkagaijin deleted the dekkagaijin:discovery-hardening branch Mar 2, 2019

@redbaron

This comment has been minimized.

Copy link
Contributor

redbaron commented Mar 3, 2019

Release notes say "Upgraded clusters preserve prior behavior", how can administrators of upgraded clusters move to hardened RBAC?

@dekkagaijin

This comment has been minimized.

Copy link
Contributor Author

dekkagaijin commented Mar 5, 2019

@redbaron (nice username btw) one only need remove the system:authenticated Subject group form the system:discovery and system:basic-user ClusterRoleBindings.

I don't think that there's a universal, one- or two-line command to trim the old Bindings since kubectl patch isn't overly smart about array elements, but if a general description of the change is fine then I can add the previous sentence to the release notes.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Mar 5, 2019

I don't think that there's a universal, one- or two-line command to trim the old Bindings since kubectl patch isn't overly smart about array elements

kubectl auth reconcile --remove-extra-subjects -f ... was made for this, but needs an input file containing only the trimmed clusterrolebindings

@dekkagaijin

This comment has been minimized.

Copy link
Contributor Author

dekkagaijin commented Mar 7, 2019

@redbaron Here's something I came up with to harden upgraded clusters to the new RBAC discovery roles:

cat <<EOF | kubectl auth reconcile --remove-extra-subjects -f -
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: system:basic-user
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: system:basic-user
subjects:
- apiGroup: rbac.authorization.k8s.io
  kind: Group
  name: system:authenticated
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: system:discovery
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: system:discovery
subjects:
- apiGroup: rbac.authorization.k8s.io
  kind: Group
  name: system:authenticated
EOF

k8s-ci-robot added a commit to kubernetes/website that referenced this pull request Mar 12, 2019

Document changes to default RBAC discovery ClusterRole(Binding)s (#12888
)

* Document changes to default RBAC discovery ClusterRole(Binding)s

Documentation for kubernetes/enhancements#789 and kubernetes/kubernetes#73807

* documentation review feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.