diff --git a/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java b/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java index d3006c294..98d007776 100644 --- a/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java +++ b/src/main/java/org/ohdsi/webapi/security/authc/OidcAuthConfig.java @@ -2,6 +2,10 @@ import java.io.IOException; import java.net.URI; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import java.time.Duration; import java.util.ArrayList; import java.util.LinkedHashSet; import java.util.List; @@ -27,8 +31,11 @@ import org.springframework.security.oauth2.jwt.Jwt; import org.springframework.security.oauth2.jwt.JwtDecoder; import org.springframework.security.oauth2.jwt.JwtException; +import org.springframework.security.oauth2.jwt.JwtValidators; import org.springframework.security.oauth2.jwt.NimbusJwtDecoder; +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.ObjectMapper; import com.nimbusds.jose.JOSEObjectType; import com.nimbusds.jose.proc.DefaultJOSEObjectTypeVerifier; import org.springframework.security.web.SecurityFilterChain; @@ -63,6 +70,15 @@ public class OidcAuthConfig { @Value("${security.auth.oidc.externalUrl:}") private String externalUrl; + // Base URL that serves the same provider as the issuer but is reachable from + // inside the deployment. The issuer a provider advertises has to stay the one + // its tokens carry, which in a split-horizon deployment is a public address the + // server itself cannot resolve; discovery and JWKS are then fetched here while + // the public issuer remains what tokens are validated against. Blank keeps the + // plain behaviour of resolving everything from the issuer. + @Value("${security.auth.oidc.internalUrl:}") + private String internalUrl; + @Value("${security.auth.oidc.extraScopes:}") private String extraScopes; @@ -96,9 +112,18 @@ public ClientRegistrationRepository oidcClientRegistrationRepository() { return registrationId -> null; } String issuer = stripDiscoverySuffix(discoveryOrIssuerUrl); - log.info("OIDC: Discovering provider metadata from issuer {}", issuer); - - ClientRegistration.Builder builder = ClientRegistrations.fromIssuerLocation(issuer) + String internalBase = normaliseBase(internalUrl); + + ClientRegistration.Builder builder; + if (internalBase == null) { + log.info("OIDC: Discovering provider metadata from issuer {}", issuer); + builder = ClientRegistrations.fromIssuerLocation(issuer); + } else { + log.info("OIDC: Discovering provider metadata for issuer {} via internal base {}", issuer, internalBase); + builder = ClientRegistrations.fromOidcConfiguration( + internalConfiguration(issuer, internalBase)); + } + builder .registrationId("openid") .clientId(clientId) .clientSecret(clientSecret) @@ -196,6 +221,68 @@ private Set buildScopes() { return scopes; } + // Fetches the discovery document from the internally reachable base and points + // the endpoints the server itself calls back at that base. The issuer and the + // endpoints a browser is sent to are deliberately left on their public + // addresses: rewriting those would put an unreachable host in front of the user + // and break issuer validation. + private static Map internalConfiguration(String issuer, String internalBase) { + Map configuration = fetchConfiguration(internalBase + DISCOVERY_SUFFIX); + for (String endpoint : List.of("token_endpoint", "jwks_uri", "userinfo_endpoint")) { + Object value = configuration.get(endpoint); + if (value instanceof String url) { + configuration.put(endpoint, toInternal(url, issuer, internalBase)); + } + } + return configuration; + } + + private static Map fetchConfiguration(String url) { + try { + HttpResponse response = HttpClient.newBuilder() + // Ask for HTTP/1.1 outright. The default attempts an HTTP/2 upgrade, + // which a provider that only speaks 1.1 answers with a 502 rather than + // by declining the upgrade -- discovery then fails against a provider + // that is serving the document perfectly well. + .version(HttpClient.Version.HTTP_1_1) + .connectTimeout(Duration.ofSeconds(10)) + .build() + .send(HttpRequest.newBuilder(URI.create(url)) + .timeout(Duration.ofSeconds(10)) + .GET() + .build(), + HttpResponse.BodyHandlers.ofString()); + if (response.statusCode() != 200) { + throw new IllegalStateException( + "OIDC discovery at " + url + " returned HTTP " + response.statusCode()); + } + return new ObjectMapper().readValue(response.body(), new TypeReference>() {}); + } catch (IOException e) { + throw new IllegalStateException("Could not read OIDC discovery from " + url, e); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException("Interrupted reading OIDC discovery from " + url, e); + } + } + + private static String toInternal(String url, String issuer, String internalBase) { + return url.startsWith(issuer) ? internalBase + url.substring(issuer.length()) : url; + } + + private static String normaliseBase(String url) { + if (url == null || url.isBlank()) { + return null; + } + String base = url.trim(); + if (base.endsWith(DISCOVERY_SUFFIX)) { + base = base.substring(0, base.length() - DISCOVERY_SUFFIX.length()); + } + while (base.endsWith("/")) { + base = base.substring(0, base.length() - 1); + } + return base; + } + private String stripDiscoverySuffix(String url) { if (url == null || url.isBlank()) { throw new IllegalStateException("security.auth.oidc.url must be configured when OIDC is enabled"); @@ -287,6 +374,7 @@ public OpenidDirect( org.ohdsi.webapi.security.authz.mapping.ExternalRoleMapService externalRoleMapService, @Value("${security.auth.oidc.enabled:false}") boolean enabled, @Value("${security.auth.oidc.url}") String discoveryOrIssuerUrl, + @Value("${security.auth.oidc.internalUrl:}") String internalUrl, @Value("${security.auth.oidc.rolesClaim:}") String rolesClaim, @Value("${security.auth.oidc.rolesToUpperCase:true}") boolean rolesToUpperCase) { this.loginService = loginService; @@ -300,16 +388,38 @@ public OpenidDirect( return; } String issuer = stripDiscoverySuffixStatic(discoveryOrIssuerUrl); - log.info("OIDC direct: building JwtDecoder for issuer {}", issuer); + String internalBase = normaliseBase(internalUrl); // Accept both `JWT` and `at+jwt` (RFC 9068) header types — Logto and other // providers tag access tokens as `at+jwt`, which Spring's default verifier rejects. - this.jwtDecoder = NimbusJwtDecoder.withIssuerLocation(issuer) - .jwtProcessorCustomizer(processor -> processor.setJWSTypeVerifier( - new DefaultJOSEObjectTypeVerifier<>( - JOSEObjectType.JWT, - new JOSEObjectType("at+jwt"), - null))) - .build(); + NimbusJwtDecoder decoder; + if (internalBase == null) { + log.info("OIDC direct: building JwtDecoder for issuer {}", issuer); + decoder = NimbusJwtDecoder.withIssuerLocation(issuer) + .jwtProcessorCustomizer(processor -> processor.setJWSTypeVerifier( + new DefaultJOSEObjectTypeVerifier<>( + JOSEObjectType.JWT, + new JOSEObjectType("at+jwt"), + null))) + .build(); + } else { + // Same split-horizon concession as the registration repository: the keys + // are fetched over the internal base, but tokens are still only accepted + // when they carry the public issuer. + Object jwks = internalConfiguration(issuer, internalBase).get("jwks_uri"); + if (!(jwks instanceof String jwkSetUri)) { + throw new IllegalStateException("OIDC discovery at " + internalBase + " advertised no jwks_uri"); + } + log.info("OIDC direct: building JwtDecoder for issuer {} with keys from {}", issuer, jwkSetUri); + decoder = NimbusJwtDecoder.withJwkSetUri(jwkSetUri) + .jwtProcessorCustomizer(processor -> processor.setJWSTypeVerifier( + new DefaultJOSEObjectTypeVerifier<>( + JOSEObjectType.JWT, + new JOSEObjectType("at+jwt"), + null))) + .build(); + decoder.setJwtValidator(JwtValidators.createDefaultWithIssuer(issuer)); + } + this.jwtDecoder = decoder; } @GetMapping("/user/login/openidDirect") diff --git a/src/test/java/org/ohdsi/webapi/security/authc/GoogleAuthConfigTest.java b/src/test/java/org/ohdsi/webapi/security/authc/GoogleAuthConfigTest.java index 7c8c194c5..cda67ad36 100644 --- a/src/test/java/org/ohdsi/webapi/security/authc/GoogleAuthConfigTest.java +++ b/src/test/java/org/ohdsi/webapi/security/authc/GoogleAuthConfigTest.java @@ -25,24 +25,26 @@ public class GoogleAuthConfigTest { public void googleRedirectUriUsesHttpsWhenForwardedHeadersAreProcessed() throws Exception { OAuth2AuthorizationRequest authorizationRequest = resolveAuthorizationRequest(true); + String expectedRedirectUri = "https://atlas-preview.ohdsi.org/WebAPI/user/oauth/callback/google"; + assertNotNull(authorizationRequest); - assertEquals( - "https://atlas-preview.ohdsi.org/WebAPI/user/oauth/callback/google", - authorizationRequest.getRedirectUri()); - assertTrue(authorizationRequest.getAuthorizationRequestUri().contains( - "redirect_uri=https%3A%2F%2Fatlas-preview.ohdsi.org%2FWebAPI%2Fuser%2Foauth%2Fcallback%2Fgoogle")); + assertEquals(expectedRedirectUri, authorizationRequest.getRedirectUri()); + // ":" and "/" are legal in a query value, so the parameter carries the URI + // as-is rather than percent-encoded. + assertTrue(authorizationRequest.getAuthorizationRequestUri() + .contains("redirect_uri=" + expectedRedirectUri)); } @Test public void googleRedirectUriFallsBackToHttpWhenForwardedHeadersAreIgnored() throws Exception { OAuth2AuthorizationRequest authorizationRequest = resolveAuthorizationRequest(false); + String expectedRedirectUri = "http://atlas-preview.ohdsi.org/WebAPI/user/oauth/callback/google"; + assertNotNull(authorizationRequest); - assertEquals( - "http://atlas-preview.ohdsi.org/WebAPI/user/oauth/callback/google", - authorizationRequest.getRedirectUri()); - assertTrue(authorizationRequest.getAuthorizationRequestUri().contains( - "redirect_uri=http%3A%2F%2Fatlas-preview.ohdsi.org%2FWebAPI%2Fuser%2Foauth%2Fcallback%2Fgoogle")); + assertEquals(expectedRedirectUri, authorizationRequest.getRedirectUri()); + assertTrue(authorizationRequest.getAuthorizationRequestUri() + .contains("redirect_uri=" + expectedRedirectUri)); } private OAuth2AuthorizationRequest resolveAuthorizationRequest(boolean applyForwardedHeaderFilter) throws Exception { @@ -52,7 +54,11 @@ private OAuth2AuthorizationRequest resolveAuthorizationRequest(boolean applyForw request.setRequestURI("/WebAPI/user/login/google"); request.setScheme("http"); request.setServerName("atlas-preview.ohdsi.org"); - request.setServerPort(8080); + // A servlet container derives these from the Host header, so a portless Host + // means the default port for the scheme. Naming a different one here would + // describe a request no container can produce, and the port would then show + // up in the redirect URI. + request.setServerPort(80); request.setSecure(false); request.addHeader("Host", "atlas-preview.ohdsi.org"); request.addHeader("X-Forwarded-Proto", "https"); diff --git a/src/test/java/org/ohdsi/webapi/security/authc/OidcAuthConfigInternalUrlTest.java b/src/test/java/org/ohdsi/webapi/security/authc/OidcAuthConfigInternalUrlTest.java new file mode 100644 index 000000000..b6a63f224 --- /dev/null +++ b/src/test/java/org/ohdsi/webapi/security/authc/OidcAuthConfigInternalUrlTest.java @@ -0,0 +1,125 @@ +package org.ohdsi.webapi.security.authc; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +import com.sun.net.httpserver.HttpServer; +import java.io.OutputStream; +import java.net.InetSocketAddress; +import java.nio.charset.StandardCharsets; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.springframework.security.oauth2.client.registration.ClientRegistration; +import org.springframework.security.oauth2.client.registration.ClientRegistrationRepository; +import org.springframework.test.util.ReflectionTestUtils; + +/** + * A provider whose issuer is a public address the server itself cannot resolve is + * the normal case behind a gateway. These cover that shape: metadata is read over + * the internal base, the endpoints the server calls follow it there, and both the + * issuer and the endpoint a browser is sent to stay public. + */ +public class OidcAuthConfigInternalUrlTest { + + private static final String PUBLIC_ISSUER = "https://gateway.invalid:41100/trex/oidc"; + + private HttpServer server; + private String internalBase; + + @Before + public void startProvider() throws Exception { + server = HttpServer.create(new InetSocketAddress("127.0.0.1", 0), 0); + internalBase = "http://127.0.0.1:" + server.getAddress().getPort() + "/trex/oidc"; + server.createContext("/trex/oidc/.well-known/openid-configuration", exchange -> { + byte[] body = ("{" + + "\"issuer\":\"" + PUBLIC_ISSUER + "\"," + + "\"authorization_endpoint\":\"" + PUBLIC_ISSUER + "/authorize\"," + + "\"token_endpoint\":\"" + PUBLIC_ISSUER + "/token\"," + + "\"userinfo_endpoint\":\"" + PUBLIC_ISSUER + "/userinfo\"," + + "\"jwks_uri\":\"" + PUBLIC_ISSUER + "/.well-known/jwks.json\"," + + "\"end_session_endpoint\":\"" + PUBLIC_ISSUER + "/session/end\"," + + "\"response_types_supported\":[\"code\"]," + + "\"grant_types_supported\":[\"authorization_code\"]," + + "\"subject_types_supported\":[\"public\"]," + + "\"id_token_signing_alg_values_supported\":[\"RS256\"]" + + "}").getBytes(StandardCharsets.UTF_8); + exchange.getResponseHeaders().add("Content-Type", "application/json"); + exchange.sendResponseHeaders(200, body.length); + try (OutputStream out = exchange.getResponseBody()) { + out.write(body); + } + }); + server.start(); + } + + @After + public void stopProvider() { + server.stop(0); + } + + private ClientRegistration buildRegistration(String externalUrl) { + OidcAuthConfig config = new OidcAuthConfig(null, null, null, null); + ReflectionTestUtils.setField(config, "oidcRuntimeEnabled", true); + ReflectionTestUtils.setField(config, "clientId", "d2e-webapi"); + ReflectionTestUtils.setField(config, "clientSecret", "secret"); + ReflectionTestUtils.setField(config, "callbackUi", "https://gateway.invalid:41100/atlas"); + ReflectionTestUtils.setField(config, "extraScopes", ""); + ReflectionTestUtils.setField(config, "discoveryOrIssuerUrl", PUBLIC_ISSUER); + ReflectionTestUtils.setField(config, "internalUrl", internalBase); + ReflectionTestUtils.setField(config, "externalUrl", externalUrl); + + ClientRegistrationRepository repository = config.oidcClientRegistrationRepository(); + ClientRegistration registration = repository.findByRegistrationId("openid"); + assertNotNull("registration was not built", registration); + return registration; + } + + @Test + public void readsMetadataOverTheInternalBaseWithoutTouchingTheIssuerHost() { + ClientRegistration registration = buildRegistration(""); + + // The issuer is what tokens carry, so it has to survive verbatim even though + // nothing was ever fetched from it. + assertEquals(PUBLIC_ISSUER, registration.getProviderDetails().getIssuerUri()); + } + + @Test + public void movesServerSideEndpointsOntoTheInternalBase() { + ClientRegistration.ProviderDetails details = buildRegistration("").getProviderDetails(); + + assertEquals(internalBase + "/token", details.getTokenUri()); + assertEquals(internalBase + "/.well-known/jwks.json", details.getJwkSetUri()); + assertEquals(internalBase + "/userinfo", details.getUserInfoEndpoint().getUri()); + } + + @Test + public void keepsTheBrowserFacingEndpointPublic() { + ClientRegistration.ProviderDetails details = buildRegistration("").getProviderDetails(); + + assertTrue( + "a browser must not be sent to an internal address, but got " + details.getAuthorizationUri(), + details.getAuthorizationUri().startsWith(PUBLIC_ISSUER)); + } + + @Test + public void leavesTheEndSessionEndpointPublic() { + ClientRegistration.ProviderDetails details = buildRegistration("").getProviderDetails(); + + // Logout is a browser redirect like authorization, so it must not be moved + // onto an address only the server can reach. + assertEquals( + PUBLIC_ISSUER + "/session/end", + details.getConfigurationMetadata().get("end_session_endpoint")); + } + + @Test + public void stillRewritesTheBrowserFacingEndpointOntoTheExternalUrl() { + String externalUrl = "https://public.example/trex/oidc"; + + ClientRegistration.ProviderDetails details = buildRegistration(externalUrl).getProviderDetails(); + + assertEquals(externalUrl + "/authorize", details.getAuthorizationUri()); + } +}