bugfix: fixed duplicated seeds. #3671

moonming · 2019-01-17T16:38:41Z

What this PR does / why we need it:
ngx.time() + ngx.worker.pid() maybe get duplicated seeds. get from /dev/urandom first.

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

Special notes for your reviewer:

ngx.time() + ngx.worker.pid() maybe get duplicated seeds. get from /dev/urandom first.

k8s-ci-robot · 2019-01-17T16:38:53Z

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://git.k8s.io/community/CLA.md#the-contributor-license-agreement 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.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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.

ElvinEfendi · 2019-01-18T14:22:17Z

rootfs/etc/nginx/lua/lua_ingress.lua

    return
  end

+  seed = get_seed_from_urandom()
+  if not seed then
+    seed = ngx.now() * 1000 + pid


Why not ngx.time()? That way you'd not have to multiply by 1000.

ngx.time() returns seconds, and ngx.now() returns seconds and milliseconds, so ngx.time() + pid maybe get the same seed, however ngx.now() * 1000 + pid is less chance to the same seed.

ElvinEfendi · 2019-01-18T14:23:41Z

rootfs/etc/nginx/lua/lua_ingress.lua

+  return seed
+end
+
+math.randomseed = function()


I'd rather keep the function signature identical to the original randomseed.

agreed, I will keep the function signature identical to the original randomseed.

ElvinEfendi · 2019-01-18T14:24:35Z

rootfs/etc/nginx/lua/lua_ingress.lua

    ngx.log(ngx.WARN,
-      string.format("ignoring math.randomseed(%d) since PRNG is already seeded for worker %d", seed, pid))


Logging user provided seed makes error message clearer, please keep this as is unless you have a reason to remove it.

ElvinEfendi · 2019-01-18T14:32:04Z

rootfs/etc/nginx/lua/lua_ingress.lua

+  local seed
+  local frandom = io.open("/dev/urandom", "rb")
+  if frandom then
+    local str = frandom:read(4)


Please make sure you ~~handle~~ log errors returned by :read and .open calls.

added error info, and the :read will return nil without err if cannot read data.

ElvinEfendi · 2019-01-18T14:34:35Z

rootfs/etc/nginx/lua/lua_ingress.lua

    return
  end

+  seed = get_seed_from_urandom()
+  if not seed then


Let's log a warning here.

added log for that.

ElvinEfendi · 2019-01-18T22:11:41Z

rootfs/etc/nginx/lua/lua_ingress.lua


-  if seeds[pid] then
+local function get_seed_from_urandom()


Can you explain the rationale behind this function implementation? What's the entropy going to be with this implementation? Why are you reading specifically 4 bytes from /dev/urandom? What does https://github.com/kubernetes/ingress-nginx/pull/3671/files#diff-d2d9d7b4e27b247474196438a3dfbdc3R13 give us?

Why not use rand_bytes from OpenSSL?

/dev/urandom provides an interface to the kernel's random number generator, 4 bytes is good enough for us.

Why not use rand_bytes from OpenSSL?

because the openssl's random is not fork safety(fixed from openssl 1.1.1): https://wiki.openssl.org/index.php/Random_fork-safety.

4 bytes is good enough for us.

How do you decide that? Why not i.e 8 bytes?

4 bytes is good enough for us.

How do you decide that? Why not i.e 8 bytes?

random in this repo is only for choose balancer, not for encryption, so we don't need to read more bytes. If for encryption, we need read 8 bytes maybe.

ElvinEfendi · 2019-01-22T05:07:24Z

rootfs/etc/nginx/lua/lua_ingress.lua

+  seed = 0
+  for i = 1, 4 do
+      seed = 256 * seed + str:byte(i)
+  end


