-
Notifications
You must be signed in to change notification settings - Fork 171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose OIDC as standalone policy #904
Conversation
226f5d9
to
89d58ba
Compare
It makes it way eaiser to verify the return status in the tests.
0f1656f
to
8031825
Compare
gateway/src/resty/oidc/discovery.lua
Outdated
@@ -15,6 +15,9 @@ local _M = { } | |||
|
|||
local mt = { __index = _M } | |||
|
|||
-- to be overridden in tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? Can't we just use the http_backend
that .new()
receives?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
gateway/src/apicast/oauth/oidc.lua
Outdated
@@ -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(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is or jwt_validators.required()
needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, I want to validate the issuer, if there is some, or just require the field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm so is it OK if iss
is there but not included in issuer
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
gateway/src/apicast/oauth/oidc.lua
Outdated
end | ||
|
||
function _M:parse(jwt, cache_key) | ||
local cached = self.cache:get(cache_key or jwt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why cache_key or jwt
? Shouldn't it be just cache_key
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because cache_key
can be nil
. So the token itself is its cache key.
|
||
local tostring = tostring | ||
|
||
_M.cache_size = 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this should be exposed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -0,0 +1,91 @@ | |||
-- This is a oidc_authentication description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting 👀
local self = new(config) | ||
|
||
self.issuer_endpoint = valid_issuer_endpoint(config and config.issuer_endpoint) | ||
self.discovery = oidc_discovery.new(self.http_backend) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't self.http_backend
always nil
at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, unless you set it on the module level. self
already has the metatable pointing to _M.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
describe('oidc_authentication policy', function() | ||
describe('.new', function() | ||
it('works without configuration', function() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these 2 cases with empty or nil config, it might be useful to check that rewrite
and access
do not raise unexpected errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
body = [[ | ||
{ | ||
"issuer": "https://example.com/issuer", | ||
"jwks_uri": "https://example.com/jwks", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that extracting this one to a local var used in both mocks would nicely show how they're related.
|
||
policy:rewrite(context) | ||
|
||
assert.contains({ payload = { iss = 'http://example.com/issuer' } }, context.jwt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this return the other claims? I don't see why it only returns the iss
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does. But the assert.contains
verifies just subset. I could extend it to all the claims.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd do that. It's a bit confusing in my opinion right now.
How about assert.same(access_token.payload, context.jwt)
?
ngx.var = {} | ||
end) | ||
|
||
it('parses access token', function() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Shouldn't this policy have an |
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. |
local _M = require('apicast.policy.oidc_authentication') | ||
local JWT = require('resty.jwt') | ||
local rsa = require('fixtures.rsa') | ||
local oidc_discovery = require('resty.oidc.discovery') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused variable 'oidc_discovery'
spec/spec_helper.lua
Outdated
local stub = require('luassert.stub') | ||
local stubbed | ||
|
||
busted.before_each(function(...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused variable length argument
in unit tests, it should default to test backend
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