Skip to content

Commit

Permalink
Merge pull request #39486 from michalvavrik/feature/tweak-user-info-e…
Browse files Browse the repository at this point in the history
…nforcement

Improve OIDC named tenant-specific configuration exceptions and make sure userinfo/token verification is enforced for named tenants
  • Loading branch information
sberyozkin committed Mar 15, 2024
2 parents d1e1161 + 0e80675 commit 0a09340
Show file tree
Hide file tree
Showing 8 changed files with 57 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,8 @@ void detectUserInfoRequired(BeanRegistrationPhaseBuildItem beanRegistrationPhase
if (isInjected(beanRegistrationPhaseBuildItem, USER_INFO_NAME, null)) {
runtimeConfigDefaultProducer.produce(
new RunTimeConfigurationDefaultBuildItem("quarkus.oidc.authentication.user-info-required", "true"));
runtimeConfigDefaultProducer.produce(
new RunTimeConfigurationDefaultBuildItem("quarkus.oidc.*.authentication.user-info-required", "true"));
}
}

Expand All @@ -312,6 +314,8 @@ void detectAccessTokenVerificationRequired(BeanRegistrationPhaseBuildItem beanRe
if (isInjected(beanRegistrationPhaseBuildItem, JSON_WEB_TOKEN_NAME, ID_TOKEN_NAME)) {
runtimeConfigDefaultProducer.produce(
new RunTimeConfigurationDefaultBuildItem("quarkus.oidc.authentication.verify-access-token", "true"));
runtimeConfigDefaultProducer.produce(
new RunTimeConfigurationDefaultBuildItem("quarkus.oidc.*.authentication.verify-access-token", "true"));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ public class UserInfoRequiredDetectionTest {
"""
quarkus.oidc.tenant-paths=/user-info/default-tenant
quarkus.oidc.user-info-path=http://${quarkus.http.host}:${quarkus.http.port}/user-info-endpoint
quarkus.oidc.named.authentication.user-info-required=true
quarkus.oidc.named.auth-server-url=${quarkus.oidc.auth-server-url}
quarkus.oidc.named.tenant-paths=/user-info/named-tenant
quarkus.oidc.named.user-info-path=http://${quarkus.http.host}:${quarkus.http.port}/user-info-endpoint
quarkus.http.auth.proactive=false
"""),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,8 @@ private Uni<TenantConfigContext> createTenantContext(Vertx vertx, OidcTenantConf
.item(new TenantConfigContext(new OidcProvider(null, null, null, null), oidcConfig));
}
}
throw new ConfigurationException("'quarkus.oidc.auth-server-url' property must be configured");
throw new ConfigurationException(
"'" + getConfigPropertyForTenant(tenantId, "auth-server-url") + "' property must be configured");
}
OidcCommonUtils.verifyEndpointUrl(oidcConfig.getAuthServerUrl().get());
OidcCommonUtils.verifyCommonConfiguration(oidcConfig, OidcUtils.isServiceApp(oidcConfig), true);
Expand All @@ -242,10 +243,13 @@ private Uni<TenantConfigContext> createTenantContext(Vertx vertx, OidcTenantConf
if (!oidcConfig.discoveryEnabled.orElse(true)) {
if (!OidcUtils.isServiceApp(oidcConfig)) {
if (!oidcConfig.authorizationPath.isPresent() || !oidcConfig.tokenPath.isPresent()) {
String authorizationPathProperty = getConfigPropertyForTenant(tenantId, "authorization-path");
String tokenPathProperty = getConfigPropertyForTenant(tenantId, "token-path");
throw new ConfigurationException(
"'web-app' applications must have 'authorization-path' and 'token-path' properties "
"'web-app' applications must have '" + authorizationPathProperty + "' and '" + tokenPathProperty
+ "' properties "
+ "set when the discovery is disabled.",
Set.of("quarkus.oidc.authorization-path", "quarkus.oidc.token-path"));
Set.of(authorizationPathProperty, tokenPathProperty));
}
}
// JWK and introspection endpoints have to be set for both 'web-app' and 'service' applications
Expand All @@ -260,31 +264,35 @@ private Uni<TenantConfigContext> createTenantContext(Vertx vertx, OidcTenantConf
}
}
if (oidcConfig.authentication.userInfoRequired.orElse(false) && !oidcConfig.userInfoPath.isPresent()) {
String configProperty = getConfigPropertyForTenant(tenantId, "user-info-path");
throw new ConfigurationException(
"UserInfo is required but 'quarkus.oidc.user-info-path' is not configured.",
Set.of("quarkus.oidc.user-info-path"));
"UserInfo is required but '" + configProperty + "' is not configured.",
Set.of(configProperty));
}
}

