Skip to content

Commit

Permalink
Secure HTTP perms exact paths to match Jakarta endpoints
Browse files Browse the repository at this point in the history
  • Loading branch information
michalvavrik committed Feb 26, 2024
1 parent 3be8518 commit 696e088
Show file tree
Hide file tree
Showing 7 changed files with 175 additions and 71 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -65,15 +65,41 @@ quarkus.http.auth.permission.roles1.policy=role-policy1
----
<1> This permission references the default built-in `permit` policy to allow `GET` methods to `/public`.
In this case, the demonstrated setting would not affect this example because this request is allowed anyway.
<2> This permission references the built-in `deny` policy for `/forbidden`.
<2> This permission references the built-in `deny` policy for both `/forbidden` and `/forbidden/` paths.
It is an exact path match because it does not end with `*`.
<3> This permission set references the previously defined policy.
`roles1` is an example name; you can call the permission sets whatever you want.

[WARNING]
[IMPORTANT]
====
The exact path `/forbidden` in the example will not secure the `/forbidden/` path.
It is necessary to add a new exact path for the `/forbidden/` path to ensure proper security coverage.
The exact path `/forbidden` in the example above also secures the `/forbidden/` path.
This way, the `forbidden` endpoint in the example below is secured by the `deny1` permission.
[source,java]
----
package org.acme.crud;
import jakarta.ws.rs.GET;
import jakarta.ws.rs.Path;
@Path("/forbidden")
public class ForbiddenResource {
@GET
public String forbidden() { <1>
return "No!";
}
}
----
<1> Both `/forbidden` and `/forbidden/` paths needs to be secured in order to secure the `forbidden` endpoint.

Check warning on line 93 in docs/src/main/asciidoc/security-authorize-web-endpoints-reference.adoc

View workflow job for this annotation

GitHub Actions / Linting with Vale

[vale] reported by reviewdog 🐶 [Quarkus.Fluff] Depending on the context, consider using 'Rewrite the sentence, or use 'must', instead of' rather than 'needs to'. Raw Output: {"message": "[Quarkus.Fluff] Depending on the context, consider using 'Rewrite the sentence, or use 'must', instead of' rather than 'needs to'.", "location": {"path": "docs/src/main/asciidoc/security-authorize-web-endpoints-reference.adoc", "range": {"start": {"line": 93, "column": 47}}}, "severity": "INFO"}

Check warning on line 93 in docs/src/main/asciidoc/security-authorize-web-endpoints-reference.adoc

View workflow job for this annotation

GitHub Actions / Linting with Vale

[vale] reported by reviewdog 🐶 [Quarkus.Fluff] Depending on the context, consider using 'Be concise: use 'to' rather than' rather than 'in order to'. Raw Output: {"message": "[Quarkus.Fluff] Depending on the context, consider using 'Be concise: use 'to' rather than' rather than 'in order to'.", "location": {"path": "docs/src/main/asciidoc/security-authorize-web-endpoints-reference.adoc", "range": {"start": {"line": 93, "column": 67}}}, "severity": "INFO"}

Check warning on line 93 in docs/src/main/asciidoc/security-authorize-web-endpoints-reference.adoc

View workflow job for this annotation

GitHub Actions / Linting with Vale

[vale] reported by reviewdog 🐶 [Quarkus.TermsWarnings] Consider using 'to' rather than 'in order to' unless updating existing content that uses the term. Raw Output: {"message": "[Quarkus.TermsWarnings] Consider using 'to' rather than 'in order to' unless updating existing content that uses the term.", "location": {"path": "docs/src/main/asciidoc/security-authorize-web-endpoints-reference.adoc", "range": {"start": {"line": 93, "column": 67}}}, "severity": "WARNING"}
If you need to secure only the `/forbidden` path, please add the exclamation mark at the end of the exact path pattern like in the example below:

Check warning on line 95 in docs/src/main/asciidoc/security-authorize-web-endpoints-reference.adoc

View workflow job for this annotation

GitHub Actions / Linting with Vale

