[keycloak] Accept client credentials for token endpoint in Authorization header #336
Conversation
mayorova
force-pushed
the
fix-keycloak-basic-auth
branch
from
d785a82 to
8cd98a1
Compare
apicast/src/oauth/keycloak.lua
Outdated
| @@ -248,7 +262,8 @@ function _M:get_token(service, client) | |||
| -- call Keycloak authorize | |||
| local url = self.config.token_url | |||
|
|
|||
| local res = http_client.post(url, req_body) | |||
| local headers = ngx.req.get_headers() | |||
| local res = http_client.post(url, ngx.req.get_post_args(), { headers = headers }) | |||
There was a problem hiding this comment.
I don't see any test covering why it should pass all request headers to the keycloak endpoint. Why it should do that? Shouldn't it be just some whitelist of headers?
There was a problem hiding this comment.
There is a test that verifies that headers are passed at all: https://github.com/3scale/apicast/pull/336/files/8cd98a1f55125a836cd94603cca2223460d0d716#diff-bc82c1966e003ef46c8dc5a71bb64c5d
Because they were not before, so the test would fail.
Now, regarding all headers VS whitelist, for now I really only need the Authorization header, but I am not sure if there can be some other headers that Keycloak may be expecting.
We can limit it to Authorization for now, and test that others are not proxy passed (although not sure how to do it with test backend).
There was a problem hiding this comment.
Test backend can just expect headers that are passed. Like
test_backend.expect{ url = 'http://example.com/t', headers = { ['User-Agent'] = tostring(user_agent) } }
.respond_with{ status = 200 }IMO we should limit what is sent to Keycloak according to the spec/documentation and not whatever client sends.
There was a problem hiding this comment.
Re: testing, yes, there is a test that checks the authorization header is passed (link, the previous one was not correct), but I don't know how to check that other headers were not passed. Could be an integration test, but maybe not so necessary?
There was a problem hiding this comment.
I think one positive test will be fine (checking that the Authorization is passed) and if we have to stub some method that returns a table then we can add some extra headers there and see that they are not in keycloak.
apicast/src/oauth/keycloak.lua
Outdated
| local creds = _M.get_client_credentials(params) | ||
|
|
||
| params.client_id = creds.client_id | ||
| params.client_secret = creds.client_secret |
There was a problem hiding this comment.
Why mutating and using params instead of using creds ?
There was a problem hiding this comment.
Because token_check_params needs all params + credentials (if they are not in params, they should be taken from headers). Happy to try something different.
There was a problem hiding this comment.
I see. Well I guess it is fine for now and will be refactored when we will rewrite OAuth from scratch.
mayorova
force-pushed
the
fix-keycloak-basic-auth
branch
from
707503f to
f8b1ea2
Compare
apicast/src/oauth/keycloak.lua
Outdated
| -- currently only passing the Authorization header used for the client authentication | ||
| function _M.token_get_headers() | ||
| local auth = ngx.var.http_authorization | ||
| return auth and { ['Authorization'] = auth } or {} |
There was a problem hiding this comment.
I think this would be the same:
return { ['Authorization'] = ngx.var.http_authorization }Because { whatever = nil } is {}.
mayorova
force-pushed
the
fix-keycloak-basic-auth
branch
from
f8b1ea2 to
4c3fcc0
Compare
[keycloak] Only pass Authorization header from client to Keycloak
mayorova
force-pushed
the
fix-keycloak-basic-auth
branch
from
4c3fcc0 to
141e81e
Compare
Close #319