Expose OIDC as standalone policy #904

mikz · 2018-09-19T14:26:08Z

We would like to use OIDC without 3scale, so we need a standalone OIDC policy for authentication.

Requirement for https://github.com/3scale/ostia/issues/14

/cc @3scale/ostia

gateway/src/apicast/policy/oidc_authentication/oidc_authentication.lua

spec/ngx_helper.lua

It makes it way eaiser to verify the return status in the tests.

davidor · 2018-09-27T13:31:01Z

gateway/src/resty/oidc/discovery.lua

@@ -15,6 +15,9 @@ local _M = { }

 local mt = { __index = _M }

+-- to be overridden in tests


Why is this needed? Can't we just use the http_backend that .new() receives?

The issue is that when you use this for example in the policy initializer, then the policy should accept this too. Pushing testing client through all levels of the stack seems wrong.

If this is only for testing, another option would be to modify self.backend in the tests. Not sure if better.

I get your point of having to pass this through multiple objects.

👍

Actually, this got me thinking. I think we should offer this in http_ng, so we can stub it globally in the whole test suite.

davidor · 2018-09-27T13:52:17Z

gateway/src/apicast/oauth/oidc.lua

@@ -51,7 +51,7 @@ function _M.new(service)
    jwt_claims = {
      -- 1. The JWT MUST contain an "iss" (issuer) claim that contains a
      -- unique identifier for the entity that issued the JWT.
-      iss = jwt_validators.equals_any_of({ issuer }),
+      iss = issuer and jwt_validators.equals_any_of({ issuer }) or jwt_validators.required(),


Why is or jwt_validators.required() needed?

Basically, I want to validate the issuer, if there is some, or just require the field.

Hmm so is it OK if iss is there but not included in issuer ? 🤔

Well, that's the thing. If there is no issuer in the configuration, what to do?

I decided to just require any value for iss.

davidor · 2018-09-27T14:00:11Z

gateway/src/apicast/oauth/oidc.lua

+end
+
+function _M:parse(jwt, cache_key)
+  local cached = self.cache:get(cache_key or jwt)


Why cache_key or jwt ? Shouldn't it be just cache_key ?

Because cache_key can be nil. So the token itself is its cache key.

davidor · 2018-09-27T14:21:36Z

gateway/src/apicast/policy/oidc_authentication/oidc_authentication.lua

+
+local tostring = tostring
+
+_M.cache_size = 100


Not sure if this should be exposed.

Other modules to this too. I think it is reasonable. If you want to you could write a policy that in the init phase bumps cache sizes of other policies.

davidor · 2018-09-27T14:38:38Z

gateway/src/apicast/policy/oidc_authentication/oidc_authentication.lua

@@ -0,0 +1,91 @@
+-- This is a oidc_authentication description.


Interesting 👀

davidor · 2018-09-27T14:43:00Z

gateway/src/apicast/policy/oidc_authentication/oidc_authentication.lua

+  local self = new(config)
+
+  self.issuer_endpoint = valid_issuer_endpoint(config and config.issuer_endpoint)
+  self.discovery = oidc_discovery.new(self.http_backend)


Isn't self.http_backend always nil at this point?

Yes, unless you set it on the module level. self already has the metatable pointing to _M.

davidor · 2018-09-27T15:04:34Z

spec/policy/oidc_authentication/oidc_authentication_spec.lua

+
+describe('oidc_authentication policy', function()
+  describe('.new', function()
+    it('works without configuration', function()


For these 2 cases with empty or nil config, it might be useful to check that rewrite and access do not raise unexpected errors.

davidor · 2018-09-27T15:37:20Z

spec/policy/oidc_authentication/oidc_authentication_spec.lua

+                             body = [[
+            {
+              "issuer": "https://example.com/issuer",
+              "jwks_uri": "https://example.com/jwks",


I think that extracting this one to a local var used in both mocks would nicely show how they're related.

davidor · 2018-09-27T15:40:59Z

spec/policy/oidc_authentication/oidc_authentication_spec.lua

+
+      policy:rewrite(context)
+
+      assert.contains({ payload = { iss = 'http://example.com/issuer' } }, context.jwt)


Shouldn't this return the other claims? I don't see why it only returns the iss.

It does. But the assert.contains verifies just subset. I could extend it to all the claims.

I'd do that. It's a bit confusing in my opinion right now.
How about assert.same(access_token.payload, context.jwt) ?

davidor · 2018-09-27T16:04:35Z

spec/policy/oidc_authentication/oidc_authentication_spec.lua

+      ngx.var = {}
+    end)
+
+    it('parses access token', function()


I'd also mention that it stores the parsed token in the context.
I think this is important because other policies might want to check if there's already a parsed jwt in there.

davidor · 2018-10-01T08:44:53Z

Shouldn't this policy have an apicast-policy.json ?

mikz · 2018-10-01T08:47:59Z

That would show it in the UI. I'm not sure we are ready for that. What would happen when a customer would add this policy to existing OIDC service? IMO we should think a bit more how that should work before exposing it.

codeclimate · 2018-10-01T15:52:47Z

spec/policy/oidc_authentication/oidc_authentication_spec.lua

+local _M = require('apicast.policy.oidc_authentication')
+local JWT = require('resty.jwt')
+local rsa = require('fixtures.rsa')
+local oidc_discovery = require('resty.oidc.discovery')


unused variable 'oidc_discovery'

codeclimate · 2018-10-01T15:52:47Z

spec/spec_helper.lua

+  local stub = require('luassert.stub')
+  local stubbed
+
+  busted.before_each(function(...)


unused variable length argument

in unit tests, it should default to test backend

mikz requested a review from a team as a code owner September 19, 2018 14:26

codeclimate bot reviewed Sep 19, 2018

View reviewed changes

gateway/src/apicast/policy/oidc_authentication/oidc_authentication.lua Show resolved Hide resolved

mikz force-pushed the oidc-policy branch 2 times, most recently from 226f5d9 to 89d58ba Compare September 27, 2018 09:32

codeclimate bot reviewed Sep 27, 2018

View reviewed changes

spec/ngx_helper.lua Show resolved Hide resolved

[spec] ngx.exit returns the status

ee123ca

It makes it way eaiser to verify the return status in the tests.

mikz force-pushed the oidc-policy branch 2 times, most recently from 0f1656f to 8031825 Compare September 27, 2018 12:27

mikz changed the title ~~[wip] expose OIDC as standalone policy~~ Expose OIDC as standalone policy Sep 27, 2018

davidor reviewed Sep 27, 2018

View reviewed changes

mikz force-pushed the oidc-policy branch from 8031825 to fc588f7 Compare October 1, 2018 15:52

codeclimate bot reviewed Oct 1, 2018

View reviewed changes

mikz force-pushed the oidc-policy branch from fc588f7 to 21015ee Compare October 1, 2018 15:57

mikz added 2 commits October 1, 2018 17:58

[resty.http_ng] globally stub http_ng default backend

576fa67

in unit tests, it should default to test backend

[oidc] expose OIDC as standalone policy

50d698c

mikz force-pushed the oidc-policy branch from 21015ee to 50d698c Compare October 1, 2018 15:58

davidor approved these changes Oct 1, 2018

View reviewed changes

mikz merged commit ab4ce9d into master Oct 1, 2018

mikz deleted the oidc-policy branch October 1, 2018 16:16

davidor added this to the 3.4 milestone Oct 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose OIDC as standalone policy #904

Expose OIDC as standalone policy #904

mikz commented Sep 19, 2018 •

edited

Loading

davidor Sep 27, 2018

mikz Sep 27, 2018

davidor Sep 27, 2018

mikz Sep 27, 2018

davidor Sep 27, 2018

mikz Sep 27, 2018

davidor Sep 27, 2018

mikz Sep 27, 2018

davidor Sep 27, 2018

mikz Sep 27, 2018

davidor Sep 27, 2018

mikz Sep 27, 2018

davidor Sep 27, 2018

davidor Sep 27, 2018

mikz Sep 27, 2018

davidor Sep 27, 2018

davidor Sep 27, 2018

mikz Sep 27, 2018

davidor Sep 27, 2018

davidor Sep 27, 2018

mikz Sep 27, 2018

davidor Sep 27, 2018

davidor Sep 27, 2018

davidor commented Oct 1, 2018

mikz commented Oct 1, 2018

codeclimate bot Oct 1, 2018

codeclimate bot Oct 1, 2018

		@@ -15,6 +15,9 @@ local _M = { }

		local mt = { __index = _M }

		-- to be overridden in tests

		@@ -0,0 +1,91 @@
		-- This is a oidc_authentication description.


		policy:rewrite(context)

		assert.contains({ payload = { iss = 'http://example.com/issuer' } }, context.jwt)

Expose OIDC as standalone policy #904

Expose OIDC as standalone policy #904

Conversation

mikz commented Sep 19, 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

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

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

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

davidor commented Oct 1, 2018

mikz commented Oct 1, 2018

codeclimate bot Oct 1, 2018

Choose a reason for hiding this comment

codeclimate bot Oct 1, 2018

Choose a reason for hiding this comment

mikz commented Sep 19, 2018 •

edited

Loading