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

ECOSYS-118 OpenID Connect session timeout association with access and/or identity token expiry #4570

Merged
merged 7 commits into from Jul 7, 2020

Conversation

jGauravGupta
Copy link
Contributor

@jGauravGupta jGauravGupta commented Mar 10, 2020

Description

This is a feature to validate the Access Token and/or Identity Token expiry and setting the correct expiry datatype.

It also provides the support for end_session_endpoint to redirect the User-Agent to the OP logout page after the log out from the RP application.

To enable the validation, add the @LogoutDefinition to @OpenIdAuthenticationDefinition or respective provider annotation:

@OpenIdAuthenticationDefinition(
        clientId = "xxxxxxxxxxxxxxxxxxx",
        clientSecret = "xxxxxxxxxxxxxxx",
        tokenAutoRefresh = true,
       ........
        logout = @LogoutDefinition(
                notifyProvider = true,
                redirectURI = "${baseURL}/logout.html",
                accessTokenExpiry = false,
                identityTokenExpiry = false
        ),
       ........
)
  • @LogoutDefinition#notifyProvider: Notify the OIDC provider (OP) that the user has logged out of the application and might want to log out of the OP as well. If true then after having logged out the user from RP, redirects the End-User's User Agent to the OP's logout endpoint URL end_session_endpoint.
  • @LogoutDefinition#redirectURI: The post logout redirect URI to which the RP is requesting that the End-User's User Agent be redirected after a logout has been performed. If redirect URI is empty then redirect to OpenID connect provider authorization_endpoint for re-authentication.
  • @LogoutDefinition#accessTokenExpiry: Invalidates the session on the expiry of Access Token.
  • @LogoutDefinition#identityTokenExpiry: Invalidates the session on the expiry of Identity Token.

Programmatic logout:
Programmatic logout feature via OpenIdContext#logout() function which Invalidates the RP's active OpenId Connect session and if fish.payara.security.annotations.LogoutDefinition#notifyProvider set to true then redirects the End-User's User Agent to the end_session_endpoint to notify the OP that the user has logged out of the RP's application and ask the user whether they want to logout from the OP as well. After successful logout, the End-User's User Agent redirect back to the RP's post_redirect_uri configured via fish.payara.security.annotations.LogoutDefinition#redirectURI

Related PRs

payara/ecosystem-security-connectors#24

Testing

Testing Performed

Manual tested with Google and Azure OIDC provider
@LogoutDefinition#notifyProvider feature tested on Azure OIDC provider as end_session_endpoint not available on Google OIDC provider metadata.

Copy link
Contributor

@Cousjava Cousjava left a comment

Choose a reason for hiding this comment

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

Copyright year needs updating

@Pandrex247 Pandrex247 added this to the 5.202 milestone May 12, 2020
@sharpedavid
Copy link

Hello @jGauravGupta. First of all, thank you for writing the OIDC adapter. My company is preparing to use it in production and it's been easy to use so far.

I'm not an OIDC expert, but I've done some reading and I have a few comments about patch. The description states:

This is a bug fix to validate access token expiry for every request and setting the correct expiry datatype.

I have two suggestions regarding this implementation (and a suggested alternative):

One: This is not a bug

I don't think this is a bug. The Core specification doesn't describe a relationship between the access token or ID token and the client session. The optional Session Management specification states that "An ID Token typically comes with an expiration date. The RP MAY rely on it to expire the RP session."

In Robert Broeckelmann's article on OIDC logout, he writes (emphasis mine):

Timeout-Based Logout
To avoid complexity, it is possible, where security constraints allow, to simply rely upon a timeout based logout mechanism for the RP and OP. There are a couple of approaches.

  1. The OIDC ID Token has an expiration timestamp. This timestamp can represent the session timeout for the RP or the valid lifetime for which the token can be used to create a session. If it’s the prior, then any request that is received after this timestamp should be considered invalid and the user’s session is expired. If refresh tokens are being used, this doesn’t really work. If information security policy requires short-lived tokens that should only be valid long enough to create the applications security session, then this doesn’t really work, which is often the case.
  2. The application can also establish its own session timeout that it enforces. A common timeout value can be established with the OP that essentially achieves the desired result. In situations like this, configuring the OP to have user’s session expire a few moments before the RP is advisable. It’s also possible that the desired behavior is to allow the user to maintain long security sessions on the RP, but to have to periodically reauthenticate against the identity provider. The desired behavior and requirements can differ significantly across applications.

And in this question on StackOverflow, the consensus is that "The ID token has to be un-expired at this point of use (which it should be, since it has just been issued). But after this it is not used again, so it does not matter if it expires while the user still has an active session. The Client has the authentication information it needs, and in turn can choose its own policy for how long the session lasts before the user has to log in again."

Two: If we do want to associate token expiry with client session expiry, use the ID token expiry, not the access token expiry

This is supported by all of the above citations -- the specifications, the blog post, and the StackOverflow answer. OIDC really concerns itself with the ID token, not the access token. The access token is used to access secure resources, such as the OpenID Provider's userinfo endpoint and other OAuth secured resources.

Suggested implementation: Don't store the tokens

After you have proof that the user is authenticated with the OpenID Provider, create the client session and discard the tokens. This is Core spec-compliant and easier to implement. We can look into implementing the optional specifications later if there's interest.

Auth0 has this to say:

If your app is using a sign in scenario that doesn't require API calls, only an ID Token is required. There is no need to store it. You can validate it and get the data from it that you required.


I hope this is helpful, but again, I don't pretend to be an expert. I've read some of the specifications, and some of your code.

Signed-off-by: Gaurav Gupta <gaurav.gupta@payara.fish>
@jGauravGupta jGauravGupta added the PR: DO NOT MERGE Don't merge PR until further notice label May 28, 2020
@jGauravGupta jGauravGupta changed the title ECOSYS-118 Validate AccessToken expiry on each request ECOSYS-118 OpenID Connect session timeout association with access and/or identity token expiry May 29, 2020
…/or identity token expiry

Signed-off-by: Gaurav Gupta <gaurav.gupta@payara.fish>
Signed-off-by: Gaurav Gupta <gaurav.gupta@payara.fish>
@jGauravGupta
Copy link
Contributor Author

jenkins test please

@jGauravGupta
Copy link
Contributor Author

Hi @sharpedavid,

Thanks for your suggestion, I have updated the PR with optional Access & Identity token expiry validation and session timeout association, please check out the updated PR description for more detail.

@jGauravGupta jGauravGupta removed the PR: DO NOT MERGE Don't merge PR until further notice label Jun 1, 2020
Copy link
Contributor

@dmatej dmatej left a comment

Choose a reason for hiding this comment

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

I did not test it and I really miss automatic tests here, but the code looks well.
If it was tested manually - could you instruct anyone in the team to verify it once more? Or could @sharpedavid confirm that it works es expected?

@sharpedavid
Copy link

Hi @dmatej I'm happy to test it, but I don't know how to build Payara from source. Do you have a guide?

After I have that, I can easily test it. I would use Keycloak as the OpenID Provider, and I have a sample application ready.

@dmatej
Copy link
Contributor

dmatej commented Jun 3, 2020

If you have Linux, JDK8, Maven and Docker installed, you can run this to get all artifacts at once. On my machine it takes some 8 minutes:

git clone git@github.com:payara/Payara.git
cd Payara
mvn clean install -PQuickBuild,BuildExtras,BuildDockerImages -Dmaven.test.skip=true 

If you need only payara.zip and no embedded, docker containers etc., you can run this (takes only 2 minutes):

mvn clean install -PQuickBuild -Dmaven.test.skip=true -TC4

In both cases the Payara Server zip file will be in ./appserver/distributions/payara/target/payara.zip and also in local Maven repository cache.

On Windows and Mac it should be similar.

@sharpedavid
Copy link

Thanks @dmatej I'm going to try this now and tomorrow.

@sharpedavid
Copy link

I ran a few ad hoc tests. I was not exhaustive. Everything I tried worked. I didn't test identityTokenExpiry because I ran out of time. I did test LogoutDefinition#redirectURI but didn't make note of it below.

Environment

  • keycloak-8.0.1
    • access token expiry set to 1 minute
    • refresh token expiry set to 2 minutes
  • java-1.8.0-openjdk-1.8.0.191-1.b12.redhat
  • simple JSF application
  • commit 5ee958716ac29b63fd7c623108732fec66fc5228

Test 1: PASS

  • tokenAutoRefresh=true
  • accessTokenExpiry=true
  • notifyProvider=true

I refresh the page every 5 seconds. When the access token expires, it's refreshed and the user is not signed out. If I wait three minutes and the refresh token expires, then refresh the page, the user is signed-out. The user is also signed-out at the OP, which means end_session_endpoint is working.

Test 2: PASS

  • same as Test 1 except
  • notifyProvider=false

Works as expected. User is signed-out on RP, but not OP.

Test 3: PASS

  • tokenAutoRefresh=true
  • accessTokenExpiry=false
  • idTokenExpiry=false

Token are never refreshed! I found this surprising and commented on it in the code.

Question

As described above, the automatic logout functionality is working. Can I trigger a programmatic logout? My test application has a "Logout" button, and I want it to do the same thing as the automatic logout: logout of the RP and the OP if notifyProvider=true. I'm not asking you to modify the pull request! I'm just making sure I'm not missing something.

Glossary

  • RP, Relying Party, the application, Payara
  • OP, the OpenID Provider, Keycloak

Thank you for this new feature! It seems to be working well.

@jGauravGupta

This comment has been minimized.

@jGauravGupta
Copy link
Contributor Author

jGauravGupta commented Jun 6, 2020

Hi @sharpedavid ,

Thanks for your valuable feedback and time, I have added the programmatic logout feature via OpenIdContext#logout() function. And also dissociated tokenAutoRefresh feature from @LogoutDefinition#accessTokenExpiry and @LogoutDefinition#idTokenExpiry.

@jGauravGupta

This comment has been minimized.

@sharpedavid
Copy link

No problem @jGauravGupta . Please let me know if you want me to re-test the new features.

I don't know if that Jenkins failure is important.

@jGauravGupta
Copy link
Contributor Author

Hi @sharpedavid ,

I have tested the feature on Azure OIDC provider. Jenkins issue is not related to this PR.

@smillidge smillidge modified the milestones: 5.202, 5.2020.3 Jul 6, 2020
@jGauravGupta
Copy link
Contributor Author

jenkins test please

@jGauravGupta jGauravGupta merged commit 94d7661 into payara:master Jul 7, 2020
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.

None yet

6 participants