[vale] reported by reviewdog 🐶 [Quarkus.Fluff] Depending on the context, consider using 'Rewrite the sentence, or use 'must', instead of' rather than 'need to'. Raw Output: {"message": "[Quarkus.Fluff] Depending on the context, consider using 'Rewrite the sentence, or use 'must', instead of' rather than 'need to'.", "location": {"path": "docs/src/main/asciidoc/security-authorize-web-endpoints-reference.adoc", "range": {"start": {"line": 95, "column": 8}}}, "severity": "INFO"}
[source,properties]
----
quarkus.http.auth.permission.deny1.paths=/forbidden! <1>
quarkus.http.auth.permission.deny1.policy=deny
----
<1> The `/forbidden/` path is not secured.
====

[[custom-http-security-policy]]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public class JakartaRestResourceHttpPermissionTest {
"quarkus.http.auth.permission.foo.policy=authenticated\n" +
"quarkus.http.auth.permission.bar.paths=api/bar*\n" +
"quarkus.http.auth.permission.bar.policy=authenticated\n" +
"quarkus.http.auth.permission.baz-fum-pub.paths=/api/baz/fum\n" +
"quarkus.http.auth.permission.baz-fum-pub.paths=/api/baz/fum!\n" +
"quarkus.http.auth.permission.baz-fum-pub.policy=permit\n" +
"quarkus.http.auth.permission.baz-fum.paths=/api/baz/fum*\n" +
"quarkus.http.auth.permission.baz-fum.policy=authenticated\n" +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public class JakartaRestResourceHttpPermissionTest {
"quarkus.http.auth.permission.foo.policy=authenticated\n" +
"quarkus.http.auth.permission.bar.paths=api/bar*\n" +
"quarkus.http.auth.permission.bar.policy=authenticated\n" +
"quarkus.http.auth.permission.baz-fum-pub.paths=/api/baz/fum\n" +
"quarkus.http.auth.permission.baz-fum-pub.paths=/api/baz/fum!\n" +
"quarkus.http.auth.permission.baz-fum-pub.policy=permit\n" +
"quarkus.http.auth.permission.baz-fum.paths=/api/baz/fum*\n" +
"quarkus.http.auth.permission.baz-fum.policy=authenticated\n" +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,8 @@ public void testHealthCheckPaths(String path) {
public void testMiscellaneousPaths() {
// /api/baz with segment indicating version shouldn't match /api/baz path policy
assurePath("/api/baz;v=1.1", 200);
// /api/baz/ is different resource than secured /api/baz, therefore request should succeed
assurePath("/api/baz/", 200);
// /api/baz/ is different resource than secured /api/baz, but we secure both when there is no exclamation mark
assurePath("/api/baz/", 401);
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.function.Consumer;
import java.util.function.Function;

import jakarta.enterprise.inject.Instance;
Expand All @@ -36,17 +38,17 @@ public class AbstractPathMatchingHttpSecurityPolicy {

private static final String PATH_MATCHING_POLICY_FOUND = AbstractPathMatchingHttpSecurityPolicy.class.getName()
+ ".POLICY_FOUND";
private final ImmutablePathMatcher<List<HttpMatcher>> pathMatcher;
private final List<ImmutablePathMatcher<List<HttpMatcher>>> sharedPermissionsPathMatchers;
private final ImmutablePathMatcher<Set<HttpMatcher>> pathMatcher;
private final List<ImmutablePathMatcher<Set<HttpMatcher>>> sharedPermissionsPathMatchers;
private final boolean hasNoPermissions;

public AbstractPathMatchingHttpSecurityPolicy(Map<String, PolicyMappingConfig> permissions,
Map<String, PolicyConfig> rolePolicy, String rootPath, Instance<HttpSecurityPolicy> installedPolicies,
PolicyMappingConfig.AppliesTo appliesTo) {
boolean hasNoPermissions = permissions.isEmpty();
var namedHttpSecurityPolicies = toNamedHttpSecPolicies(rolePolicy, installedPolicies);
List<ImmutablePathMatcher<List<HttpMatcher>>> sharedPermsMatchers = new ArrayList<>();
final var builder = ImmutablePathMatcher.<List<HttpMatcher>> builder().handlerAccumulator(List::addAll);
List<ImmutablePathMatcher<Set<HttpMatcher>>> sharedPermsMatchers = new ArrayList<>();
final var builder = ImmutablePathMatcher.<Set<HttpMatcher>> builder().handlerAccumulator(Set::addAll);
for (PolicyMappingConfig policyMappingConfig : permissions.values()) {
if (appliesTo != policyMappingConfig.appliesTo) {
continue;
Expand All @@ -55,7 +57,7 @@ public AbstractPathMatchingHttpSecurityPolicy(Map<String, PolicyMappingConfig> p
hasNoPermissions = false;
}
if (policyMappingConfig.shared) {
final var builder1 = ImmutablePathMatcher.<List<HttpMatcher>> builder().handlerAccumulator(List::addAll);
final var builder1 = ImmutablePathMatcher.<Set<HttpMatcher>> builder().handlerAccumulator(Set::addAll);
addPermissionToPathMatcher(namedHttpSecurityPolicies, rootPath, policyMappingConfig, builder1);
sharedPermsMatchers.add(builder1.build());
} else {
Expand All @@ -69,7 +71,7 @@ public AbstractPathMatchingHttpSecurityPolicy(Map<String, PolicyMappingConfig> p

public String getAuthMechanismName(RoutingContext routingContext) {
if (sharedPermissionsPathMatchers != null) {
for (ImmutablePathMatcher<List<HttpMatcher>> matcher : sharedPermissionsPathMatchers) {
for (ImmutablePathMatcher<Set<HttpMatcher>> matcher : sharedPermissionsPathMatchers) {
String authMechanismName = getAuthMechanismName(routingContext, matcher);
if (authMechanismName != null) {
return authMechanismName;
Expand All @@ -90,7 +92,7 @@ public Uni<CheckResult> checkPermission(RoutingContext routingContext, Uni<Secur
permissionCheckers = findPermissionCheckers(routingContext, pathMatcher);
} else {
permissionCheckers = new ArrayList<>();
for (ImmutablePathMatcher<List<HttpMatcher>> matcher : sharedPermissionsPathMatchers) {
for (ImmutablePathMatcher<Set<HttpMatcher>> matcher : sharedPermissionsPathMatchers) {
permissionCheckers.addAll(findPermissionCheckers(routingContext, matcher));
}
permissionCheckers.addAll(findPermissionCheckers(routingContext, pathMatcher));
Expand Down Expand Up @@ -136,8 +138,8 @@ public Uni<? extends CheckResult> apply(CheckResult checkResult) {
}

private static String getAuthMechanismName(RoutingContext routingContext,
ImmutablePathMatcher<List<HttpMatcher>> pathMatcher) {
PathMatch<List<HttpMatcher>> toCheck = pathMatcher.match(routingContext.normalizedPath());
ImmutablePathMatcher<Set<HttpMatcher>> pathMatcher) {
PathMatch<Set<HttpMatcher>> toCheck = pathMatcher.match(routingContext.normalizedPath());
if (toCheck.getValue() == null || toCheck.getValue().isEmpty()) {
return null;
}
Expand All @@ -151,30 +153,35 @@ private static String getAuthMechanismName(RoutingContext routingContext,

private static void addPermissionToPathMatcher(Map<String, HttpSecurityPolicy> permissionCheckers, String rootPath,
PolicyMappingConfig policyMappingConfig,
ImmutablePathMatcher.ImmutablePathMatcherBuilder<List<HttpMatcher>> builder) {
ImmutablePathMatcher.ImmutablePathMatcherBuilder<Set<HttpMatcher>> builder) {
HttpSecurityPolicy checker = permissionCheckers.get(policyMappingConfig.policy);
if (checker == null) {
throw new RuntimeException("Unable to find HTTP security policy " + policyMappingConfig.policy);
}

if (policyMappingConfig.enabled.orElse(Boolean.TRUE)) {
for (String path : policyMappingConfig.paths.orElse(Collections.emptyList())) {
path = path.trim();
if (!path.startsWith("/")) {
path = rootPath + path;
policyMappingConfig.paths.ifPresent(new Consumer<List<String>>() {
@Override
public void accept(List<String> paths) {
for (String path : paths) {
path = path.trim();
if (!path.startsWith("/")) {
path = rootPath + path;
}
HttpMatcher m = new HttpMatcher(policyMappingConfig.authMechanism.orElse(null),
new HashSet<>(policyMappingConfig.methods.orElse(Collections.emptyList())), checker);
Set<HttpMatcher> perms = new HashSet<>();
perms.add(m);
builder.addPath(path, perms);
}
}
HttpMatcher m = new HttpMatcher(policyMappingConfig.authMechanism.orElse(null),
new HashSet<>(policyMappingConfig.methods.orElse(Collections.emptyList())), checker);
List<HttpMatcher> perms = new ArrayList<>();
perms.add(m);
builder.addPath(path, perms);
}
});
}
}

private static List<HttpSecurityPolicy> findPermissionCheckers(RoutingContext context,
ImmutablePathMatcher<List<HttpMatcher>> pathMatcher) {
PathMatch<List<HttpMatcher>> toCheck = pathMatcher.match(context.normalizedPath());
ImmutablePathMatcher<Set<HttpMatcher>> pathMatcher) {
PathMatch<Set<HttpMatcher>> toCheck = pathMatcher.match(context.normalizedPath());
if (toCheck.getValue() == null || toCheck.getValue().isEmpty()) {
return Collections.emptyList();
}
Expand Down Expand Up @@ -375,5 +382,21 @@ static class HttpMatcher {
this.checker = checker;
this.authMechanism = authMechanism;
}

@Override
public boolean equals(Object o) {
if (this == o)
return true;
if (o == null || getClass() != o.getClass())
return false;
HttpMatcher that = (HttpMatcher) o;
return Objects.equals(authMechanism, that.authMechanism) && Objects.equals(methods, that.methods)
&& Objects.equals(checker, that.checker);
}

@Override
public int hashCode() {
return Objects.hash(authMechanism, methods, checker);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
* Handler that dispatches to a given handler based on a match of the path.
*/
public class ImmutablePathMatcher<T> {

private final ImmutableSubstringMap<T> paths;
private final Map<String, T> exactPathMatches;

Expand Down Expand Up @@ -119,7 +118,11 @@ public T getValue() {
}

public static class ImmutablePathMatcherBuilder<T> {

/**
* When exact path pattern ends with the exclamation mark, we must only match exactly that path.
* For example '/api/hello!' means we only match '/api/hello', but not '/api/hello/'.
*/
private static final String EXACT_PATH_ONLY = "!";
private static final String STRING_PATH_SEPARATOR = "/";
private final Map<String, T> exactPathMatches = new HashMap<>();
private final Map<String, Path<T>> pathsWithWildcard = new HashMap<>();
Expand Down Expand Up @@ -229,7 +232,7 @@ private ImmutablePathMatcherBuilder<T> addPath(String originalPath, String path,
}
final int wildcardIdx = path.indexOf('*');
if (wildcardIdx == -1) {
addExactPath(path, handler);
addExactPaths(path, handler);
} else {
addWildcardPath(path, handler, wildcardIdx, originalPath);
}
Expand Down Expand Up @@ -280,6 +283,27 @@ private void addWildcardPath(String path, T handler, int wildcardIdx, String ori
}
}

private void addExactPaths(final String path, final T handler) {
if (path.endsWith(EXACT_PATH_ONLY)) {
// remove ! and match only one path
addExactPath(path.substring(0, path.length() - 1), handler);
} else if (path.length() == 1 && STRING_PATH_SEPARATOR.equals(path)) {
// each path must start with a slash and so must paths used for matching
// hence no need to add double pattern the '/' path
addExactPath(path, handler);
} else {
// secure both '/api/hello' and '/api/hello/'
final String other;
if (path.endsWith(STRING_PATH_SEPARATOR)) {
other = path.substring(0, path.length() - 1);
} else {
other = path + STRING_PATH_SEPARATOR;
}
addExactPath(path, handler);
addExactPath(other, handler);
}
}

private void addExactPath(final String path, final T handler) {
if (path.isEmpty()) {
throw new IllegalArgumentException("Path not specified");
Expand Down
Loading

0 comments on commit 696e088

Please sign in to comment.