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

ClientConfig should not default to http://localhost:8080 #30808

Merged
merged 1 commit into from
Aug 30, 2016
Merged

ClientConfig should not default to http://localhost:8080 #30808

merged 1 commit into from
Aug 30, 2016
+50 −15

Conversation

smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Aug 17, 2016 •
edited
Loading

This changes clientcmd to skip the default cluster, but preserves the
behavior in kubectl. This prevents the possibility of an administrator
misconfiguration in kubelet or other server component from allowing a
third party who can bind to 8080 on that host from potentially
impersonating an API server and gaining root access.

@mikedanese @deads2k this removes the defaulting of http://localhost:8080 for server from everything except kubectl.

This change is Reviewable

Kubernetes server components using `kubeconfig` files no longer default to `http://localhost:8080`.  Administrators must specify a server value in their kubeconfig files.

@deads2k
Copy link
Contributor

deads2k commented Aug 17, 2016

lgtm

@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 Aug 17, 2016
@smarterclayton smarterclayton 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 Aug 17, 2016
@smarterclayton
Copy link
Contributor Author

Added a release note.

@mikedanese
Copy link
Member

@kubernetes/sig-api-machinery

@smarterclayton
Copy link
Contributor Author

Any other comments on top of David's review?

@lavalamp
Copy link
Member

none from me.

@mikedanese
Copy link
Member

lgtm

@mikedanese mikedanese added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 18, 2016
@smarterclayton smarterclayton added the priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. label Aug 20, 2016
@smarterclayton smarterclayton added this to the v1.4 milestone Aug 20, 2016
@mikedanese
Copy link
Member

@k8s-bot ok to test, issue: #IGNORE

@k8s-github-robot k8s-github-robot mentioned this pull request Aug 25, 2016
Closed
@k8s-bot
Copy link

k8s-bot commented Aug 29, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

@mikedanese
Copy link
Member

@smarterclayton looks like this is breaking a test. Might want to take a look if you want this in v1.4

@smarterclayton
Copy link
Contributor Author

Ah, new code added. Will fix.

@smarterclayton smarterclayton mentioned this pull request Aug 30, 2016
Closed
@smarterclayton
ClientConfig should not default to http://localhost:8080
06cbb29 
This changes clientcmd to skip the default cluster, but preserves the
behavior in kubectl. This prevents the possibility of an administrator
misconfiguration in kubelet or other server component from allowing a
third party who can bind to 8080 on that host from potentially
impersonating an API server and gaining root access.
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2016
@smarterclayton
Copy link
Contributor Author

@k8s-bot test this issue #31706

@k8s-bot
Copy link

k8s-bot commented Aug 30, 2016

GCE e2e build/test passed for commit 06cbb29.

@smarterclayton
Copy link
Contributor Author

Fixed the new test the same way, reapplying label

@smarterclayton smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2016
@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 Aug 30, 2016

GCE e2e build/test passed for commit 06cbb29.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit be859b1 into kubernetes:master Aug 30, 2016
@caesarxuchao caesarxuchao mentioned this pull request Sep 20, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@deads2k deads2k

Labels
area/security lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. 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
v1.4
Development

Successfully merging this pull request may close these issues.
8 participants
@smarterclayton @deads2k @mikedanese @lavalamp @k8s-bot @k8s-github-robot @caesarxuchao @googlebot