if (OidcUtils.isServiceApp(oidcConfig)) {
if (oidcConfig.token.refreshExpired) {
throw new ConfigurationException(
"The 'token.refresh-expired' property can only be enabled for " + ApplicationType.WEB_APP
"The '" + getConfigPropertyForTenant(tenantId, "token.refresh-expired")
+ "' property can only be enabled for " + ApplicationType.WEB_APP
+ " application types");
}
if (!oidcConfig.token.refreshTokenTimeSkew.isEmpty()) {
throw new ConfigurationException(
"The 'token.refresh-token-time-skew' property can only be enabled for " + ApplicationType.WEB_APP
"The '" + getConfigPropertyForTenant(tenantId, "token.refresh-token-time-skew")
+ "' property can only be enabled for " + ApplicationType.WEB_APP
+ " application types");
}
if (oidcConfig.logout.path.isPresent()) {
throw new ConfigurationException(
"The 'logout.path' property can only be enabled for " + ApplicationType.WEB_APP
+ " application types");
"The '" + getConfigPropertyForTenant(tenantId, "logout.path") + "' property can only be enabled for "
+ ApplicationType.WEB_APP + " application types");
}
if (oidcConfig.roles.source.isPresent() && oidcConfig.roles.source.get() == Source.idtoken) {
throw new ConfigurationException(
"The 'roles.source' property can only be set to 'idtoken' for " + ApplicationType.WEB_APP
"The '" + getConfigPropertyForTenant(tenantId, "roles.source")
+ "' property can only be set to 'idtoken' for " + ApplicationType.WEB_APP
+ " application types");
}
} else {
Expand Down Expand Up @@ -320,10 +328,12 @@ private Uni<TenantConfigContext> createTenantContext(Vertx vertx, OidcTenantConf
}

if (!oidcConfig.token.isIssuedAtRequired() && oidcConfig.token.getAge().isPresent()) {
String tokenIssuedAtRequired = getConfigPropertyForTenant(tenantId, "token.issued-at-required");
String tokenAge = getConfigPropertyForTenant(tenantId, "token.age");
throw new ConfigurationException(
"The 'token.issued-at-required' can only be set to false if 'token.age' is not set." +
" Either set 'token.issued-at-required' to true or do not set 'token.age'.",
Set.of("quarkus.oidc.token.issued-at-required", "quarkus.oidc.token.age"));
"The '" + tokenIssuedAtRequired + "' can only be set to false if '" + tokenAge + "' is not set." +
" Either set '" + tokenIssuedAtRequired + "' to true or do not set '" + tokenAge + "'.",
Set.of(tokenIssuedAtRequired, tokenAge));
}

return createOidcProvider(oidcConfig, tlsConfig, vertx)
Expand All @@ -335,6 +345,14 @@ public TenantConfigContext apply(OidcProvider p) {
});
}

private static String getConfigPropertyForTenant(String tenantId, String configSubKey) {
if (DEFAULT_TENANT_ID.equals(tenantId)) {
return "quarkus.oidc." + configSubKey;
} else {
return "quarkus.oidc." + tenantId + "." + configSubKey;
}
}

