Add token introspection feature as a policy. [THREESCALE-312] #619

tmogi001 · 2018-02-22T09:16:42Z

Hi team,

I created a new policy to enable RFC7662 OAuth2.0 Token Introspection.
This PR is a rework of the previous request (#435) as a policy.

And Also added sample configuration in the examples/policies directory.

mikz · 2018-02-22T10:42:39Z

gateway/src/apicast/policy/token_introspection/token_introspection.lua

+  local introspection_url = config.introspection_url
+  local client = config.client_id
+  local secret = config.client_secret
+  local credential = 'Basic ' .. ngx.encode_base64(table.concat({ client or '', secret or '' }, ':'))


This header can be moved to the _M.new, right?

mikz · 2018-02-22T10:45:16Z

gateway/src/apicast/policy/token_introspection/token_introspection.lua

+  if self.config.introspection_url then
+    local authorization = http_authorization.new(ngx.var.http_authorization)
+    local access_token = authorization.token
+    if not introspect_token(self, access_token) then


I think it would be a bit nicer for introspect_token to return a table so this would look like:

if introspect_token(self, access_token).active then ... else ... end

Or rename introspect_token to active_token because it actually returns true/false if the token is active or not.

mikz · 2018-02-22T10:46:19Z

gateway/src/apicast/policy/token_introspection/token_introspection.lua

+
+  if res.status == 200 then
+    local token_info = cjson.decode(res.body)
+    return token_info.active


Is this active field defined in some RFC? Could we link to the relevant RFC section in a comment?

Came by to look at your PR review automation, stayed to help out w/ a little PR doc pointing. :-) Seems the appropriate RFC doc entry point is: https://tools.ietf.org/html/rfc7662#section-2.2 Where active is the only REQUIRED member of the returned JSON introspection result.

@reedstrm very nice. Thanks! This looks like the right doc.

@tmogi001 for cases where is RFC available I try to implement it as:
https://github.com/3scale/apicast/blob/5168008db2abb2e217d23f925599fd33da5248cb/gateway/src/resty/http_ng/cache_store.lua#L85-L110

Because the RFC is immutable the comments won't get out of sync and act as relevant context with links to the other parts of the RFC. In this case just the relevant parts to the code in question to document the requests and responses for example.

BTW it is pity OpenID Connect Discovery does not define token introspection endpoint. Some IDPs provide it (like Keycloak as token_introspection_endpoint), but I don't see it being part of the spec.

mikz · 2018-02-22T10:46:56Z

gateway/src/apicast/policy/token_introspection/token_introspection.lua

+    }
+  }
+
+  local res, err = self.http_client.post(introspection_url , { token = token, token_type_hint = 'access_token'}, opts)


Would be good to link to the RFC section describing that introspection endpoint receives POST and what parameters.

https://tools.ietf.org/html/rfc7662#section-2.1 - only REQUIRED param is the token

mikz · 2018-02-22T10:47:54Z

spec/policy/token_introspection/token_introspection_spec.lua

+      local token_policy = require('apicast.policy.token_introspection').new(policy_config)
+      token_policy:access(context)
+
+      test_backend.verify_no_outstanding_expectations()


This line would be better in after_each block so it is not repeated in every test.

mikz · 2018-02-22T11:04:23Z

spec/policy/token_introspection/token_introspection_spec.lua

+        client_secret = "secret"
+      }
+
+      test_backend.expect{ url = introspection_url }.


It probably should expect right http method (POST) too ? And some other parameters like token_type_hint ? And possibly the headers?

mikz · 2018-02-22T11:05:43Z

gateway/src/apicast/policy/token_introspection/token_introspection.lua

+  local introspection_url = config.introspection_url
+  local client = config.client_id
+  local secret = config.client_secret
+  local credential = 'Basic ' .. ngx.encode_base64(table.concat({ client or '', secret or '' }, ':'))


@davidor I think we should provide a way to do this in http_ng constructor. Because it is done one several places IIRC.

mikz · 2018-02-22T11:06:39Z

gateway/src/apicast/policy/token_introspection/token_introspection.lua

+    backend = config.client,
+    options = {
+      header = { ['User-Agent'] = user_agent() },
+      ssl = { verify = resty_env.enabled('OPENSSL_VERIFY') }


@davidor we probably should provide a default value for these options/headers in the http_ng constructor to simplify its initialization.

mikz · 2018-02-22T11:09:12Z

@tmogi001 this is very good! 👍 🥇 My comments will be just nitpicks. This should be just minimal effort to be mergeable.
One of the things missing would be an integration test similar to other policy tests: https://github.com/3scale/apicast/blob/master/t/apicast-policy-cors.t
I think we could help with those a bit.

reedstrm · 2018-02-26T15:03:38Z

@mikz - As a random drive-by developer, with a group working on making our PR review policies more explicit (and automating as much as possible :-) - I landed here following examples of use of the CHANGELOG github app) I just wanted to say that I liked your review style here - not only do you review the code-as-presented, you call out issues w/ the shared testing infrastructure, to make future coding easier.

