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: Only require UserInfo on successful callback #22192

Closed
FroMage opened this issue Dec 14, 2021 · 4 comments
Closed

OIDC: Only require UserInfo on successful callback #22192

FroMage opened this issue Dec 14, 2021 · 4 comments
Labels
area/oidc kind/enhancement New feature or request triage/duplicate This issue or pull request already exists

Comments

@FroMage
Copy link
Member

FroMage commented Dec 14, 2021

Description

The documentation for quarkus.oidc.authentication.user-info-required does not tell me if this will be required for my success OIDC endpoint, or for every subsequent authorised request.

In my case (and most websites that use OIDC for login/register prior to handling their own user and roles), we only need it to fetch info that's missing from the IdToken during a success OIDC login and will never need it.

So, we should clarify the docs to say if this will be called for every authenticated request or just the callback, and allow the option to only call it for our callback. Or perhaps we can make this lazy? Have UserInfo not be injectable, but turn it into a Uni<UserInfo> in which case it's only called if accessed and required?

Implementation ideas

No response

@FroMage FroMage added the kind/enhancement New feature or request label Dec 14, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Dec 14, 2021

/cc @pedroigor, @sberyozkin

@sberyozkin
Copy link
Member

sberyozkin commented Dec 15, 2021

Hi @FroMage

In my case (and most websites that use OIDC for login/register prior to handling their own user and roles), we only need it to fetch info that's missing from the IdToken during a success OIDC login and will never need it.

That may not be possible to assert for all the websites, one can't really assert that all of them just get something of the cookie for the next month or whatever duration the session has been opened for, without any sanity checks.

As far as quarkus-oidc is concerned it must re-authenticate every request coming into the endpoint, avoiding it is not possible, we just can't start opening some paths where an endpoint has been invoked without quarkus-oidc doing the re-authentication check.

But we have options to avoid doing the remote calls on every request.

If it is for example Google then I believe all the info is in IdToken and it is re-verified with the Google public keys fetched at the start up. See - we don't skip the verification here.

If it is GitHub - then it does not ever return IdToken so we create a faked one, and it is not possible to introspect the access token it returned because it is not JWT and GitHub has no introspection endpoint - we can only do it indirectly by using it to fetch UserInfo. But the same concept must hold here as with Google, the reauthentication must happen, it just happens that in case of GitHub only by fetching UserInfo that we can verify the access token returned to Quarkus has not been revoked, and still considered as valid at GitHub - this is what it means to reauthenticate in case if GitHub from the point of view of quarkus-oidc.

But in case of GitHub there are 2 options, one is already available:

  1. use UserInfo cache if you'd like to avoid fetching it on every request. I know you don't like this option, still not sure why, if you site will have to scale across many nodes then the worst what can happen is that some nodes will refetch this UserInfo. And we can go ahead and prepare an infinispan-client based cache if needed.
  2. this is not done yet, but as I said in the other issue, UserInfo will be injected into the faked IdToken, so it will be kept in a cookie.
    I know you also don't like this option and would prefer to intercept this process, but IMHO it is not necessary, I'd like to avoid creating a new API above the hack which is what creating a faked IdToken really is, and JSON-wise, it will be a very small difference, ex, instead of
Faked IdToken claims:
{
  "id":"myid"
}

it will be

Faked IdToken claims:
{
  "userinfo": {"id":"myid"}
}

This is really the only difference (you can check GitHub won't return much more in its user info), so IMHO it is not worth starting complicating things further with the API on top of the hack as we need to keep the options open to change this hack to some other solution.

But more relevant to this discussion is that, by avoiding cache and injecting UserInfo in the faked ID token, we can generically restore UserInfo and continue support the code which expects to get it as a SecurityIdentiy attribute or injected.

Thanks

@sberyozkin
Copy link
Member

Closing as duplicate of #22030

@sberyozkin sberyozkin added the triage/duplicate This issue or pull request already exists label Dec 15, 2021
@FroMage
Copy link
Member Author

FroMage commented Dec 15, 2021

If it is for example Google then I believe all the info is in IdToken and it is re-verified with the Google public keys fetched at the start up. See - we don't skip the verification here.

If it is GitHub - then it does not ever return IdToken so we create a faked one, and it is not possible to introspect the access token it returned because it is not JWT and GitHub has no introspection endpoint - we can only do it indirectly by using it to fetch UserInfo.

That's not true, we can sign it with an internal key and verify it on every call. It will be just as verified as the Google one. I thought we were already doing that.

I don't know if this is a duplicate of #22030, because I still don't know if by injecting the UserInfo in the IdToken I'll do a single call to the UserInfo endpoint or not (without cache).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/oidc kind/enhancement New feature or request triage/duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

2 participants