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

Issue #8216 - OpenID Connect RP-Initiated Logout #8286

Merged
merged 6 commits into from
Jul 20, 2022
Merged

Issue #8216 - OpenID Connect RP-Initiated Logout #8286

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5282ca3
Issue #8216 - provide logout for OpenID via OpenidAuthenticator
keller-j Jun 29, 2022
92cf466
Merge commit '5282ca37ef7a81789b3c1d0a41854259898bce96' into jetty-10…
lachlan-roberts Jul 12, 2022
26732c9
Issue #8216 - Use HttpServletRequest.logout() for openid end_session_…
lachlan-roberts Jul 12, 2022
90fe562
Issue #8216 - improve testing for end_session_endpoint
lachlan-roberts Jul 12, 2022
6de385e
Issue #8216 - change logic on when to redirect
lachlan-roberts Jul 13, 2022
c38b326
Issue #8216 - fix broken test OpenIdAuthenticationTest
lachlan-roberts Jul 13, 2022
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
73 changes: 72 additions & 1 deletion jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ public class OpenIdAuthenticator extends LoginAuthenticator
public static final String RESPONSE = "org.eclipse.jetty.security.openid.response";
public static final String ISSUER = "org.eclipse.jetty.security.openid.issuer";
public static final String REDIRECT_PATH = "org.eclipse.jetty.security.openid.redirect_path";
public static final String LOGOUT_REDIRECT_PATH = "org.eclipse.jetty.security.openid.logout_redirect_path";
public static final String ERROR_PAGE = "org.eclipse.jetty.security.openid.error_page";
public static final String J_URI = "org.eclipse.jetty.security.openid.URI";
public static final String J_POST = "org.eclipse.jetty.security.openid.POST";
Expand All @@ -82,6 +83,7 @@ public class OpenIdAuthenticator extends LoginAuthenticator
private final SecureRandom _secureRandom = new SecureRandom();
private OpenIdConfiguration _openIdConfiguration;
private String _redirectPath;
private String _logoutRedirectPath = "/";
private String _errorPage;
private String _errorPath;
private String _errorQuery;
Expand All @@ -103,11 +105,18 @@ public OpenIdAuthenticator(OpenIdConfiguration configuration, String errorPage)
}

public OpenIdAuthenticator(OpenIdConfiguration configuration, String redirectPath, String errorPage)
{
this(configuration, redirectPath, errorPage, null);
}

public OpenIdAuthenticator(OpenIdConfiguration configuration, String redirectPath, String errorPage, String logoutRedirectPath)
{
_openIdConfiguration = configuration;
setRedirectPath(redirectPath);
if (errorPage != null)
setErrorPage(errorPage);
if (logoutRedirectPath != null)
setLogoutRedirectPath(logoutRedirectPath);
}

@Override
Expand All @@ -123,11 +132,15 @@ public void setConfiguration(AuthConfiguration authConfig)

String redirectPath = authConfig.getInitParameter(REDIRECT_PATH);
if (redirectPath != null)
_redirectPath = redirectPath;
setRedirectPath(redirectPath);

String error = authConfig.getInitParameter(ERROR_PAGE);
if (error != null)
setErrorPage(error);

String logout = authConfig.getInitParameter(LOGOUT_REDIRECT_PATH);
if (logout != null)
setLogoutRedirectPath(logout);

super.setConfiguration(new OpenIdAuthConfiguration(_openIdConfiguration, authConfig));
}
Expand Down Expand Up @@ -166,6 +179,22 @@ else if (!redirectPath.startsWith("/"))
_redirectPath = redirectPath;
}

