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

Add support for RHSSO and OpenId Connect #283

Merged
merged 57 commits into from
Mar 16, 2017
Merged

Add support for RHSSO and OpenId Connect #283

merged 57 commits into from
Mar 16, 2017

Conversation

mpguerra
Copy link
Contributor

@mpguerra mpguerra commented Mar 7, 2017

This PR integrates Red Hat Single Sign-On with APIcast to verify the identity of an End-User and issue JWT tokens for use by 3scale applications (clients in Red Hat Single Sign-On) wishing to access the End-User resources.

JWTs will be issued by Red Hat Single Sign-On and verified by APIcast based on the public key for the Red Hat Single Sign-On realm configured. Rate limiting and usage on 3scale will be done based on the client_id (aud value encoded in JWT.)

TODO

@@ -4,7 +4,8 @@ version = '0.1-0'
dependencies = {
'lua-resty-http == 0.09-0',
'inspect == 3.1.0-1',
'router == 2.1-0'
'router == 2.1-0',
'lua-resty-jwt == 0.1.9-0'
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to give heads up to @rnc and @3scale/productization about this.

Copy link

Choose a reason for hiding this comment

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

Ack. Creating jira for it

function _M.new()
local keycloak = env.get('RHSSO_ENDPOINT')
if keycloak then
oauth = require 'oauth.keycloak'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is using module local variable. Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if this is the correct way to do it, but I wanted to only require the correct oauth module, i.e require keycloak if we're using keycloak and apicast_oauth if we're using the built in functionality

Copy link
Contributor

Choose a reason for hiding this comment

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

but why ? this means the require would be called on every request instead of once on boot


local config = { endpoint = endpoint}
if _M.configured then
_M.configuration = config
Copy link
Contributor

Choose a reason for hiding this comment

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

We will have to revamp this to use the new configuration store and not using module level objects.

local formatted_key = "-----BEGIN PUBLIC KEY-----\n"
local key_len = len(key)
for i=1,key_len,64 do
formatted_key = formatted_key..string.sub(key, i, i+63).."\n"
Copy link
Contributor

@mikz mikz Mar 7, 2017

Choose a reason for hiding this comment

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

What is this trying to do? Also it totally should be a function.

Copy link
Contributor

Choose a reason for hiding this comment

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

In RH SSO the public key is just a string 392 chars long. lua-resty-jwt library expects the key formatted as:

-----BEGIN PUBLIC KEY-----
<64 chars>
<64 chars>
etc.
-----END PUBLIC KEY-----

(sorry, I don't know how this format is called :) )

So, this function does just that.

Copy link
Contributor

Choose a reason for hiding this comment

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

So this would be great comment for the new extracted function :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Where is that documented BTW ?

Because it uses AES from https://github.com/openresty/lua-resty-string and it does not mention anything.

Could that be what OpenSSL requires?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, I discovered by trial and error :)
Could dig a bit deeper to understand why it's this way.

local headers = {
['Content-Type'] = 'application/json;charset=UTF-8'
}
local body = '{"error":"'..message..'"}"'
Copy link
Contributor

Choose a reason for hiding this comment

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

cjson.encode

end

function _M.authorize_check_params(params)
local response_type = params['response_type']
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for string access, params.response_type is just fine

end

function _M.token_check_params(params)
local grant_type = params['grant_type']
Copy link
Contributor

Choose a reason for hiding this comment

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

the same as above, no need for string access

function _M.parse_and_verify_token(jwt_token, public_key)
local jwt_obj = jwt:verify(public_key, jwt_token)
if not jwt_obj.verified then
ngx.log(ngx.INFO, "[jwt] failed verification for token: "..jwt_token)
Copy link
Contributor

Choose a reason for hiding this comment

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

don't concatenate strings, just use them as parameters: ngx.log(ngx.INFO, '..... ', jwt_token)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also it looks like this should fail if the jwt is not verified?

@@ -361,6 +362,15 @@ function _M:access(service)

local credentials, err = service:extract_credentials()

if keycloak then
Copy link
Contributor

Choose a reason for hiding this comment

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

Only when the method is oauth, right? Otherwise this would be happening for user_key too.

Also this might belong to the service:extract_credentials method somehow. Not sure how yet though.

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 think it was there in a previous iteration and we moved it because extract_credentials is just reading the credentials from the request, not parsing/validating them.

I think you suggested moving to apicast.lua at the time, but I couldn't see where, so I put in proxy.lua instead

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. Here it is well isolated. It can wait here for a while until we figure out better place.

But still it should be executed only when the auth method is oauth.

rockspec Outdated
@@ -5,7 +5,8 @@ dependencies = {
'luacheck >= 0',
'busted >= 0',
'lua-cjson >= 0',
'ldoc >= 0'
'ldoc >= 0',
'lua-resty-jwt >= 0'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is not necessary as development dependency because it is already a runtime dependency in the other rock spec.

@@ -0,0 +1,105 @@
local redis_pool = require 'client-registrations/redis_pool'
local lom = require 'lxp.lom'
local xpath = require 'luaxpath'
Copy link
Contributor

Choose a reason for hiding this comment

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

The example misses a rock spec defining what versions of what libraries are expected.

@@ -0,0 +1,105 @@
local redis_pool = require 'client-registrations/redis_pool'
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 argue it is not necessary to have redis pool because it does not really do anything.


local _M = {}

local initial_access_token = "CHANGE_ME_INITIAL_ACCESS_TOKEN"
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO our examples should not suggest editing them but rather provide means to inject the correct value like through environment variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that we'd need to declare the env var in conf/nginx.conf, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. For some reason I thought this is standalone configuration.

APIcast should support customisation for adding more env variables. Open an issue?

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 don't want to make the client registrations code part of APIcast as I don't think it belongs in the API Gateway.

However, customers want to see a solution that synchronises clients in 3scale and RHSSO without needing additional work/infrastructure on their side

Adding this as an example is my compromise, but I don't want to add this into the main APIcast nginx.conf as an env variable.

Happy to work with other suggestions, e.g @mikz suggestion for APIcast supporting custom env variables

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened an issue: #284 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a PR now: #292

Would be good to verify it fits your needs. I'd say the automatic forwarding should be the way.

set $client_id null;
set $access_token null;
set $registration_url null;
access_by_lua "require('client-registrations/webhook_handler').handle()";
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't be content_by_lua more meaningful ? This is not access policy.

Also by using content_by_lua_block you can embed it inside and have just one file.

-- Redis connection parameters
local _M = {
host = os.getenv("REDIS_HOST") or '127.0.0.1',
port = 6379,
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use REDIS_PORT here

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually this is something that should be abstracted in APIcast itself.

There is no reason to duplicate this code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Then it can be reused in threescale_utils, and also in apicast-xc


local function update_client(client_details)
local client_id = client_details.client_id
ngx.var.access_token = get_access_token(client_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

So if this is using apicast, it can use the http-ng client and not proxy_pass.

This example will not work on openshift because it does hardcode 8.8.8.8 as DNS.

APIcast has foundations for HTTP client, DNS resolving and others.

@@ -0,0 +1,105 @@
local redis_pool = require 'client-registrations/redis_pool'
local lom = require 'lxp.lom'
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 consider https://github.com/Phrogz/SLAXML which is pure Lua with no c dependency.

Both are kind of not maintained, so it does not matter much (most active around 2014).

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! I didn't see it before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the only issue I would have with slaxml is that it doesn't seem to be available from luarocks so I guess you'd have to clone the project instead

Copy link
Contributor

@mikz mikz Mar 8, 2017

Choose a reason for hiding this comment

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

It has rockspec so maybe it just has to be pushed (Phrogz/SLAXML#9 (comment)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change in a later iteration (when it's in luarocks)


## Red Hat Single Sign-On Configuration for APIcast

1. [Create a new realm](https://access.redhat.com/documentation/en-us/red_hat_single_sign-on/7.0/html/getting_started_guide/create_a_realm_and_user#create-realm) (different from Master)
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 say it makes sense to export the RHSSO configuration and add it to the example.

From there it wouldn't be that hard to just have docker compose to start keycloak and have full working example with no external dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mayorova are you able to do this for my realm? I believe the command should be something like: bin/standalone.sh -Dkeycloak.migration.action=export -Dkeycloak.migration.provider=singleFile -Dkeycloak.migration.file=pili-realm.json -Dkeycloak.migration.realmName=pili -Dkeycloak.migration.usersExportStrategy=SKIP

According to this article

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the configuration exported, there's a lot of "confidential" data in there, I'll look into extracting relevant parts and adding a Dockerfile for a later iteration though.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can export one realm from the UI. We also have example private keys stored in this repo.
And it is fine because it is meant to be example. For real use case you configure it from scratch.

But having working example is important, because it can be verified that it works.

- The following lua libraries should be installed using luarocks:
- luaexpat: `sudo luarocks install luaexpat --tree=/usr/local/openresty/luajit`
- luaxpath: `sudo luarocks install luaxpath --tree=/usr/local/openresty/luajit`
- cjson: `sudo luarocks install lua-cjson --tree=/usr/local/openresty/luajit`
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed. Cjson is part of OpenResty.

- luaxpath: `sudo luarocks install luaxpath --tree=/usr/local/openresty/luajit`
- cjson: `sudo luarocks install lua-cjson --tree=/usr/local/openresty/luajit`
- And a symbolic link created: so openresty can find them
`ln -s /usr/local/openresty/luajit/lib64/lua/5.1/* /usr/local/openresty/luajit/lib/lua/5.1/`
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be necessary. Just Installing them to the correct directory should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

This specific one is for luaxpath & luaexpat, for some reason OpenResty doesn't find them on RHEL. How to install them to a specific directory?
Anyway, if we switch to the purely Lua lib for XML, then it shouldn't be necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

When openresty can't find them it will print nice error with all the load paths where they can be.
So it should be installed to one of them that make the most sense.

- And a symbolic link created: so openresty can find them
`ln -s /usr/local/openresty/luajit/lib64/lua/5.1/* /usr/local/openresty/luajit/lib/lua/5.1/`

Otherwise, please follow the instructions on the 3scale support site based on your chosen deployment method.
Copy link
Contributor

Choose a reason for hiding this comment

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

link to the 3scale support site?


### APIcast Set up

APIcast v2 should be installed as usual with the following additional dependencies:
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 not call it v2 because it is v3 already.

Copy link
Contributor

Choose a reason for hiding this comment

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

v3 is not yet a GA release, is it? We have quite a few mentions of v2 on the support site. If it's GA (supported version), then we need to change it too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Writing stuff that changes to documentation does not make sense. It gets out of date and will be confusing. We can just write APIcast.


Otherwise, please follow the instructions on the 3scale support site based on your chosen deployment method.

When it comes to running APIcast v2, we will use the same approach as in the Custom Configuration example to add an additional server block to handle the client registration in RH SSO. In this case, clients will be created in 3scale first and imported into RH SSO, e.g the way to do this is in docker would be by mounting a volume inside `sites.d` folder in the container.
Copy link
Contributor

Choose a reason for hiding this comment

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

A link to the Custom Configuration example?

### 3scale Configuration

#### Webhooks
In order to register applications created in 3scale as clients in RH SSO, you need to enable and configure [webhooks](https://support.3scale.net/docs/api-bizops/webhooks) on 3scale.
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 extend the link to webhooks on 3scale or configure webhooks to be more descriptive.

local app_id = jwt.payload.aud
credentials.app_id = app_id
else
return error_authorization_failed(service)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no test covering this change.

@octobot
Copy link

octobot commented Mar 10, 2017

1 Warning
⚠️ Big PR

Proselint found issues

doc/parameters.md

Line Message Severity

examples/rh-sso/README.md

Line Message Severity

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.
140 Setting it to particual version will make it not auto
144 ance for 3scale Applications (a.k.a. clients in Red Hat Single Sig

examples/rh-sso/README.md

Line Typo
1 th Red Hat Single Sign-On and OpenID Connect for API Authenticatio
8 - Red Hat SSO instance - [Installation](htt
26 n-On e.g you can use the same webhook mechanism, but hosted outside
39 - The following lua libraries should be installed
39 ies should be installed using luarocks:
47 deal with client registration webhooks, this is all included in `web
47 t Single Sign-On based on the webhooks sent by 3scale. You can read
55 will need to create the client**_registrations** directory first) and call API
62 #### Webhooks
63 need to enable and [configure webhooks](https://support.3scale.net/d
65 1. Enter the APIcast url followed by the /webhooks p
66 2. Turn Webhooks "ON."
67 3. Select the following Webhooks under Settings:
74 Once the webhooks are configured, you will want
78 or Red Hat Single Sign-On and OpenID Connect
81 then need to add the redirect url for your client. We're going
83 he background, 3scale sends a webhook to APIcast, which in turn mak
94 th Red Hat Single Sign-On and OpenID Connect support as follows, e
102 Authorization", select "OAuth 2.0"

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.
140 Setting it to particual version will make it not auto
144 ance for 3scale Applications (a.k.a. clients in Red Hat Single Sig

examples/rh-sso/README.md

Line Typo
1 th Red Hat Single Sign-On and OpenID Connect for API Authenticatio
8 - Red Hat SSO instance - [Installation](htt
26 n-On e.g you can use the same webhook mechanism, but hosted outside
39 - The following lua libraries should be installed
39 ies should be installed using luarocks:
47 deal with client registration webhooks, this is all included in `web
47 t Single Sign-On based on the webhooks sent by 3scale. You can read
55 will need to create the client**_registrations** directory first) and call API
62 #### Webhooks
63 need to enable and [configure webhooks](https://support.3scale.net/d
65 1. Enter the APIcast url followed by the /webhooks p
66 2. Turn Webhooks "ON."
67 3. Select the following Webhooks under Settings:
74 Once the webhooks are configured, you will want
78 or Red Hat Single Sign-On and OpenID Connect
81 then need to add the redirect url for your client. We're going
83 he background, 3scale sends a webhook to APIcast, which in turn mak
94 th Red Hat Single Sign-On and OpenID Connect support as follows, e
102 Authorization", select "OAuth 2.0"

Generated by 🚫 Danger

ngx.var.backend_host = backend.host or server or ngx.var.backend_host
end

local function debug_headers(service, usage, credentials)
Copy link
Contributor

Choose a reason for hiding this comment

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

The name is a bit confusing, as it suggests that it only has to do with the 3scale debug headers, while there are some lines which are an important part of the functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. It should be split/renamed.

Maria Pilar Guerra Arias and others added 3 commits March 16, 2017 11:52
local jwt_obj, err = parse_and_verify_token(self, credentials.access_token)

if err then
ngx.log(ngx.INFO, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be improved to say something meaningful like: 'failed to parse and verify JWT: err'

@@ -164,9 +169,6 @@ function _M:transform_credentials(credentials)
-- @field app_id Client id
-- @table credentials_oauth
Copy link
Contributor

Choose a reason for hiding this comment

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

This documentation is useless now.

@@ -105,6 +107,12 @@ function boot.init(configuration)
ngx.log(ngx.EMERG, 'cache is off, cannot store configuration, exiting')
os.exit(0)
end

local keycloak_config = _M.init(nil, "keycloak")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is pretty poor interface and should be improved. Passing nil is just weird.

local r = router:new()
function _M.new(configuration)
if configuration.keycloak then
ngx.log(ngx.INFO, "keycloak configured")
Copy link
Contributor

Choose a reason for hiding this comment

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

this is going to print this message on every request, not really necessary no ?

@@ -0,0 +1 @@
env RHSS0_INITIAL_TOKEN;
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this O 0 ?


if not jwt_obj.verified then
ngx.log(ngx.INFO, "[jwt] failed verification for token, reason: ", jwt_obj.reason)
return jwt_obj, "JWT not verified"
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 change it to:

return jwt_obj, "JWT not verified: "..jwt_obj.reason

just not to lose the reason, in case we want to use it in proxy later.

Copy link
Contributor

Choose a reason for hiding this comment

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

reason is not lost and passed in jwt_obj

every string concatenation has a cost and it is better to avoid it, error messages should be pretty much the same strings over and over again

Copy link
Contributor

Choose a reason for hiding this comment

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

True.

local jwt_obj, err = parse_and_verify_token(self, credentials.access_token)

if err then
ngx.log(ngx.INFO, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this log statement is a bit redundant, as we already print the error inside parse_and_verify_token.

@mayorova
Copy link
Contributor

We are using the complete JWT in the cached_key, this might not be optimal, as it can be huge (and the shared dict is limited in size), and we really just need the extracted app_id.

@mikz
Copy link
Contributor

mikz commented Mar 16, 2017

@mayorova Agree that would be good, but I'm not comfortable making more changes without test coverage. IMO that can wait for CR1 and be done with proper test coverage (like full integration test with keycloak).

@mpguerra
Copy link
Contributor Author

Regarding the JWT in the cached key, we need to think about the trade off of verifying the jwt each time (to extract the client_id) versus once only, not sure what's best, for now I decided to just keep the jwt in the cache

@mpguerra
Copy link
Contributor Author

Thinking about it some more, the JWT will expire at some point so we should take that into consideration... I guess this would be fixed if we verify the JWT each time

@mikz
Copy link
Contributor

mikz commented Mar 16, 2017

True. It is quite trivial to extract the TTL from the token and add the cache key with expiry.

We should add it to some list of known issues and fix it in the next release.

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.

7 participants