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

Validate Tenant annotation is applied before authentication happened and fail if wrong tenant was used to authenticate the HTTP request #40054

Merged
merged 1 commit into from
Apr 28, 2024
Merged

Validate Tenant annotation is applied before authentication happened and fail if wrong tenant was used to authenticate the HTTP request #40054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 25 additions & 0 deletions extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcRecorder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import io.quarkus.runtime.TlsConfig;
import io.quarkus.runtime.annotations.Recorder;
import io.quarkus.runtime.configuration.ConfigurationException;
import io.quarkus.security.AuthenticationFailedException;
import io.quarkus.security.identity.AuthenticationRequestContext;
import io.quarkus.security.identity.SecurityIdentity;
import io.quarkus.security.identity.request.TokenAuthenticationRequest;
Expand Down Expand Up @@ -600,6 +601,30 @@ public Consumer<RoutingContext> apply(String tenantId) {
return new Consumer<RoutingContext>() {
@Override
public void accept(RoutingContext routingContext) {
OidcTenantConfig tenantConfig = routingContext.get(OidcTenantConfig.class.getName());
sberyozkin marked this conversation as resolved.
Show resolved Hide resolved
if (tenantConfig != null) {
// authentication has happened before @Tenant annotation was matched with the HTTP request
String tenantUsedForAuth = tenantConfig.tenantId.orElse(null);
if (tenantId.equals(tenantUsedForAuth)) {
// @Tenant selects the same tenant as already selected
return;
} else {
// @Tenant selects the different tenant than already selected
throw new AuthenticationFailedException(
"""
The '%1$s' selected with the @Tenant annotation must be used to authenticate
the request but it was already authenticated with the '%2$s' tenant. It
can happen if the '%1$s' is selected with an annotation but '%2$s' is
resolved during authentication required by the HTTP Security Policy which
is enforced before the JAX-RS chain is run. In such cases, please set the
'quarkus.http.auth.permission."permissions".applies-to=JAXRS' to all HTTP
Security Policies which secure the same REST endpoints as the ones
where the '%1$s' tenant is resolved by the '@Tenant' annotation.
"""
.formatted(tenantId, tenantUsedForAuth));
}
}

LOG.debugf("@Tenant annotation set a '%s' tenant id on the %s request path", tenantId,
routingContext.request().path());
routingContext.put(OidcUtils.TENANT_ID_SET_BY_ANNOTATION, tenantId);
Expand Down
40 changes: 36 additions & 4 deletions ...rc/test/java/io/quarkus/resteasy/test/security/JakartaRestResourceHttpPermissionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@

import org.jboss.shrinkwrap.api.asset.StringAsset;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

import io.quarkus.security.identity.SecurityIdentity;
import io.quarkus.security.test.utils.TestIdentityController;
import io.quarkus.security.test.utils.TestIdentityProvider;
import io.quarkus.test.QuarkusUnitTest;
Expand All @@ -40,18 +42,23 @@ public class JakartaRestResourceHttpPermissionTest {
"quarkus.http.auth.permission.root.paths=/\n" +
"quarkus.http.auth.permission.root.policy=authenticated\n" +
"quarkus.http.auth.permission.dot.paths=dot,dot/\n" +
"quarkus.http.auth.permission.dot.policy=authenticated\n";
"quarkus.http.auth.permission.dot.policy=authenticated\n" +
"quarkus.http.auth.permission.jax-rs.paths=jax-rs\n" +
"quarkus.http.auth.permission.jax-rs.policy=admin-role\n" +
"quarkus.http.auth.policy.admin-role.roles-allowed=admin";

@RegisterExtension
static QuarkusUnitTest runner = new QuarkusUnitTest()
.withApplicationRoot((jar) -> jar
.addClasses(TestIdentityProvider.class, TestIdentityController.class, ApiResource.class,
RootResource.class, PublicResource.class)
RootResource.class, PublicResource.class, JaxRsResource.class)
.addAsResource(new StringAsset(APP_PROPS), "application.properties"));

@BeforeAll
public static void setup() {
TestIdentityController.resetRoles().add("test", "test", "test");
TestIdentityController.resetRoles()
.add("admin", "admin", "admin")
.add("test", "test", "test");
}

@TestHTTPResource
Expand Down Expand Up @@ -97,13 +104,34 @@ public void testSecuredNotFound(String path) {
assurePathAuthenticated(path, 404);
}

@Test
public void testJaxRsRolesHttpSecurityPolicy() {
// insufficient role, expected admin
assurePath("/jax-rs", 401);
assurePath("///jax-rs///", 401);

assurePath("/jax-rs", 200, "admin", true, "admin:admin");
}

private static String getLastNonEmptySegmentContent(String path) {
while (path.endsWith("/") || path.endsWith(".")) {
path = path.substring(0, path.length() - 1);
}
return path.substring(path.lastIndexOf('/') + 1);
}

@Path("jax-rs")
public static class JaxRsResource {

@Inject
SecurityIdentity identity;

@GET
public String getPrincipalName() {
return identity.getPrincipal().getName();
}
}

@Path("/api")
public static class ApiResource {

Expand Down Expand Up @@ -201,13 +229,17 @@ private void assurePathAuthenticated(String path, String body) {
}

private void assurePath(String path, int expectedStatusCode, String body, boolean auth) {
assurePath(path, expectedStatusCode, body, auth, "test:test");
}

private void assurePath(String path, int expectedStatusCode, String body, boolean auth, String credentials) {
var httpClient = vertx.createHttpClient();
try {
httpClient
.request(HttpMethod.GET, url.getPort(), url.getHost(), path)
.map(r -> {
if (auth) {
r.putHeader("Authorization", "Basic " + encodeBase64URLSafeString("test:test".getBytes()));
r.putHeader("Authorization", "Basic " + encodeBase64URLSafeString(credentials.getBytes()));
}
return r;
})
Expand Down
40 changes: 36 additions & 4 deletions ...quarkus/resteasy/reactive/server/test/security/JakartaRestResourceHttpPermissionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

import io.quarkus.security.identity.SecurityIdentity;
import io.quarkus.security.test.utils.TestIdentityController;
import io.quarkus.security.test.utils.TestIdentityProvider;
import io.quarkus.test.QuarkusUnitTest;
Expand All @@ -40,19 +42,24 @@ public class JakartaRestResourceHttpPermissionTest {
"quarkus.http.auth.permission.root.paths=/\n" +
"quarkus.http.auth.permission.root.policy=authenticated\n" +
"quarkus.http.auth.permission.fragment.paths=/#stuff,/#stuff/\n" +
"quarkus.http.auth.permission.fragment.policy=authenticated\n";
"quarkus.http.auth.permission.fragment.policy=authenticated\n" +
"quarkus.http.auth.permission.jax-rs.paths=jax-rs\n" +
"quarkus.http.auth.permission.jax-rs.policy=admin-role\n" +
"quarkus.http.auth.policy.admin-role.roles-allowed=admin";
private static WebClient client;

@RegisterExtension
static QuarkusUnitTest runner = new QuarkusUnitTest()
.withApplicationRoot((jar) -> jar
.addClasses(TestIdentityProvider.class, TestIdentityController.class, ApiResource.class,
RootResource.class, PublicResource.class)
RootResource.class, PublicResource.class, JaxRsResource.class)
.addAsResource(new StringAsset(APP_PROPS), "application.properties"));

@BeforeAll
public static void setup() {
TestIdentityController.resetRoles().add("test", "test", "test");
TestIdentityController.resetRoles()
.add("admin", "admin", "admin")
.add("test", "test", "test");
}

@AfterAll
Expand Down Expand Up @@ -106,13 +113,34 @@ public void testNotSecuredPaths(String path) {
assurePathAuthenticated(path);
}

@Test
public void testJaxRsRolesHttpSecurityPolicy() {
// insufficient role, expected admin
assurePath("/jax-rs", 401);
assurePath("///jax-rs///", 401);

assurePath("/jax-rs", 200, "admin", true, "admin");
}

private static String getLastNonEmptySegmentContent(String path) {
while (path.endsWith("/") || path.endsWith(".")) {
path = path.substring(0, path.length() - 1);
}
return path.substring(path.lastIndexOf('/') + 1);
}

@Path("jax-rs")
public static class JaxRsResource {

@Inject
SecurityIdentity identity;

@GET
public String getPrincipalName() {
return identity.getPrincipal().getName();
}
}

@Path("/api")
public static class ApiResource {

Expand Down Expand Up @@ -212,9 +240,13 @@ private void assurePathAuthenticated(String path, String body) {
}

private void assurePath(String path, int expectedStatusCode, String body, boolean auth) {
assurePath(path, expectedStatusCode, body, auth, "test");
}

private void assurePath(String path, int expectedStatusCode, String body, boolean auth, String username) {
var req = getClient().get(url.getPort(), url.getHost(), path);
if (auth) {
req.basicAuthentication("test", "test");
req.basicAuthentication(username, username);
}
var result = req.send();
await().atMost(REQUEST_TIMEOUT).until(result::isComplete);
Expand Down
13 changes: 13 additions & 0 deletions integration-tests/oidc-wiremock/src/main/java/io/quarkus/it/keycloak/TenantEchoResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,19 @@ public String getHrTenantIdentityAugmentation() {
return getTenantInternal();
}

@Path("/http-security-policy-applies-all-diff")
@GET
public String httpSecurityPolicyAppliesAllDiff() {
throw new IllegalStateException("An exception should have been thrown because authentication happened" +
" before Tenant was selected with the @Tenant annotation");
}

@Path("/http-security-policy-applies-all-same")
@GET
public String httpSecurityPolicyAppliesAllSame() {
return getTenantInternal();
}

private String getTenantInternal() {
return OidcUtils.TENANT_ID_ATTRIBUTE + "=" + routingContext.get(OidcUtils.TENANT_ID_ATTRIBUTE)
+ ", static.tenant.id=" + routingContext.get("static.tenant.id")
Expand Down
62 changes: 47 additions & 15 deletions ...n-tests/oidc-wiremock/src/test/java/io/quarkus/it/keycloak/AnnotationBasedTenantTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ public Map<String, String> getConfigOverrides() {
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"),
Map.entry("quarkus.oidc.hr.tenant-paths", "/api/tenant-echo/http-security-policy-applies-all-same"),
Map.entry("quarkus.oidc.hr.token.audience", "http://hr.service"),
Map.entry("quarkus.http.auth.policy.roles1.roles-allowed", "role1"),
Map.entry("quarkus.http.auth.policy.roles2.roles-allowed", "role2"),
Expand Down Expand Up @@ -59,7 +60,11 @@ public Map<String, String> getConfigOverrides() {
Map.entry("quarkus.http.auth.permission.identity-augmentation.paths",
"/api/tenant-echo/hr-identity-augmentation"),
Map.entry("quarkus.http.auth.permission.identity-augmentation.policy", "roles3"),
Map.entry("quarkus.http.auth.permission.identity-augmentation.applies-to", "JAXRS"));
Map.entry("quarkus.http.auth.permission.identity-augmentation.applies-to", "JAXRS"),
Map.entry("quarkus.http.auth.permission.tenant-annotation-applies-all.paths",
"/api/tenant-echo/http-security-policy-applies-all-diff,/api/tenant-echo/http-security-policy-applies-all-same"),
Map.entry("quarkus.http.auth.permission.tenant-annotation-applies-all.policy", "admin-role"),
Map.entry("quarkus.http.auth.policy.admin-role.roles-allowed", "admin"));
}
}

Expand Down Expand Up @@ -204,9 +209,7 @@ public void testClassicHttpSecurityPolicyWithRbac() {
token = getTokenWithRole("role1");
RestAssured.given().auth().oauth2(token)
.when().get("/api/tenant-echo2/hr-classic-perm-check")
.then().statusCode(200)
.body(Matchers.equalTo(("tenant-id=hr, static.tenant.id=null, name=alice, "
+ OidcUtils.TENANT_ID_SET_BY_ANNOTATION + "=hr")));
.then().statusCode(401);

token = getTokenWithRole("wrong-role");
RestAssured.given().auth().oauth2(token)
Expand Down Expand Up @@ -239,21 +242,18 @@ public void testJaxRsAndClassicHttpSecurityPolicyNoRbac() {
token = getTokenWithRole("role2");
RestAssured.given().auth().oauth2(token)
.when().get("/api/tenant-echo/hr-classic-and-jaxrs-perm-check")
.then().statusCode(403);
.then().statusCode(401);

// roles allowed security check (created for @RolesAllowed) fails over missing role "role3"
token = getTokenWithRole("role2", "role1");
RestAssured.given().auth().oauth2(token)
.when().get("/api/tenant-echo/hr-classic-and-jaxrs-perm-check")
.then().statusCode(403);
.then().statusCode(401);

token = getTokenWithRole("role3", "role2", "role1");
RestAssured.given().auth().oauth2(token)
.when().get("/api/tenant-echo/hr-classic-and-jaxrs-perm-check")
.then().statusCode(200)
// static tenant is null as the permission check "combined-part1" happened before @Tenant
.body(Matchers.equalTo(("tenant-id=hr, static.tenant.id=null, name=alice, "
+ OidcUtils.TENANT_ID_SET_BY_ANNOTATION + "=hr")));
.then().statusCode(401);
} finally {
server.stop();
}
Expand Down Expand Up @@ -282,15 +282,12 @@ public void testJaxRsAndClassicHttpSecurityPolicyWithRbac() {
token = getTokenWithRole("role2");
RestAssured.given().auth().oauth2(token)
.when().get("/api/tenant-echo2/hr-classic-and-jaxrs-perm-check")
.then().statusCode(403);
.then().statusCode(401);

token = getTokenWithRole("role2", "role1");
RestAssured.given().auth().oauth2(token)
.when().get("/api/tenant-echo2/hr-classic-and-jaxrs-perm-check")
.then().statusCode(200)
// static tenant is null as the permission check "combined-part1" happened before @Tenant
.body(Matchers.equalTo(("tenant-id=hr, static.tenant.id=null, name=alice, "
+ OidcUtils.TENANT_ID_SET_BY_ANNOTATION + "=hr")));
.then().statusCode(401);
} finally {
server.stop();
}
Expand Down Expand Up @@ -325,6 +322,31 @@ public void testJaxRsIdentityAugmentation() {
}
}

@Test
public void testPolicyAppliedBeforeTenantAnnotationMatched() {
WiremockTestResource server = new WiremockTestResource();
server.start();
try {
// policy applied before @Tenant annotation has been matched and different tenant has been used for auth
// than the one that @Tenant annotation selects
var token = getNonHrTenantAccessToken(Set.of("admin"));
RestAssured.given().auth().oauth2(token)
.when().get("/api/tenant-echo/http-security-policy-applies-all-diff")
.then().statusCode(401);

// policy applied before @Tenant annotation has been matched and different tenant has been used for auth
// than the one that @Tenant annotation selects
token = getTokenWithRole("admin");
RestAssured.given().auth().oauth2(token)
.when().get("/api/tenant-echo/http-security-policy-applies-all-same")
.then().statusCode(200)
.body(Matchers
.equalTo("tenant-id=null, static.tenant.id=hr, name=alice, tenant-id-set-by-annotation=null"));
} finally {
server.stop();
}
}

private static String getTokenWithRole(String... roles) {
return Jwt.preferredUserName("alice")
.groups(Set.of(roles))
Expand All @@ -333,4 +355,14 @@ private static String getTokenWithRole(String... roles) {
.keyId("1")
.sign("privateKey.jwk");
}

private String getNonHrTenantAccessToken(Set<String> groups) {
return Jwt.preferredUserName("alice")
.groups(groups)
.issuer("https://server.example.com")
.audience("https://service.example.com")
.jws()
.keyId("1")
.sign();
}
}