public void setLogoutRedirectPath(String logoutRedirectPath)
{
if (logoutRedirectPath == null)
{
LOG.warn("redirect path must not be null, defaulting to /");
logoutRedirectPath = "/";
}
Comment on lines +184 to +188
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to allow null to be set here and have that mean "do not redirect on logout"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This redirect path is actually for the OpenId provider redirecting back to the webapp, not for the webapp to redirect to the OpenID Provider on logout.

post_logout_redirect_uri
OPTIONAL. URI to which the RP is requesting that the End-User's User Agent be redirected after a logout has been performed. This URI SHOULD use the https scheme and MAY contain port, path, and query parameter components; however, it MAY use the http scheme, provided that the Client Type is confidential, as defined in Section 2.1 of OAuth 2.0 [RFC6749], and provided the OP allows the use of http RP URIs. The URI MAY use an alternate scheme, such as one that is intended to identify a callback into a native application. The value MUST have been previously registered with the OP, either using the post_logout_redirect_uris Registration parameter or via another mechanism. An id_token_hint is also RECOMMENDED when this parameter is included.

So the "do not redirect on logout" setting is actually defined in OpenIdConfiguration as the endSessionEndpoint. But since this is optional we could not include this parameter if it is not explicitly set. Either way I think this does need some javadoc/documentation to explain this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand it is the redirect after the logout redirect. If there is no endSessionEndpoint, you just redirect to the logout redirect, which you are currently insisting is non null.
But you could allow null and define that to mean no redirecting at all on logout, thus giving the old behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I have changed to logic to not redirect back to application if LOGOUT_REDIRECT_PATH is null.

And updated the javadoc of attemptLogoutRedirect to explain how it works.

else if (!logoutRedirectPath.startsWith("/"))
{
LOG.warn("redirect path must start with /");
logoutRedirectPath = "/" + logoutRedirectPath;
}

_logoutRedirectPath = logoutRedirectPath;
}

