Sandbox policy loading #566

mikz · 2018-01-26T09:06:03Z

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)

change all policy.lua to ${policy_name}.lua and create init.lua for each policy (according to the spec) ([policy] rename policy.lua to init.lua #579)

mikz · 2018-01-31T16:31:30Z

gateway/src/apicast/policy_loader.lua

+    if not mod then
+      local ok
+      ngx.log(ngx.DEBUG, 'native require for: ', modname)
+      ok, mod = prequire(modname)


This pcall might not be needed if we want to raise on the next line anyway.

mikz · 2018-01-31T16:32:21Z

gateway/src/apicast/policy_loader.lua

+    for i=1, #package.searchers do
+      ret, err = package.searchers[i](modname)
+
+      if type(ret) == 'function' then


Would be good to paste parts of the Lua documentation describing what loaders can return.

mikz · 2018-01-31T17:15:37Z

gateway/src/apicast/policy_loader.lua

+
+local preload = package.preload
+
+preload['apicast.policy_loader'] = function() return _M end


This might not be needed anymore.

davidor · 2018-02-06T09:18:23Z

spec/policy_loader_spec.lua

+
+describe('APIcast Policy Loader', function()
+
+  describe('.call', function()


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.

davidor · 2018-02-06T09:20:14Z

gateway/src/apicast/policy_chain.lua

-        local mod = require(module)
+        if sub(module, 1, 14) == 'apicast.policy' then
+            module = sub(module, 16)
+            version = 'builtin'


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.

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.

davidor · 2018-02-06T09:21:40Z

gateway/cpanfile

@@ -1,2 +1,2 @@
-requires 'Test::APIcast', '0.03';
+requires 'Test::APIcast', '0.05';


I see that you released 0.06 some days ago. Any reason not to use that one?

No reason. 0.06 was released after I changed this. Will update 👍

davidor · 2018-02-06T09:29:31Z

spec/executor_spec.lua

-    for _, phase in ipairs(phases) do
-      executor[phase](executor)
-      for _, policy in ipairs(default_executor_chain) do
+    for _, phase in Policy.phases() do


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 👍

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.

davidor · 2018-02-06T09:36:28Z

spec/policy_loader_spec.lua

+      assert.is_table(sandbox)
+      assert.are_not.same(root, sandbox)
+      assert.equal(root._NAME, sandbox._NAME)
+      assert.equal(root._VERSION, sandbox._VERSION)


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?

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.

davidor · 2018-02-06T09:43:45Z

gateway/src/apicast/policy_loader.lua

@@ -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.


Maybe emphasize somehow that this loader allows us to load several versions of the same policy without problems?

Yep. It even allows loading the same policy several times without global state. Will emphasize that.

davidor · 2018-02-06T10:11:55Z

gateway/src/apicast/policy_loader.lua

+    if mod ~= nil then
+      package.loaded[modname] = mod
+    else
+      package.loaded[modname] = true


Why this assignment?

Will add there the comment from the docs 👍

davidor · 2018-02-06T10:13:51Z

gateway/src/apicast/policy_loader.lua

+--- 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 = {


Just curious, how did you decide what to include and what not to include here ?

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.

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> }

Made a new sandbox in 0ef60e7

* 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

davidor

Awesome 🏅

octobot assigned mikz Jan 26, 2018

mikz force-pushed the policy-loader branch 13 times, most recently from 7d42c94 to 95ddd64 Compare January 31, 2018 15:58

mikz commented Jan 31, 2018

View reviewed changes

This was referenced Feb 1, 2018

[busted] test runner improvements #568

Merged

[policy] implement policy equality and tostring #569

Merged

mikz force-pushed the policy-loader branch 4 times, most recently from 187319a to 70f3983 Compare February 5, 2018 17:12

mikz requested a review from davidor February 5, 2018 17:28

davidor reviewed Feb 6, 2018

View reviewed changes

mikz force-pushed the policy-loader branch 3 times, most recently from e626d7e to 0ef60e7 Compare February 6, 2018 12:58

mikz mentioned this pull request Feb 6, 2018

cleanup spec and integration tests #578

Merged

mikz force-pushed the policy-loader branch from 0ef60e7 to d77b035 Compare February 6, 2018 13:38

mikz changed the title ~~[wip] Sandbox policy loading~~ Sandbox policy loading Feb 6, 2018

mikz requested a review from davidor February 6, 2018 13:47

[policy] implement sandboxed policy loader

31c064b

* 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

mikz force-pushed the policy-loader branch from d77b035 to 31c064b Compare February 6, 2018 14:08

mikz mentioned this pull request Feb 6, 2018

[policy] rename policy.lua to init.lua #579

Merged

2 tasks

davidor approved these changes Feb 6, 2018

View reviewed changes

davidor merged commit 88439dd into master Feb 6, 2018

davidor deleted the policy-loader branch February 6, 2018 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sandbox policy loading #566

Sandbox policy loading #566

mikz commented Jan 26, 2018 •

edited

Loading

mikz Jan 31, 2018 •

edited

Loading

mikz Jan 31, 2018

mikz Jan 31, 2018

davidor Feb 6, 2018

davidor Feb 6, 2018

mikz Feb 6, 2018

davidor Feb 6, 2018

mikz Feb 6, 2018

davidor Feb 6, 2018

mikz Feb 6, 2018

davidor Feb 6, 2018

mikz Feb 6, 2018

davidor Feb 6, 2018

mikz Feb 6, 2018

davidor Feb 6, 2018

mikz Feb 6, 2018

davidor Feb 6, 2018

mikz Feb 6, 2018

mikz Feb 6, 2018

mikz Feb 6, 2018 •

edited

Loading

davidor left a comment


		local preload = package.preload

		preload['apicast.policy_loader'] = function() return _M end


		describe('APIcast Policy Loader', function()

		describe('.call', function()

		@@ -1,2 +1,2 @@
		requires 'Test::APIcast', '0.03';
		requires 'Test::APIcast', '0.05';

Sandbox policy loading #566

Sandbox policy loading #566

Conversation

mikz commented Jan 26, 2018 • edited Loading

TODO (in future PRs)

mikz Jan 31, 2018 • 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

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

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

Choose a reason for hiding this comment

mikz Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

davidor left a comment

Choose a reason for hiding this comment

mikz commented Jan 26, 2018 •

edited

Loading

mikz Jan 31, 2018 •

edited

Loading

mikz Feb 6, 2018 •

edited

Loading