mikz · 2018-02-26T15:27:05Z

@reedstrm thank you very much! Means a lot.

Regarding the Changelog app (if you mean https://github.com/mikz/probot-changelog).
I'd say we are mostly happy. But because the changelog is changed in almost every PR it is hard when there is a lot of concurrent changes because they conflict. That means every PR then has to be resolved after there is another one merged. That can be quite frustrating. Maybe not as much when your testsuite runs in less than few minutes, but if you have to wait several minutes it is easy to forget.

I'm quite obsessed with automation and other approaches I was considering were:

https://github.com/conventional-changelog/conventional-changelog
generating changelog from PR description + labels

But a changelog is very simple approach and can scale up to some level. Even some huge projects like Ruby on Rails still use one changelog file per component and it works.

tmogi001 · 2018-02-27T09:27:37Z

@mikz , @reedstrm Thank you for reviewing. I pushed fixed code and also added integration test case.

mikz · 2018-02-27T12:52:40Z

gateway/src/apicast/policy/token_introspection/token_introspection.lua

+  self.http_client = http_ng.new{
+    backend = config.client,
+    options = {
+      header = { ['User-Agent'] = user_agent() },


@tmogi001 any reason why not to add the 'Authorization' header here too?

mikz · 2018-02-27T12:53:38Z

gateway/src/apicast/policy/token_introspection/token_introspection.lua

+  --- Parameters for the token introspection endpoint.
+  -- https://tools.ietf.org/html/rfc7662#section-2.1
+  local res, err = self.http_client.post(self.introspection_url , { token = token, token_type_hint = 'access_token'}, opts)
+  if res and err then


Is the if res really necessary? Shouldn't this be just if err ?

mikz · 2018-02-27T12:54:57Z

gateway/src/apicast/policy/token_introspection/token_introspection.lua

+
+  if res.status == 200 then
+    local token_info = cjson.decode(res.body)
+    return token_info


This can return nil when cjson.decode returns nil, err. That can happen for example when the response body is not valid json (html usually).

Just when fixing #626 I realised there is plenty of error cases.

If the API would return null then this would be ngx.null and return the same error as in #626.

This function should always return a table even when the json response can be a string, number, null, ...

And it should normalize active because if ngx.null is truthy so { "active": null } would evaluate to true.

mikz

There is one potential crash that should be fixed:
https://github.com/3scale/apicast/pull/619/files#r170911776

The rest is just nitpicks and should be easy to fix.
And we will need a changelog entry.

mikz · 2018-02-27T13:03:39Z

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

+        "type": "string"
+      },
+      "clientId": {
+        "description": "Clilent ID for the Token Introspection Endpoint",


A typo here. Should be Client ID.

mikz · 2018-02-27T13:04:32Z

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

+        "description": "Introspection Endpoint URL",
+        "type": "string"
+      },
+      "clientId": {


Looks like this name is not the same as in the tests/code. Everywhere it uses client_id.

mikz · 2018-02-27T13:04:46Z

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

+        "description": "Clilent ID for the Token Introspection Endpoint",
+        "type": "string"
+      },
+      "clientSecret": {


The same issue as https://github.com/3scale/apicast/pull/619/files#r170914144.

tmogi001 · 2018-03-01T04:14:00Z

I fixed my source code and added test cases (unit, integration) of invalid response from IdP.
And also added a change log entry.

mikz · 2018-03-01T08:53:11Z

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

+  "configuration": {
+    "type": "object",
+    "properties": {
+      "introspectionUrl": {


This does not match the use in the policy. Here it is introspectionUrl but there it is introspection_url.
We plan to have JSON schema validation during Policy initialization to verify those cases in CI, but right now it is just manual process, sorry.

mikz · 2018-03-01T08:55:53Z

gateway/src/apicast/policy/token_introspection/token_introspection.lua

+    local access_token = authorization.token
+    --- Introspection Response must have an "active" boolean value.
+    -- https://tools.ietf.org/html/rfc7662#section-2.2
+    if not introspect_token(self, access_token).active then


This will incorrectly validate case when the response is { "active": null }. It should check for active == true.

mikz · 2018-03-01T08:57:29Z

gateway/src/apicast/policy/token_introspection/token_introspection.lua

+      ngx.log(ngx.ERR, 'failed to parse token introspection response:', decode_err)
+      return { active = false }
+    end
+    return token_info


This will return whatever the JSON was. So if the JSON is null this would crash a bit later.

I think it should verify type(token_info) == 'table' and return the error otherwise instead of checking just the decode_err.

In my opinion, checking the decode_err is enough. Because it must be an error when token_info is null.
So, cjson.decode() always return error and decode_err must not be null when token_info JSON is null.

I checked cjson.decode(""),cjson.decode("{}"), cjson.decode(nil) and cjson.decode(ngx.null) returned decode_err.

If the response body is null then the JSON will be correctly decoded to ngx.null like:

cjson.decode('null')

That will later crash on trying to access .active (attempt to index a userdata value):

cjson.decode('null').active

The same can happen when a number is returned like:

cjson.decode('42').active

What I'm saying is introspect_token always has to return a table, because later it tries to get the key .access from it. And the only way to verify it always returns table is to check the type.
Because the response body is basically user input it can be any valid JSON, including just string "null" or numbers.

Thank you for information. I missed 'null' and numbers.

Do you think it is good as following? This worked fine with test code.

if decode_err or not (type(token_info) == 'table') then ngx.log(ngx.ERR, 'failed to parse token introspection response:', decode_err) return { active = false } end

I think it could be easier to read as:

if type(token_info) == 'table' then return token_info else ngx.log(ngx.ERR, 'failed to parse token introspection response: ', decode_err or 'not a table') return { active = false } end

Thank you! I think this is nice.

…hi/apicast into token_introspection_policy

mikz · 2018-03-01T10:58:00Z

Thank you @tmogi001 for this excellent PR! 🎉

You did a really good job.

tmogi001 added 2 commits February 22, 2018 03:36

Add Token Introspection policy

1214674

Add an example configuration file for token introspection.

8b46701

mikz reviewed Feb 22, 2018

View reviewed changes

tmogi001 added 3 commits February 27, 2018 04:11

Add some comments to link RFC documents.

1740323

Add check for http method, parameters, headers in unit test

f5e5f3e

Add integration test case for token introspection policy

912d3eb

mikz reviewed Feb 27, 2018

View reviewed changes

mikz changed the title ~~Add token introspection feature as a plicy.~~ Add token introspection feature as a policy. Feb 27, 2018

mikz mentioned this pull request Feb 28, 2018

OIDC - Add support for logout and token revocation [THREESCALE-312] #435

Closed

tmogi001 added 4 commits March 1, 2018 12:02

Move 'Authorization' header setup to _M.new

87493b7

Improve error cases

e66bd83

Fix typo in apicast-policy.json

508339a

Remove trailing spaces

bfdb2f5

tmogi001 added 2 commits March 1, 2018 13:00

Add change log entry

20c705b

Merge branch 'master' into token_introspection_policy

dc7e7ff

mikz reviewed Mar 1, 2018

View reviewed changes

tmogi001 added 3 commits March 1, 2018 19:47

fix typo in apicast-policy.json

b9ad6d7

Improve error cases to avoid null reference

2f2e79b

Merge branch 'token_introspection_policy' of https://github.com/Hitac…

65ad4e2

…hi/apicast into token_introspection_policy

mikz approved these changes Mar 1, 2018

View reviewed changes

mikz merged commit dbbf2fb into 3scale:master Mar 1, 2018

mikz changed the title ~~Add token introspection feature as a policy.~~ Add token introspection feature as a policy. [THREESCALE-312] Apr 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add token introspection feature as a policy. [THREESCALE-312] #619

Add token introspection feature as a policy. [THREESCALE-312] #619

tmogi001 commented Feb 22, 2018

mikz Feb 22, 2018

mikz Feb 22, 2018

mikz Feb 22, 2018

reedstrm Feb 26, 2018

mikz Feb 26, 2018

mikz Feb 22, 2018 •

edited

Loading

reedstrm Feb 26, 2018

mikz Feb 22, 2018

mikz Feb 22, 2018

mikz Feb 22, 2018

mikz Feb 22, 2018

mikz commented Feb 22, 2018

reedstrm commented Feb 26, 2018

mikz commented Feb 26, 2018

tmogi001 commented Feb 27, 2018

mikz Feb 27, 2018

mikz Feb 27, 2018

mikz Feb 27, 2018

mikz Feb 27, 2018

mikz left a comment

mikz Feb 27, 2018

mikz Feb 27, 2018

mikz Feb 27, 2018

tmogi001 commented Mar 1, 2018

mikz Mar 1, 2018

mikz Mar 1, 2018

mikz Mar 1, 2018

tmogi001 Mar 1, 2018

mikz Mar 1, 2018

tmogi001 Mar 1, 2018

mikz Mar 1, 2018 •

edited

Loading

tmogi001 Mar 1, 2018

mikz commented Mar 1, 2018

Add token introspection feature as a policy. [THREESCALE-312] #619

Add token introspection feature as a policy. [THREESCALE-312] #619

Conversation

tmogi001 commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz commented Feb 22, 2018

reedstrm commented Feb 26, 2018

mikz commented Feb 26, 2018

tmogi001 commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmogi001 commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz commented Mar 1, 2018

mikz Feb 22, 2018 •

edited

Loading

mikz Mar 1, 2018 •

edited

Loading