public void setErrorPage(String path)
{
if (path == null || path.trim().length() == 0)
Expand Down Expand Up @@ -218,6 +247,7 @@ public UserIdentity login(String username, Object credentials, ServletRequest re
@Override
public void logout(ServletRequest request)
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
{
attemptLogoutRedirect(request);
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
super.logout(request);
HttpServletRequest httpRequest = (HttpServletRequest)request;
HttpSession session = httpRequest.getSession(false);
Expand All @@ -230,6 +260,47 @@ public void logout(ServletRequest request)
session.removeAttribute(SessionAuthentication.__J_AUTHENTICATED);
session.removeAttribute(CLAIMS);
session.removeAttribute(RESPONSE);
session.removeAttribute(ISSUER);
}
}

private void attemptLogoutRedirect(ServletRequest request)
{
try
{
Request baseRequest = Objects.requireNonNull(Request.getBaseRequest(request));
Response baseResponse = baseRequest.getResponse();

StringBuilder redirectUri = new StringBuilder(128);
URIUtil.appendSchemeHostPort(redirectUri, request.getScheme(), request.getServerName(), request.getServerPort());
redirectUri.append(baseRequest.getContextPath());
redirectUri.append(_logoutRedirectPath);

String endSessionEndpoint = _openIdConfiguration.getEndSessionEndpoint();
HttpSession session = baseRequest.getSession(false);
if (endSessionEndpoint == null || session == null)
{
baseResponse.sendRedirect(redirectUri.toString(), true);
return;
}

Object openIdResponse = session.getAttribute(OpenIdAuthenticator.RESPONSE);
if (!(openIdResponse instanceof Map))
{
baseResponse.sendRedirect(redirectUri.toString(), true);
return;
}

@SuppressWarnings("rawtypes")
String idToken = (String)((Map)openIdResponse).get("id_token");
baseResponse.sendRedirect(endSessionEndpoint +
"?id_token_hint=" + UrlEncoded.encodeString(idToken, StandardCharsets.UTF_8) +
"&post_logout_redirect_uri=" + UrlEncoded.encodeString(redirectUri.toString(), StandardCharsets.UTF_8),
true);
}
catch (Throwable t)
{
LOG.warn("failed to redirect to end_session_endpoint", t);
}
}

Expand Down
37 changes: 36 additions & 1 deletion jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ public class OpenIdConfiguration extends ContainerLifeCycle
private static final String CONFIG_PATH = "/.well-known/openid-configuration";
private static final String AUTHORIZATION_ENDPOINT = "authorization_endpoint";
private static final String TOKEN_ENDPOINT = "token_endpoint";
private static final String END_SESSION_ENDPOINT = "end_session_endpoint";
private static final String ISSUER = "issuer";

private final HttpClient httpClient;
Expand All @@ -52,6 +53,7 @@ public class OpenIdConfiguration extends ContainerLifeCycle
private final String authMethod;
private String authEndpoint;
private String tokenEndpoint;
private String endSessionEndpoint;
private boolean authenticateNewUsers = false;

/**
Expand Down Expand Up @@ -97,14 +99,38 @@ public OpenIdConfiguration(@Name("issuer") String issuer,
@Name("clientSecret") String clientSecret,
@Name("authMethod") String authMethod,
@Name("httpClient") HttpClient httpClient)
{
this(issuer, authorizationEndpoint, tokenEndpoint, null, clientId, clientSecret, authorizationEndpoint, httpClient);
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Create an OpenID configuration for a specific OIDC provider.
* @param issuer The URL of the OpenID provider.
* @param authorizationEndpoint the URL of the OpenID provider's authorization endpoint if configured.
* @param tokenEndpoint the URL of the OpenID provider's token endpoint if configured.
* @param endSessionEndpoint the URL of the OpdnID provider's end session endpoint if configured.
* @param clientId OAuth 2.0 Client Identifier valid at the Authorization Server.
* @param clientSecret The client secret known only by the Client and the Authorization Server.
* @param authMethod Authentication method to use with the Token Endpoint.
* @param httpClient The {@link HttpClient} instance to use.
*/
public OpenIdConfiguration(@Name("issuer") String issuer,
@Name("authorizationEndpoint") String authorizationEndpoint,
@Name("tokenEndpoint") String tokenEndpoint,
@Name("endSessionEndpoint") String endSessionEndpoint,
@Name("clientId") String clientId,
@Name("clientSecret") String clientSecret,
@Name("authMethod") String authMethod,
@Name("httpClient") HttpClient httpClient)
{
this.issuer = issuer;
this.clientId = clientId;
this.clientSecret = clientSecret;
this.authEndpoint = authorizationEndpoint;
this.endSessionEndpoint = endSessionEndpoint;
this.tokenEndpoint = tokenEndpoint;
this.httpClient = httpClient != null ? httpClient : newHttpClient();
this.authMethod = authMethod;
this.authMethod = authMethod == null ? "client_secret_post" : authMethod;

if (this.issuer == null)
throw new IllegalArgumentException("Issuer was not configured");
Expand Down Expand Up @@ -140,6 +166,10 @@ protected void processMetadata(Map<String, Object> discoveryDocument)
if (tokenEndpoint == null)
throw new IllegalStateException(TOKEN_ENDPOINT);

// End session endpoint is optional.
if (endSessionEndpoint == null)
endSessionEndpoint = (String)discoveryDocument.get(END_SESSION_ENDPOINT);

// We are lenient and not throw here as some major OIDC providers do not conform to this.
if (!Objects.equals(discoveryDocument.get(ISSUER), issuer))
LOG.warn("The issuer in the metadata is not correct.");
Expand Down Expand Up @@ -213,6 +243,11 @@ public String getTokenEndpoint()
{
return tokenEndpoint;
}

public String getEndSessionEndpoint()
{
return endSessionEndpoint;
}

public String getAuthMethod()
{
Expand Down
12 changes: 9 additions & 3 deletions jetty-openid/src/test/java/org/eclipse/jetty/security/openid/OpenIdAuthenticationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import java.io.IOException;
import java.security.Principal;
import java.util.Map;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
Expand All @@ -35,6 +36,7 @@

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.greaterThanOrEqualTo;
import static org.hamcrest.Matchers.is;

Expand Down Expand Up @@ -155,6 +157,11 @@ public void testLoginLogout() throws Exception
assertThat(response.getStatus(), is(HttpStatus.OK_200));
content = response.getContentAsString();
assertThat(content, containsString("not authenticated"));

// Test that the user was logged out successfully on the openid provider.
assertThat(openIdProvider.getLoggedInUsers().getCurrent(), equalTo(0L));
assertThat(openIdProvider.getLoggedInUsers().getMax(), equalTo(1L));
assertThat(openIdProvider.getLoggedInUsers().getTotal(), equalTo(1L));
}

public static class LoginPage extends HttpServlet
Expand All @@ -171,10 +178,9 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response) t
public static class LogoutPage extends HttpServlet
{
@Override
protected void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException
protected void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
{
request.getSession().invalidate();
response.sendRedirect("/");
request.logout();
}
}

Expand Down
62 changes: 56 additions & 6 deletions jetty-openid/src/test/java/org/eclipse/jetty/security/openid/OpenIdProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.eclipse.jetty.servlet.ServletHolder;
import org.eclipse.jetty.util.StringUtil;
import org.eclipse.jetty.util.component.ContainerLifeCycle;
import org.eclipse.jetty.util.statistic.CounterStatistic;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -48,6 +49,7 @@ public class OpenIdProvider extends ContainerLifeCycle
private static final String CONFIG_PATH = "/.well-known/openid-configuration";
private static final String AUTH_PATH = "/auth";
private static final String TOKEN_PATH = "/token";
private static final String END_SESSION_PATH = "/end_session";
private final Map<String, User> issuedAuthCodes = new HashMap<>();

protected final String clientId;
Expand All @@ -58,6 +60,7 @@ public class OpenIdProvider extends ContainerLifeCycle
private int port = 0;
private String provider;
private User preAuthedUser;
private final CounterStatistic loggedInUsers = new CounterStatistic();

public static void main(String[] args) throws Exception
{
Expand Down Expand Up @@ -91,9 +94,10 @@ public OpenIdProvider(String clientId, String clientSecret)

ServletContextHandler contextHandler = new ServletContextHandler();
contextHandler.setContextPath("/");
contextHandler.addServlet(new ServletHolder(new OpenIdConfigServlet()), CONFIG_PATH);
contextHandler.addServlet(new ServletHolder(new OpenIdAuthEndpoint()), AUTH_PATH);
contextHandler.addServlet(new ServletHolder(new OpenIdTokenEndpoint()), TOKEN_PATH);
contextHandler.addServlet(new ServletHolder(new ConfigServlet()), CONFIG_PATH);
contextHandler.addServlet(new ServletHolder(new AuthEndpoint()), AUTH_PATH);
contextHandler.addServlet(new ServletHolder(new TokenEndpoint()), TOKEN_PATH);
contextHandler.addServlet(new ServletHolder(new EndSessionEndpoint()), END_SESSION_PATH);
server.setHandler(contextHandler);

addBean(server);
Expand All @@ -112,6 +116,11 @@ public OpenIdConfiguration getOpenIdConfiguration()
return new OpenIdConfiguration(provider, authEndpoint, tokenEndpoint, clientId, clientSecret, null);
}

public CounterStatistic getLoggedInUsers()
{
return loggedInUsers;
}

@Override
protected void doStart() throws Exception
{
Expand Down Expand Up @@ -144,7 +153,7 @@ public void addRedirectUri(String uri)
redirectUris.add(uri);
}

public class OpenIdAuthEndpoint extends HttpServlet
public class AuthEndpoint extends HttpServlet
{
@Override
protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException
Expand Down Expand Up @@ -252,7 +261,7 @@ public void redirectUser(HttpServletRequest request, User user, String redirectU
}
}

public class OpenIdTokenEndpoint extends HttpServlet
private class TokenEndpoint extends HttpServlet
{
@Override
protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
Expand Down Expand Up @@ -285,12 +294,44 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws S
"\"token_type\": \"Bearer\"" +
"}";

loggedInUsers.increment();
resp.setContentType("text/plain");
resp.getWriter().print(response);
}
}

