Skip to content

Commit

Permalink
Merge pull request quarkusio#40054 from michalvavrik/feature/add-oidc…
Browse files Browse the repository at this point in the history 
…-tenant-annotation-validation

Validate Tenant annotation is applied before authentication happened and fail if wrong tenant was used to authenticate the HTTP request
  • Loading branch information
@sberyozkin
sberyozkin authored Apr 28, 2024
2 parents 1d30716 + 62615a6 commit 79af515
Show file tree
Hide file tree
Showing 5 changed files with 157 additions and 23 deletions.
25 changes: 25 additions & 0 deletions extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcRecorder.java
View file
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());
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
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
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
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
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();
}
}

0 comments on commit 79af515

Please sign in to comment.