Policy: IpCheck default deny.

Right now, when the IP cannot be retrieved on the IPCheck policy, the request is accepted, but we cannot ensure the origin, so we should deny always by default, and if the verdict is allowed, continue with the request. Fix THREESCALE-7076 Fix THREESCALE-7075 Signed-off-by: Eloy Coto <[email protected]>
3scale · May 25, 2021 · 0d95ee7 · 0d95ee7
1 parent 4aa6b91
commit 0d95ee7
Show file tree

Hide file tree

Showing 4 changed files with 80 additions and 5 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Fixed host header format on http_ng resty [PR #1264](https://github.com/3scale/APIcast/pull/1264) [THREESCALE-2235](https://issues.redhat.com/browse/THREESCALE-2235)
 - Fixed issues on OIDC jwk discovery [PR #1268](https://github.com/3scale/APIcast/pull/1268) [THREESCALE-6913](https://issues.redhat.com/browse/THREESCALE-6913)
 - Fixed Payload limit content-length response header [PR #1266](https://github.com/3scale/APIcast/pull/1266) [THREESCALE-6736](https://issues.redhat.com/browse/THREESCALE-6736)
+- Fixed IPcheck policy issues with invalid IP [PR #1273](https://github.com/3scale/APIcast/pull/1273) [THREESCALE-7075](https://issues.redhat.com/browse/THREESCALE-7075)
+
 
 
 ### Added

diff --git a/gateway/src/apicast/policy/ip_check/ip_check.lua b/gateway/src/apicast/policy/ip_check/ip_check.lua
@@ -60,9 +60,13 @@ end
 function _M:access()
   local client_ip = ClientIP.get_from(self.client_ip_sources)
 
-  if client_ip then
-    self:check_client_ip(client_ip)
-  end
+  if not client_ip then
+    ngx.log(ngx.INFO, "Rejecting request due to is invalid to retrieve the IP information")
+    deny_request(self.error_msg)
+    return
+  end 
+
+  self:check_client_ip(client_ip)
 end
 
 return _M
diff --git a/spec/policy/ip_check/ip_check_spec.lua b/spec/policy/ip_check/ip_check_spec.lua
@@ -129,15 +129,15 @@ describe('IP Check policy', function()
     end)
 
     describe('when the client IP cannot be obtained', function()
-      it('does not deny the request', function()
+      it('denies the request', function()
         stub(ClientIP, 'get_from', function() return nil end)
         local ip_check = IpCheckPolicy.new(
           { ips = { '1.2.3.4' }, check_type = 'blacklist' }
         )
 
         ip_check:access()
 
-        assert.stub(ngx.exit).was_not_called()
+        assert.stub(ngx.exit).was_called()
       end)
     end)
   end)

diff --git a/t/apicast-policy-ip-check.t b/t/apicast-policy-ip-check.t
@@ -358,3 +358,72 @@ is always the valid one.
 [ "A custom error message\n", "GET / HTTP/1.1\n"]
 --- error_code eval
 [403, 200]
+
+=== TEST 11: X-forwarded-for header with invalid data
+--- configuration
+{
+  "services": [
+    {
+      "id": 42,
+      "proxy": {
+        "policy_chain": [
+          {
+            "name": "apicast.policy.ip_check",
+            "configuration": {
+              "ips": [ "9.9.9.9" ],
+              "client_ip_sources": [
+                "X-Forwarded-For"
+              ],
+              "check_type": "whitelist"
+            }
+          },
+          { "name": "apicast.policy.echo" }
+        ]
+      }
+    }
+  ]
+}
+--- request
+GET /
+--- response_body
+IP address not allowed
+--- more_headers eval
+X-forwarded-for: ,9.9.9.9
+--- error_code: 403
+--- no_error_log
+[error]
+
+
+=== TEST 12: X-forwarded-for header without data
+--- configuration
+{
+  "services": [
+    {
+      "id": 42,
+      "proxy": {
+        "policy_chain": [
+          {
+            "name": "apicast.policy.ip_check",
+            "configuration": {
+              "ips": [ "9.9.9.9" ],
+              "client_ip_sources": [
+                "X-Forwarded-For"
+              ],
+              "check_type": "whitelist"
+            }
+          },
+          { "name": "apicast.policy.echo" }
+        ]
+      }
+    }
+  ]
+}
+--- request
+GET /
+--- response_body
+IP address not allowed
+--- more_headers eval
+X-forwarded-for: ,
+--- error_code: 403
+--- no_error_log
+[error]