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

Jump to bottom

Issue #6205 - Fix issues with OpenID redirecting to wrong URI #6211

Merged
merged 3 commits into from May 10, 2021
Merged

Issue #6205 - Fix issues with OpenID redirecting to wrong URI #6211

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8481dda
Issue #6205 - Use the OpenID state param to map to the redirect URI.
lachlan-roberts Apr 26, 2021
3995b85
Changes from review.
lachlan-roberts Apr 29, 2021
d26c59f
Changes from review.
lachlan-roberts May 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
262 changes: 167 additions & 95 deletions jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdAuthenticator.java
View file
Open in desktop
Expand Up @@ -14,9 +14,12 @@
package org.eclipse.jetty.security.openid;

import java.io.IOException;
import java.io.Serializable;
import java.math.BigInteger;
import java.nio.charset.StandardCharsets;
import java.security.SecureRandom;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Objects;
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
Expand Down Expand Up @@ -67,9 +70,12 @@ public class OpenIdAuthenticator extends LoginAuthenticator
public static final String J_URI = "org.eclipse.jetty.security.openid.URI";
public static final String J_POST = "org.eclipse.jetty.security.openid.POST";
public static final String J_METHOD = "org.eclipse.jetty.security.openid.METHOD";
public static final String CSRF_TOKEN = "org.eclipse.jetty.security.openid.csrf_token";
public static final String J_SECURITY_CHECK = "/j_security_check";
public static final String ERROR_PARAMETER = "error_description_jetty";
private static final String CSRF_MAP = "org.eclipse.jetty.security.openid.csrf_map";

@Deprecated
public static final String CSRF_TOKEN = "org.eclipse.jetty.security.openid.csrf_token";

private OpenIdConfiguration _configuration;
private String _errorPage;
Expand Down Expand Up @@ -112,18 +118,13 @@ public String getAuthMethod()
return Constraint.__OPENID_AUTH;
}

/**
* If true, uris that cause a redirect to a login page will always
* be remembered. If false, only the first uri that leads to a login
* page redirect is remembered.
*
* @param alwaysSave true to always save the uri
*/
@Deprecated
public void setAlwaysSaveUri(boolean alwaysSave)
{
_alwaysSaveUri = alwaysSave;
}

@Deprecated
public boolean isAlwaysSaveUri()
{
return _alwaysSaveUri;
Expand Down Expand Up @@ -202,16 +203,24 @@ public void prepareRequest(ServletRequest request)
//See Servlet Spec 3.1 sec 13.6.3
HttpServletRequest httpRequest = (HttpServletRequest)request;
HttpSession session = httpRequest.getSession(false);
if (session == null || session.getAttribute(SessionAuthentication.__J_AUTHENTICATED) == null)
if (session == null)
return; //not authenticated yet

String juri = (String)session.getAttribute(J_URI);
if (juri == null || juri.length() == 0)
return; //no original uri saved
String juri;
String method;
synchronized (session)
{
if (session.getAttribute(SessionAuthentication.__J_AUTHENTICATED) == null)
return; //not authenticated yet

String method = (String)session.getAttribute(J_METHOD);
if (method == null || method.length() == 0)
return; //didn't save original request method
juri = (String)session.getAttribute(J_URI);
if (juri == null || juri.length() == 0)
return; //no original uri saved

method = (String)session.getAttribute(J_METHOD);
if (method == null || method.length() == 0)
return; //didn't save original request method
}

StringBuffer buf = httpRequest.getRequestURL();
if (httpRequest.getQueryString() != null)
Expand All @@ -220,10 +229,10 @@ public void prepareRequest(ServletRequest request)
if (!juri.equals(buf.toString()))
return; //this request is not for the same url as the original

//restore the original request's method on this request
// Restore the original request's method on this request.
if (LOG.isDebugEnabled())
LOG.debug("Restoring original method {} for {} with method {}", method, juri, httpRequest.getMethod());
Request baseRequest = Request.getBaseRequest(request);
Request baseRequest = Objects.requireNonNull(Request.getBaseRequest(request));
baseRequest.setMethod(method);
}

