Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions core/src/main/java/io/confluent/rest/auth/AuthUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,15 @@ private static ConstraintMapping createConstraint(

if (authenticate) {
configureAuthentication(constraint, restConfig);
} else {
// Explicitly mark as ALLOWED rather than leaving Authorization unset (which
// defaults to INHERIT). If this pathSpec exactly matches another already-registered
// constraint (e.g. the global "/*" auth constraint created by
// createGlobalAuthConstraint), ConstraintSecurityHandler merges the two mappings
// together. INHERIT always defers to the other side of that merge, which silently
// discards this "unsecured" mapping's intent whenever a skip path collides with an
// existing pathSpec such as "/*".
constraint.authorization(Constraint.ALLOWED.getAuthorization());
}

final ConstraintMapping mapping = new ConstraintMapping();
Expand Down
61 changes: 61 additions & 0 deletions core/src/test/java/io/confluent/rest/ApplicationServerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,26 @@ private TestRestConfig configBasic() {
return new TestRestConfig(props);
}

private TestRestConfig configBasicWithWildcardSkipPath() {
Properties props = new Properties();
props.put(RestConfig.AUTHENTICATION_METHOD_CONFIG, RestConfig.AUTHENTICATION_METHOD_BASIC);
props.put(RestConfig.AUTHENTICATION_REALM_CONFIG, "c3");
props.put(RestConfig.AUTHENTICATION_ROLES_CONFIG, Collections.singletonList("Administrators"));
props.put(RestConfig.AUTHENTICATION_SKIP_PATHS, "/*");

return new TestRestConfig(props);
}

private TestRestConfig configBasicWithSkipPaths(String skipPaths) {
Properties props = new Properties();
props.put(RestConfig.AUTHENTICATION_METHOD_CONFIG, RestConfig.AUTHENTICATION_METHOD_BASIC);
props.put(RestConfig.AUTHENTICATION_REALM_CONFIG, "c3");
props.put(RestConfig.AUTHENTICATION_ROLES_CONFIG, Collections.singletonList("Administrators"));
props.put(RestConfig.AUTHENTICATION_SKIP_PATHS, skipPaths);

return new TestRestConfig(props);
}

/* Ensure security handlers are confined to a single context */
@Test
public void testSecurityHandlerIsolation() throws Exception {
Expand All @@ -97,6 +117,47 @@ public void testSecurityHandlerIsolation() throws Exception {
assertThat(makeGetRequest( "/app2/resource"), is(Code.UNAUTHORIZED));
}

/*
* Regression test: authentication.skip.paths="/*" must actually disable authentication for
* all paths, even though this skip pathSpec is identical to the hardcoded global auth
* constraint's pathSpec ("/*"). Before the fix in AuthUtil#createConstraint, an unauthenticated
* request here was rejected because Jetty's ConstraintSecurityHandler merges two mappings that
* share the same pathSpec, and the skip mapping's default Authorization.INHERIT deferred to the
* global constraint's real BASIC auth requirement instead of actually allowing the request.
*/
@Test
public void testUnsecuredWildcardSkipPathOverridesGlobalAuthConstraint() throws Exception {
TestApp app1 = new TestApp("/app1");
TestApp app2 = new TestApp(configBasicWithWildcardSkipPath(), "/app2");

server.registerApplication(app1);
server.registerApplication(app2);
server.start();

// app1 has no auth configured at all; sanity check that it stays open.
assertThat(makeGetRequest("/app1/resource"), is(Code.OK));
// app2 has BASIC auth enabled globally, but skips it for "/*": an unauthenticated request
// must still succeed.
assertThat(makeGetRequest("/app2/resource"), is(Code.OK));
}

/*
* Non-wildcard skip path: only the exact configured pathSpec should bypass authentication;
* every other path under the same app must remain protected by the global auth constraint.
*/
@Test
public void testUnsecuredSpecificSkipPathLeavesOtherPathsSecured() throws Exception {
TestApp app = new TestApp(configBasicWithSkipPaths("/resource"), "/app");

server.registerApplication(app);
server.start();

// "/resource" is an explicit skip path: unauthenticated requests must succeed.
assertThat(makeGetRequest("/app/resource"), is(Code.OK));
// "/exception" was not listed as a skip path, so it must still require authentication.
assertThat(makeGetRequest("/app/exception"), is(Code.UNAUTHORIZED));
}

/* Test Exception Mapper isolation */
@Test
public void testExceptionMapperIsolation() throws Exception {
Expand Down
34 changes: 30 additions & 4 deletions core/src/test/java/io/confluent/rest/ApplicationTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -236,13 +236,39 @@ public void testSetUnsecurePathConstraintsWithUnSecure() {
assertThat(mappings.get(0).getPathSpec(), is("/*"));
assertEquals(Constraint.Authorization.KNOWN_ROLE, securityHandler.getConstraintMappings().get(0).getConstraint().getAuthorization());

// Refer to https://javadoc.jetty.org/jetty-12/org/eclipse/jetty/security/Constraint.Authorization.html#INHERIT
// In Jetty 12, when no roles are set and setAuthenticate(false), the authorization is set to INHERIT.
assertThat(mappings.get(1).getPathSpec(), is("/path/1"));
assertEquals(Constraint.Authorization.INHERIT, securityHandler.getConstraintMappings().get(1).getConstraint().getAuthorization());
assertEquals(Constraint.Authorization.ALLOWED, securityHandler.getConstraintMappings().get(1).getConstraint().getAuthorization());

assertThat(mappings.get(2).getPathSpec(), is("/path/2"));
assertEquals(Constraint.Authorization.INHERIT, securityHandler.getConstraintMappings().get(2).getConstraint().getAuthorization());
assertEquals(Constraint.Authorization.ALLOWED, securityHandler.getConstraintMappings().get(2).getConstraint().getAuthorization());
}

@Test
public void testUnsecurePathConstraintWildcardOverridesGlobalConstraint() throws Exception {
// Regression test: authentication.skip.paths="/*" must actually disable authentication for
// all paths, matching the documented behavior of AUTHENTICATION_SKIP_PATHS. Previously, since
// this pathSpec is identical to the hardcoded global auth constraint's pathSpec ("/*"), both
// mappings were merged by ConstraintSecurityHandler into a single entry, and the unsecured
// mapping's Authorization.INHERIT default always deferred to the global constraint's real
// authorization requirement, silently discarding the skip path.
final Map<String, Object> config = ImmutableMap.of(
RestConfig.AUTHENTICATION_METHOD_CONFIG, RestConfig.AUTHENTICATION_METHOD_BASIC,
RestConfig.AUTHENTICATION_REALM_CONFIG, REALM,
RestConfig.AUTHENTICATION_SKIP_PATHS, "/*");

ConstraintSecurityHandler securityHandler = new TestApp(config).createBasicSecurityHandler();
securityHandler.start();

org.eclipse.jetty.server.Request request =
org.mockito.Mockito.mock(org.eclipse.jetty.server.Request.class);
org.mockito.Mockito.when(request.getMethod()).thenReturn("GET");

java.lang.reflect.Method getConstraint = ConstraintSecurityHandler.class
.getDeclaredMethod("getConstraint", String.class, org.eclipse.jetty.server.Request.class);
getConstraint.setAccessible(true);
Constraint constraint = (Constraint) getConstraint.invoke(securityHandler, "/subjects", request);

assertEquals(Constraint.Authorization.ALLOWED, constraint.getAuthorization());
}

@Test
Expand Down
8 changes: 3 additions & 5 deletions core/src/test/java/io/confluent/rest/auth/AuthUtilTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -212,11 +212,10 @@ public void shouldCreateUnsecuredPathConstraints() {
assertThat(mappings.size(), is(2));
assertThat(mappings.get(0).getMethod(), is("*"));
assertThat(mappings.get(0).getPathSpec(), is("/path/1"));
// Refer to https://javadoc.jetty.org/jetty-12/org/eclipse/jetty/security/Constraint.Authorization.html#INHERIT
assertEquals(Constraint.Authorization.INHERIT, mappings.get(0).getConstraint().getAuthorization());
assertEquals(Constraint.Authorization.ALLOWED, mappings.get(0).getConstraint().getAuthorization());
assertThat(mappings.get(1).getMethod(), is("*"));
assertThat(mappings.get(1).getPathSpec(), is("/path/2"));
assertEquals(Constraint.Authorization.INHERIT, mappings.get(1).getConstraint().getAuthorization());
assertEquals(Constraint.Authorization.ALLOWED, mappings.get(1).getConstraint().getAuthorization());
}

@Test
Expand All @@ -229,10 +228,9 @@ public void shouldCreateUnsecuredPathConstraint() {
AuthUtil.createUnsecuredConstraint(config, "/path/*");

// Then:
// Refer to https://javadoc.jetty.org/jetty-12/org/eclipse/jetty/security/Constraint.Authorization.html#INHERIT
assertThat(mappings.getMethod(), is("*"));
assertThat(mappings.getPathSpec(), is("/path/*"));
assertEquals(Constraint.Authorization.INHERIT, mappings.getConstraint().getAuthorization());
assertEquals(Constraint.Authorization.ALLOWED, mappings.getConstraint().getAuthorization());
}

@Test
Expand Down