public class OpenIdConfigServlet extends HttpServlet
private class EndSessionEndpoint extends HttpServlet
{
@Override
protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException
{
doPost(req, resp);
}

@Override
protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException
{
String idToken = req.getParameter("id_token_hint");
if (idToken == null)
{
resp.sendError(HttpServletResponse.SC_BAD_REQUEST, "no id_token_hint");
return;
}

String logoutRedirect = req.getParameter("post_logout_redirect_uri");
if (logoutRedirect == null)
{
resp.sendError(HttpServletResponse.SC_BAD_REQUEST, "no post_logout_redirect_uri");
return;
}

loggedInUsers.decrement();
resp.setContentType("text/plain");
resp.sendRedirect(logoutRedirect);
github-advanced-security[bot] marked this conversation as resolved.
Show resolved Hide resolved
}
}

private class ConfigServlet extends HttpServlet
{
@Override
protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException
Expand All @@ -299,6 +340,7 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IO
"\"issuer\": \"" + provider + "\"," +
"\"authorization_endpoint\": \"" + provider + AUTH_PATH + "\"," +
"\"token_endpoint\": \"" + provider + TOKEN_PATH + "\"," +
"\"end_session_endpoint\": \"" + provider + END_SESSION_PATH + "\"," +
"}";

resp.getWriter().write(discoveryDocument);
Expand Down Expand Up @@ -336,5 +378,13 @@ public String getIdToken(String provider, String clientId)
long expiry = System.currentTimeMillis() + Duration.ofMinutes(1).toMillis();
return JwtEncoder.createIdToken(provider, clientId, subject, name, expiry);
}

@Override
public boolean equals(Object obj)
{
if (!(obj instanceof User))
return false;
return Objects.equals(subject, ((User)obj).subject) && Objects.equals(name, ((User)obj).name);
}
}
}