diff --git a/CHANGELOG.md b/CHANGELOG.md index 19f5506ff..f2385672a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Fixed Conditional policy evaluating incorrectly: second policy in policy chain that implement export() always triggers [PR #1485](https://github.com/3scale/APIcast/pull/1485) [THREESCALE-9320](https://issues.redhat.com/browse/THREESCALE-9320) - Fix APIcast using stale configuration for deleted products [PR #1488](https://github.com/3scale/APIcast/pull/1488) [THREESCALE-10130](https://issues.redhat.com/browse/THREESCALE-10130) +- Fixed Mutual TLS between APIcast and the Backend API fails when using a Forward Proxy [PR #1499](https://github.com/3scale/APIcast/pull/1499) [THREESCALE-5105](https://issues.redhat.com/browse/THREESCALE-5105) ### Added diff --git a/gateway/src/apicast/http_proxy.lua b/gateway/src/apicast/http_proxy.lua index 0c868365b..0481b2a9c 100644 --- a/gateway/src/apicast/http_proxy.lua +++ b/gateway/src/apicast/http_proxy.lua @@ -143,9 +143,7 @@ local function forward_https_request(proxy_uri, uri, proxy_opts) path = format('%s%s%s', ngx.var.uri, ngx.var.is_args, ngx.var.query_string or ''), body = body, proxy_uri = proxy_uri, - proxy_auth = opts.proxy_auth, - upstream_connection_opts = opts.upstream_connection_opts, - skip_https_connect = opts.skip_https_connect + proxy_options = opts } local httpc, err = http_proxy.new(request) @@ -226,7 +224,8 @@ function _M.request(upstream, proxy_uri) proxy_auth = proxy_auth, skip_https_connect = upstream.skip_https_connect, request_unbuffered = upstream.request_unbuffered, - upstream_connection_opts = upstream.upstream_connection_opts + upstream_connection_opts = upstream.upstream_connection_opts, + upstream_ssl = upstream.upstream_ssl } forward_https_request(proxy_uri, uri, proxy_opts) diff --git a/gateway/src/apicast/policy/apicast/apicast.lua b/gateway/src/apicast/policy/apicast/apicast.lua index b53e6f314..1e68f7739 100644 --- a/gateway/src/apicast/policy/apicast/apicast.lua +++ b/gateway/src/apicast/policy/apicast/apicast.lua @@ -5,6 +5,9 @@ local math = math local setmetatable = setmetatable local assert = assert local table_insert = table.insert +local base = require "resty.core.base" +local get_request = base.get_request +local tls = require 'resty.tls' local user_agent = require('apicast.user_agent') @@ -156,6 +159,40 @@ function _M:export() } end -_M.balancer = balancer.call +function _M:balancer(context) + -- All of this happens on balancer because this is subrequest inside APICAst + --to @upstream, so the request need to be the one that connects to the + --upstreamssl_client_raw_cert0 + local r = get_request() + if not r then + ngx.log(ngx.WARN, "Invalid request") + return + end + + if context.upstream_certificate and context.upstream_key then + local ok, err = tls.set_upstream_cert_and_key(context.upstream_certificate, context.upstream_key) + if ok ~= nil then + ngx.log(ngx.ERR, "Certificate cannot be set correctly, err: ", err) + end + end + + if context.upstream_verify then + local ok, err = tls.set_upstream_ssl_verify(true, 1) + if ok ~= nil then + ngx.log(ngx.WARN, "Cannot verify SSL upstream connection, err: ", err) + end + + if not context.upstream_ca_store then + ngx.log(ngx.WARN, "Set verify without including CA certificates") + end + + ok, err = tls.set_upstream_ca_cert(context.upstream_ca_store) + if ok ~= nil then + ngx.log(ngx.WARN, "Cannot set a valid trusted CA store, err: ", err) + end + end + + balancer:call(context) +end return _M diff --git a/gateway/src/apicast/policy/upstream_mtls/Readme.md b/gateway/src/apicast/policy/upstream_mtls/Readme.md index 1a04e4edc..0803c86b2 100644 --- a/gateway/src/apicast/policy/upstream_mtls/Readme.md +++ b/gateway/src/apicast/policy/upstream_mtls/Readme.md @@ -42,3 +42,5 @@ When using http forms and file upload This policy overwrites `APICAST_PROXY_HTTPS_CERTIFICATE_KEY` and `APICAST_PROXY_HTTPS_CERTIFICATE` values and it uses the certificates set by the policy, so those environment variables will have no effect. + +This policy is not compatible with Camel proxy. diff --git a/gateway/src/apicast/policy/upstream_mtls/upstream_mtls.lua b/gateway/src/apicast/policy/upstream_mtls/upstream_mtls.lua index cbc714457..235ac26e5 100644 --- a/gateway/src/apicast/policy/upstream_mtls/upstream_mtls.lua +++ b/gateway/src/apicast/policy/upstream_mtls/upstream_mtls.lua @@ -1,28 +1,14 @@ -- This policy enables MTLS with the upstream API endpoint local ssl = require('ngx.ssl') -local ffi = require "ffi" -local base = require "resty.core.base" local data_url = require('resty.data_url') local util = require 'apicast.util' -local C = ffi.C -local get_request = base.get_request local pairs = pairs local X509_STORE = require('resty.openssl.x509.store') local X509 = require('resty.openssl.x509') -ffi.cdef([[ - int ngx_http_apicast_ffi_set_proxy_cert_key( - ngx_http_request_t *r, void *cdata_chain, void *cdata_key); - int ngx_http_apicast_ffi_set_proxy_ca_cert( - ngx_http_request_t *r, void *cdata_ca); - int ngx_http_apicast_ffi_set_ssl_verify( - ngx_http_request_t *r, int verify, int verify_deph); -]]) - - local policy = require('apicast.policy') local _M = policy.new('mtls', "builtin") @@ -104,54 +90,11 @@ function _M.new(config) return self end - --- Set the certs for the upstream connection. Need to receive the pointers from --- parse_* functions. ---- Public function to be able to unittest this. -function _M.set_certs(r, cert, key) - local val = C.ngx_http_apicast_ffi_set_proxy_cert_key(r, cert, key) - if val ~= ngx.OK then - ngx.log(ngx.ERR, "Certificate cannot be set correctly") - end -end - -function _M.set_ca_cert(r, store) - local val = C.ngx_http_apicast_ffi_set_proxy_ca_cert(r, store) - if val ~= ngx.OK then - ngx.log(ngx.WARN, "Cannot set a valid trusted CA store") - return - end -end - --- All of this happens on balancer because this is subrequest inside APICAst ---to @upstream, so the request need to be the one that connects to the ---upstream0 -function _M:balancer(context) - local r = get_request() - if not r then - ngx.log(ngx.WARN, "Invalid request") - return - end - - if self.cert and self.cert_key then - self.set_certs(r, self.cert, self.cert_key) - end - - if not self.verify then - return - end - - local val = C.ngx_http_apicast_ffi_set_ssl_verify(r, ffi.new("int", 1), ffi.new("int", 1)) - if val ~= ngx.OK then - ngx.log(ngx.WARN, "Cannot verify SSL upstream connection") - end - - if not self.ca_store then - ngx.log(ngx.WARN, "Set verify without including CA certificates") - return - end - - self.set_ca_cert(r, self.ca_store) +function _M:access(context) + context.upstream_certificate = self.cert + context.upstream_key = self.cert_key + context.upstream_verify = self.verify + context.upstream_ca_store = self.ca_store end return _M diff --git a/gateway/src/apicast/upstream.lua b/gateway/src/apicast/upstream.lua index 0feb3420b..ef2097dc9 100644 --- a/gateway/src/apicast/upstream.lua +++ b/gateway/src/apicast/upstream.lua @@ -233,6 +233,11 @@ function _M:call(context) self.request_unbuffered = context.request_unbuffered self.upstream_connection_opts = context.upstream_connection_opts + self.upstream_ssl = { + ssl_verify = context.upstream_verify, + ssl_client_cert = context.upstream_certificate, + ssl_client_priv_key = context.upstream_key + } http_proxy.request(self, proxy_uri) else local err = self:rewrite_request() diff --git a/gateway/src/resty/http/proxy.lua b/gateway/src/resty/http/proxy.lua index 637ed0e3e..e02132c10 100644 --- a/gateway/src/resty/http/proxy.lua +++ b/gateway/src/resty/http/proxy.lua @@ -32,9 +32,10 @@ end local function connect(request) request = request or { } local httpc = http.new() + local proxy_options = request.proxy_options or {} - if request.upstream_connection_opts then - local con_opts = request.upstream_connection_opts + if proxy_options.upstream_connection_opts then + local con_opts = request.proxy_options.upstream_connection_opts ngx.log(ngx.DEBUG, 'setting timeouts (secs), connect_timeout: ', con_opts.connect_timeout, ' send_timeout: ', con_opts.send_timeout, ' read_timeout: ', con_opts.read_timeout) -- lua-resty-http uses nginx API for lua sockets @@ -51,7 +52,7 @@ local function connect(request) local scheme = uri.scheme local host = uri.host local port = default_port(uri) - local skip_https_connect = request.skip_https_connect + local skip_https_connect = proxy_options.skip_https_connect -- set ssl_verify: lua-resty-http set ssl_verify to true by default if scheme is https, whereas -- openresty treat nil as false, so we need to explicitly set ssl_verify to false if nil @@ -68,6 +69,10 @@ local function connect(request) if scheme == 'https' then options.ssl_server_name = host options.ssl_verify = ssl_verify + if proxy_options.upstream_ssl then + options.ssl_client_cert = proxy_options.upstream_ssl.ssl_client_cert + options.ssl_client_priv_key = proxy_options.upstream_ssl.ssl_client_priv_key + end end -- Connect via proxy @@ -79,7 +84,7 @@ local function connect(request) end local proxy_url = format("%s://%s:%s", proxy_uri.scheme, proxy_uri.host, proxy_uri.port) - local proxy_auth = request.proxy_auth + local proxy_auth = proxy_options.proxy_auth if scheme == 'http' then -- Used by http_ng module to send request to 3scale backend through proxy. diff --git a/gateway/src/resty/tls.lua b/gateway/src/resty/tls.lua new file mode 100644 index 000000000..0b5a65e7f --- /dev/null +++ b/gateway/src/resty/tls.lua @@ -0,0 +1,91 @@ +local base = require "resty.core.base" + +local type = type +local tostring = tostring + +local get_request = base.get_request +local ffi = require "ffi" +local C = ffi.C + +local _M = {} + +local NGX_OK = ngx.OK + +local ngx_http_apicast_ffi_set_proxy_cert_key; +local ngx_http_apicast_ffi_set_proxy_ca_cert; +local ngx_http_apicast_ffi_set_ssl_verify + +ffi.cdef([[ + int ngx_http_apicast_ffi_set_proxy_cert_key( + ngx_http_request_t *r, void *cdata_chain, void *cdata_key); + int ngx_http_apicast_ffi_set_proxy_ca_cert( + ngx_http_request_t *r, void *cdata_ca); + int ngx_http_apicast_ffi_set_ssl_verify( + ngx_http_request_t *r, int verify, int verify_deph); +]]) + +ngx_http_apicast_ffi_set_proxy_cert_key = C.ngx_http_apicast_ffi_set_proxy_cert_key +ngx_http_apicast_ffi_set_proxy_ca_cert = C.ngx_http_apicast_ffi_set_proxy_ca_cert +ngx_http_apicast_ffi_set_ssl_verify = C.ngx_http_apicast_ffi_set_ssl_verify + +-- Set the certs for the upstream connection. Need to receive the pointers from +-- parse_* functions. +function _M.set_upstream_cert_and_key(cert, key) + local r = get_request() + if not r then + error("no request found") + end + + if not cert or not key then + return nil, "cert and key must not be nil" + end + + local ret = ngx_http_apicast_ffi_set_proxy_cert_key(r, cert, key) + if ret ~= NGX_OK then + return nil, "error while setting upstream client certificate and key" + end +end + +-- Set the trusted store for the upstream connection. +function _M.set_upstream_ca_cert(store) + local r = get_request() + if not r then + error("no request found") + end + + if not store then + return nil, "trusted store must not be nil" + end + + local ret = ngx_http_apicast_ffi_set_proxy_ca_cert(r, store) + if ret ~= NGX_OK then + return nil, "error while setting upstream trusted CA store" + end +end + +-- Verify upstream connection +function _M.set_upstream_ssl_verify(verify, verify_deph) + local r = get_request() + if not r then + error("no request found") + end + + if type(verify) ~= 'boolean' then + return nil, "verify expects a boolean but found " .. type(verify) + end + + if type(verify_deph) ~= 'number' then + return nil, "verify depth expects a number but found " .. type(verify) + end + + if verify_deph < 0 then + return nil, "verify_deph expects a non-negative interger but found" .. tostring(verify_deph) + end + + local val = ngx_http_apicast_ffi_set_ssl_verify(r, verify, verify_deph) + if val ~= NGX_OK then + return nil, "error while setting upstream verify" + end +end + +return _M diff --git a/spec/policy/apicast/apicast_spec.lua b/spec/policy/apicast/apicast_spec.lua index f1ea05310..21460f200 100644 --- a/spec/policy/apicast/apicast_spec.lua +++ b/spec/policy/apicast/apicast_spec.lua @@ -1,4 +1,10 @@ local _M = require 'apicast.policy.apicast' +local util = require("apicast.util") +local ssl = require('ngx.ssl') +local tls = require('resty.tls') +local X509_STORE = require('resty.openssl.x509.store') +local X509 = require('resty.openssl.x509') +local balancer = require('apicast.balancer') describe('APIcast policy', function() local ngx_on_abort_stub @@ -31,6 +37,77 @@ describe('APIcast policy', function() end) end) + describe(".balancer", function() + local certificate_path = 't/fixtures/CA/root-ca.crt' + local certificate_key_path = 't/fixtures/CA/root-ca.key' + + local certificate_content = util.read_file(certificate_path) + local key_content = util.read_file(certificate_key_path) + local ca_cert, _ = X509.parse_pem_cert(certificate_content) + + local ca_store = X509_STORE.new() + ca_store:add_cert(ca_cert) + + local cert = ssl.parse_pem_cert(certificate_content) + local key = ssl.parse_pem_priv_key(key_content) + + before_each(function() + stub.new(balancer, 'call', function() return true end) + end) + + it("correctly set certificate and key", function() + local apicast = _M.new() + local context = { + upstream_certificate = cert, + upstream_key = key, + } + + spy.on(tls, "set_upstream_cert_and_key") + apicast:balancer(context) + assert.spy(tls.set_upstream_cert_and_key).was.called() + end) + + it("ignore invalid certificate and key", function() + local apicast = _M.new() + local context = { + upstream_certificate = nil, + upstream_key = nil, + } + + spy.on(tls, "set_upstream_cert_and_key") + apicast:balancer(context) + assert.spy(tls.set_upstream_cert_and_key).was_not.called() + end) + + it("CA certificate is not used if verify is not enabled", function() + local apicast = _M.new() + local context = { + upstream_certificate = cert, + upstream_key = key, + upstream_verify = false, + upstream_ca_store = cert + } + + spy.on(tls, "set_upstream_ca_store") + apicast:balancer(context) + assert.spy(tls.set_upstream_ca_store).was_not.called() + end) + + it("CA certificate is used if verify is enabled", function() + local apicast = _M.new() + local context = { + upstream_certificate = cert, + upstream_key = key, + upstream_verify = true, + upstream_ca_store = ca_store.store + } + + spy.on(tls, "set_upstream_ca_store") + apicast:balancer(context) + assert.spy(tls.set_upstream_ca_store).was_not.called() + end) + end) + describe('.post_action', function() describe('when the "run_post_action" flag is set to true', function() it('runs its logic', function() diff --git a/spec/policy/upstream_mtls/upstream_mtls_spec.lua b/spec/policy/upstream_mtls/upstream_mtls_spec.lua index 04d4db62c..25a048553 100644 --- a/spec/policy/upstream_mtls/upstream_mtls_spec.lua +++ b/spec/policy/upstream_mtls/upstream_mtls_spec.lua @@ -1,21 +1,13 @@ local upstream_mtls = require("apicast.policy.upstream_mtls") local ssl = require('ngx.ssl') -local open = io.open - -local function read_file(path) - local file = open(path, "rb") - if not file then return nil end - local content = file:read "*a" -- *a or *all reads the whole file - file:close() - return content -end +local util = require("apicast.util") describe('Upstream MTLS policy', function() local certificate_path = 't/fixtures/CA/root-ca.crt' local certificate_key_path = 't/fixtures/CA/root-ca.key' - local certificate_content = read_file(certificate_path) + local certificate_content = util.read_file(certificate_path) -- Set here the const to not use the pakcage ones, if not test will not fail -- if changes0 local path_type = "path" @@ -43,9 +35,10 @@ describe('Upstream MTLS policy', function() assert.truthy(object.cert) assert.truthy(object.cert_key) - spy.on(object, "set_certs") - object:balancer(context) - assert.spy(object.set_certs).was.called() + local context = {} + object:access(context) + assert.truthy(context.upstream_certificate) + assert.truthy(context.upstream_key) end) @@ -62,9 +55,10 @@ describe('Upstream MTLS policy', function() assert.is_falsy(object.cert) assert.is_falsy(object.cert_key) - spy.on(object, "set_certs") - object:balancer(context) - assert.spy(object.set_certs).was_not_called() + local context = {} + object:access(context) + assert.is_falsy(context.upstream_certificate) + assert.is_falsy(context.upstream_key) end) end) @@ -86,9 +80,10 @@ describe('Upstream MTLS policy', function() assert.truthy(object.cert) assert.truthy(object.cert_key) - spy.on(object, "set_certs") - object:balancer(context) - assert.spy(object.set_certs).was.called() + local context = {} + object:access(context) + assert.truthy(context.upstream_certificate) + assert.truthy(context.upstream_key) end) it("Nil certificate", function() @@ -103,10 +98,10 @@ describe('Upstream MTLS policy', function() assert.spy(ssl.parse_pem_priv_key).was_not_called() assert.falsy(object.cert) assert.falsy(object.cert_key) - - spy.on(object, "set_certs") - object:balancer(context) - assert.spy(object.set_certs).was_not_called() + local context = {} + object:access(context) + assert.is_falsy(context.upstream_certificate) + assert.is_falsy(context.upstream_key) end) it("Invalid certificate", function() @@ -122,9 +117,10 @@ describe('Upstream MTLS policy', function() assert.falsy(object.cert) assert.falsy(object.cert_key) - spy.on(object, "set_certs") - object:balancer(context) - assert.spy(object.set_certs).was_not_called() + local context = {} + object:access(context) + assert.is_falsy(context.upstream_certificate) + assert.is_falsy(context.upstream_key) end) end) @@ -160,30 +156,6 @@ describe('Upstream MTLS policy', function() local object = upstream_mtls.new(config) assert.same(type(object.ca_store), "cdata") end) - - it("CA certificate is not used if verify is not enabled", function() - config.ca_certificates = { certificate_content} - config.verify = false - - local object = upstream_mtls.new(config) - assert.same(type(object.ca_store), "cdata") - - spy.on(object, "set_ca_cert") - object:balancer({}) - assert.spy(object.set_ca_cert).was_not.called() - end) - - it("CA certificate is used if verify is enabled", function() - config.ca_certificates = { certificate_content} - config.verify = true - - local object = upstream_mtls.new(config) - assert.same(type(object.ca_store), "cdata") - - spy.on(object, "set_ca_cert") - object:balancer({}) - assert.spy(object.set_ca_cert).was.called() - end) end) end) diff --git a/t/apicast-policy-http-proxy.t b/t/apicast-policy-http-proxy.t index 52b442f8c..b4a8452f0 100644 --- a/t/apicast-policy-http-proxy.t +++ b/t/apicast-policy-http-proxy.t @@ -1143,3 +1143,139 @@ POST /test?user_key= --- no_error_log env ["[error]", "using proxy: $TEST_NGINX_HTTP_PROXY "] + + +=== TEST 18: MTLS connection to upstream via proxy failed +--- configuration random_port env +{ + "services": [ + { + "backend_version": 1, + "proxy": { + "api_backend": "https://test-upstream.lvh.me:$TEST_NGINX_RANDOM_PORT", + "proxy_rules": [ + { "pattern": "/", "http_method": "GET", "metric_system_name": "hits", "delta": 2 } + ], + "policy_chain": [ + { + "name": "apicast.policy.apicast" + }, + { + "name": "apicast.policy.http_proxy", + "configuration": { + "https_proxy": "$TEST_NGINX_HTTPS_PROXY" + } + } + ] + } + } + ] +} +--- backend env + server_name test-backend.lvh.me; + listen $TEST_NGINX_RANDOM_PORT ssl; + ssl_certificate $TEST_NGINX_SERVER_ROOT/html/server.crt; + ssl_certificate_key $TEST_NGINX_SERVER_ROOT/html/server.key; + location /transactions/authrep.xml { + content_by_lua_block { + ngx.exit(ngx.OK) + } + } +--- upstream env +server_name test-upstream.lvh.me; +listen $TEST_NGINX_RANDOM_PORT ssl; +ssl_certificate $TEST_NGINX_SERVER_ROOT/html/server.crt; +ssl_certificate_key $TEST_NGINX_SERVER_ROOT/html/server.key; +ssl_client_certificate $TEST_NGINX_SERVER_ROOT/html/client.crt; +ssl_verify_client on; +location /test { + echo 'ssl_client_s_dn: \$ssl_client_s_dn'; + echo 'ssl_client_i_dn: \$ssl_client_i_dn'; +} +--- request +GET /test?user_key=value +--- error_code: 400 +--- error_log env +using proxy: $TEST_NGINX_HTTPS_PROXY +proxy request: CONNECT test-upstream.lvh.me:$TEST_NGINX_RANDOM_PORT HTTP/1.1 +client sent no required SSL certificate while reading client request headers +--- no_error_log +[error] +--- user_files fixture=mutual_ssl.pl eval + + + +=== TEST 19: MTLS connection to upstream via proxy when certificates are provided +--- configuration random_port env eval +< $ENV{TEST_NGINX_HTTPS_PROXY}, + 'BACKEND_ENDPOINT_OVERRIDE' => "https://test-backend.lvh.me:$ENV{TEST_NGINX_RANDOM_PORT}" +) +--- configuration random_port env eval +<