-
Notifications
You must be signed in to change notification settings - Fork 171
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.
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
Sandbox policy loading #566
Conversation
7d42c94
to
95ddd64
Compare
if not mod then | ||
local ok | ||
ngx.log(ngx.DEBUG, 'native require for: ', modname) | ||
ok, mod = prequire(modname) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pcall might not be needed if we want to raise on the next line anyway.
for i=1, #package.searchers do | ||
ret, err = package.searchers[i](modname) | ||
|
||
if type(ret) == 'function' then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to paste parts of the Lua documentation describing what loaders can return.
|
||
local preload = package.preload | ||
|
||
preload['apicast.policy_loader'] = function() return _M end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might not be needed anymore.
187319a
to
70f3983
Compare
spec/policy_loader_spec.lua
Outdated
|
||
describe('APIcast Policy Loader', function() | ||
|
||
describe('.call', function() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need tests that show that the code works correctly when it loads two different versions of the same policy. I guess that testing this it's a bit difficult, because we'll need, at least, to create some test policies in the fixtures
folder.
Maybe worth taking a look whether injecting apicast_dir
would make that test easier or clearer.
local mod = require(module) | ||
if sub(module, 1, 14) == 'apicast.policy' then | ||
module = sub(module, 16) | ||
version = 'builtin' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implies that we will not be offering several versions of the same policy in a given version of Apicast. Not saying I don't agree with that, just mentioning it so we're aware of this important decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, no two versions of builtin policies. For now. This is quite easy change to make if we want to.
IMO it would be quite difficult to maintain > 1 versions of builtin policies, so I think it is not worth the effort. Internals of this method could be changed if we change that in the future.
gateway/cpanfile
Outdated
@@ -1,2 +1,2 @@ | |||
requires 'Test::APIcast', '0.03'; | |||
requires 'Test::APIcast', '0.05'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you released 0.06
some days ago. Any reason not to use that one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason. 0.06 was released after I changed this. Will update 👍
spec/executor_spec.lua
Outdated
for _, phase in ipairs(phases) do | ||
executor[phase](executor) | ||
for _, policy in ipairs(default_executor_chain) do | ||
for _, phase in Policy.phases() do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to say that the test would be incomplete after this change, but I see that in the specs of policy we're already testing that Policy.phases()
returns the correct phases. Nice cleanup 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the test was incomplete before this change :) Because It didn't had content
. I think there might be one more place where it has own list.
assert.is_table(sandbox) | ||
assert.are_not.same(root, sandbox) | ||
assert.equal(root._NAME, sandbox._NAME) | ||
assert.equal(root._VERSION, sandbox._VERSION) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These asserts
are good, but how can we know that this is really the Apicast policy 👀 ? Should we at least check that it contains some of its more important methods? The phases for example. I feel like we need something more than testing that it's just a table with the correct name and version. Ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that if you load the same policy twice by the policy loader it is actually different table with different functions. The file is evaluated and returned twice. And it is always going to be different than the one returned by require.
Each policy has all the phases, so we can't really compare that.
I tried adding there some random id, but it is different every time you load the policy. I really have no nice way of comparing them.
@@ -0,0 +1,199 @@ | |||
--- Policy loader | |||
-- This module loads a policy defined by its name and version. | |||
-- It uses sandboxed require to isolate dependencies and not mutate global state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe emphasize somehow that this loader allows us to load several versions of the same policy without problems?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. It even allows loading the same policy several times without global state. Will emphasize that.
if mod ~= nil then | ||
package.loaded[modname] = mod | ||
else | ||
package.loaded[modname] = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this assignment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add there the comment from the docs 👍
--- this is environment exposed to the policies | ||
-- that means this is very light sandbox so policies don't mutate global env | ||
-- and most importantly we replace the require function with our own | ||
_M.env = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, how did you decide what to include and what not to include here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to include everything that is in standard Lua env. And it was just trial & error. So I guess something might be missing still.
I could go through every key in the normal env and make sure it is here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't want to expose code loading functions that could escape the sandbox, right?
The sandbox is meant to be just light shell, not a thorough security measure, but still.
Here is global from repl (without package):
<1>{
_ = <table 1>,
_G = <table 1>,
_VERSION = "Lua 5.1",
assert = <function 1>,
bit = {
arshift = <function 2>,
band = <function 3>,
bnot = <function 4>,
bor = <function 5>,
bswap = <function 6>,
bxor = <function 7>,
lshift = <function 8>,
rol = <function 9>,
ror = <function 10>,
rshift = <function 11>,
tobit = <function 12>,
tohex = <function 13>
},
collectgarbage = <function 14>,
coroutine = {
create = <function 15>,
isyieldable = <function 16>,
resume = <function 17>,
running = <function 18>,
status = <function 19>,
wrap = <function 20>,
yield = <function 21>
},
debug = {
debug = <function 22>,
getfenv = <function 23>,
gethook = <function 24>,
getinfo = <function 25>,
getlocal = <function 26>,
getmetatable = <function 27>,
getregistry = <function 28>,
getupvalue = <function 29>,
getuservalue = <function 30>,
setfenv = <function 31>,
sethook = <function 32>,
setlocal = <function 33>,
setmetatable = <function 34>,
setupvalue = <function 35>,
setuservalue = <function 36>,
traceback = <function 37>,
upvalueid = <function 38>,
upvaluejoin = <function 39>
},
dofile = <function 40>,
error = <function 41>,
gcinfo = <function 42>,
getfenv = <function 43>,
getmetatable = <function 44>,
io = {
close = <function 45>,
flush = <function 46>,
input = <function 47>,
lines = <function 48>,
open = <function 49>,
output = <function 50>,
popen = <function 51>,
read = <function 52>,
stderr = <userdata 1>,
stdin = <userdata 2>,
stdout = <userdata 3>,
tmpfile = <function 53>,
type = <function 54>,
write = <function 55>
},
ipairs = <function 56>,
jit = {
arch = "x64",
attach = <function 57>,
flush = <function 58>,
off = <function 59>,
on = <function 60>,
opt = {
start = <function 61>
},
os = "OSX",
status = <function 62>,
version = "LuaJIT 2.1.0-beta3",
version_num = 20100
},
load = <function 63>,
loadfile = <function 64>,
loadstring = <function 65>,
math = {
abs = <function 66>,
acos = <function 67>,
asin = <function 68>,
atan = <function 69>,
atan2 = <function 70>,
ceil = <function 71>,
cos = <function 72>,
cosh = <function 73>,
deg = <function 74>,
exp = <function 75>,
floor = <function 76>,
fmod = <function 77>,
frexp = <function 78>,
huge = inf,
ldexp = <function 79>,
log = <function 80>,
log10 = <function 81>,
max = <function 82>,
min = <function 83>,
modf = <function 84>,
pi = 3.1415926535898,
pow = <function 85>,
rad = <function 86>,
random = <function 87>,
randomseed = <function 88>,
sin = <function 89>,
sinh = <function 90>,
sqrt = <function 91>,
tan = <function 92>,
tanh = <function 93>
},
module = <function 94>,
newproxy = <function 95>,
next = <function 96>,
os = {
clock = <function 97>,
date = <function 98>,
difftime = <function 99>,
execute = <function 100>,
exit = <function 101>,
getenv = <function 102>,
remove = <function 103>,
rename = <function 104>,
setlocale = <function 105>,
time = <function 106>,
tmpname = <function 107>
},
pairs = <function 108>,
pcall = <function 109>,
print = <function 110>,
rawequal = <function 111>,
rawget = <function 112>,
rawlen = <function 113>,
rawset = <function 114>,
require = <function 115>,
select = <function 116>,
setfenv = <function 117>,
setmetatable = <function 118>,
string = {
byte = <function 119>,
char = <function 120>,
dump = <function 121>,
find = <function 122>,
format = <function 123>,
gmatch = <function 124>,
gsub = <function 125>,
len = <function 126>,
lower = <function 127>,
match = <function 128>,
rep = <function 129>,
reverse = <function 130>,
sub = <function 131>,
upper = <function 132>
},
table = {
concat = <function 133>,
foreach = <function 134>,
foreachi = <function 135>,
getn = <function 136>,
insert = <function 137>,
maxn = <function 138>,
move = <function 139>,
pack = <function 140>,
remove = <function 141>,
sort = <function 142>,
unpack = <function 143>
},
tonumber = <function 144>,
tostring = <function 145>,
type = <function 146>,
unpack = <function 143>,
xpcall = <function 147>
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a new sandbox in 0ef60e7
e626d7e
to
0ef60e7
Compare
* allows sandboxed load of code - includes vendored dependencies - policy can access just own source tree + shared APIcast code - loading same policy twice results in two different instances
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 🏅
Depends on #578
Policies will be able to load only own code (in their subpath).
Loading the policy twice results in different policy.
Different policies (and versions) can have different dependencies.
Very light sandbox is not meant for security, but for ease of use and preventing mistakes. Policies should not be able to access the global environment (except
ngx
variable).Spec: https://gist.github.com/mikz/ceb8f141de2e8f74206761abda231f70
TODO (in future PRs)
policy.lua
to${policy_name}.lua
and createinit.lua
for each policy (according to the spec) ([policy] rename policy.lua to init.lua #579)