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

Switch to ABAC authorization from AllowAll #24210

Merged
merged 1 commit into from
Apr 27, 2016
Merged

Switch to ABAC authorization from AllowAll #24210

merged 1 commit into from
Apr 27, 2016
+29 −31

Conversation

cjcullen
Copy link
Member

@cjcullen cjcullen commented Apr 13, 2016 •
edited
Loading

Switch from AllowAll to ABAC. All existing identities (that are created by deployment scripts) are given full permissions through ABAC. Manually created identities will need policies added to the policy.jsonl file on the master.

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Apr 13, 2016
@cjcullen cjcullen mentioned this pull request Apr 13, 2016
Closed
@deads2k
Copy link
Contributor

deads2k commented Apr 14, 2016

@kubernetes/kube-iam

I don't have any experience with GCP. Is there a sane way for someone to administer this file?

@cjcullen
Copy link
Member Author

The only way to administer the current ABAC setup is by editing the policy file and then restarting the apiserver.

Authorization is done as a union of plugins, so as long as we have AllowAll in there, we can't do anything useful.

This may bite somebody who is minting their own client certs with different CNs, or is manually adding tokens to known_tokens.csv.

erictune
erictune reviewed Apr 16, 2016
View reviewed changes
cluster/gce/configure-vm.sh
@@ -27,6 +27,7 @@ is_push=$@

readonly KNOWN_TOKENS_FILE="/srv/salt-overlay/salt/kube-apiserver/known_tokens.csv"
readonly BASIC_AUTH_FILE="/srv/salt-overlay/salt/kube-apiserver/basic_auth.csv"
readonly ABAC_POLICY_FILE="/srv/salt-overlay/salt/kube-apiserver/policy.jsonl"
Copy link
Member

Choose a reason for hiding this comment

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

@mikedanese

Copy link
Contributor

Choose a reason for hiding this comment

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

@andyzheng0831

Copy link

Choose a reason for hiding this comment

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

Thanks for heads-up. I will make a change for our stuff.

Copy link
Member

@mikedanese mikedanese Apr 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

what is jsonl? list? i haven't seen that before.

Copy link
Member Author

Choose a reason for hiding this comment

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

JSON lines: http://jsonlines.org/

I hadn't heard of it before this.

@erictune
Copy link
Member

This seems like a safe change. Possibly breaking for uncommon cases, but okay.

  • If people are doing custom cluster setup, then this won't break them.
  • If they are already using ABAC, then they can't be using plain kube-up.
  • If they are creating their cluster with kube-up and then adding tokens in some automated way, I guess they could break. Seems unlikely but worth a release note.

Looks good to me, but I'd like @mikedanese to just take one look too.

roberthbailey
roberthbailey reviewed Apr 16, 2016
View reviewed changes
cluster/gce/util.sh
@@ -1354,3 +1356,8 @@ function ssh-to-node {
function prepare-e2e() {
detect-project
}

# Writes configure-vm.sh to a temporary location with comments stripped.
Copy link
Contributor

Choose a reason for hiding this comment

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

You should expand this comment a bit to explain why we are doing this (because otherwise we are going to overflow the size of a single metadata field). This is the simplest minification that we can do and reduces the number of bytes by about a third.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@mikedanese
Copy link
Member

LGTM

@cjcullen cjcullen added release-note-none Denotes a PR that doesn't merit a release note. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 20, 2016
@cjcullen cjcullen assigned erictune and unassigned roberthbailey Apr 22, 2016
@cjcullen cjcullen changed the title Enable ABAC authorization on GCP Switch to ABAC authorization from AllowAll Apr 27, 2016
@cjcullen
Copy link
Member Author

Updated the PR to add the policy file to source control. Also clarified the title (it isn't just changing GCP).

@erictune
Copy link
Member

LGTM

@erictune erictune added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 27, 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 Apr 27, 2016
roberthbailey
roberthbailey reviewed Apr 27, 2016
View reviewed changes
cluster/saltbase/salt/kube-apiserver/init.sls
/srv/kubernetes/basic_auth.csv:
file.managed:
- source: salt://kube-apiserver/basic_auth.csv
- user: root
- group: root
- mode: 600
/srv/kubernetes/abac-authz-policy.jsonl:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add a newline before the definition for the new managed file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 27, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 27, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 27, 2016

GCE e2e build/test passed for commit 3253739.

@cjcullen cjcullen added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 27, 2016
@cjcullen
Copy link
Member Author

Re-adding LGTM.

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Apr 27, 2016

GCE e2e build/test passed for commit 3253739.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 7a09fbc into kubernetes:master Apr 27, 2016
@andyzheng0831 andyzheng0831 mentioned this pull request Apr 28, 2016
Merged
@aaronlevy aaronlevy mentioned this pull request May 18, 2016
Open
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 Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@cjcullen @deads2k @erictune @mikedanese @k8s-bot @k8s-github-robot @roberthbailey @andyzheng0831 @googlebot @brendandburns