From d87a1e1907607d63b13c113b234c33f1d397306c Mon Sep 17 00:00:00 2001 From: leslie Date: Fri, 3 Dec 2021 13:43:36 +0800 Subject: [PATCH 01/12] fix(patch): add global `math.randomseed` patch support --- apisix/patch.lua | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/apisix/patch.lua b/apisix/patch.lua index 51cb14bff8e9..4d08e6309670 100644 --- a/apisix/patch.lua +++ b/apisix/patch.lua @@ -20,18 +20,24 @@ local ipmatcher = require("resty.ipmatcher") local socket = require("socket") local unix_socket = require("socket.unix") local ssl = require("ssl") +local ngx = ngx local get_phase = ngx.get_phase local ngx_socket = ngx.socket local original_tcp = ngx.socket.tcp local original_udp = ngx.socket.udp local concat_tab = table.concat +local debug = debug local new_tab = require("table.new") local log = ngx.log local WARN = ngx.WARN local ipairs = ipairs local select = select local setmetatable = setmetatable +local string = string +local table = table local type = type +local tonumber = tonumber +local tostring = tostring local config_local @@ -86,6 +92,53 @@ do end +-- Inspired by kong.globalpatches +do -- `_G.math.randomseed` patch + local resty_random = require("resty.random") + local math_randomseed = math.randomseed + local seeded = {} + -- make linter happy + -- luacheck: ignore + _G.math.randomseed = function() + local seed + local worker_pid = ngx.worker.pid() + + -- check seed mark + if seeded[worker_pid] then + log(WARN, debug.traceback("attempt to seed already seeded random number " .. + "generator on process #" .. tostring(worker_pid), 2)) + return + end + + -- get randomseed + local bytes = resty_random.bytes(8) + if bytes then + log(ngx.INFO, "seeding from resty.random.bytes") + + local t = {} + for i = 1, #bytes do + t[i] = string.byte(bytes, i) + end + + local str = table.concat(t) + if #str > 12 then + str = string.sub(str, 1, 12) + end + seed = tonumber(str) + else + log(ngx.ERR, "could not seed from resty.random.bytes, seeding ", + "seeding with time and process id instead (this can ", + "result to duplicated seeds)") + + seed = ngx.now() * 1000 + worker_pid + end + + seeded[worker_pid] = true + math_randomseed(seed) + end +end -- do + + local patch_udp_socket do local old_udp_sock_setpeername From 0322ee0c3b11097663ab491df2148657ab1bece7 Mon Sep 17 00:00:00 2001 From: leslie Date: Sun, 5 Dec 2021 15:56:05 +0800 Subject: [PATCH 02/12] lint: fix code lint for patch.lua --- .luacheckrc | 7 +++++++ apisix/patch.lua | 5 ++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/.luacheckrc b/.luacheckrc index f3dc1249c76f..3b6f11175e2f 100644 --- a/.luacheckrc +++ b/.luacheckrc @@ -2,6 +2,13 @@ std = "ngx_lua" unused_args = false redefined = false max_line_length = 100 + exclude_files = { "apisix/cli/ngx_tpl.lua", } + +files["apisix/patch.lua"] = { + globals = { + "math.randomseed", + }, +} diff --git a/apisix/patch.lua b/apisix/patch.lua index 4d08e6309670..697c21d1b7ce 100644 --- a/apisix/patch.lua +++ b/apisix/patch.lua @@ -97,9 +97,8 @@ do -- `_G.math.randomseed` patch local resty_random = require("resty.random") local math_randomseed = math.randomseed local seeded = {} - -- make linter happy - -- luacheck: ignore - _G.math.randomseed = function() + + math.randomseed = function() local seed local worker_pid = ngx.worker.pid() From 6b9615d6467cc6aa7bd1ba1453bf41af9e9cdcfe Mon Sep 17 00:00:00 2001 From: leslie Date: Sun, 5 Dec 2021 15:57:24 +0800 Subject: [PATCH 03/12] chore: change log level --- apisix/patch.lua | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apisix/patch.lua b/apisix/patch.lua index 697c21d1b7ce..8c1fd298e2d8 100644 --- a/apisix/patch.lua +++ b/apisix/patch.lua @@ -104,7 +104,7 @@ do -- `_G.math.randomseed` patch -- check seed mark if seeded[worker_pid] then - log(WARN, debug.traceback("attempt to seed already seeded random number " .. + log(ngx.DEBUG, debug.traceback("attempt to seed already seeded random number " .. "generator on process #" .. tostring(worker_pid), 2)) return end @@ -112,7 +112,7 @@ do -- `_G.math.randomseed` patch -- get randomseed local bytes = resty_random.bytes(8) if bytes then - log(ngx.INFO, "seeding from resty.random.bytes") + log(ngx.DEBUG, "seeding from resty.random.bytes") local t = {} for i = 1, #bytes do From 60f4f635fe90ea3ef079fbddfbe81e56b957578e Mon Sep 17 00:00:00 2001 From: leslie Date: Sun, 5 Dec 2021 15:59:42 +0800 Subject: [PATCH 04/12] chore: explain why 12 was chosen --- apisix/patch.lua | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/apisix/patch.lua b/apisix/patch.lua index 8c1fd298e2d8..6e6f3a9908f5 100644 --- a/apisix/patch.lua +++ b/apisix/patch.lua @@ -121,6 +121,12 @@ do -- `_G.math.randomseed` patch local str = table.concat(t) if #str > 12 then + -- truncate the final number to prevent integer overflow, + -- since math.randomseed() could get cast to a platform-specific + -- integer with a different size and get truncated, hence, lose + -- randomness. + -- double-precision floating point should be able to represent numbers + -- without rounding with up to 15/16 digits but let's use 12 of them. str = string.sub(str, 1, 12) end seed = tonumber(str) From df1b4be60dbf500bdca0bfb19e73a5929bf77bfc Mon Sep 17 00:00:00 2001 From: leslie Date: Sun, 5 Dec 2021 16:18:12 +0800 Subject: [PATCH 05/12] chore: explain why `_G.math.randomseed` need to be patched --- apisix/patch.lua | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/apisix/patch.lua b/apisix/patch.lua index 6e6f3a9908f5..6eae5a228a90 100644 --- a/apisix/patch.lua +++ b/apisix/patch.lua @@ -92,8 +92,15 @@ do end --- Inspired by kong.globalpatches do -- `_G.math.randomseed` patch + + -- Seeds the random generator, use with care. + -- Once - properly - seeded, this method is replaced with a stub + -- one. This is to enforce best-practices for seeding in ngx_lua, + -- and prevents third-party modules from overriding our correct seed + -- (many modules make a wrong usage of `math.randomseed()` by calling + -- it multiple times or by not using unique seeds for Nginx workers). + -- Inspired by kong.globalpatches local resty_random = require("resty.random") local math_randomseed = math.randomseed local seeded = {} @@ -105,7 +112,7 @@ do -- `_G.math.randomseed` patch -- check seed mark if seeded[worker_pid] then log(ngx.DEBUG, debug.traceback("attempt to seed already seeded random number " .. - "generator on process #" .. tostring(worker_pid), 2)) + "generator on process #" .. tostring(worker_pid), 2)) return end From fb17c8fc15f0ba77c0d8f956936ec57a6f1a7408 Mon Sep 17 00:00:00 2001 From: leslie Date: Mon, 6 Dec 2021 08:33:24 +0800 Subject: [PATCH 06/12] chore: update linter config --- .luacheckrc | 7 ------- apisix/patch.lua | 2 ++ 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/.luacheckrc b/.luacheckrc index 3b6f11175e2f..f3dc1249c76f 100644 --- a/.luacheckrc +++ b/.luacheckrc @@ -2,13 +2,6 @@ std = "ngx_lua" unused_args = false redefined = false max_line_length = 100 - exclude_files = { "apisix/cli/ngx_tpl.lua", } - -files["apisix/patch.lua"] = { - globals = { - "math.randomseed", - }, -} diff --git a/apisix/patch.lua b/apisix/patch.lua index 6eae5a228a90..3a1f24f72efd 100644 --- a/apisix/patch.lua +++ b/apisix/patch.lua @@ -105,6 +105,8 @@ do -- `_G.math.randomseed` patch local math_randomseed = math.randomseed local seeded = {} + -- make linter happy + -- luacheck: ignore math.randomseed = function() local seed local worker_pid = ngx.worker.pid() From b773c8b0858bf4ca7207c86756f1296ea657503a Mon Sep 17 00:00:00 2001 From: leslie Date: Mon, 6 Dec 2021 08:48:31 +0800 Subject: [PATCH 07/12] chore: apply code review suggestion --- apisix/patch.lua | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/apisix/patch.lua b/apisix/patch.lua index 3a1f24f72efd..7a9fb229a60f 100644 --- a/apisix/patch.lua +++ b/apisix/patch.lua @@ -38,6 +38,7 @@ local table = table local type = type local tonumber = tonumber local tostring = tostring +local min = math.min local config_local @@ -124,20 +125,17 @@ do -- `_G.math.randomseed` patch log(ngx.DEBUG, "seeding from resty.random.bytes") local t = {} - for i = 1, #bytes do + -- truncate the final number to prevent integer overflow, + -- since math.randomseed() could get cast to a platform-specific + -- integer with a different size and get truncated, hence, lose + -- randomness. + -- double-precision floating point should be able to represent numbers + -- without rounding with up to 15/16 digits but let's use 12 of them. + for i = 1, min(#bytes, 12) do t[i] = string.byte(bytes, i) end local str = table.concat(t) - if #str > 12 then - -- truncate the final number to prevent integer overflow, - -- since math.randomseed() could get cast to a platform-specific - -- integer with a different size and get truncated, hence, lose - -- randomness. - -- double-precision floating point should be able to represent numbers - -- without rounding with up to 15/16 digits but let's use 12 of them. - str = string.sub(str, 1, 12) - end seed = tonumber(str) else log(ngx.ERR, "could not seed from resty.random.bytes, seeding ", From f9bc5a3e78cec1b7462d1a09af049529c23a4cbd Mon Sep 17 00:00:00 2001 From: leslie <59061168+leslie-tsang@users.noreply.github.com> Date: Mon, 6 Dec 2021 10:05:13 +0800 Subject: [PATCH 08/12] chore: apply suggestion from spacewander MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: 罗泽轩 --- apisix/patch.lua | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apisix/patch.lua b/apisix/patch.lua index 7a9fb229a60f..b26a68daf5c4 100644 --- a/apisix/patch.lua +++ b/apisix/patch.lua @@ -93,7 +93,7 @@ do end -do -- `_G.math.randomseed` patch +do -- `math.randomseed` patch -- Seeds the random generator, use with care. -- Once - properly - seeded, this method is replaced with a stub From 982bea5592e674911521dd812eb53a73668cbc44 Mon Sep 17 00:00:00 2001 From: leslie Date: Tue, 7 Dec 2021 13:51:06 +0800 Subject: [PATCH 09/12] refactor: refactor the patch implementation --- apisix/patch.lua | 58 ++++++++++++++++-------------------------------- 1 file changed, 19 insertions(+), 39 deletions(-) diff --git a/apisix/patch.lua b/apisix/patch.lua index b26a68daf5c4..bd2ab10e40b2 100644 --- a/apisix/patch.lua +++ b/apisix/patch.lua @@ -94,59 +94,39 @@ end do -- `math.randomseed` patch + -- `math.random` generates PRND(pseudo-random numbers) from the seed set by `math.randomseed` + -- Many module libraries use `ngx.time` and `ngx.worker.pid` to generate seeds which may + -- loss randomness in container env (where pids are identical, e.g. root pid is 1) + -- Kubernetes may launch multi instance with deployment RS, so `ngx.time` may get same return in pods. + -- Therefore, this global patch enforce entire framework to use the best-practice PRND generates. - -- Seeds the random generator, use with care. - -- Once - properly - seeded, this method is replaced with a stub - -- one. This is to enforce best-practices for seeding in ngx_lua, - -- and prevents third-party modules from overriding our correct seed - -- (many modules make a wrong usage of `math.randomseed()` by calling - -- it multiple times or by not using unique seeds for Nginx workers). - -- Inspired by kong.globalpatches local resty_random = require("resty.random") local math_randomseed = math.randomseed - local seeded = {} + local seeded -- make linter happy -- luacheck: ignore math.randomseed = function() - local seed - local worker_pid = ngx.worker.pid() - -- check seed mark - if seeded[worker_pid] then - log(ngx.DEBUG, debug.traceback("attempt to seed already seeded random number " .. - "generator on process #" .. tostring(worker_pid), 2)) + if seeded or false then + log(ngx.DEBUG, debug.traceback("Random seed has been inited", 2)) return end - -- get randomseed - local bytes = resty_random.bytes(8) - if bytes then - log(ngx.DEBUG, "seeding from resty.random.bytes") - - local t = {} - -- truncate the final number to prevent integer overflow, - -- since math.randomseed() could get cast to a platform-specific - -- integer with a different size and get truncated, hence, lose - -- randomness. - -- double-precision floating point should be able to represent numbers - -- without rounding with up to 15/16 digits but let's use 12 of them. - for i = 1, min(#bytes, 12) do - t[i] = string.byte(bytes, i) - end + -- generate randomseed + -- chose 6 from APISIX's SIX, 256 ^ 6 should do the trick + -- it shouldn't be large than 16 to prevent overflow. + local random_bytes = resty_random.bytes(6) + local t = {} - local str = table.concat(t) - seed = tonumber(str) - else - log(ngx.ERR, "could not seed from resty.random.bytes, seeding ", - "seeding with time and process id instead (this can ", - "result to duplicated seeds)") - - seed = ngx.now() * 1000 + worker_pid + for i = 1, #random_bytes do + t[i] = string.byte(random_bytes, i) end - seeded[worker_pid] = true - math_randomseed(seed) + local s = table.concat(t) + + seeded = true + math_randomseed(tonumber(s)) end end -- do From 7dd38ca12722708d4035aef811e404fbdc1b423f Mon Sep 17 00:00:00 2001 From: leslie Date: Tue, 7 Dec 2021 14:11:54 +0800 Subject: [PATCH 10/12] chore: fix the code lint --- apisix/patch.lua | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/apisix/patch.lua b/apisix/patch.lua index bd2ab10e40b2..9100ebb8b3d8 100644 --- a/apisix/patch.lua +++ b/apisix/patch.lua @@ -37,8 +37,6 @@ local string = string local table = table local type = type local tonumber = tonumber -local tostring = tostring -local min = math.min local config_local @@ -97,8 +95,10 @@ do -- `math.randomseed` patch -- `math.random` generates PRND(pseudo-random numbers) from the seed set by `math.randomseed` -- Many module libraries use `ngx.time` and `ngx.worker.pid` to generate seeds which may -- loss randomness in container env (where pids are identical, e.g. root pid is 1) - -- Kubernetes may launch multi instance with deployment RS, so `ngx.time` may get same return in pods. - -- Therefore, this global patch enforce entire framework to use the best-practice PRND generates. + -- Kubernetes may launch multi instance with deployment RS at the same time, `ngx.time` may + -- get same return in the pods. + -- Therefore, this global patch enforce entire framework to use + -- the best-practice PRND generates. local resty_random = require("resty.random") local math_randomseed = math.randomseed From d8c36ec31bb543ffba354c125dc5bd998410bf7e Mon Sep 17 00:00:00 2001 From: leslie Date: Tue, 7 Dec 2021 14:18:24 +0800 Subject: [PATCH 11/12] chore: apply code review suggestion --- apisix/patch.lua | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apisix/patch.lua b/apisix/patch.lua index 9100ebb8b3d8..6ce1dcbfdd7d 100644 --- a/apisix/patch.lua +++ b/apisix/patch.lua @@ -108,7 +108,7 @@ do -- `math.randomseed` patch -- luacheck: ignore math.randomseed = function() -- check seed mark - if seeded or false then + if not seeded then log(ngx.DEBUG, debug.traceback("Random seed has been inited", 2)) return end @@ -125,8 +125,8 @@ do -- `math.randomseed` patch local s = table.concat(t) - seeded = true math_randomseed(tonumber(s)) + seeded = true end end -- do From cb8de985d4a443dc12559149a181d0f641cb4c25 Mon Sep 17 00:00:00 2001 From: leslie Date: Tue, 7 Dec 2021 16:15:00 +0800 Subject: [PATCH 12/12] fix: make sure seed mark work as expected --- apisix/patch.lua | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/apisix/patch.lua b/apisix/patch.lua index 6ce1dcbfdd7d..69506e6594bb 100644 --- a/apisix/patch.lua +++ b/apisix/patch.lua @@ -102,13 +102,15 @@ do -- `math.randomseed` patch local resty_random = require("resty.random") local math_randomseed = math.randomseed - local seeded + local seeded = {} -- make linter happy -- luacheck: ignore math.randomseed = function() + local worker_pid = ngx.worker.pid() + -- check seed mark - if not seeded then + if seeded[worker_pid] then log(ngx.DEBUG, debug.traceback("Random seed has been inited", 2)) return end @@ -126,7 +128,7 @@ do -- `math.randomseed` patch local s = table.concat(t) math_randomseed(tonumber(s)) - seeded = true + seeded[worker_pid] = true end end -- do