Expand All @@ -235,6 +244,9 @@ public Authentication validateRequest(ServletRequest req, ServletResponse res, b
final Request baseRequest = Objects.requireNonNull(Request.getBaseRequest(request));
final Response baseResponse = baseRequest.getResponse();

if (LOG.isDebugEnabled())
LOG.debug("validateRequest({},{},{})", req, res, mandatory);

String uri = request.getRequestURI();
if (uri == null)
uri = URIUtil.SLASH;
Expand All @@ -260,71 +272,78 @@ public Authentication validateRequest(ServletRequest req, ServletResponse res, b
if (isJSecurityCheck(uri))
{
String authCode = request.getParameter("code");
if (authCode != null)
if (authCode == null)
{
// Verify anti-forgery state token
String state = request.getParameter("state");
String antiForgeryToken = (String)session.getAttribute(CSRF_TOKEN);
if (antiForgeryToken == null || !antiForgeryToken.equals(state))
{
sendError(request, response, "auth failed: invalid state parameter");
return Authentication.SEND_FAILURE;
}
sendError(request, response, "auth failed: no code parameter");
return Authentication.SEND_FAILURE;
}

// Attempt to login with the provided authCode
OpenIdCredentials credentials = new OpenIdCredentials(authCode, getRedirectUri(request));
UserIdentity user = login(null, credentials, request);
if (user != null)
{
// Redirect to original request
String nuri;
synchronized (session)
{
nuri = (String)session.getAttribute(J_URI);
String state = request.getParameter("state");
if (state == null)
{
sendError(request, response, "auth failed: no state parameter");
return Authentication.SEND_FAILURE;
}

if (nuri == null || nuri.length() == 0)
{
nuri = request.getContextPath();
if (nuri.length() == 0)
nuri = URIUtil.SLASH;
}
}
OpenIdAuthentication openIdAuth = new OpenIdAuthentication(getAuthMethod(), user);
if (LOG.isDebugEnabled())
LOG.debug("authenticated {}->{}", openIdAuth, nuri);
// Verify anti-forgery state token.
UriRedirectInfo uriRedirectInfo;
synchronized (session)
{
uriRedirectInfo = removeAndClearCsrfMap(session, state);
}
if (uriRedirectInfo == null)
{
sendError(request, response, "auth failed: invalid state parameter");
return Authentication.SEND_FAILURE;
}

response.setContentLength(0);
baseResponse.sendRedirect(nuri, true);
return openIdAuth;
}
// Attempt to login with the provided authCode.
OpenIdCredentials credentials = new OpenIdCredentials(authCode, getRedirectUri(request));
UserIdentity user = login(null, credentials, request);
if (user == null)
{
sendError(request, response, null);
return Authentication.SEND_FAILURE;
}

// Not authenticated.
sendError(request, response, null);
return Authentication.SEND_FAILURE;
OpenIdAuthentication openIdAuth = new OpenIdAuthentication(getAuthMethod(), user);
if (LOG.isDebugEnabled())
LOG.debug("authenticated {}->{}", openIdAuth, uriRedirectInfo.getUri());

// Save redirect info in session so original request can be restored after redirect.
synchronized (session)
{
session.setAttribute(J_URI, uriRedirectInfo.getUri());
session.setAttribute(J_METHOD, uriRedirectInfo.getMethod());
session.setAttribute(J_POST, uriRedirectInfo.getFormParameters());
}

// Redirect to the original URI.
response.setContentLength(0);
baseResponse.sendRedirect(uriRedirectInfo.getUri(), true);
return openIdAuth;
}

// Look for cached authentication in the Session.
Authentication authentication = (Authentication)session.getAttribute(SessionAuthentication.__J_AUTHENTICATED);
if (authentication != null)
synchronized (session)
{
// Has authentication been revoked?
if (authentication instanceof Authentication.User && _loginService != null &&
!_loginService.validate(((Authentication.User)authentication).getUserIdentity()))
{
if (LOG.isDebugEnabled())
LOG.debug("auth revoked {}", authentication);
session.removeAttribute(SessionAuthentication.__J_AUTHENTICATED);
}
else
Authentication authentication = (Authentication)session.getAttribute(SessionAuthentication.__J_AUTHENTICATED);
if (authentication != null)
{
synchronized (session)
// Has authentication been revoked?
if (authentication instanceof Authentication.User && _loginService != null &&
!_loginService.validate(((Authentication.User)authentication).getUserIdentity()))
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
{
if (LOG.isDebugEnabled())
LOG.debug("auth revoked {}", authentication);
session.removeAttribute(SessionAuthentication.__J_AUTHENTICATED);
}
else
{
String jUri = (String)session.getAttribute(J_URI);
if (jUri != null)
{
//check if the request is for the same url as the original and restore
//params if it was a post
// Check if the request is for the same url as the original and restore params if it was a post.
if (LOG.isDebugEnabled())
LOG.debug("auth retry {}->{}", authentication, jUri);
StringBuffer buf = request.getRequestURL();
Expand Down Expand Up @@ -361,29 +380,8 @@ public Authentication validateRequest(ServletRequest req, ServletResponse res, b
return Authentication.UNAUTHENTICATED;
}

// Remember the current URI.
synchronized (session)
{
// But only if it is not set already, or we save every uri that leads to a login redirect
if (session.getAttribute(J_URI) == null || isAlwaysSaveUri())
{
StringBuffer buf = request.getRequestURL();
if (request.getQueryString() != null)
buf.append("?").append(request.getQueryString());
session.setAttribute(J_URI, buf.toString());
session.setAttribute(J_METHOD, request.getMethod());

if (MimeTypes.Type.FORM_ENCODED.is(req.getContentType()) && HttpMethod.POST.is(request.getMethod()))
{
MultiMap<String> formParameters = new MultiMap<>();
baseRequest.extractFormParameters(formParameters);
session.setAttribute(J_POST, formParameters);
}
}
}

// Send the the challenge.
String challengeUri = getChallengeUri(request);
String challengeUri = getChallengeUri(baseRequest);
if (LOG.isDebugEnabled())
LOG.debug("challenge {}->{}", session.getId(), challengeUri);
baseResponse.sendRedirect(challengeUri, true);
Expand Down Expand Up @@ -464,16 +462,15 @@ private String getRedirectUri(HttpServletRequest request)
return redirectUri.toString();
}

protected String getChallengeUri(HttpServletRequest request)
protected String getChallengeUri(Request request)
{
HttpSession session = request.getSession();
String antiForgeryToken;
synchronized (session)
{
antiForgeryToken = (session.getAttribute(CSRF_TOKEN) == null)
? new BigInteger(130, new SecureRandom()).toString(32)
: (String)session.getAttribute(CSRF_TOKEN);
session.setAttribute(CSRF_TOKEN, antiForgeryToken);
Map<String, UriRedirectInfo> csrfMap = ensureCsrfMap(session);
antiForgeryToken = new BigInteger(130, new SecureRandom()).toString(32);
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
csrfMap.put(antiForgeryToken, new UriRedirectInfo(request));
}

// any custom scopes requested from configuration
Expand All @@ -494,7 +491,82 @@ protected String getChallengeUri(HttpServletRequest request)
@Override
public boolean secureResponse(ServletRequest req, ServletResponse res, boolean mandatory, User validatedUser)
{
return true;
return req.isSecure();
}

private UriRedirectInfo removeAndClearCsrfMap(HttpSession session, String csrf)
{
@SuppressWarnings("unchecked")
Map<String, UriRedirectInfo> csrfMap = (Map<String, UriRedirectInfo>)session.getAttribute(CSRF_MAP);
if (csrfMap == null)
return null;

UriRedirectInfo uriRedirectInfo = csrfMap.get(csrf);
csrfMap.clear();
return uriRedirectInfo;
}

private Map<String, UriRedirectInfo> ensureCsrfMap(HttpSession session)
{
@SuppressWarnings("unchecked")
Map<String, UriRedirectInfo> csrfMap = (Map<String, UriRedirectInfo>)session.getAttribute(CSRF_MAP);
if (csrfMap == null)
{
// Create a custom Map so we can only have a limited number of request URIs saved.
csrfMap = new LinkedHashMap<>()
{
private static final int MAX_SIZE = 64;

@Override
protected boolean removeEldestEntry(Map.Entry<String, UriRedirectInfo> eldest)
janbartel marked this conversation as resolved.
Show resolved Hide resolved
{
return size() > MAX_SIZE;
}
};
session.setAttribute(CSRF_MAP, csrfMap);
}
return csrfMap;
}

private static class UriRedirectInfo implements Serializable
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
{
private static final long serialVersionUID = 139567755844461433L;

private final String _uri;
private final String _method;
private final MultiMap<String> _formParameters;

public UriRedirectInfo(Request request)
{
_uri = request.getRequestURI();
_method = request.getMethod();

if (MimeTypes.Type.FORM_ENCODED.is(request.getContentType()) && HttpMethod.POST.is(request.getMethod()))
{
MultiMap<String> formParameters = new MultiMap<>();
request.extractFormParameters(formParameters);
_formParameters = formParameters;
}
else
{
_formParameters = null;
}
}

public String getUri()
{
return _uri;
}

public String getMethod()
{
return _method;
}

public MultiMap<String> getFormParameters()
{
return _formParameters;
}
}

/**
Expand Down