Policy: add on_failed policy

Some users complained that if a policy fails, the request didn't
terminate. This problem is strategic for some users because it can raise
a security flaw if a policy is not executed correctly(jwt_claim_check
policy as an example)

This pull request adds some "pcalls" on policy_chain, where the error is
checked and, if the context has a callback function defined, is called.

Fix THREESCALE-6705

Signed-off-by: Eloy Coto <[email protected]>

CHANGELOG.md

@@ -37,8 +37,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Fix a warning message on invalid upstream [PR #1285](https://github.com/3scale/APIcast/pull/1285) [THREESCALE-5225](https://issues.redhat.com/browse/THREESCALE-5225)
 - Upstream MTLS server verify [PR #1280](https://github.com/3scale/APIcast/pull/1280) [THREESCALE-7099](https://issues.redhat.com/browse/THREESCALE-7099)
 - Add Nginx filter policy [PR #1279](https://github.com/3scale/APIcast/pull/1279) [THREESCALE-6704](https://issues.redhat.com/browse/THREESCALE-6704)
-
-
+- Added on_failed policy [PR#1286](https://github.com/3scale/APIcast/pull/1286) [THREESCALE-6705](https://issues.redhat.com/browse/THREESCALE-6705)
 
 
 ## [3.10.0] 2021-01-04

gateway/src/apicast/configuration.lua

@@ -65,19 +65,14 @@ end
 local function build_policy_chain(policies)
   if not value(policies) then return nil, 'no policy chain' end
 
-  local chain = tab_new(#policies, 0)
-
+  local built_chain = policy_chain.new()
   for i=1, #policies do
-      local policy, err = policy_chain.load_policy(policies[i].name, policies[i].version, policies[i].configuration)
-
-      if policy then
-        insert(chain, policy)
-      elseif err then
-        ngx.log(ngx.WARN, 'failed to load policy: ', policies[i].name, ' version: ', policies[i].version, ' err: ', err)
-      end
+    local ok, err = built_chain:add_policy(policies[i].name, policies[i].version, policies[i].configuration)
+    if err then
+      ngx.log(ngx.WARN, 'failed to load policy: ', policies[i].name, ' version: ', policies[i].version, ' err: ', err)
+    end
   end
 
-  local built_chain = policy_chain.new(chain)
   built_chain:check_order()
   return built_chain
 end

gateway/src/apicast/policy/on_failed/Readme.md

@@ -0,0 +1,4 @@
+# Policy on_failed
+
+When any policy fails, this policy block the request and send back a given
+status code to the user.

gateway/src/apicast/policy/on_failed/apicast-policy.json

@@ -0,0 +1,26 @@
+{
+  "$schema": "http://apicast.io/policy-v1.1/schema#manifest#",
+  "name": "On fail",
+  "summary": "Block request if any policy fails",
+  "description": "When a policy fails, this policy allows to set an error message back to the user and stop processing the request to the upstream API.",
+  "version": "builtin",
+  "order": {
+    "before": [
+      {
+        "name": "apicast",
+        "version": "builtin"
+      }
+    ]
+  },
+  "configuration": {
+    "type": "object",
+    "properties": {
+      "error_status_code": {
+        "description": "Status code that will send to the user if any policy fails",
+        "type": "integer",
+        "minimum": 100,
+        "exclusiveMaximum": 700
+      }
+    }
+  }
+}

gateway/src/apicast/policy/on_failed/init.lua

@@ -0,0 +1 @@
+return require("on_failed")

gateway/src/apicast/policy/on_failed/on_failed.lua

@@ -0,0 +1,20 @@
+local _M  = require('apicast.policy').new('On failed', 'builtin')
+local new = _M.new
+
+
+function _M.new(config)
+  local self = new(config)
+  self.error_status_code = config.error_status_code or ngx.HTTP_SERVICE_UNAVAILABLE
+  return self
+end
+
+function _M:export()
+  return {
+    policy_error_callback = function(policy_name, error_message)
+      ngx.log(ngx.DEBUG, "Stop request because policy: '", policy_name, "' failed, error='", error_message, "'")
+      ngx.exit(self.error_status_code)
+    end
+  }
+end
+
+return _M

gateway/src/apicast/policy_chain.lua

@@ -16,6 +16,7 @@ local sub = string.sub
 local format = string.format
 local pcall = pcall
 local noop = function() end
+local get_phase = ngx.get_phase
 
 require('apicast.loader')
 
@@ -26,7 +27,6 @@ local PolicyOrderChecker = require('apicast.policy_order_checker')
 local policy_manifests_loader = require('apicast.policy_manifests_loader')
 
 local _M = {
-
 }
 
 local mt = {
@@ -178,9 +178,16 @@ function _M:add_policy(name, version, ...)
         return self:insert(policy)
 
     elseif err then
+        -- This will only report the last one that failed, but at least users
+        -- can be aware of the issue
+        self.init_failed = true
+        self.init_failed_policy = {
+          name = name,
+          err = err
+        }
+        -- self.init_failed_err =  err
         ngx.log(ngx.WARN, 'failed to load policy: ', name, ' version: ', version)
         ngx.log(ngx.DEBUG, err)
-
         return false, err
     end
 end
@@ -195,9 +202,26 @@ end
 
 local function call_chain(phase_name)
     return function(self, context)
+
+        if self.init_failed then
+          if context.policy_error_callback then
+            context.policy_error_callback(self.init_failed_policy.name, self.init_failed_policy.err)
+          end
+        end
+
         for i=1, #self do
             ngx.log(ngx.DEBUG, 'policy chain execute phase: ', phase_name, ', policy: ', self[i]._NAME, ', i: ', i)
-            self[i][phase_name](self[i], context)
+            local status, return_val = pcall(self[i][phase_name], self[i], context)
+            if not status then
+              if context.policy_error_callback then
+                  context.policy_error_callback(self[i]._NAME, return_val)
+              end
+              -- This is important because Openresty just died on error on init
+              -- phase, and we should keep in this way.
+              if get_phase() == "init" then
+                error(return_val)
+              end
+            end
         end
 
         return ipairs(self)

spec/policy_chain_spec.lua

@@ -221,4 +221,41 @@ describe('policy_chain', function()
       end)
     end)
   end)
+
+  describe("policy_error_callback", function()
+    local context = {}
+    before_each(function()
+      context = {
+        policy_error_callback = function(name, error_message)
+          return name
+        end
+      }
+      spy.on(context, 'policy_error_callback')
+    end)
+
+    it("trigger when fails on init", function()
+      local policy_chain = _M.new()
+      local res, err = policy_chain:add_policy("invalid", "builtin")
+      assert.falsy(res)
+      assert(err)
+
+      assert.truthy(policy_chain.init_failed)
+      assert.same(policy_chain.init_failed_policy.name, "invalid")
+      assert.truthy(policy_chain.init_failed_policy.err)
+
+      policy_chain:access(context)
+      assert.spy(context.policy_error_callback).was.called()
+    end)
+
+    it("trigger when fails on any phase", function()
+      local policy_chain = _M.new()
+      local res, err = policy_chain:add_policy("echo", "builtin")
+      assert.truthy(res)
+      assert.falsy(err)
+      policy_chain[1].access = function() error("invalid one") end
+
+      policy_chain:access(context)
+      assert.spy(context.policy_error_callback).was.called()
+    end)
+  end)
 end)

t/apicast-policy-on_failed.t

@@ -0,0 +1,111 @@
+use lib 't';
+use Test::APIcast::Blackbox 'no_plan';
+
+use Cwd qw(abs_path);
+
+BEGIN {
+    $ENV{TEST_NGINX_APICAST_POLICY_LOAD_PATH} = 't/fixtures/policies';
+}
+
+env_to_apicast(
+    'APICAST_POLICY_LOAD_PATH' => abs_path($ENV{TEST_NGINX_APICAST_POLICY_LOAD_PATH}),
+);
+
+repeat_each();
+run_tests();
+
+__DATA__
+
+=== TEST 1: policy with invalid configuration return 503
+--- configuration
+{
+  "services": [
+    {
+      "id": 42,
+      "proxy": {
+        "policy_chain": [
+          { "name": "example_policy", "version": "1.0.0", "configuration": { } },
+          { "name": "apicast.policy.on_failed", "configuration": {} },
+          { "name": "apicast.policy.echo" }
+        ]
+      }
+    }
+  ]
+}
+--- request
+GET /test
+--- error_code: 503
+--- error_log
+Stop request because policy: 'example_policy' failed, error=
+
+
+=== TEST 2: policy with access phase issues return 503
+--- configuration
+{
+  "services": [
+    {
+      "id": 42,
+      "proxy": {
+        "policy_chain": [
+          {
+            "name": "example_policy",
+            "version": "1.0.0",
+            "configuration": {
+              "message": "foo",
+              "fail_access": true
+            }
+          },
+          {
+            "name": "apicast.policy.on_failed",
+            "configuration": {}
+          },
+          {
+            "name": "apicast.policy.echo"
+          }
+        ]
+      }
+    }
+  ]
+}
+--- request
+GET /test
+--- error_code: 503
+--- error_log
+Stop request because policy: 'example_policy' failed, error=
+
+
+=== TEST 3: policy with access phase issues return provided status code
+--- configuration
+{
+  "services": [
+    {
+      "id": 42,
+      "proxy": {
+        "policy_chain": [
+          {
+            "name": "example_policy",
+            "version": "1.0.0",
+            "configuration": {
+              "message": "foo",
+              "fail_access": true
+            }
+          },
+          {
+            "name": "apicast.policy.on_failed",
+            "configuration": {
+              "error_status_code": 401
+            }
+          },
+          {
+            "name": "apicast.policy.echo"
+          }
+        ]
+      }
+    }
+  ]
+}
+--- request
+GET /test
+--- error_code: 401
+--- error_log
+Stop request because policy: 'example_policy' failed, error='

t/fixtures/policies/example_policy/1.0.0/example_policy.lua

@@ -11,9 +11,17 @@ function _M.new(configuration)
     policy.message = configuration.message
   end
 
+  policy.fail_access = configuration.fail_access
+
   return policy
 end
 
+function _M:access()
+  if self.fail_access then
+    self.fail()
+  end
+end
+
 function _M:content()
   ngx.say(self.message)
 end