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

Kong Oauth2 backend headers NULL and discussion point. #3609

Closed
jeremyjpj0916 opened this issue Jul 11, 2018 · 7 comments
Closed

Kong Oauth2 backend headers NULL and discussion point. #3609

jeremyjpj0916 opened this issue Jul 11, 2018 · 7 comments
Labels

Comments

@jeremyjpj0916
Copy link
Contributor

jeremyjpj0916 commented Jul 11, 2018

Summary

When backend receives a valid Oauth2 Client, headers that are null should not be set and passed.

One other thing I wanted to bring up, X-Consumer-Groups is not a valuable header to us, would kong accept a PR boolean to Oauth2 that disables this header? Think about it. A consumer at scale with an ACL to each proxy(if you do it like that) ends up being 100-200+ uuids thrown in this header, not super performant. Would Kong accept a PR to enable/disable that in the Oauth2 schema?

X-Consumer-ID: 8aa31296-a4ec-4cb4-bfc7-64a5428b3c65
X-Consumer-Custom-ID: userdata: NULL
X-Consumer-Username: admin
X-Consumer-Groups: c3af0477-4747-4700-b9bc-520d158fdc15, 614c8995-a153-408b-b67d-06f0fc524898

Steps To Reproduce

  1. Create service + route + acl + oauth2 on a proxy and use client credentials flow
  2. Inspect the headers backend received on the proxy
  3. Notice X-Consumer-Custom-ID (which is not set in Kong) getting weirdly null populated on back-end, I think this to be sort of buggy/bad practice.

Additional Details & Logs

  • Kong version 0.14.0
@bungle
Copy link
Member

bungle commented Jul 14, 2018

X-Authenticated-Groups(?), do you mean X-Authenticated-Scope? okay got it, you mean ACLplugin.

@bungle
Copy link
Member

bungle commented Jul 14, 2018

@jeremyjpj0916 I would give a + for such PR on ACL plugin. I think we also need ngx.ctx.authenticated_groups that any plugin can set (such as OpenID Connect or LDAP plugins), so that they can also be used together with ACL plugin (ACL plugin then first checks that, and after that it checks if there is a consumer, like it is now).

@jeremyjpj0916
Copy link
Contributor Author

jeremyjpj0916 commented Jul 14, 2018

@bungle yeah I was looking at the OAuth2 code the other day and realized X-Consumer-Groups header had to deal with ACL plguin code and not OAuth2 in my suggestion. So glad to hear you think that would be an appropriate change as each API Provider does not need to know all the groups(which really represent either access to one or many proxies) a consumer has access to(must be a specific use case yall were thinking about when it was designed). And as for X-Consumer-Custom-ID: userdata: NULL being populated by the OAuth2 plugin you would consider that to be a bug right and a PR to the OAuth2 plugin checking for nil before assignment is appropriate?

@bungle
Copy link
Member

bungle commented Jul 16, 2018

Yes, that is a bug, that is lurking around in a lot of places as new dao (kong.db) returns ngx.null (that doesn't evaluate false). And consumer entity was just moved to new. Old dao returned nil in such cases, and evaluated false.

@jeremyjpj0916
Copy link
Contributor Author

jeremyjpj0916 commented Jul 31, 2018

Edit... the placeholder for the comment I had here is already resolved with merged PR! null in the other header value this issue was originally opened for may still persist.

@jeremyjpj0916
Copy link
Contributor Author

@bungle Just checking, your PR here: #3710 fixes this issue as well as #3617 right? Just if you wanted to bundle and close them together for that same PR for documentation purposes.

@bungle
Copy link
Member

bungle commented Aug 16, 2018

@jeremyjpj0916, Yes, I think so. Thanks!

thibaultcha pushed a commit that referenced this issue Aug 17, 2018
Minimal changes to add a { nulls = true } options argument to the new
DAO, so that by default it returns Lua `nil` on null values, but can
also return explicit `ngx.null` upon request. Adjusts the Admin API
endpoint generator to pass this option, and also the router (so that
code changes to the router at this point are minimal).

Fix #3609
Fix #3617
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants