PS 10999 8.4 OIDC authentication#5941
Conversation
jankowsk
force-pushed
the
PS-10999-8.4-OIDC_Authentication
branch
3 times, most recently
from
f8b2007 to
9df8db7
Compare
jankowsk
force-pushed
the
PS-10999-8.4-OIDC_Authentication
branch
8 times, most recently
from
1b4c342 to
b10ec38
Compare
jankowsk
force-pushed
the
PS-10999-8.4-OIDC_Authentication
branch
6 times, most recently
from
f373aeb to
fdaa978
Compare
|
|
||
| ###################### INIT ####################### | ||
| --echo ### INITIALIZE TESTS | ||
| --let $IDP_URL = https://keycloak.int.percona.com/realms/master/protocol/openid-connect |
There was a problem hiding this comment.
This will likely change in the future and, for example in jenkins may be different. I wonder if there's a better way to handle this. At least as a first step I would try to reduce the 4 places where we reference this link to only one.
There was a problem hiding this comment.
The keycloak host appears 2 times in the tests and 2 times in config.
Regarding the tests: if we use a variable it can be defined
- in "inc" file, we spare 1 occurrence.
- as a envvar set before running mtr or passed as some parameter to mtr script -seems that LDAP plugin does that, but this complicates how the tests are called
- the above may be partially solved by setting a default value in mtr script (I'd prefer not to touch it more than necessary) or in tests (we are back to point 1 actually).
Regarding the config: well, a Perl snippet replacing the host placeholders in the config could be written. Then enhanced config is saved, used by the plugin and removed at the end of the tast. Doable, but a bit an overkill.
On the other hand, changing the host in future means grep and replace at 4 places. Also as I remember from implementing LDAP or AWS stuff in Oracle nobody worried about hardcoding URLS in tests.
I'd prefer doing nothing or point 1 only, but I'm open for discussion.
There was a problem hiding this comment.
What happens if the repo is taken down or renamed? (highly unlikely but still a possibility) I think the repo should be cloned into Percona-Lab and we should reference that
There was a problem hiding this comment.
Probably you are right, but I'd like to discuss it wider.
On the one hand, by cloning we may miss some important updates (note, that it is a security fragile stuff). On the second hand, the core security functionality is in OpenSSL (like signature verification) used by jwt-cpp.
| roles += role; | ||
| } | ||
| } else if (groups_claim.get_type() == jwt::json::type::string) { | ||
| roles = groups_claim.as_string(); |
There was a problem hiding this comment.
isn't this an issue as above you to idp.get_role?
There was a problem hiding this comment.
Could you explain?
jankowsk
force-pushed
the
PS-10999-8.4-OIDC_Authentication
branch
from
fdaa978 to
a0f563b
Compare
…oles When OpenID Connect authentication maps external roles during login, acl_authenticate() called grant_role() with mpvio->acl_user. That ACL_USER is a copy allocated on the connection's mem_root and is freed when dispatch_command() ends. grant_role() stores ACL_USER by value in the role graph, including the raw user/host pointers. Later DROP USER walks that graph and reads those pointers after the mem_root was cleared, causing a heap-use-after-free (ASAN failure in auth_openid_connect.idp cleanup). Fix: lookup the durable ACL cache entry with find_acl_user() and pass that to grant_role() instead of the mem_root copy. The same problem probably would occur with any other plugin granting roles.
Upstream added OIDC authentication in 9.x, by this commit the client side plugin is backported to 8.4. Follow up of WL#16269 OpenID Connect (Oauth2 - JWT) Authentication Support Change-Id in upstream: I11944643d4a6098312edd16550c0160e86905063 The upstream commit introduces client side OpenID Connect authentication plugin to MySQL 9.x. Here we port it to 8.4 as part of work on Percona OpenID Connect authentication.
jankowsk
force-pushed
the
PS-10999-8.4-OIDC_Authentication
branch
from
a0f563b to
b827469
Compare
2 participants
No description provided.