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.

Sign up for GitHub

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

Jump to bottom

Allow anonymous API server access, decorate authenticated users with system:authenticated group #32386

Merged
merged 5 commits into from
Sep 29, 2016
Merged

Allow anonymous API server access, decorate authenticated users with system:authenticated group #32386

merged 5 commits into from
Sep 29, 2016

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Sep 9, 2016 •
edited
Loading

When writing authorization policy, it is often necessary to allow certain actions to any authenticated user. For example, creating a service or configmap, and granting read access to all users

It is also frequently necessary to allow actions to any unauthenticated user. For example, fetching discovery APIs might be part of an authentication process, and therefore need to be able to be read without access to authentication credentials.

This PR:

  • Adds an option to allow anonymous requests to the secured API port. If enabled, requests to the secure port that are not rejected by other configured authentication methods are treated as anonymous requests, and given a username of system:anonymous and a group of system:unauthenticated. Note: this should only be used with an --authorization-mode other than AlwaysAllow
  • Decorates user.Info returned from configured authenticators with the group system:authenticated.

This is related to defining a default set of roles and bindings for RBAC (kubernetes/enhancements#2). The bootstrap policy should allow all users (anonymous or authenticated) to request the discovery APIs.

kube-apiserver learned the '--anonymous-auth' flag, which defaults to true. When enabled, requests to the secure port that are not rejected by other configured authentication methods are treated as anonymous requests, and given a username of 'system:anonymous' and a group of 'system:unauthenticated'. 

Authenticated users are decorated with a 'system:authenticated' group.

NOTE: anonymous access is enabled by default. If you rely on authentication alone to authorize access, change to use an authorization mode other than AlwaysAllow, or or set '--anonymous-auth=false'.

c.f. #29177 (comment)

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Sep 9, 2016
@liggitt liggitt assigned erictune and unassigned lavalamp Sep 9, 2016
@liggitt liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Sep 9, 2016
@liggitt
Copy link
Member Author

liggitt commented Sep 9, 2016

cc @kubernetes/sig-auth

@deads2k
Copy link
Contributor

deads2k commented Sep 9, 2016

lgtm

@deads2k
Copy link
Contributor

deads2k commented Sep 9, 2016

lgtm

You'll want a doc pull too.

@liggitt
Copy link
Member Author

liggitt commented Sep 9, 2016 •
edited
Loading

@k8s-bot test this please issue #32377

@rmmh
Copy link
Contributor

rmmh commented Sep 11, 2016

@k8s-bot gke test this issue #32431

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 11, 2016
@liggitt liggitt force-pushed the anonymous-authenticated-groups branch from 7d0f953 to 85d1288 Compare September 11, 2016 18:19
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 11, 2016
@liggitt
Copy link
Member Author

liggitt commented Sep 12, 2016 •
edited
Loading

@cjcullen I can't tell if the gke smoke test failures are flakes, or related to the system:authenticated group being sent to the authorizer. Do you know what the GKE webhook authorizer would do with that?

@zhouhaibing089
Copy link
Contributor

Very useful, thanks!

@liggitt liggitt mentioned this pull request Sep 13, 2016
Merged
erictune
erictune reviewed Sep 13, 2016
View reviewed changes
pkg/genericapiserver/options/server_run_options.go
@@ -258,6 +259,11 @@ func (s *ServerRunOptions) AddUniversalFlags(fs *pflag.FlagSet) {
"If specified, a username which avoids RBAC authorization checks and role binding "+
"privilege escalation checks, to be used with --authorization-mode=RBAC.")

fs.BoolVar(&s.AnonymousAuth, "anonymous-auth", s.AnonymousAuth, ""+
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this flag be called "forbid-anonymous-auth", and default to false.

That is because we expect most people to enable it going forward, and we don't want a flag that is expected to be required to be specified.

The release note can have a warning that: people who are relying on authentication alone to authorize access should add authorization or set --forbid-anonymous-auth.

Copy link
Member Author

@liggitt liggitt Sep 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I'd like to avoid a negative in the flag name if possible. Just updated the default to true.

@erictune
Copy link
Member

Please change flag default. Other than that, LGTM.

@cjcullen
Copy link
Member

@k8s-bot gke test this issue: #IGNORE

@cjcullen
Copy link
Member

(kicking the GKE tests to confirm that the unknown user thing is fixed).

@liggitt
Copy link
Member Author

liggitt commented Sep 14, 2016

Tracked down the GKE failure to SubjectAccessReview serializing groups as group (#32709). The GKE webhook didn't expect that field, and returned an error.

@liggitt liggitt added this to the v1.5 milestone Sep 15, 2016
@liggitt liggitt force-pushed the anonymous-authenticated-groups branch from 85d1288 to c3f37a1 Compare September 16, 2016 00:00
@k8s-bot
Copy link

k8s-bot commented Sep 16, 2016

GCE e2e build/test passed for commit c3f37a1345b1d3af5a45a73ceaf561f6b1374cac.

@liggitt liggitt added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Sep 16, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2016
@liggitt liggitt force-pushed the anonymous-authenticated-groups branch from c3f37a1 to 41746f1 Compare September 22, 2016 18:57
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 22, 2016
@liggitt liggitt force-pushed the anonymous-authenticated-groups branch from 41746f1 to 5599ca3 Compare September 26, 2016 21:19
@liggitt liggitt removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GCI Kubemark GCE e2e failed for commit 5599ca3. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark gci e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 5599ca3. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit 5599ca3. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot k8s-github-robot mentioned this pull request Sep 26, 2016
Closed
@liggitt liggitt changed the title Decorate users with system:authenticated and system:unauthenticated groups Decorate users with system:authenticated and system:unauthenticated groups, allow anonymous API server access Sep 27, 2016
@liggitt liggitt changed the title Decorate users with system:authenticated and system:unauthenticated groups, allow anonymous API server access Allow anonymous API server access, decorate authenticated users with system:authenticated group Sep 27, 2016
@liggitt
Copy link
Member Author

liggitt commented Sep 27, 2016

flag default, help text, and release note updated, GKE smoke test passing

@liggitt liggitt mentioned this pull request Sep 28, 2016
Merged
@liggitt
Copy link
Member Author

liggitt commented Sep 28, 2016

doc PR at kubernetes/website#1342

@deads2k
Copy link
Contributor

deads2k commented Sep 29, 2016

@liggitt can you confirm that this: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/32386/kubernetes-pull-build-test-gci-e2e-gce/188/ is a flake, not a result of this change?

@deads2k
Copy link
Contributor

deads2k commented Sep 29, 2016

@liggitt once you've confirmed, this lgtm.

@liggitt
Copy link
Member Author

liggitt commented Sep 29, 2016

@k8s-bot gci gce e2e test this
@k8s-bot gci gke e2e test this
@k8s-bot kubemark gci e2e test this

@deads2k deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit d187997 into kubernetes:master Sep 29, 2016
@liggitt liggitt deleted the anonymous-authenticated-groups branch September 29, 2016 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@erictune erictune

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.