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
michalvavrik committed Feb 27, 2024
1 parent 8b567ac commit 8fe4233
Show file tree
Hide file tree
Showing 6 changed files with 79 additions and 20 deletions.
34 changes: 30 additions & 4 deletions docs/src/main/asciidoc/security-authorize-web-endpoints-reference.adoc
View file
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 pattern `/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 need 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 '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": 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": 66}}}, "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": 66}}}, "severity": "WARNING"}
If you need to permit access to the `/forbidden/` path, please add new permission with more specific exact path 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.permit1.paths=/forbidden/ <1>
quarkus.http.auth.permission.permit1.policy=permit
----
<1> The `/forbidden/` path is not secured.
====

[[custom-http-security-policy]]
Expand Down
2 changes: 2 additions & 0 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 @@ -33,6 +33,8 @@ public class JakartaRestResourceHttpPermissionTest {
"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.policy=permit\n" +
"quarkus.http.auth.permission.baz-fum-deny.paths=/api/baz/fum/\n" +
"quarkus.http.auth.permission.baz-fum-deny.policy=authenticated\n" +
"quarkus.http.auth.permission.baz-fum.paths=/api/baz/fum*\n" +
"quarkus.http.auth.permission.baz-fum.policy=authenticated\n" +
"quarkus.http.auth.permission.root.paths=/\n" +
Expand Down
2 changes: 2 additions & 0 deletions ...quarkus/resteasy/reactive/server/test/security/JakartaRestResourceHttpPermissionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ public class JakartaRestResourceHttpPermissionTest {
"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.policy=permit\n" +
"quarkus.http.auth.permission.baz-fum-deny.paths=/api/baz/fum/\n" +
"quarkus.http.auth.permission.baz-fum-deny.policy=authenticated\n" +
"quarkus.http.auth.permission.baz-fum.paths=/api/baz/fum*\n" +
"quarkus.http.auth.permission.baz-fum.policy=authenticated\n" +
"quarkus.http.auth.permission.root.paths=/\n" +
Expand Down
4 changes: 2 additions & 2 deletions ...ment/src/test/java/io/quarkus/vertx/http/security/PathMatchingHttpSecurityPolicyTest.java
View file
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
25 changes: 25 additions & 0 deletions ...tp/runtime/src/main/java/io/quarkus/vertx/http/runtime/security/ImmutablePathMatcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,14 @@ public static class ImmutablePathMatcherBuilder<T> {

private static final String STRING_PATH_SEPARATOR = "/";
private final Map<String, T> exactPathMatches = new HashMap<>();
/**
* Exact paths we proactively secure when more specify permissions are not specified.
* For example path for exact path '/api/hello' we add extra pattern for the '/api/hello/'.
* This helps to secure Jakarta REST endpoints by default as both paths may point to the same endpoint there.
* However, we only do that when user didn't declare any permission for the '/api/hello/'.
* This way, user can still forbid access to the `/api/hello' path and permit access to the '/api/hello/' path.
*/
private final Map<String, T> additionalExactPathMatches = new HashMap<>();
private final Map<String, Path<T>> pathsWithWildcard = new HashMap<>();
private BiConsumer<T, T> handlerAccumulator;

Expand Down Expand Up @@ -188,6 +196,9 @@ public void accept(SubstringMatch<T> match1, SubstringMatch<T> match2) {

paths.put(p.path, handler, subPathMatcher);
}
for (var e : additionalExactPathMatches.entrySet()) {
exactPathMatches.putIfAbsent(e.getKey(), e.getValue());
}
int[] lengths = buildLengths(paths.keys());
return new ImmutablePathMatcher<>(defaultHandler, paths.asImmutableMap(), exactPathMatches, lengths,
hasPathWithInnerWildcard);
Expand Down Expand Up @@ -289,6 +300,20 @@ private void addExactPath(final String path, final T handler) {
} else {
exactPathMatches.put(path, handler);
}
// when 'path.equals("/api/hello")' then the other path is '/api/hello/'
final String otherPath;
if (path.endsWith(STRING_PATH_SEPARATOR)) {
if (path.length() == 1) {
// path '/' is only valid option, '' is not allowed
return;
}
// drop path separator
otherPath = path.substring(0, path.length() - 1);
} else {
otherPath = path + STRING_PATH_SEPARATOR;
}
// if key is already present, then we have the right handler into which new ones have already been merged
additionalExactPathMatches.putIfAbsent(otherPath, handler);
}

