diff --git a/backend/src/main/java/com/bakdata/conquery/models/config/auth/JwtPkceVerifyingRealmFactory.java b/backend/src/main/java/com/bakdata/conquery/models/config/auth/JwtPkceVerifyingRealmFactory.java index 11ab2d08a4..18c635192f 100644 --- a/backend/src/main/java/com/bakdata/conquery/models/config/auth/JwtPkceVerifyingRealmFactory.java +++ b/backend/src/main/java/com/bakdata/conquery/models/config/auth/JwtPkceVerifyingRealmFactory.java @@ -10,12 +10,14 @@ import java.util.Map; import java.util.Optional; import java.util.UUID; +import java.util.concurrent.TimeUnit; import java.util.function.BiFunction; import java.util.function.Supplier; import java.util.stream.Collectors; import jakarta.validation.constraints.Min; import jakarta.validation.constraints.NotEmpty; import jakarta.validation.constraints.NotNull; +import jakarta.ws.rs.BadRequestException; import jakarta.ws.rs.client.Client; import jakarta.ws.rs.container.ContainerRequestContext; import jakarta.ws.rs.core.Cookie; @@ -37,6 +39,8 @@ import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.JsonNode; +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; import com.nimbusds.jwt.JWTParser; import com.nimbusds.oauth2.sdk.AccessTokenResponse; import com.nimbusds.oauth2.sdk.AuthorizationCode; @@ -57,7 +61,9 @@ import io.dropwizard.core.setup.Environment; import io.dropwizard.jersey.DropwizardResourceConfig; import io.dropwizard.validation.ValidationMethod; +import lombok.AccessLevel; import lombok.Data; +import lombok.Getter; import lombok.NoArgsConstructor; import lombok.NonNull; import lombok.SneakyThrows; @@ -129,6 +135,13 @@ public class JwtPkceVerifyingRealmFactory implements AuthenticationRealmFactory @JsonIgnore private Supplier> idpConfigurationSupplier; + @JsonIgnore + @Getter(AccessLevel.NONE) + private final Cache pendingAuthorizationRequests = Caffeine.newBuilder() + .maximumSize(10_000) + .expireAfterWrite(10, TimeUnit.MINUTES) + .build(); + /** * Authentication cookie creator for using the Admin API */ @@ -154,6 +167,9 @@ public record IdpConfiguration( @NotEmpty String issuer) { } + record PendingAuthorizationRequest(URI callbackUri, URI returnUri) { + } + public ConqueryAuthenticationRealm createRealm(Environment environment, ConqueryConfig config, AuthorizationController authorizationController) { final List> additionalVerifiers = new ArrayList<>(); @@ -326,12 +342,16 @@ private URI initiateLogin(ContainerRequestContext request) { return null; } JwtPkceVerifyingRealmFactory.IdpConfiguration idpConfiguration = idpConfigurationOpt.get(); + final URI callbackUri = UriBuilder.fromUri(RequestHelper.getRequestURL(request)).path(AdminServlet.ADMIN_UI).build(); + final URI returnUri = toRootRelativeUri(request.getUriInfo().getRequestUri()); + final String state = registerAuthorizationRequest(callbackUri, returnUri); + return UriBuilder.fromUri(idpConfiguration.authorizationEndpoint()) .queryParam("response_type", "code") .queryParam("client_id", client) - .queryParam("redirect_uri", UriBuilder.fromUri(RequestHelper.getRequestURL(request)).path(AdminServlet.ADMIN_UI).build()) + .queryParam("redirect_uri", callbackUri) .queryParam("scope", "openid") - .queryParam("state", UUID.randomUUID()).build(); + .queryParam("state", state).build(); } @@ -346,15 +366,19 @@ private Response checkAndRedeemAuthzCode(ContainerRequestContext request) { return null; } - // Build the original redirect uri (the request uri without the query added by the IDP) - final URI redirectedUri = + // Build the callback URI without the query added by the IDP and compare it to the URI used to initiate this authorization request. + final URI callbackUri = UriBuilder.fromUri(RequestHelper.getRequestURL(request)).replacePath(request.getUriInfo().getAbsolutePath().getPath()).replaceQuery("").build(); - log.trace("Redirect URI: {}", redirectedUri); + final PendingAuthorizationRequest authorizationRequest = validateAndConsumeAuthorizationRequest( + request.getUriInfo().getQueryParameters().getFirst("state"), + callbackUri + ); + log.trace("Redirect URI: {}", authorizationRequest.returnUri()); // Prepare code for exchange with access token final AuthorizationCodeGrant authzGrant = new AuthorizationCodeGrant( new AuthorizationCode(code), - redirectedUri + authorizationRequest.callbackUri() ); // Redeem code @@ -368,7 +392,7 @@ private Response checkAndRedeemAuthzCode(ContainerRequestContext request) { final NewCookie refreshTokenCookie = prepareRefreshTokenCookie(request, tokenResponse); // Let the client call the same uri again, but this time with valid credentials - return prepareRedirectResponse(redirectedUri, accessTokenCookie, refreshTokenCookie); + return prepareRedirectResponse(authorizationRequest.returnUri(), accessTokenCookie, refreshTokenCookie); } @@ -394,7 +418,7 @@ private Response checkAndRedeemRefreshToken(ContainerRequestContext request) { final Cookie accessTokenCookie = prepareAccessTokenCookie(request, tokenResponse); final NewCookie refreshTokenCookie = prepareRefreshTokenCookie(request, tokenResponse); - return prepareRedirectResponse(request.getUriInfo().getRequestUriBuilder().replaceQuery("").build(), accessTokenCookie, refreshTokenCookie); + return prepareRedirectResponse(toRootRelativeUri(request.getUriInfo().getRequestUri()), accessTokenCookie, refreshTokenCookie); } /** @@ -408,6 +432,38 @@ private Response prepareRedirectResponse(URI uri, Cookie accessTokenCookie, NewC .build(); } + String registerAuthorizationRequest(URI callbackUri, URI returnUri) { + final String state = UUID.randomUUID().toString(); + pendingAuthorizationRequests.put(state, new PendingAuthorizationRequest(callbackUri, returnUri)); + return state; + } + + PendingAuthorizationRequest validateAndConsumeAuthorizationRequest(String state, URI callbackUri) { + if (state == null) { + throw new BadRequestException("Authorization callback is missing its state"); + } + + final PendingAuthorizationRequest authorizationRequest = pendingAuthorizationRequests.asMap().remove(state); + if (authorizationRequest == null || !authorizationRequest.callbackUri().equals(callbackUri)) { + throw new BadRequestException("Authorization callback URI does not match the authorization request"); + } + return authorizationRequest; + } + + static URI toRootRelativeUri(URI requestUri) { + if (requestUri == null || !requestUri.isAbsolute()) { + throw new BadRequestException("Request URI must be absolute"); + } + + final String rawPath = requestUri.getRawPath(); + if (rawPath == null || !rawPath.startsWith("/") || rawPath.startsWith("//")) { + throw new BadRequestException("Request URI has an invalid path"); + } + + final String rawQuery = requestUri.getRawQuery(); + return URI.create(rawQuery == null ? rawPath : rawPath + "?" + rawQuery); + } + private Cookie prepareAccessTokenCookie(ContainerRequestContext request, AccessTokenResponse tokenResponse) { com.nimbusds.oauth2.sdk.token.AccessToken accessToken = tokenResponse.getTokens().getAccessToken(); return authCookieCreator.apply(request, accessToken.getValue()); diff --git a/backend/src/test/java/com/bakdata/conquery/models/config/auth/JwtPkceVerifyingRealmFactoryTest.java b/backend/src/test/java/com/bakdata/conquery/models/config/auth/JwtPkceVerifyingRealmFactoryTest.java new file mode 100644 index 0000000000..9b7cbbd1f0 --- /dev/null +++ b/backend/src/test/java/com/bakdata/conquery/models/config/auth/JwtPkceVerifyingRealmFactoryTest.java @@ -0,0 +1,63 @@ +package com.bakdata.conquery.models.config.auth; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.net.URI; + +import jakarta.ws.rs.BadRequestException; + +import org.junit.jupiter.api.Test; + +class JwtPkceVerifyingRealmFactoryTest { + + @Test + void shouldAcceptCallbackUriFromAuthorizationRequest() { + final JwtPkceVerifyingRealmFactory factory = new JwtPkceVerifyingRealmFactory(); + final URI callbackUri = URI.create("https://example.com/admin-ui"); + final URI returnUri = URI.create("/admin-ui/users?filter=active"); + final String state = factory.registerAuthorizationRequest(callbackUri, returnUri); + + assertEquals( + new JwtPkceVerifyingRealmFactory.PendingAuthorizationRequest(callbackUri, returnUri), + factory.validateAndConsumeAuthorizationRequest(state, callbackUri) + ); + } + + @Test + void shouldRejectCallbackUriNotUsedForAuthorizationRequest() { + final JwtPkceVerifyingRealmFactory factory = new JwtPkceVerifyingRealmFactory(); + final String state = factory.registerAuthorizationRequest( + URI.create("https://example.com/admin-ui"), + URI.create("/admin-ui/users") + ); + + assertThrows( + BadRequestException.class, + () -> factory.validateAndConsumeAuthorizationRequest(state, URI.create("https://attacker.example/admin-ui")) + ); + } + + @Test + void shouldRejectMissingOrReusedState() { + final JwtPkceVerifyingRealmFactory factory = new JwtPkceVerifyingRealmFactory(); + final URI callbackUri = URI.create("https://example.com/admin-ui"); + final String state = factory.registerAuthorizationRequest(callbackUri, URI.create("/admin-ui/users")); + factory.validateAndConsumeAuthorizationRequest(state, callbackUri); + + assertThrows(BadRequestException.class, () -> factory.validateAndConsumeAuthorizationRequest(null, callbackUri)); + assertThrows(BadRequestException.class, () -> factory.validateAndConsumeAuthorizationRequest(state, callbackUri)); + } + + @Test + void shouldCreateRootRelativeReturnUri() { + assertEquals( + URI.create("/admin-ui/users/123?tab=permissions"), + JwtPkceVerifyingRealmFactory.toRootRelativeUri(URI.create("https://example.com/admin-ui/users/123?tab=permissions")) + ); + assertThrows( + BadRequestException.class, + () -> JwtPkceVerifyingRealmFactory.toRootRelativeUri(URI.create("https://example.com//attacker.example/path")) + ); + } +}