OAuth fixes and improvements (#353)

* Fix App ID caching issue (and add tests to match)
* Improve error messages for all OAuth providers

Author: mhamann

Dockerfile.test.unit

@@ -24,11 +24,13 @@
 
 FROM alpine:3.9
 
+ENV CJOSE_VERSION=0.5.1
+
 RUN apk update && \
     apk add \
     gcc tar zlib wget make musl-dev g++ curl \
     libtool readline luajit luajit-dev unzip \
-    openssl openssl-dev
+    openssl openssl-dev git jansson jansson-dev
 
 WORKDIR /tmp
 RUN wget https://luarocks.org/releases/luarocks-3.1.3.tar.gz && \
@@ -38,6 +40,15 @@ RUN wget https://luarocks.org/releases/luarocks-3.1.3.tar.gz && \
     make build && \
     make install
 
+RUN echo " ... installing cjose ... " \
+    && mkdir -p /tmp/api-gateway \
+    && curl -L -k https://github.com/cisco/cjose/archive/${CJOSE_VERSION}.tar.gz -o /tmp/api-gateway/cjose-${CJOSE_VERSION}.tar.gz \
+    && tar -xf /tmp/api-gateway/cjose-${CJOSE_VERSION}.tar.gz -C /tmp/api-gateway/ \
+    && cd /tmp/api-gateway/cjose-${CJOSE_VERSION} \
+    && sh configure \
+    && make && make install \
+    && rm -rf /tmp/api-gateway
+
 COPY . /etc/api-gateway
 
 WORKDIR /etc/api-gateway/tests

scripts/lua/oauth/app-id.lua

@@ -23,26 +23,39 @@ local _M = {}
 local http = require 'resty.http'
 local cjose = require 'resty.cjose'
 
-function _M.process(dataStore, token, securityObj)
-  local result = dataStore:getOAuthToken('appId', token)
-  local httpc = http.new()
-  local json_resp
-  if result ~= ngx.null then
-    json_resp = cjson.decode(result)
-    ngx.header['X-OIDC-Email'] = json_resp['email']
-    ngx.header['X-OIDC-Sub'] = json_resp['sub']
-    return json_resp
-  end
-  local keyUrl = utils.concatStrings({APPID_PKURL, securityObj.tenantId, '/publickeys'})
+local function inject_req_headers(token_obj)
+  ngx.header['X-OIDC-Email'] = token_obj['email']
+  ngx.header['X-OIDC-Sub'] = token_obj['sub']
+end
+
+local function fetchJWKs(tenantId)
+  local keyUrl = utils.concatStrings({APPID_PKURL, tenantId, '/publickeys'})
   local request_options = {
     headers = {
       ["Accept"] = "application/json"
     },
-    ssl_verify = false
+    ssl_verify = true
   }
-  local res, err = httpc:request_uri(keyUrl, request_options)
-  if err then
-    request.err(500, 'error getting app id key: ' .. err)
+  return httpc:request_uri(keyUrl, request_options)
+end
+
+function _M.process(dataStore, token, securityObj)
+  local cache_key = 'appid_' .. securityObj.tenantId
+  local result = dataStore:getOAuthToken(cache_key, token)
+  local httpc = http.new()
+  local token_obj
+
+  -- Was the token in the cache?
+  if result ~= ngx.null then
+    token_obj = cjson.decode(result)
+    inject_req_headers(token_obj)
+    return token_obj
+  end
+
+  -- Cache miss. Proceed to validate the token
+  local res, err = fetchJWKs
+  if err or res.status ~= 200 then
+    request.err(500, 'An error occurred while fetching the App ID JWK configuration: ' .. err or res.body)
   end
 
   local key
@@ -52,24 +65,26 @@ function _M.process(dataStore, token, securityObj)
   end
   local result = cjose.validateJWS(token, cjson.encode(key))
   if not result then
-    request.err(401, 'AppId key signature verification failed.')
+    request.err(401, 'The token signature did not match any known JWK.')
     return nil
   end
-  local jwt_obj = cjson.decode(cjose.getJWSInfo(token))
-  local expireTime = jwt_obj['exp']
+
+  token_obj = cjson.decode(cjose.getJWSInfo(token))
+  local expireTime = token_obj['exp']
   if expireTime < os.time() then
-    request.err(401, 'Access token expired.')
+    request.err(401, 'The access token has expired.')
     return nil
   end
-  ngx.header['X-OIDC-Email'] = jwt_obj['email']
-  ngx.header['X-OIDC-Sub'] = jwt_obj['sub']
+
+  -- Add token metadata to the request headers
+  inject_req_headers(token_obj)
+
   -- keep token in cache until it expires
   local ttl = expireTime - os.time()
-  dataStore:saveOAuthToken('appId', token, cjson.encode(jwt_obj), ttl)
-  return jwt_obj
+  dataStore:saveOAuthToken(cache_key, token, cjson.encode(token_obj), ttl)
+  return token_obj
 end
 
-
 return _M
 
 

scripts/lua/oauth/facebook.lua

@@ -56,7 +56,7 @@ function exchangeOAuthToken(dataStore, token, facebookAppToken)
 -- convert response
   if not res then
     ngx.log(ngx.WARN, 'Could not invoke Facebook API. Error=', err)
-    request.err(500, 'OAuth provider error.')
+    request.err(500, 'Connection to the OAuth provider failed.')
     return
   end
   local json_resp = cjson.decode(res.body)

scripts/lua/oauth/github.lua

@@ -45,7 +45,7 @@ function _M.process(dataStore, token)
 -- convert response
   if not res then
     ngx.log(ngx.WARN, utils.concatStrings({"Could not invoke Github API. Error=", err}))
-    request.err(500, 'OAuth provider error.')
+    request.err(500, 'Connection to the OAuth provider failed.')
     return
   end
 

scripts/lua/oauth/google.lua

@@ -50,7 +50,7 @@ function _M.process (dataStore, token)
 -- convert response
   if not res then
     ngx.log(ngx.WARN, utils.concatStrings({"Could not invoke Google API. Error=", err}))
-    request.err(500, 'OAuth provider error.')
+    request.err(500, 'Connection to the OAuth provider failed.')
     return nil
   end
   local json_resp = cjson.decode(res.body)

scripts/lua/policies/security/oauth2.lua

@@ -66,7 +66,7 @@ function exchange(dataStore, token, provider, securityObj)
     local loaded, impl = pcall(require, utils.concatStrings({'oauth/', provider}))
     if not loaded then
       request.err(500, 'Error loading OAuth provider authentication module')
-      print("error loading provider.")
+      print("error loading provider:", impl)
       return nil
     end
 

tests/install-deps.sh

@@ -28,3 +28,5 @@ luarocks install --tree=lua_modules sha1
 luarocks install --tree=lua_modules md5
 luarocks install --tree=lua_modules net-url
 luarocks install --tree=lua_modules luafilesystem
+luarocks install --tree=lua_modules lua-resty-http 0.10
+luarocks install --tree=lua_modules https://github.com/mhamann/lua-resty-cjose/raw/master/lua-resty-cjose-0.5-0.rockspec

tests/scripts/lua/security.lua

@@ -213,6 +213,7 @@ describe('OAuth security module', function()
     assert.same(red:exists('oauth:providers:mock:tokens:test'), 1)
     assert(result)
   end)
+
   it('Exchanges a bad token, doesn\'t cache it and returns false', function()
     local red = fakeredis.new()
     local token = "bad"
@@ -237,31 +238,34 @@ describe('OAuth security module', function()
     assert.same(red:exists('oauth:providers:mock:tokens:bad'), 0)
     assert.falsy(result)
   end)
-  it('Loads a facebook token from the cache with a valid app id', function()
+
+  it('Has no cross-contamination between App ID caches', function()
     local red = fakeredis.new()
     local ds = require "lib/dataStore"
     local dataStore = ds.initWithDriver(red)
-    local token = "test"
+    local token = "test_token"
     local appid = "app"
     local ngxattrs = [[
       {
         "http_Authorization":"]] .. token .. [[",
-        "http_x_facebook_app_token":"]] .. appid .. [[",
         "tenant":"1234",
         "gatewayPath":"v1/test"
       }
     ]]
     local ngx = fakengx.new()
+    ngx.config = { ngx_lua_version = 'test' }
     ngx.var = cjson.decode(ngxattrs)
     _G.ngx = ngx
     local securityObj = [[
       {
         "type":"oauth2",
-        "provider":"facebook",
-        "scope":"resource"
+        "provider":"app-id",
+        "tenantId": "tenant1",
+        "scope":"api"
       }
     ]]
-    red:set('oauth:providers:facebook:tokens:testapp', '{"token":"good"}')
+    red:set('oauth:providers:appid_tenant2:tokens:test_token', '{"token":"good"}')
+    red:set('oauth:providers:appid_tenant1:tokens:test_token', '{"token":"good"}')
     local result = oauth.process(dataStore, cjson.decode(securityObj))
     assert.truthy(result)
   end)