private static boolean enableUserInfo(OidcTenantConfig oidcConfig) {
Optional<Boolean> userInfoRequired = oidcConfig.authentication.isUserInfoRequired();
if (userInfoRequired.isPresent()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,19 +143,25 @@ quarkus.oidc.tenant-idtoken-only.client-id=quarkus-app
quarkus.oidc.tenant-idtoken-only.credentials.secret=secret
quarkus.oidc.tenant-idtoken-only.token-state-manager.strategy=id-token
quarkus.oidc.tenant-idtoken-only.application-type=web-app
quarkus.oidc.tenant-idtoken-only.authentication.user-info-required=false
quarkus.oidc.tenant-idtoken-only.authentication.verify-access-token=false

quarkus.oidc.tenant-id-refresh-token.auth-server-url=${quarkus.oidc.auth-server-url}
quarkus.oidc.tenant-id-refresh-token.client-id=quarkus-app
quarkus.oidc.tenant-id-refresh-token.credentials.secret=secret
quarkus.oidc.tenant-id-refresh-token.token-state-manager.strategy=id-refresh-tokens
quarkus.oidc.tenant-id-refresh-token.application-type=web-app
quarkus.oidc.tenant-id-refresh-token.authentication.user-info-required=false
quarkus.oidc.tenant-id-refresh-token.authentication.verify-access-token=false

quarkus.oidc.tenant-split-id-refresh-token.auth-server-url=${quarkus.oidc.auth-server-url}
quarkus.oidc.tenant-split-id-refresh-token.client-id=quarkus-app
quarkus.oidc.tenant-split-id-refresh-token.credentials.secret=secret
quarkus.oidc.tenant-split-id-refresh-token.token-state-manager.strategy=id-refresh-tokens
quarkus.oidc.tenant-split-id-refresh-token.token-state-manager.split-tokens=true
quarkus.oidc.tenant-split-id-refresh-token.application-type=web-app
quarkus.oidc.tenant-split-id-refresh-token.authentication.user-info-required=false
quarkus.oidc.tenant-split-id-refresh-token.authentication.verify-access-token=false

quarkus.oidc.tenant-split-tokens.auth-server-url=${quarkus.oidc.auth-server-url}
quarkus.oidc.tenant-split-tokens.client-id=quarkus-app
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@

# Configuration file
quarkus.oidc.auth-server-url=http://localhost:8180/auth/realms/quarkus2/
quarkus.oidc.client-id=quarkus-app
quarkus.oidc.credentials.secret=secret
quarkus.oidc.authentication.scopes=profile,email,phone
quarkus.oidc.authentication.user-info-required=false

quarkus.oidc.no-discovery.auth-server-url=http://localhost:8180/auth/realms/quarkus2/
quarkus.oidc.no-discovery.authentication.user-info-required=false
quarkus.oidc.no-discovery.discovery-enabled=false
quarkus.oidc.no-discovery.jwks-path=protocol/openid-connect/certs
quarkus.oidc.no-discovery.client-id=quarkus-app
Expand All @@ -13,6 +16,8 @@ quarkus.oidc.no-discovery.authentication.scopes=profile,email,phone

quarkus.oidc.code-flow.auth-server-url=${keycloak.url}/realms/quarkus/
quarkus.oidc.code-flow.client-id=quarkus-web-app
quarkus.oidc.code-flow.authentication.user-info-required=false
quarkus.oidc.code-flow.authentication.verify-access-token=false
quarkus.oidc.code-flow.logout.path=/code-flow/logout
quarkus.oidc.code-flow.logout.post-logout-path=/code-flow/post-logout
quarkus.oidc.code-flow.logout.post-logout-uri-param=returnTo
Expand Down Expand Up @@ -40,6 +45,7 @@ quarkus.oidc.code-flow-encrypted-id-token-pem.token.decryption-key-location=priv
quarkus.oidc.code-flow-encrypted-id-token-pem.token.audience=any

quarkus.oidc.code-flow-form-post.auth-server-url=${keycloak.url}/realms/quarkus-form-post/
quarkus.oidc.code-flow-form-post.authentication.user-info-required=false
quarkus.oidc.code-flow-form-post.client-id=quarkus-web-app
quarkus.oidc.code-flow-form-post.credentials.secret=AyM1SysPpbyDfgZld3umj1qzKObwVMkoqQ-EstJQLr_T-1qS0gZH75aKtMN3Yj0iPS4hcgUuTwjAzZr1Z9CAow
quarkus.oidc.code-flow-form-post.application-type=web-app
Expand All @@ -61,6 +67,7 @@ quarkus.oidc.code-flow-user-info-only.authorization-path=/
quarkus.oidc.code-flow-user-info-only.token-path=access_token
quarkus.oidc.code-flow-user-info-only.user-info-path=protocol/openid-connect/userinfo
quarkus.oidc.code-flow-user-info-only.authentication.id-token-required=false
quarkus.oidc.code-flow-user-info-only.authentication.verify-access-token=false
quarkus.oidc.code-flow-user-info-only.code-grant.extra-params.extra-param=extra-param-value
quarkus.oidc.code-flow-user-info-only.code-grant.headers.X-Custom=XCustomHeaderValue
quarkus.oidc.code-flow-user-info-only.client-id=quarkus-web-app
Expand All @@ -69,6 +76,7 @@ quarkus.oidc.code-flow-user-info-only.application-type=web-app

quarkus.oidc.code-flow-user-info-github.provider=github
quarkus.oidc.code-flow-user-info-github.authentication.internal-id-token-lifespan=7H
quarkus.oidc.code-flow-user-info-github.authentication.verify-access-token=false
quarkus.oidc.code-flow-user-info-github.auth-server-url=${keycloak.url}/realms/quarkus/
quarkus.oidc.code-flow-user-info-github.authorization-path=/
quarkus.oidc.code-flow-user-info-github.user-info-path=protocol/openid-connect/userinfo
Expand Down Expand Up @@ -146,6 +154,7 @@ quarkus.oidc.bearer-required-algorithm.credentials.secret=secret
quarkus.oidc.bearer-required-algorithm.token.signature-algorithm=PS256

quarkus.oidc.bearer-azure.provider=microsoft
quarkus.oidc.bearer-azure.authentication.user-info-required=false
quarkus.oidc.bearer-azure.application-type=service
quarkus.oidc.bearer-azure.discovery-enabled=false
quarkus.oidc.bearer-azure.jwks-path=${keycloak.url}/azure/jwk
Expand Down Expand Up @@ -180,6 +189,7 @@ quarkus.oidc.bearer-certificate-full-chain-root-only-wrongcname.certificate-chai
quarkus.oidc.bearer-certificate-full-chain-root-only-wrongcname.certificate-chain.leaf-certificate-name=www.quarkusio.com

quarkus.oidc.bearer-key-without-kid-thumbprint.auth-server-url=${keycloak.url}/realms/quarkus/
quarkus.oidc.bearer-key-without-kid-thumbprint.authentication.user-info-required=false
quarkus.oidc.bearer-key-without-kid-thumbprint.discovery-enabled=false
quarkus.oidc.bearer-key-without-kid-thumbprint.jwks-path=single-key-without-kid-thumbprint
quarkus.oidc.bearer-key-without-kid-thumbprint.client-id=quarkus-app
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public class AnnotationBasedTenantTest {
public static class NoProactiveAuthTestProfile implements QuarkusTestProfile {
public Map<String, String> getConfigOverrides() {
return Map.ofEntries(Map.entry("quarkus.http.auth.proactive", "false"),
Map.entry("quarkus.oidc.authentication.user-info-required", "false"),
Map.entry("quarkus.oidc.hr.authentication.user-info-required", "false"),
Map.entry("quarkus.oidc.hr.auth-server-url", "http://localhost:8180/auth/realms/quarkus2/"),
Map.entry("quarkus.oidc.hr.client-id", "quarkus-app"),
Map.entry("quarkus.oidc.hr.credentials.secret", "secret"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

import java.util.Arrays;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;

import org.hamcrest.Matchers;
Expand All @@ -14,23 +13,13 @@
import org.junit.jupiter.api.TestMethodOrder;

import io.quarkus.test.junit.QuarkusTest;
import io.quarkus.test.junit.QuarkusTestProfile;
import io.quarkus.test.junit.TestProfile;
import io.restassured.RestAssured;
import io.smallrye.jwt.build.Jwt;

@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
@QuarkusTest
@TestProfile(BearerTokenOidcRecoveredTest.DisabledUserInfoTestProfile.class)
public class BearerTokenOidcRecoveredTest {

public static class DisabledUserInfoTestProfile implements QuarkusTestProfile {
@Override
public Map<String, String> getConfigOverrides() {
return Map.of("quarkus.oidc.authentication.user-info-required", "false");
}
}

@Order(1)
@Test
public void testOidcRecoveredWithDiscovery() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ public void testCodeFlowFormPostAndFrontChannelLogout() throws Exception {
@Test
public void testCodeFlowUserInfo() throws Exception {
defineCodeFlowAuthorizationOauth2TokenStub();

wireMockServer.resetRequests();
doTestCodeFlowUserInfo("code-flow-user-info-only", 300);
clearCache();
doTestCodeFlowUserInfo("code-flow-user-info-github", 25200);
Expand Down Expand Up @@ -315,6 +315,7 @@ public void testCodeFlowTokenIntrospection() throws Exception {
private void doTestCodeFlowUserInfo(String tenantId, long internalIdTokenLifetime) throws Exception {
try (final WebClient webClient = createWebClient()) {
webClient.getOptions().setRedirectEnabled(true);
wireMockServer.verify(0, getRequestedFor(urlPathMatching("/auth/realms/quarkus/protocol/openid-connect/userinfo")));
HtmlPage page = webClient.getPage("http://localhost:8081/" + tenantId);

HtmlForm form = page.getFormByName("form");
Expand Down Expand Up @@ -344,6 +345,7 @@ private void doTestCodeFlowUserInfo(String tenantId, long internalIdTokenLifetim
assertTrue(date.toInstant().getEpochSecond() - issuedAt <= internalIdTokenLifetime + 3);

webClient.getCookieManager().clearCookies();
wireMockServer.resetRequests();
}
}

Expand Down

0 comments on commit 0a09340

Please sign in to comment.