Merge pull request #646 from 3scale/validate-policy-configs

davidor · web-flow · commit 2affe4e40e26 · 2018-03-07T16:50:40.000+01:00
Validation for policy configurations
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 - New property `summary` in the policy manifests [PR #633](https://github.com/3scale/apicast/pull/633)
 - OAuth2.0 Token Introspection policy [PR #619](https://github.com/3scale/apicast/pull/619)
+- New `metrics` phase that runs when prometheus is collecting metrics [PR #629](https://github.com/3scale/apicast/pull/629)
+- Validation of policy configs both in integration and unit tests [PR #646](https://github.com/3scale/apicast/pull/646)
 
 ## Fixed
 
@@ -19,10 +21,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Bug in URL rewriting policy that ignored the `commands` attribute in the policy manifest [PR #641](https://github.com/3scale/apicast/pull/641)
 - Skip comentaries after `search` values in resolv.conf [PR #635](https://github.com/3scale/apicast/pull/635)
 
-## Added
-
-- New `metrics` phase that runs when prometheus is collecting metrics [PR #629](https://github.com/3scale/apicast/pull/629)
-
 ## [3.2.0-beta1] - 2018-02-20
 
 ## Added
diff --git a/gateway/Roverfile b/gateway/Roverfile
@@ -4,6 +4,7 @@ luarocks {
 
     group 'testing' {
         module { 'busted' },
+        module { 'ljsonschema' },
     },
 
     group 'development' {
diff --git a/gateway/Roverfile.lock b/gateway/Roverfile.lock
@@ -4,6 +4,7 @@ dkjson 2.5-2||testing
 inspect 3.1.1-0||production
 ldoc 1.4.6-2||development
 liquid 0.1.0-1||production
+ljsonschema 0.1.0-1||testing
 lua-resty-env 0.4.0-1||production
 lua-resty-execvp 0.1.0-1||production
 lua-resty-http 0.12-0||production
@@ -18,6 +19,7 @@ luassert 1.7.10-0||testing
 luasystem 0.2.1-0||testing
 markdown 0.33-1||development
 mediator_lua 1.1.2-0||testing
+net-url 0.9-1||testing
 nginx-lua-prometheus 0.20171117-4||production
 penlight 1.5.4-1||production,development,testing
 router 2.1-0||production
diff --git a/gateway/src/apicast/policy/caching/apicast-policy.json b/gateway/src/apicast/policy/caching/apicast-policy.json
@@ -27,19 +27,19 @@
         "type": "string",
         "oneOf": [
           {
-            "const": "resilient",
+            "enum": ["resilient"],
             "description": "Authorize according to last request when backend is down."
           },
           {
-            "const": "strict",
+            "enum": ["strict"],
             "description": "It only caches authorized calls."
           },
           {
-            "const": "allow",
+            "enum": ["allow"],
             "description": "When backend is down, allow everything unless seen before and denied."
           },
           {
-            "const": "none",
+            "enum": ["none"],
             "description": "Disables caching."
           }
         ]
diff --git a/gateway/src/apicast/policy/caching/caching.lua b/gateway/src/apicast/policy/caching/caching.lua
@@ -98,7 +98,7 @@ end
 -- @tparam[opt] table config
 -- @field caching_type Caching type (strict, resilient, allow, none)
 function _M.new(config)
-  local self = new()
+  local self = new(config)
   self.cache_handler = handler(config or {})
   return self
 end
diff --git a/gateway/src/apicast/policy/cors/cors.lua b/gateway/src/apicast/policy/cors/cors.lua
@@ -25,7 +25,7 @@ local new = _M.new
 -- @field[opt] allow_origin Allowed origins (e.g. 'http://example.com', '*')
 -- @field[opt] allow_credentials Boolean
 function _M.new(config)
-  local self = new()
+  local self = new(config)
   self.config = config or {}
   return self
 end
diff --git a/gateway/src/apicast/policy/echo/apicast-policy.json b/gateway/src/apicast/policy/echo/apicast-policy.json
@@ -18,11 +18,11 @@
         "type": "string",
         "oneOf": [
           {
-            "const": "request",
+            "enum": ["request"],
             "description": "Interrupts the processing of the request."
           },
           {
-            "const": "set",
+            "enum": ["set"],
             "description": "Only skips the rewrite phase."
           }
         ]
diff --git a/gateway/src/apicast/policy/headers/apicast-policy.json b/gateway/src/apicast/policy/headers/apicast-policy.json
@@ -22,15 +22,15 @@
               "type": "string",
               "oneOf": [
                 {
-                  "const": "add",
+                  "enum": ["add"],
                   "description": "Adds a value to an existing header."
                 },
                 {
-                  "const": "set",
+                  "enum": ["set"],
                   "description": "Creates the header when not set, replaces its value when set."
                 },
                 {
-                  "const": "push",
+                  "enum": ["push"],
                   "description": "Creates the header when not set, adds the value when set."
                 }
               ]
diff --git a/gateway/src/apicast/policy/headers/headers.lua b/gateway/src/apicast/policy/headers/headers.lua
@@ -106,7 +106,7 @@ end
 --   2) When the header is set, it creates a new header with the same name and
 --      the given value.
 function _M.new(config)
-  local self = new()
+  local self = new(config)
   self.config = init_config(config)
   return self
 end
diff --git a/gateway/src/apicast/policy/token_introspection/token_introspection.lua b/gateway/src/apicast/policy/token_introspection/token_introspection.lua
@@ -10,7 +10,7 @@ local resty_env = require('resty.env')
 local new = _M.new
 
 function _M.new(config)
-  local self = new()
+  local self = new(config)
   self.config = config or {}
   --- authorization for the token introspection endpoint.
   -- https://tools.ietf.org/html/rfc7662#section-2.2
diff --git a/gateway/src/apicast/policy/url_rewriting/apicast-policy.json b/gateway/src/apicast/policy/url_rewriting/apicast-policy.json
@@ -23,11 +23,11 @@
               "type": "string",
               "oneOf": [
                 {
-                  "const": "sub",
+                  "enum": ["sub"],
                   "description": "Substitutes the first match of the regex applied."
                 },
                 {
-                  "const": "gsub",
+                  "enum": ["gsub"],
                   "description": "Substitutes all the matches of the regex applied."
                 }
               ]
diff --git a/gateway/src/apicast/policy/url_rewriting/url_rewriting.lua b/gateway/src/apicast/policy/url_rewriting/url_rewriting.lua
@@ -57,7 +57,7 @@ end
 --   - break[opt]: defaults to false. When set to true, if the command rewrote
 --     the URL, it will be the last command applied.
 function _M.new(config)
-  local self = new()
+  local self = new(config)
   self.commands = (config and config.commands) or {}
   return self
 end
diff --git a/gateway/src/apicast/policy_config_validator.lua b/gateway/src/apicast/policy_config_validator.lua
@@ -0,0 +1,20 @@
+--- Policy Config Validator
+-- @module policy_config_validator
+-- Validates a policy configuration against a policy config JSON schema.
+
+local jsonschema = require('jsonschema')
+
+local _M = { }
+
+--- Validate a policy configuration
+-- Checks if a policy configuration is valid according to the given schema.
+-- @tparam table config Policy configuration
+-- @tparam table config_schema Policy configuration schema
+-- @treturn boolean True if the policy configuration is valid. False otherwise.
+-- @treturn string Error message only when the policy config is invalid.
+function _M.validate_config(config, config_schema)
+  local validator = jsonschema.generate_validator(config_schema or {})
+  return validator(config or {})
+end
+
+return _M
diff --git a/gateway/src/apicast/policy_loader.lua b/gateway/src/apicast/policy_loader.lua
@@ -14,6 +14,8 @@ local insert = table.insert
 local setmetatable = setmetatable
 local pcall = pcall
 
+local policy_config_validator = require('apicast.policy_config_validator')
+
 local _M = {}
 
 local resty_env = require('resty.env')
@@ -38,6 +40,15 @@ do
   end
 end
 
+-- Returns true if config validation has been enabled via ENV or if we are
+-- running Test::Nginx integration tests. We know that the framework always
+-- sets TEST_NGINX_BINARY so we can use it to detect whether we are running the
+-- tests.
+local function policy_config_validation_is_enabled()
+  return resty_env.enabled('APICAST_VALIDATE_POLICY_CONFIGS')
+    or resty_env.value('TEST_NGINX_BINARY')
+end
+
 local function read_manifest(path)
   local handle = io.open(format('%s/%s', path, 'apicast-policy.json'))
 
@@ -71,43 +82,69 @@ local function load_manifest(name, version, path)
   return nil, lua_load_path(path)
 end
 
+local function with_config_validator(policy, policy_config_schema)
+  local original_new = policy.new
+
+  local new_with_validator = function(config)
+    local is_valid, err = policy_config_validator.validate_config(
+      config, policy_config_schema)
+
+    if not is_valid then
+      error(format('Invalid config for policy: %s', err))
+    end
+
+    return original_new(config)
+  end
+
+  return setmetatable(
+    { new = new_with_validator },
+    { __index = policy }
+  )
+end
+
 function _M:load_path(name, version, paths)
   local failures = {}
 
   for _, path in ipairs(paths or self.policy_load_paths()) do
-    local ok, load_path = load_manifest(name, version, format('%s/%s/%s', path, name, version) )
+    local manifest, load_path = load_manifest(name, version, format('%s/%s/%s', path, name, version) )
 
-    if ok then
-      return load_path
+    if manifest then
+      return load_path, manifest.configuration
     else
       insert(failures, load_path)
     end
   end
 
   if version == 'builtin' then
-    local ok, load_path = load_manifest(name, version, format('%s/%s', self.builtin_policy_load_path(), name) )
+    local manifest, load_path = load_manifest(name, version, format('%s/%s', self.builtin_policy_load_path(), name) )
 
-    if ok then
-      return load_path
+    if manifest then
+      return load_path, manifest.configuration
     else
       insert(failures, load_path)
     end
   end
 
-  return nil, failures
+  return nil, nil, failures
 end
 
 function _M:call(name, version, dir)
   local v = version or 'builtin'
-  local load_path, invalid_paths = self:load_path(name, v, dir)
+  local load_path, policy_config_schema, invalid_paths = self:load_path(name, v, dir)
 
   local loader = sandbox.new(load_path and { load_path } or invalid_paths)
 
   ngx.log(ngx.DEBUG, 'loading policy: ', name, ' version: ', v)
 
   -- passing the "exclusive" flag for the require so it does not fallback to native require
   -- it should load only policies and not other code and fail if there is no such policy
-  return loader('init', true)
+  local res = loader('init', true)
+
+  if policy_config_validation_is_enabled() then
+    return with_config_validator(res, policy_config_schema)
+  else
+    return res
+  end
 end
 
 function _M:pcall(name, version, dir)
diff --git a/spec/policy/caching/caching_spec.lua b/spec/policy/caching/caching_spec.lua
@@ -24,16 +24,6 @@ describe('Caching policy', function()
       ctx.cache_handler(cache, 'a_key', { status = 200 }, nil)
       assert.is_nil(cache:get('a_key'))
     end)
-
-    it('disables caching when invalid caching type is specified', function()
-      local config = { caching_type = 'invalid_caching_type' }
-      local caching_policy = require('apicast.policy.caching').new(config)
-      local ctx = {}
-      caching_policy:rewrite(ctx)
-
-      ctx.cache_handler(cache, 'a_key', { status = 200 }, nil)
-      assert.is_nil(cache:get('a_key'))
-    end)
   end)
 
   describe('.access', function()
diff --git a/spec/policy_config_validator_spec.lua b/spec/policy_config_validator_spec.lua
@@ -0,0 +1,58 @@
+local cjson = require('cjson')
+local policy_config_validator = require('apicast.policy_config_validator')
+
+describe('policy_config_validator', function()
+  -- We use the echo policy schema as an example for the tests.
+  local test_config_schema = cjson.decode([[
+    {
+      "type": "object",
+      "properties": {
+        "status": {
+          "description": "HTTP status code to be returned",
+          "type": "integer"
+        },
+        "exit": {
+          "description": "Exit mode",
+          "type": "string",
+          "oneOf": [{
+              "enum": ["request"],
+              "description": "Interrupts the processing of the request."
+            },
+            {
+              "enum": ["set"],
+              "description": "Only skips the rewrite phase."
+            }
+          ]
+        }
+      }
+    }
+  ]])
+
+  it('returns true with a config that conforms with the schema', function()
+    local valid_config = { status = 200, exit = "request" }
+
+    local is_valid, err = policy_config_validator.validate_config(
+      valid_config, test_config_schema)
+
+    assert.is_true(is_valid)
+    assert.is_nil(err)
+  end)
+
+  it('returns false with a config that does not conform with the schema', function()
+    local invalid_config = { status = "not_an_integer" }
+
+    local is_valid, err = policy_config_validator.validate_config(
+      invalid_config, test_config_schema)
+
+    assert.is_false(is_valid)
+    assert.is_not_nil(err)
+  end)
+
+  it('returns true when the schema is empty', function()
+    assert.is_true(policy_config_validator.validate_config({ a_param = 1 }, {}))
+  end)
+
+  it('returns true when the schema is nil', function()
+    assert.is_true(policy_config_validator.validate_config({ a_param = 1 }, nil))
+  end)
+end)
diff --git a/spec/spec_helper.lua b/spec/spec_helper.lua
@@ -38,6 +38,9 @@ local function reset()
 
   previous_env = {}
 
+  -- To make sure that we are using valid policy configs in the tests.
+  set('APICAST_VALIDATE_POLICY_CONFIGS', true)
+
   env.reset()
 end
 
diff --git a/t/apicast-policy-invalid-config.t b/t/apicast-policy-invalid-config.t
diff --git a/t/fixtures/policies/example_policy/1.0.0/apicast-policy.json b/t/fixtures/policies/example_policy/1.0.0/apicast-policy.json
diff --git a/t/fixtures/policies/example_policy/1.0.0/example_policy.lua b/t/fixtures/policies/example_policy/1.0.0/example_policy.lua
diff --git a/t/fixtures/policies/example_policy/1.0.0/init.lua b/t/fixtures/policies/example_policy/1.0.0/init.lua

Original file line number	Diff line number	Diff line change
`@@ -27,19 +27,19 @@`
`27`	`27`	`"type": "string",`
`28`	`28`	`"oneOf": [`
`29`	`29`	`{`
`30`		`- "const": "resilient",`
	`30`	`+ "enum": ["resilient"],`
`31`	`31`	`"description": "Authorize according to last request when backend is down."`
`32`	`32`	`},`
`33`	`33`	`{`
`34`		`- "const": "strict",`
	`34`	`+ "enum": ["strict"],`
`35`	`35`	`"description": "It only caches authorized calls."`
`36`	`36`	`},`
`37`	`37`	`{`
`38`		`- "const": "allow",`
	`38`	`+ "enum": ["allow"],`
`39`	`39`	`"description": "When backend is down, allow everything unless seen before and denied."`
`40`	`40`	`},`
`41`	`41`	`{`
`42`		`- "const": "none",`
	`42`	`+ "enum": ["none"],`
`43`	`43`	`"description": "Disables caching."`
`44`	`44`	`}`
`45`	`45`	`]`
Original file line number	Diff line number	Diff line change
`@@ -18,11 +18,11 @@`
`18`	`18`	`"type": "string",`
`19`	`19`	`"oneOf": [`
`20`	`20`	`{`
`21`		`- "const": "request",`
	`21`	`+ "enum": ["request"],`
`22`	`22`	`"description": "Interrupts the processing of the request."`
`23`	`23`	`},`
`24`	`24`	`{`
`25`		`- "const": "set",`
	`25`	`+ "enum": ["set"],`
`26`	`26`	`"description": "Only skips the rewrite phase."`
`27`	`27`	`}`
`28`	`28`	`]`
Original file line number	Diff line number	Diff line change
`@@ -22,15 +22,15 @@`
`22`	`22`	`"type": "string",`
`23`	`23`	`"oneOf": [`
`24`	`24`	`{`
`25`		`- "const": "add",`
	`25`	`+ "enum": ["add"],`
`26`	`26`	`"description": "Adds a value to an existing header."`
`27`	`27`	`},`
`28`	`28`	`{`
`29`		`- "const": "set",`
	`29`	`+ "enum": ["set"],`
`30`	`30`	`"description": "Creates the header when not set, replaces its value when set."`
`31`	`31`	`},`
`32`	`32`	`{`
`33`		`- "const": "push",`
	`33`	`+ "enum": ["push"],`
`34`	`34`	`"description": "Creates the header when not set, adds the value when set."`
`35`	`35`	`}`
`36`	`36`	`]`