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

Enable basic auth username rotation for GCI #44590

Merged
merged 4 commits into from
May 5, 2017
Merged

Enable basic auth username rotation for GCI #44590

merged 4 commits into from
May 5, 2017

Conversation

ikehz
Copy link
Contributor

@ikehz ikehz commented Apr 18, 2017

When changing basic auth creds, just delete the whole file, in order to be able to rotate username in addition to password.

When changing basic auth creds, just delete the whole file, in order …
2acf60e 
…to be able to rotate username in addition to password
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 18, 2017
@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Apr 18, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@ikehz
Copy link
Contributor Author

ikehz commented Apr 18, 2017

This should probably go to @cjcullen and/or @cheftako for review.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 18, 2017
@ikehz
Copy link
Contributor Author

ikehz commented Apr 18, 2017

(I signed the CLA as an employee of Google.)

@gmarek
Copy link
Contributor

gmarek commented Apr 18, 2017

I don't think we want to delete whole file, as user might have added stuff to it. But I'll let @cjcullen do the review.

@gmarek
Copy link
Contributor

gmarek commented Apr 18, 2017

/approve

@gmarek gmarek assigned cjcullen and unassigned gmarek and jszczepkowski Apr 18, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 18, 2017
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 21, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 21, 2017
@ikehz
Copy link
Contributor Author

ikehz commented Apr 21, 2017

@cjcullen ptal.

cjcullen
cjcullen reviewed Apr 21, 2017
View reviewed changes
cluster/gce/config-default.sh
@@ -233,6 +233,7 @@ ENABLE_LEGACY_ABAC="${ENABLE_LEGACY_ABAC:-true}" # true, false
# Kernel panic upon soft lockup issue
SOFTLOCKUP_PANIC="${SOFTLOCKUP_PANIC:-false}" # true, false

# Indicates if the values (eg. kube password) in metadata should be treated as
# canonical, and therefore disk copies ought to be recreated/clobbered.
# Indicates if the values (i.e. KUBE_USER and KUBE_PASSWORD for basic
Copy link
Member

Choose a reason for hiding this comment

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

nit: e.g.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an intentional change; KUBE_USER and KUBE_PASSWORD aren't examples, they are actually the things that are overwritten. https://english.stackexchange.com/questions/1629/e-g-versus-i-e/1631

Copy link
Member

Choose a reason for hiding this comment

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

Ahh gotcha.

I'd say "Indicates if the KUBE_USER and KUBE_PASSWORD values..." because I won't be the only one who misunderstands this, and assumes this env var has wider scope than it does.

cjcullen
cjcullen reviewed Apr 21, 2017
View reviewed changes
cluster/gce/gci/configure-helper.sh Outdated
@@ -286,31 +286,30 @@ function create-master-auth {
local -r auth_dir="/etc/srv/kubernetes"
local -r basic_auth_csv="${auth_dir}/basic_auth.csv"
if [[ -n "${KUBE_PASSWORD:-}" && -n "${KUBE_USER:-}" ]]; then
# Remove basic_auth_csv because we will rewrite it.
Copy link
Member

Choose a reason for hiding this comment

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

Can you put this comment one line below and be a little more specific (because this bash is so gross).

Maybe:
When METADATA_CLOBBERS_CONFIG==true, just remove the basic auth file, because the append_or_replace calls below will regenerate the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@cjcullen
Copy link
Member

@cheftako Can you take a quick look at this too?

cheftako
cheftako reviewed Apr 22, 2017
View reviewed changes
cluster/gce/gci/configure-helper.sh
# completely, because if we're changing KUBE_USER and KUBE_PASSWORD, we
# have nothing to match on. The file is replaced just below with
# append_or_replace_prefixed_line.
rm "${basic_auth_csv}"
Copy link
Member

Choose a reason for hiding this comment

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

The original sed nastiness was chosen rather than a file remove because there was a belief that in GCE a customer might have manually added their own users. This fix is clearly fine in GKE, but I'm not sure if our original concerns about additional GCE user accounts no longer applies. If it does still apply then this does not seem safe.

cheftako
cheftako reviewed Apr 22, 2017
View reviewed changes
Copy link
Member

@cheftako cheftako left a comment

Choose a reason for hiding this comment

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

With the exception of the file rm this seems good to me. If we believe the original extra user account in GCE issue no longer applies, then all good.

@ikehz
Copy link
Contributor Author

ikehz commented Apr 24, 2017

With the exception of the file rm this seems good to me. If we believe the original extra user account in GCE issue no longer applies, then all good.

I wasn't part of that original discussion, but I'd give two arguments that this is okay (1) This defaults to false on GCE, so a user has to explicitly set it to true to clobber their existing config, and (2) if they're manually adding auth to the auth file, why would they use metadata to automatically edit that file anyway? Seems like a pretty unlikely use-case to me, but of course it's entirely possible.

The alternative is to create a separate flag, (METADATA_CLOBBERS_ALL_BASIC_AUTH_CONFIG or something) that does this. The name of the original flag (METADATA_CLOBBERS_CONFIG) suggests to me that we clobber arbitrary config; if we only wanted to edit one username in the basic auth file, shouldn't it be called something like METADATA_CLOBBERS_AUTOGEN_AUTH_USER or something?

@ikehz
Copy link
Contributor Author

ikehz commented Apr 27, 2017

Ping @cheftako @cjcullen.

@cjcullen
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 27, 2017
@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Apr 27, 2017
@ikehz
Copy link
Contributor Author

ikehz commented Apr 28, 2017

/assign @mikedanese for OWNERS approval.

@mikedanese
Copy link
Member

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjcullen, gmarek, ihmccreery, mikedanese

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2017
@ikehz
Copy link
Contributor Author

ikehz commented May 2, 2017

@cjcullen Can you please add the appropriate release notes labels? "release-note-none," I guess?

@cjcullen cjcullen added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 3, 2017
@spxtr spxtr removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label May 5, 2017
@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 (batch tested with PRs 44590, 44969, 45325, 45208, 44714)

@k8s-github-robot k8s-github-robot merged commit d7f72dc into kubernetes:master May 5, 2017
@ikehz
Copy link
Contributor Author

ikehz commented May 15, 2017

@cjcullen Can you please go through the motions of getting this cherrypicked (add the v1.6 milestone and cherrypick-candidate label)? I'm not an admin on kubernetes/kubernetes, so I can't...

@cjcullen cjcullen added this to the v1.6 milestone May 15, 2017
@cjcullen cjcullen mentioned this pull request May 16, 2017
Merged
@enisoc enisoc added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 16, 2017
k8s-github-robot pushed a commit that referenced this pull request May 17, 2017
Merge pull request #45866 from cjcullen/automated-cherry-pick-of-#445…
51b4579 
…90-upstream-release-1.6

Automatic merge from submit-queue

Automated cherry pick of #44590

Cherry pick of #44590 on release-1.6.

#44590: When changing basic auth creds, just delete the whole
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.6" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cheftako cheftako cheftako left review comments

@cjcullen cjcullen cjcullen left review comments

Assignees

@cjcullen cjcullen

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. 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
v1.6
Development

Successfully merging this pull request may close these issues.