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

Secure HTTP Permission exact paths both with/without ending path separator so that Jakarta REST endpoint paths are secured by default #39012

Merged
merged 1 commit into from
Feb 28, 2024
Merged

Secure HTTP Permission exact paths both with/without ending path separator so that Jakarta REST endpoint paths are secured by default #39012

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
34 changes: 30 additions & 4 deletions docs/src/main/asciidoc/security-authorize-web-endpoints-reference.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,15 +65,41 @@
----
<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
Open in desktop
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
Open in desktop
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
Open in desktop
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 not more specific exact path pattern
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
Open in desktop
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
Open in desktop
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
Loading