private static int[] buildLengths(Iterable<String> keys) {
Expand Down
32 changes: 18 additions & 14 deletions ...sions/vertx-http/runtime/src/test/java/io/quarkus/vertx/http/runtime/PathMatcherTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ public void testPrefixPathWithEndingWildcard() {
final Object prefixPathMatcher2 = new Object();
matcher = ImmutablePathMatcher.builder().addPath("/one/two/*", prefixPathMatcher1)
.addPath("/one/two/three", exactPathMatcher1).addPath("/one/two", exactPathMatcher2)
.addPath("/one/two/three*", prefixPathMatcher2).addPath("/one/two/three/four", exactPathMatcher3).build();
.addPath("/one/two/", prefixPathMatcher1).addPath("/one/two/three/", prefixPathMatcher2)
.addPath("/one/two/three*", prefixPathMatcher2).addPath("/one/two/three/four/", prefixPathMatcher2)
.addPath("/one/two/three/four", exactPathMatcher3).build();
assertMatched(matcher, "/one/two/three", exactPathMatcher1);
assertMatched(matcher, "/one/two", exactPathMatcher2);
assertMatched(matcher, "/one/two/three/four", exactPathMatcher3);
Expand Down Expand Up @@ -72,8 +74,10 @@ public void testPrefixPathDefaultHandler() {
final Object prefixPathMatcher1 = new Object();
final Object prefixPathMatcher2 = new Object();
matcher = ImmutablePathMatcher.builder().addPath("/one/two/*", prefixPathMatcher1).addPath("/*", defaultHandler)
.addPath("/one/two/three", exactPathMatcher1).addPath("/one/two", exactPathMatcher2)
.addPath("/one/two/three*", prefixPathMatcher2).addPath("/one/two/three/four", exactPathMatcher3).build();
.addPath("/one/two/three", exactPathMatcher1).addPath("/one/two/three/", prefixPathMatcher2)
.addPath("/one/two", exactPathMatcher2).addPath("/one/two/", prefixPathMatcher1)
.addPath("/one/two/three*", prefixPathMatcher2).addPath("/one/two/three/four", exactPathMatcher3)
.addPath("/one/two/three/four/", prefixPathMatcher2).build();
assertMatched(matcher, "/one/two/three", exactPathMatcher1);
assertMatched(matcher, "/one/two", exactPathMatcher2);
assertMatched(matcher, "/one/two/three/four", exactPathMatcher3);
Expand Down Expand Up @@ -120,7 +124,7 @@ public void testSpecialChars() {
ImmutablePathMatcher<Object> matcher = ImmutablePathMatcher.builder().addPath("/one/two#three", handler2)
.addPath("/one/two?three=four", handler1).addPath("/one/*/three?one\\\\\\=two", handler3)
.addPath("/one/two#three*", handler4).addPath("/*/two#three*", handler5).addPath("/*", HANDLER)
.build();
.addPath("/one/two#three/", handler4).build();
assertMatched(matcher, "/one/two#three", handler2);
assertMatched(matcher, "/one/two?three=four", handler1);
assertMatched(matcher, "/one/any-value/three?one\\\\\\=two", handler3);
Expand All @@ -142,7 +146,7 @@ public void testSpecialChars() {
assertMatched(matcher, "/one1/two#three/christmas!", handler5);
assertMatched(matcher, "/one1/two#thre");
// no default handler
matcher = ImmutablePathMatcher.builder().addPath("/one/two#three", handler2)
matcher = ImmutablePathMatcher.builder().addPath("/one/two#three", handler2).addPath("/one/two#three/", handler4)
.addPath("/one/two?three=four", handler1).addPath("/one/*/three?one\\\\\\=two", handler3)
.addPath("/one/two#three*", handler4).addPath("/*/two#three*", handler5).build();
assertMatched(matcher, "/one/two#three", handler2);
Expand Down Expand Up @@ -181,7 +185,7 @@ public void testInnerWildcardsWithExactMatches() {
.addPath("/one/two/three", handler2).addPath("/one/two/three/four", handler3)
.addPath("/", handler4).addPath("/*", HANDLER).addPath("/one/two/*/four", handler5)
.addPath("/one/*/three/four", handler6).addPath("/*/two/three/four", handler7)
.addPath("/*/two", handler8).build();
.addPath("/*/two", handler8).addPath("/*/two/three/four/", HANDLER).build();
assertMatched(matcher, "/one/two", handler1);
assertMatched(matcher, "/one/two/three", handler2);
assertMatched(matcher, "/one/two/three/four", handler3);
Expand Down Expand Up @@ -216,7 +220,7 @@ public void testInnerWildcardsOnly() {
assertMatched(matcher, "/one/two/three/4/five", handler5);
assertMatched(matcher, "/one/two/three/sergey/five", handler5);
assertMatched(matcher, "/one/two/three/sergey/five-ish");
assertMatched(matcher, "/one/two/three/sergey/five/");
assertMatched(matcher, "/one/two/three/sergey/five/", handler5);
assertMatched(matcher, "/one/two/three/four", handler4);
assertMatched(matcher, "/one/two/3/four", handler4);
assertMatched(matcher, "/one/two/three", handler3);
Expand All @@ -228,11 +232,11 @@ public void testInnerWildcardsOnly() {
assertMatched(matcher, "/ho-hey/two", handler2);
assertMatched(matcher, "/ho-hey/two2");
assertMatched(matcher, "/ho-hey/two2/");
assertMatched(matcher, "/ho-hey/two/");
assertMatched(matcher, "/ho-hey/two/", handler2);
assertMatched(matcher, "/ho-hey/hey-ho/three", handler1);
assertMatched(matcher, "/1/2/three", handler1);
assertMatched(matcher, "/1/two/three", handler1);
assertMatched(matcher, "/1/two/three/");
assertMatched(matcher, "/1/two/three/", handler1);
assertMatched(matcher, "/1/two/three/f");
// no default path handler
matcher = ImmutablePathMatcher.builder().addPath("/*/two", handler2)
Expand All @@ -242,8 +246,8 @@ public void testInnerWildcardsOnly() {
assertMatched(matcher, "/one/two/three/four/five", handler5);
assertMatched(matcher, "/one/two/three/4/five", handler5);
assertMatched(matcher, "/one/two/three/sergey/five", handler5);
assertMatched(matcher, "/one/two/three/sergey/five/", handler5);
assertNotMatched(matcher, "/one/two/three/sergey/five-ish");
assertNotMatched(matcher, "/one/two/three/sergey/five/");
assertMatched(matcher, "/one/two/three/four", handler4);
assertMatched(matcher, "/one/two/3/four", handler4);
assertMatched(matcher, "/one/two/three", handler3);
Expand All @@ -253,13 +257,13 @@ public void testInnerWildcardsOnly() {
assertMatched(matcher, "/two/two", handler2);
assertMatched(matcher, "/2/two", handler2);
assertMatched(matcher, "/ho-hey/two", handler2);
assertMatched(matcher, "/ho-hey/two/", handler2);
assertNotMatched(matcher, "/ho-hey/two2");
assertNotMatched(matcher, "/ho-hey/two2/");
assertNotMatched(matcher, "/ho-hey/two/");
assertMatched(matcher, "/ho-hey/hey-ho/three", handler1);
assertMatched(matcher, "/1/2/three", handler1);
assertMatched(matcher, "/1/two/three", handler1);
assertNotMatched(matcher, "/1/two/three/");
assertMatched(matcher, "/1/two/three/", handler1);
assertNotMatched(matcher, "/1/two/three/f");
}

Expand Down Expand Up @@ -377,7 +381,7 @@ public void testPrefixPathHandlerMerging() {
handler6.add("AgentBrown");
var matcher = ImmutablePathMatcher.<List<String>> builder().handlerAccumulator(List::addAll).addPath("/path*", handler1)
.addPath("/path*", handler2).addPath("/path/*", handler3).addPath("/path/", handler4)
.addPath("/path/*/", handler5).addPath("/*", handler6).build();
.addPath("/path/*/", handler5).addPath("/*", handler6).addPath("/path", handler1).build();
var handler = matcher.match("/path").getValue();
assertNotNull(handler);
assertTrue(handler.contains("Neo"));
Expand Down Expand Up @@ -505,11 +509,11 @@ public void testDefaultHandlerOneInnerWildcard() {
assertMatched(matcher, "/3/one");
assertMatched(matcher, "/4/one");
assertMatched(matcher, "/4/one");
assertMatched(matcher, "/1/one/");
assertNotMatched(matcher, "/");
assertNotMatched(matcher, "/1");
assertNotMatched(matcher, "/1/");
assertNotMatched(matcher, "/1/two");
assertNotMatched(matcher, "/1/one/");
assertNotMatched(matcher, "/1/one1");
assertNotMatched(matcher, "/1/on");
assertNotMatched(matcher, "/1/one/two");
Expand Down

0 comments on commit 8fe4233

Please sign in to comment.