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.

Sign up for GitHub

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

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

Merged
merged 9 commits into from
Feb 20, 2020
Merged

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

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d47ef1e
Sychronize request validation with HTTP session
parysto Dec 30, 2019
f8c954b
Disabled session registration after refreshing token (causes session …
parysto Dec 30, 2019
c3c60b0
Removed duplicated line (registration of session)
parysto Jan 9, 2020
7e7d33e
Fixed typo in comment
parysto Jan 9, 2020
ce0e871
Cleanup of imports.
parysto Jan 13, 2020
529e5c9
Synchronize re-authentication of token only, not whole authentication…
parysto Jan 19, 2020
63ece4b
Added double check for access token expiration.
parysto Jan 19, 2020
af59a1c
Fixed compilation failure due to invalid position of return value.
parysto Jan 20, 2020
2245156
Replaced lock with implementation of Serializable to support session …
parysto Feb 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 54 additions & 31 deletions ...urity-openid/src/main/java/fish/payara/security/openid/OpenIdAuthenticationMechanism.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,10 @@
import fish.payara.security.openid.domain.OpenIdContextImpl;
import fish.payara.security.openid.domain.RefreshTokenImpl;
import java.io.IOException;
import java.io.Serializable;
import java.io.StringReader;
import java.security.Principal;
import java.util.Date;
import static java.util.Objects.isNull;
import static java.util.Objects.nonNull;

import java.util.Optional;
import java.util.logging.Level;
import static java.util.logging.Level.WARNING;
Expand All @@ -72,8 +70,8 @@
import javax.json.JsonObject;
import javax.json.JsonReader;
import javax.security.auth.callback.Callback;
import javax.security.auth.message.callback.CallerPrincipalCallback;
import javax.security.auth.callback.UnsupportedCallbackException;
import javax.security.auth.message.callback.CallerPrincipalCallback;
import javax.security.enterprise.AuthenticationException;
import javax.security.enterprise.AuthenticationStatus;
import static javax.security.enterprise.AuthenticationStatus.SUCCESS;
Expand All @@ -86,6 +84,7 @@
import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;
import javax.ws.rs.core.Response;
import javax.ws.rs.core.Response.Status;
import static org.glassfish.common.util.StringHelper.isEmpty;
Expand Down Expand Up @@ -149,6 +148,11 @@ public class OpenIdAuthenticationMechanism implements HttpAuthenticationMechanis

private static final Logger LOGGER = Logger.getLogger(OpenIdAuthenticationMechanism.class.getName());

private static class Lock implements Serializable {
}

private static final String SESSION_LOCK_NAME = OpenIdAuthenticationMechanism.class.getName();

/**
* Creates an {@link OpenIdAuthenticationMechanism}.
* <p>
Expand Down Expand Up @@ -182,21 +186,18 @@ public OpenIdAuthenticationMechanism setConfiguration(OpenIdAuthenticationDefini
this.configuration = configurationController.buildConfig(definition);
return this;
}

@Override
public AuthenticationStatus validateRequest(
HttpServletRequest request,
HttpServletResponse response,
HttpMessageContext httpContext) throws AuthenticationException {

Principal userPrincipal = request.getUserPrincipal();

if (isNull(userPrincipal)) {
if (isNull(request.getUserPrincipal())) {
LOGGER.fine("UserPrincipal is not set, authenticate user using OpenId Connect protocol.");
// User is not authenticated
// Perform steps (1) to (6)
return this.authenticate(request, response, httpContext);

} else {
// User has been authenticated in request before

Expand Down Expand Up @@ -226,7 +227,7 @@ private AuthenticationStatus authenticate(
HttpServletRequest request,
HttpServletResponse response,
HttpMessageContext httpContext) throws AuthenticationException {

if (httpContext.isProtected() && isNull(request.getUserPrincipal())) {
// (1) The End-User is not already authenticated
return authenticationController.authenticateUser(configuration, httpContext);
Expand Down Expand Up @@ -258,7 +259,8 @@ private AuthenticationStatus authenticate(
* Authorization Code Flow must be validated and exchanged for an ID Token,
* an Access Token and optionally a Refresh Token directly.
*
* @param httpContext the {@link HttpMessageContext} to validate authorization code from
* @param httpContext the {@link HttpMessageContext} to validate
* authorization code from
* @return the authentication status.
*/
private AuthenticationStatus validateAuthorizationCode(HttpMessageContext httpContext) {
Expand All @@ -281,7 +283,7 @@ private AuthenticationStatus validateAuthorizationCode(HttpMessageContext httpCo
updateContext(tokensObject);
OpenIdCredential credential = new OpenIdCredential(tokensObject, httpContext, configuration);
CredentialValidationResult validationResult = identityStoreHandler.validate(credential);

// Register session manually (if @AutoApplySession used, this would be done by its interceptor)
httpContext.setRegisterSession(validationResult.getCallerPrincipal().getName(), validationResult.getCallerGroups());
return httpContext.notifyContainerAboutLogin(validationResult);
Expand All @@ -293,34 +295,38 @@ private AuthenticationStatus validateAuthorizationCode(HttpMessageContext httpCo
return httpContext.notifyContainerAboutLogin(INVALID_RESULT);
}
}

private AuthenticationStatus reAuthenticate(
HttpServletRequest request,
HttpServletResponse response,
HttpMessageContext httpContext) throws AuthenticationException {

if (this.context.getAccessToken().isExpired()) {
// Access Token expired
LOGGER.fine("Access Token is expired. Request new Access Token with Refresh Token.");

AuthenticationStatus refreshStatus = this.context.getRefreshToken()
.map(rt -> this.refreshTokens(httpContext, rt))
.orElse(AuthenticationStatus.SEND_FAILURE);

if (refreshStatus != AuthenticationStatus.SUCCESS) {
LOGGER.log(Level.FINE, "Failed to refresh Access Token (Refresh Token might be invalid).");
try {
request.logout();
} catch (ServletException ex) {
LOGGER.log(WARNING, "Failed to logout user after failing to refresh token.", ex);
synchronized (this.getSessionLock(request)) {
if (this.context.getAccessToken().isExpired()) {
// Access Token expired
LOGGER.fine("Access Token is expired. Request new Access Token with Refresh Token.");

AuthenticationStatus refreshStatus = this.context.getRefreshToken()
.map(rt -> this.refreshTokens(httpContext, rt))
.orElse(AuthenticationStatus.SEND_FAILURE);

if (refreshStatus != AuthenticationStatus.SUCCESS) {
LOGGER.log(Level.FINE, "Failed to refresh Access Token (Refresh Token might be invalid).");
try {
request.logout();
} catch (ServletException ex) {
LOGGER.log(WARNING, "Failed to logout user after failing to refresh token.", ex);
}
// Redirect user to OpenID connect provider for re-authentication
return authenticationController.authenticateUser(configuration, httpContext);
}
}
// Redirect user to OpenID connect provider for re-authentication
return authenticationController.authenticateUser(configuration, httpContext);

}
}

return SUCCESS;

}

private AuthenticationStatus refreshTokens(HttpMessageContext httpContext, RefreshToken refreshToken) {
Expand All @@ -332,8 +338,9 @@ private AuthenticationStatus refreshTokens(HttpMessageContext httpContext, Refre
updateContext(tokensObject);
OpenIdCredential credential = new OpenIdCredential(tokensObject, httpContext, configuration);
CredentialValidationResult validationResult = identityStoreHandler.validate(credential);
// Register session manually (if @AutoApplySession used, this would be done by its interceptor)
httpContext.setRegisterSession(validationResult.getCallerPrincipal().getName(), validationResult.getCallerGroups());

// Do not register session, as this will invalidate the currently active session (destroys session beans and removes attributes set in session)!
// httpContext.setRegisterSession(validationResult.getCallerPrincipal().getName(), validationResult.getCallerGroups());
return httpContext.notifyContainerAboutLogin(validationResult);
} else {
// Token Request is invalid (refresh token invalid or expired)
Expand Down Expand Up @@ -364,4 +371,20 @@ private void updateContext(JsonObject tokensObject) {
}
}

private Object getSessionLock(HttpServletRequest request) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 ;-)

Copy link
Contributor

@pzygielo pzygielo Feb 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

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);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

"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

HttpSession session = request.getSession();
Object lock = session.getAttribute(SESSION_LOCK_NAME);
if (isNull(lock)) {
synchronized (OpenIdAuthenticationMechanism.class) {
lock = session.getAttribute(SESSION_LOCK_NAME);
if (isNull(lock)) {
lock = new Lock();
session.setAttribute(SESSION_LOCK_NAME, lock);
}

}
}
return lock;
}

}