CUSTCOM-168 OpenID Connect: Fixed simultaneous redirects and invalidation of session #4419

parysto · 2020-01-09T08:38:15Z

Description

This is a bug fix / feature.

If a page contains multiple protected resources and authentication (via OpenID Connect) is activated, multiple redirects to the OpenID Connect provider are performed (one for each protected resource). This is especially the case when auto refresh of the token is activated, the token expires and a new access token must be requested. To prevent multiple redirects, I have synchronized the authentication/refresh logic on the user's session.
If auto refresh is enabled for the access token, the principal was registered in the session again after refreshing the token. This caused the currently active session to be invalidated (destroys all session scoped beans and session attributes).

Testing

Testing Performed

Deployed developed app on instance with fix installed and compared/validated behavior while using the app (especially when refresh of token happened).

Testing Environment

OpenJDK 11 on Ubuntu 19.10

…to be invalidated)

...security-openid/src/main/java/fish/payara/security/openid/OpenIdAuthenticationMechanism.java

jGauravGupta · 2020-01-13T08:09:15Z

Hi @parysto ,

Thanks for the PR . A import is missing in OpenIdAuthenticationMechanism class, please fix the compilation error:

project openid-client-integration: Compilation failure
/var/lib/jenkins/workspace/PayaraQuickBuildAndTest/appserver/payara-appserver-modules/security-openid/src/main/java/fish/payara/security/openid/OpenIdAuthenticationMechanism.java:[371,9] cannot find symbol
  symbol:   class HttpSession
  location: class fish.payara.security.openid.OpenIdAuthenticationMechanism

And can you also share the sample application with multiple protected resources on a page?

parysto · 2020-01-13T18:07:29Z

Hi @jGauravGupta ,

have pushed the sample application to: https://github.com/parysto/OpenIDJsfDemo
It's a JSF application that includes some JS (JQuery to increase size/load time) files. All resources, except callback.xhtml, (index.xhtml and *.js) are protected.
Please set providerURI, clientID and clientSecret in Configuration.java to your OpenID connect provider.

To trigger the race condition, set the validity time of the access token to a small value (like 1 min). Open the sample app, log in and wait until the token expired. Press F5 repeatedly, so that multiple requests are sent to Payara at the same time. Due to the race condition, you will be logged out and redirected to callback.xhtml. If you enabled logging for OpenIdAuthenticationMechanism, you will see log messages like:

FEIN: UserPrincipal is set, check if Access Token is valid.
FEIN: Access Token is expired. Request new Access Token with Refresh Token.
FEIN: UserPrincipal is set, check if Access Token is valid.
FEIN: Access Token is expired. Request new Access Token with Refresh Token.
FEIN: UserPrincipal is set, check if Access Token is valid.
FEIN: Access Token is expired. Request new Access Token with Refresh Token.
FEIN: UserPrincipal is set, check if Access Token is valid.
FEIN: Access Token is expired. Request new Access Token with Refresh Token.
AM FEINSTEN: Sending the request to the userinfo endpoint
AM FEINSTEN: Sending the request to the userinfo endpoint
AM FEINSTEN: Sending the request to the userinfo endpoint
AM FEINSTEN: Sending the request to the userinfo endpoint
FEIN: UserPrincipal is not set, authenticate user using OpenId Connect protocol.
AM FEINSTEN: Redirecting for authentication to https://(...)

Will fix the missing imports ;-)

jGauravGupta

Hi @parysto,

Instead of locking the complete OpenIdAuthenticationMechanism#validateRequest call per request, I recommend synchronizing in specific snippets e.g If access token expired in OpenIdAuthenticationMechanism#reAuthenticate.

parysto · 2020-01-14T18:33:09Z

Hi @jGauravGupta ,

I will check if synchronizing OpenIdAuthenticationMechanism#reAuthenticate is sufficient.

… process.

parysto · 2020-01-19T13:41:23Z

Hi @jGauravGupta,

hadn't any issues synchronizing on OpenIdAuthenticationMechanism#reAuthenticate only :-)
Have updated the PR

jGauravGupta · 2020-01-19T15:45:42Z

Hi @parysto,

Thanks for updating the PR, Can you perform the locking operation only in case of token expired as mentioned above.
e.g:

if (this.context.getAccessToken().isExpired()) {
            synchronized (this.getSessionLock(request)) {
                if (this.context.getAccessToken().isExpired()) {
                   // refreshTokens
                }
            }
        }

parysto · 2020-01-19T17:28:37Z

That's a good point (to improve performance). Will update the PR.

jGauravGupta · 2020-01-20T14:21:14Z

Hi @parysto,

Please fix the following syntax error:

/appserver/payara-appserver-modules/security-openid/src/main/java/fish/payara/security/openid/OpenIdAuthenticationMechanism.java:[327,5] missing return statement

jGauravGupta · 2020-01-20T17:26:33Z

jenkins test please

parysto · 2020-02-08T15:55:06Z

Hi @jGauravGupta,

is there something more I can do to complete this merge request?

dmatej · 2020-02-09T09:06:19Z

...security-openid/src/main/java/fish/payara/security/openid/OpenIdAuthenticationMechanism.java

@@ -364,4 +368,20 @@ private void updateContext(JsonObject tokensObject) {
        }
    }

+    private Object getSessionLock(HttpServletRequest request) {


Is really the plan to put the lock to the session object and leave it there forever? What if the token expires for second time? The lock will be still in session attributes from previous iteration.

Also replication in cluster will have some latency, is it or isn't it a problem? (probably it isn't)

I don't say it is incorrect, I am only asking ;-)

Interesting...
I know raw Object is not Serializable. I do not remember perfectly, but I do wonder what happens at HttpSession passivation... Or if session migrates to other VM...

Hi @dmatej, hi @pzygielo

Is really the plan to put the lock to the session object and leave it there forever? What if the token expires for second time? The lock will be still in session attributes from previous iteration.

You mean the lock-object? Yes, when the token expires again, the same instance of the lock will be reused. So, we do not need to set the "lock-attribute" again as long as the user's session is not invalidated. When the user's session is invalidated, the lock will be removed. Or am I wrong?

Also replication in cluster will have some latency, is it or isn't it a problem? (probably it isn't)

Object itself does not have any fields that need to be transferred. So there shouldn't be a huge impact on latency. Although I haven't measured that ;-)

I know raw Object is not Serializable. I do not remember perfectly, but I do wonder what happens at HttpSession passivation... Or if session migrates to other VM...

Good point, haven't thought about that. Yes, Object is not Serializable, so passivation or replication will fail. Will change the lock to an "empty" implementation of Serializable.

I don't say it is incorrect, I am only asking ;-)

I appreciate your feedback :-)

@pzygielo I think I investigated in years ago when I was fixing SSO session replication and I found that nonserializable objects are simply ignored if there is no special implementation for them.

@dmatej thanks for checking

I think I investigated in years ago when I was fixing SSO session replication and I found that nonserializable objects are simply ignored if there is no special implementation for them.

However it's not so clear for me, given section 7.7.2 of Servlet 4.0 spec, and:

The distributed servlet container must throw an IllegalArgumentException for
objects where the container cannot support the mechanism necessary for migration
of the session storing them.

thus I'm not so sure about simply ignoring such session attributes. So it seems I have to investigate myself 😉

Here is one implementation piece I found, that confirms your statement, about "how it is":

Payara/appserver/web/web-core/src/main/java/org/apache/catalina/session/StandardSession.java

Lines 2172 to 2178 in bdb468a

} else if (isSerializable(value)) {

saveNames.add(keys[i]);

saveValues.add(value);

//end HERCULES:mod

} else {

removeAttribute(keys[i], true, true);

}

"the container cannot support the mechanism necessary" ... such mechanism might be even ignoring objects which are managed by some mechanism on the other side.
But ... yes, I think there are some "blank spaces" in impl ... the problem is that implementing it would crash nearly all older+larger applications I have seen :D

…passivation.

dmatej · 2020-02-19T11:37:58Z

Jenkins test please

jGauravGupta · 2020-02-19T13:29:25Z

Jenkins test please

dmatej · 2020-02-19T17:34:30Z

Jenkins test please

parysto added 2 commits December 30, 2019 18:44

Sychronize request validation with HTTP session

d47ef1e

Disabled session registration after refreshing token (causes session …

f8c954b

…to be invalidated)

smillidge requested a review from jGauravGupta January 9, 2020 13:41

smillidge added the PR: CLA CLA submitted on PR by the contributor label Jan 9, 2020

dmatej reviewed Jan 9, 2020

View reviewed changes

...security-openid/src/main/java/fish/payara/security/openid/OpenIdAuthenticationMechanism.java Outdated Show resolved Hide resolved

parysto added 2 commits January 9, 2020 19:14

Removed duplicated line (registration of session)

c3c60b0

Fixed typo in comment

7e7d33e

parysto requested a review from dmatej January 10, 2020 17:12

This comment has been minimized.

Sign in to view

Cleanup of imports.

ce0e871

This comment has been minimized.

Sign in to view

jGauravGupta requested changes Jan 14, 2020

View reviewed changes

Synchronize re-authentication of token only, not whole authentication…

529e5c9

… process.

parysto requested a review from jGauravGupta January 19, 2020 13:46

Added double check for access token expiration.

63ece4b

This comment has been minimized.

Sign in to view

jGauravGupta approved these changes Jan 20, 2020

View reviewed changes

Fixed compilation failure due to invalid position of return value.

af59a1c

dmatej changed the title ~~OpenID Connect: Fixed simultaneous redirects and invalidation of session~~ CUSTCOM-168 OpenID Connect: Fixed simultaneous redirects and invalidation of session Jan 24, 2020

parysto requested a review from jGauravGupta January 24, 2020 18:32

dmatej reviewed Feb 9, 2020

View reviewed changes

Replaced lock with implementation of Serializable to support session …

2245156

…passivation.

smillidge added the community-contribution label Feb 16, 2020

dmatej self-assigned this Feb 19, 2020

dmatej self-requested a review February 19, 2020 11:44

dmatej approved these changes Feb 19, 2020

View reviewed changes

jGauravGupta approved these changes Feb 19, 2020

View reviewed changes

dmatej merged commit f742bad into payara:master Feb 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CUSTCOM-168 OpenID Connect: Fixed simultaneous redirects and invalidation of session #4419

CUSTCOM-168 OpenID Connect: Fixed simultaneous redirects and invalidation of session #4419

parysto commented Jan 9, 2020

This comment has been minimized.

jGauravGupta commented Jan 13, 2020 •

edited

Loading

parysto commented Jan 13, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

jGauravGupta left a comment

parysto commented Jan 14, 2020

parysto commented Jan 19, 2020

jGauravGupta commented Jan 19, 2020 •

edited

Loading

parysto commented Jan 19, 2020

This comment has been minimized.

jGauravGupta commented Jan 20, 2020 •

edited

Loading

jGauravGupta commented Jan 20, 2020

parysto commented Feb 8, 2020

dmatej Feb 9, 2020

pzygielo Feb 9, 2020 •

edited

Loading

parysto Feb 9, 2020

dmatej Feb 19, 2020

pzygielo Feb 19, 2020

pzygielo Feb 19, 2020

dmatej Feb 19, 2020

dmatej commented Feb 19, 2020

jGauravGupta commented Feb 19, 2020

dmatej commented Feb 19, 2020

	} else if (isSerializable(value)) {
	saveNames.add(keys[i]);
	saveValues.add(value);
	//end HERCULES:mod
	} else {
	removeAttribute(keys[i], true, true);
	}

CUSTCOM-168 OpenID Connect: Fixed simultaneous redirects and invalidation of session #4419

CUSTCOM-168 OpenID Connect: Fixed simultaneous redirects and invalidation of session #4419

Conversation

parysto commented Jan 9, 2020

Description

Testing

Testing Performed

Testing Environment

This comment has been minimized.

jGauravGupta commented Jan 13, 2020 • edited Loading

parysto commented Jan 13, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

jGauravGupta left a comment

Choose a reason for hiding this comment

parysto commented Jan 14, 2020

parysto commented Jan 19, 2020

jGauravGupta commented Jan 19, 2020 • edited Loading

parysto commented Jan 19, 2020

This comment has been minimized.

jGauravGupta commented Jan 20, 2020 • edited Loading

jGauravGupta commented Jan 20, 2020

parysto commented Feb 8, 2020

dmatej Feb 9, 2020

Choose a reason for hiding this comment

pzygielo Feb 9, 2020 • edited Loading

Choose a reason for hiding this comment

parysto Feb 9, 2020

Choose a reason for hiding this comment

dmatej Feb 19, 2020

Choose a reason for hiding this comment

pzygielo Feb 19, 2020

Choose a reason for hiding this comment

pzygielo Feb 19, 2020

Choose a reason for hiding this comment

dmatej Feb 19, 2020

Choose a reason for hiding this comment

dmatej commented Feb 19, 2020

jGauravGupta commented Feb 19, 2020

dmatej commented Feb 19, 2020

jGauravGupta commented Jan 13, 2020 •

edited

Loading

parysto commented Jan 13, 2020 •

edited

Loading

jGauravGupta commented Jan 19, 2020 •

edited

Loading

jGauravGupta commented Jan 20, 2020 •

edited

Loading

pzygielo Feb 9, 2020 •

edited

Loading