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

Fix an issue about generation of secret key with invalid key size in gce #67139

Merged
merged 2 commits into from
Sep 6, 2018
Merged

Fix an issue about generation of secret key with invalid key size in gce #67139

merged 2 commits into from
Sep 6, 2018

Conversation

yogi-sagar
Copy link
Contributor

@yogi-sagar yogi-sagar commented Aug 8, 2018 •
edited
Loading

Replace /dev/random to /dev/urandom to avoid generation of secret key with invalid key size.

What this PR does / why we need it:
In GCE env, AESGCM encryption of secrets by default generates a secret key with /dev/random which sometime generates a key with invalid size.
This cause the cluster/kube-up.sh to fail in gce environment.

This PR replaces /dev/random with /dev/urandom to have a secret key generated consistently with right size.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #67091

Special notes for your reviewer:

Release note:

cluster/gce: generate consistent key sizes in config-default.sh using /dev/urandom instead of /dev/random

@yogi-sagar
Fix for issue kubernetes#67091
f42f6d8 
Replace /dev/random to /dev/urandom to avoid generation of secret key with invalid key size.
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 8, 2018
@yogi-sagar yogi-sagar changed the title Fix for issue #67091 Fix an issue about generation of secret key with invalid key size in gce Aug 8, 2018
@neolit123
Copy link
Member

/cc @kubernetes/sig-gcp-bugs
/kind bug
/ok-to-test

@yogi-sagar please add a release note explaining the change.

@k8s-ci-robot k8s-ci-robot added sig/gcp kind/bug Categorizes issue or PR as related to a bug. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 8, 2018
@yogi-sagar
Copy link
Contributor Author

@neolit123 Please review, if the Release Notes comment is sufficient.

@neolit123
Copy link
Member

@yogi-sagar

Currently, by default secret key is being generated using /dev/random which sometimes generates the key with invalid size. Therefore, we need to use /dev/urandom to make secret key generation consistent.

make the note in imperative form and give a location of the change, here is how:

cluster/gce: generate consistent key sizes in config-default.sh using /dev/urandom instead of /dev/random

@yogi-sagar
Copy link
Contributor Author

Thanks @neolit123 ! Updated the release notes comments.

@neolit123
Copy link
Member

@yogi-sagar
thanks, the release note is OK.
you should wait on maintainer comments now.

@abgworrall
Copy link
Contributor

/assign @tallclair
Tim, WDYT

@tallclair
Copy link
Member

I think this LGTM.
/assign @immutableT
for a second opinion

@k8s-ci-robot
Copy link
Contributor

@tallclair: GitHub didn't allow me to assign the following users: immutableT.

Note that only kubernetes members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

I think this LGTM.
/assign @immutableT
for a second opinion

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.

@immutableT
Copy link
Contributor

LGTM

@tallclair
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 Aug 17, 2018
@jagosan
Copy link
Contributor

jagosan commented Aug 21, 2018

/retest

1 similar comment
@yogi-sagar
Copy link
Contributor Author

/retest

@tpepper
Copy link
Member

tpepper commented Aug 23, 2018

What about using the "iflag=fullblock" flag for dd? And adding logic, since this is a shell script, to confirm the requested length was received? Since this is the deprecated cluster/ area, do we not care about security implications to use of urandom? Does GCE make a high entropy device available to guest instances?

@tallclair
Copy link
Member

/assign @mwielgus

@saad-ali
Copy link
Member

saad-ali commented Sep 4, 2018

/milestone v1.12

@k8s-ci-robot k8s-ci-robot added this to the v1.12 milestone Sep 4, 2018
@saad-ali saad-ali added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 4, 2018
@saad-ali
Copy link
Member

saad-ali commented Sep 4, 2018

Remove hold once @destijl approves

@yogi-sagar
Copy link
Contributor Author

/retest

@destijl
Copy link
Member

destijl commented Sep 4, 2018

@yogi-sagar just waiting on fullblock, can you do that today?

dd if=/dev/urandom bs=32 count=1 iflag=fullblock 2>/dev/null | base64 | tr -d '\r\n'

@mikedanese
Copy link
Member

/lgtm cancel

@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 4, 2018
@yogi-sagar
Fix for issue kubernetes#67091
0f9c7b4 
Replace /dev/random to /dev/urandom to avoid generation of secret key with invalid key size.
@k8s-ci-robot k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Sep 4, 2018
@yogi-sagar
Copy link
Contributor Author

/retest

@destijl
Copy link
Member

destijl commented Sep 4, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 4, 2018
@yogi-sagar
Copy link
Contributor Author

/retest

@yogi-sagar
Copy link
Contributor Author

yogi-sagar commented Sep 4, 2018 •
edited
Loading

@destijl Uploaded new patch with the suggested change.
Never mind, just saw your lgtm comment. Thanks!

@yogi-sagar
Copy link
Contributor Author

/retest

1 similar comment
@yogi-sagar
Copy link
Contributor Author

/retest

@mikedanese
Copy link
Member

Related: #68256

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: destijl, mikedanese, tallclair, yogi-sagar

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2018
@yogi-sagar
Copy link
Contributor Author

/retest

@destijl
Copy link
Member

destijl commented Sep 5, 2018

@mikedanese can you remove the merge hold?

@mikedanese
Copy link
Member

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 5, 2018
@mikedanese mikedanese added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Sep 5, 2018
@mikedanese
Copy link
Member

Low impact bug fix that reduces flakiness of cluster-up

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

@k8s-github-robot k8s-github-robot merged commit a555044 into kubernetes:master Sep 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bowei bowei Awaiting requested review from bowei

@destijl destijl Awaiting requested review from destijl

Assignees

@destijl destijl

@mikedanese mikedanese

@tallclair tallclair

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. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.12
Development

Successfully merging this pull request may close these issues.

Use of /dev/random generates secret key with invalid size in gce