Skip to content
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

OIDC Integration #382

Merged
merged 7 commits into from
Jul 18, 2017
Merged

OIDC Integration #382

merged 7 commits into from
Jul 18, 2017

Conversation

mikz
Copy link
Contributor

@mikz mikz commented Jul 3, 2017

OIDC integration expects the Client data are already in the IDP.
Then it just uses the OpenID Connect Discovery protocol to get the public key and verify the access token. The application id to report to backend is extracted from the JWT azp or aud field. Once JWT is decoded and verified it is stored in local LRU cache that can fit 10000 entries per worker.

APIcast does not communicate with the OpenID Connect Issuer for anything else than to get the certificate view the OpenID Connect Discovery. Clients are synchronised from 3scale to the IDP via http://github.com/3scale/zync.

@octobot
Copy link

octobot commented Jul 3, 2017

1 Warning
⚠️ Big PR

Spell Checker found issues

doc/parameters.md

Line Typo
3 APIcast v2 has a number of parameters co
3 ariables) that can modify the behavior of the gateway. The following
5 e that when deploying APIcast v2 with OpenShift, some of thee
15 Default: **stderr**
17 or more information. The file pathcan be either absolute, or relati
21 nfo
21 warn
37 Default: "apicast"
68 Values: a number > **60**
69 Default: 0
71 r. The value should be set to 0 or more than 60. For example,
71 ould be set to 0 or more than 60. For example, if `APICAST_CON
71 ONFIGURATION_CACHE` is set to 120, the gateway will reload the
71 eload the configuration every 2 minutes (120 seconds).
71 onfiguration every 2 minutes (120 seconds).
75 Default: "127.0.0.1"
77 ning Redis instance for OAuth 2.0 flow. REDIS_HOST parameter
81 Default: 6379
83 ning Redis instance for OAuth 2.0 flow. REDIS_PORT parameter
89 ning Redis instance for OAuth 2.0 flow. REDIS_URL parameter c
89 e used to set the full URI as DSN format like: `redis://PASSWOR
93 pty, the DNS resolver will be autodiscovered.
145 Setting it to particual version will make it not auto
157 tificate bundle generated by [export-builtin-trusted-certs](https://github.com/openresty

Generated by 🚫 Danger

@mikz mikz force-pushed the oidc-integration branch 4 times, most recently from e92b5dd to 01aff0b Compare July 7, 2017 08:09
@mikz mikz force-pushed the oidc-integration branch 5 times, most recently from 859d97c to 83ec731 Compare July 14, 2017 05:46
@mikz mikz changed the title [wip] OIDC Integration OIDC Integration Jul 14, 2017
local config = res.headers.content_type == 'application/json' and cjson.decode(res.body)

if not config then
ngx.log(ngx.STDERR, 'failed to get OIDC Issuer Configuration from ', uri, ' status: ', res.status, ' body: ', res.body)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to change the log to ERR and move this to the TODO a few lines above.
And here log explicitly that that the config couldn't be parsed because the content is not a valid JSON.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3bc2222

return jwt_obj, '[jwt] invalid alg'
end
-- TODO: this should be able to use DER format instead of PEM
local pubkey = format_public_key(self.config.public_key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe before formatting first check whether it's already in PEM format? (or maybe the format function itself should do it...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? We support only Keycloak and I could not find this in the spec. So looks like this might be keycloak specific.
If you have spec that defines this I'll be happy to validate the input.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikz Hm, well, the code doesn't "say" that it's only Keycloak...
So, if we can make it more generic, and potentially make it work for any other IDP (even if not officially supported), that would be just great.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not find spec for sharing the public key. I don't really have access to other IDPs. And zync supports only Keycloak.

We support only Keycloak initially. The code is made to support other implementations too, but we need spec for them. We don't have spec and don't want to support anything else than Keycloak initially. Once that changes this will change too.

I'm happy to modify this if I'd have at least example / spec of other IDPs.

@mikz mikz force-pushed the oidc-integration branch 3 times, most recently from 3bc2222 to 9378081 Compare July 17, 2017 19:42
@mikz mikz force-pushed the oidc-integration branch from 9378081 to 6281c5b Compare July 17, 2017 19:47
@mikz mikz merged commit b06118f into master Jul 18, 2017
@mikz mikz deleted the oidc-integration branch July 18, 2017 06:43
@mikz mikz removed the B-current label Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants