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

gce: use getrandom instead of urandom for on node rng #68256

Merged
merged 1 commit into from
Sep 6, 2018
Merged

gce: use getrandom instead of urandom for on node rng #68256

merged 1 commit into from
Sep 6, 2018

Conversation

mikedanese
Copy link
Member

@mikedanese mikedanese commented Sep 5, 2018 •
edited
Loading

In the context, our urandoms where generally safe, however getrandom has
built in invariants around entropy pool initialization, making getrandom
safe in all contexts. This should protect us from cryptopasta errors or
weird entropy issues.

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 5, 2018
@k8s-ci-robot k8s-ci-robot added 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. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Sep 5, 2018
tallclair
tallclair reviewed Sep 5, 2018
View reviewed changes
cluster/gce/gci/configure-helper.sh Outdated
# ARGS:
# #1: number of secure bytes to generate. We round up to the nearest factor of 32.
function secure_random {
local infobits="${1}"
Copy link
Member

Choose a reason for hiding this comment

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

bits or bytes? be consistent.

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.

cluster/gce/gci/configure-helper.sh Outdated
function secure_random {
local infobits="${1}"
if ((infobits <= 0)); then
echo "Requested insufficient entropy" 1>&2
Copy link
Member

Choose a reason for hiding this comment

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

nit: everywhere else we just output errors & warnings to stdout
EDIT: realized that would conflict with the return value

Copy link
Member

Choose a reason for hiding this comment

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

This might be a more informative message: Invalid argument to secure_random: infobits=$infobits

cluster/gce/gci/configure-helper.sh Outdated
) | sha256sum \
| head -c 64 \
| xxd -r -p
);
Copy link
Member

Choose a reason for hiding this comment

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

Should this be quoted?

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.

cluster/gce/gci/configure-helper.sh Outdated
) | sha256sum \
| head -c 64 \
| xxd -r -p
);
Copy link
Member

Choose a reason for hiding this comment

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

I ran this a few times, and sometimes get:

bash: warning: command substitution: ignored null byte in input

I think that might be losing some entropy?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, this is fixed by defering the hex decode out of the loop to when we do the base64 encode.

cluster/gce/gci/configure-helper.sh
KUBE_CLUSTER_AUTOSCALER_TOKEN=$(dd if=/dev/urandom bs=128 count=1 2>/dev/null | base64 | tr -d "=+/" | dd bs=32 count=1 2>/dev/null)
KUBE_CONTROLLER_MANAGER_TOKEN="$(secure_random 32)"
KUBE_SCHEDULER_TOKEN="$(secure_random 32)"
KUBE_CLUSTER_AUTOSCALER_TOKEN="$(secure_random 32)"
Copy link
Member

Choose a reason for hiding this comment

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

The output of the new function is longer & has a different format. Is that a problem?

$ for i in $(seq 1 10); do secure_random 32; done
2rh09yQmL2OkCrLBl+NzjFZXfW8p/I1waEcdcInPt74=
V2P0Sx8DbQ2fSHmBlD5TIQ13WnE+krXzSytQdYSavIs=
bash: warning: command substitution: ignored null byte in input
onSYBtYtkZSDiMswwlsZJJyOg07hMnGzq+QBRTlU
bash: warning: command substitution: ignored null byte in input
ujDlqE9CwYkwTVYGhHbYbc9qK4jJg1H2dloz0ghSPA==
lIsXSn4xCUDZm6FMwdsywU25anFsb4da7l6jgfVHl0w=
ZB0ant3bWCDrIo4nomBWSL2T91MNxnSMtIU5U/kRQw==
mcWNml7f0s8/lnI30/6+YscTwPkMcZKQzKwgPF/hh5k=
V5kpzout27rBdxp2tw8MfGcrnmG+J08MaZYBavt40Yg=
ZIbO4M2pQhYW9DMY3kxD4Y8y3rRdGfIVFR50IRwyLmM=
bash: warning: command substitution: ignored null byte in input
n9JL2XTEBra7bNUM6P8d5jbAvmAPa3cIMud9jyxb/Q==

vs

for i in $(seq 1 10); do dd if=/dev/urandom bs=128 count=1 2>/dev/null | base64 | tr -d "=+/" | dd bs=32 count=1 2>/dev/null; echo; done
g4SCrOKRgSZoNbdCK5kRyTXQhyG5FzdP
LPl5C4OGwjaxQgAsU7UKv57nLEGwURsk
qpZ9s0XxJlpbrVv6fpEpiKyKHYZVF4us
k88N0Ap7ElIsDpEzyqPyHozYUJGYwH41
b0FEzZgpO0SYsik6zSwWk4wYKTYLcYng
reLHdWw4griQ8ilXDms7UZkcLhyV0nGD
o5jgSUGTNumsLB0lxaMLs36004wrBX7o
tW5xFRmIzCtZBq5BA1TOhEEVZGhd9OdG
IGSlrmfazSXXAcaElkJqhlz5UlAkgtkr
YX3Hw8QXyCqsraV3hWH0ZyJy9VIu8y7k

Copy link
Member Author

Choose a reason for hiding this comment

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

The output changed from 32 characters to 44 characters. 44 characters is the minimum number of bytes required to carry 32 bytes of entropy base64 encoded. I don't think this should be an issue.

Copy link
Member

Choose a reason for hiding this comment

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

The other difference is the tr -d "=+/" - does it matter?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's neither helpful nor harmful. I don't feel strongly about it either way.

@mikedanese mikedanese mentioned this pull request Sep 5, 2018
Merged
@tallclair
Copy link
Member

/lgtm
/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 5, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2018
@mikedanese mikedanese added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2018
tytso
tytso approved these changes Sep 5, 2018
View reviewed changes
Copy link

@tytso tytso left a comment

Choose a reason for hiding this comment

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

I would have added a comment at line 65 of configure-helper.sh explaining that sha256sum emits hex digits in ascii, so we need 64 characters, but that's just a matter of personal taste. The question whether this will be obvious to a future reader of the code.

@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2018
@mikedanese
gce: use getrandom instead of urandom for on node rng
eac0410 
In the context, our urandoms where generally safe, however getrandom has
built in invariants around entropy pool initialization, making getrandom
safe in all contexts. This should protect us from cryptopasta errors or
weird entropy issues.
@mikedanese
Copy link
Member Author

/retest

@mikedanese mikedanese 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 this to the v1.12 milestone Sep 5, 2018
@mikedanese mikedanese added the lgtm "Looks good to me", indicates that a PR is ready to be merged. 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 6, 2018
@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 (batch tested with PRs 68087, 68256, 64621, 68299, 68296). 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 5878b28 into kubernetes:master Sep 6, 2018
@k8s-ci-robot
Copy link
Contributor

@mikedanese: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-integration eac0410 link /test pull-kubernetes-integration
pull-kubernetes-e2e-gke eac0410 link /test pull-kubernetes-e2e-gke

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@mikedanese mikedanese deleted the nourand branch September 7, 2018 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tallclair tallclair tallclair left review comments

@tytso tytso tytso approved these changes

@jingax10 jingax10 Awaiting requested review from jingax10

@mwielgus mwielgus Awaiting requested review from mwielgus

Assignees

@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. 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-none Denotes a PR that doesn't merit a release note. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.12
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mikedanese @tallclair @k8s-ci-robot @k8s-github-robot @tytso