If I understand this correctly you treat the 4 bytes as a base 256 number (where str:byte(4) is the least significant) and then convert it into decimal. Why? Why not simply concatenate the 4 bytes and then use tonumber cast it to a decimal? (I'm not suggesting/asking any change, just want to know what's going on - I don't have much experience with random numbers and crypto in general).

both solutions are ok, however, if we used the tonumber, we need one more function call, and one more table to contact the 4 bytes.
BTW, there is 4 bytes, so we don't need to consider number overflow.

This is not a hot code path, I'd be okay to tradeoff performance for readability but reading this again I think treating the bytes we get from /dev/urandom as base 256 number when converting to decimal makes more sense since they are bytes.

ElvinEfendi · 2019-02-01T18:05:40Z

@moonming I'd really like to get this PR merged. Can you answer my two questions above?

moonming · 2019-02-10T13:45:29Z

@moonming I'd really like to get this PR merged. Can you answer my two questions above?

sorry for late reply, long holiday for Chinese new year.

ElvinEfendi · 2019-02-10T19:29:23Z

/lgtm
/approve

Thanks for the patch and answering my questions @moonming ! And Happy Chinese New Year :)

k8s-ci-robot · 2019-02-10T19:29:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ElvinEfendi, moonming

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:

~~OWNERS~~ [ElvinEfendi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bugfix: fixed duplicated seeds.

0110629

ngx.time() + ngx.worker.pid() maybe get duplicated seeds. get from /dev/urandom first.

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 17, 2019

k8s-ci-robot requested review from aledbf and ElvinEfendi January 17, 2019 16:38

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jan 17, 2019

fixed test case for math.randomseed.

c782f22

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 Jan 18, 2019

aledbf assigned ElvinEfendi Jan 18, 2019

ElvinEfendi reviewed Jan 18, 2019

View reviewed changes

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 21, 2019

moonming force-pushed the randomseed-bugfix branch from b460197 to 4dcbe64 Compare January 21, 2019 09:29

added more error info and keep test cases.

8ea7501

moonming force-pushed the randomseed-bugfix branch from 4dcbe64 to 8ea7501 Compare January 21, 2019 09:32

k8s-ci-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 Jan 21, 2019

ElvinEfendi reviewed Jan 22, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 10, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 10, 2019

k8s-ci-robot merged commit d9845c7 into kubernetes:master Feb 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix: fixed duplicated seeds. #3671

bugfix: fixed duplicated seeds. #3671

moonming commented Jan 17, 2019

k8s-ci-robot commented Jan 17, 2019

ElvinEfendi Jan 18, 2019

moonming Jan 19, 2019

ElvinEfendi Jan 18, 2019

moonming Jan 19, 2019

ElvinEfendi Jan 18, 2019

ElvinEfendi Jan 18, 2019 •

edited

Loading

moonming Jan 21, 2019

ElvinEfendi Jan 18, 2019

moonming Jan 21, 2019

ElvinEfendi Jan 18, 2019 •

edited

Loading

moonming Jan 21, 2019 •

edited

Loading

ElvinEfendi Jan 22, 2019

moonming Feb 10, 2019

ElvinEfendi Jan 22, 2019

moonming Feb 10, 2019

ElvinEfendi Feb 10, 2019

ElvinEfendi commented Feb 1, 2019

moonming commented Feb 10, 2019

ElvinEfendi commented Feb 10, 2019

k8s-ci-robot commented Feb 10, 2019

		ngx.log(ngx.WARN,
		string.format("ignoring math.randomseed(%d) since PRNG is already seeded for worker %d", seed, pid))

bugfix: fixed duplicated seeds. #3671

bugfix: fixed duplicated seeds. #3671

Conversation

moonming commented Jan 17, 2019

k8s-ci-robot commented Jan 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

moonming Jan 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi commented Feb 1, 2019

moonming commented Feb 10, 2019

ElvinEfendi commented Feb 10, 2019

k8s-ci-robot commented Feb 10, 2019

ElvinEfendi Jan 18, 2019 •

edited

Loading

ElvinEfendi Jan 18, 2019 •

edited

Loading

moonming Jan 21, 2019 •

edited

Loading