diff --git a/core/src/main/java/io/confluent/rest/auth/AuthUtil.java b/core/src/main/java/io/confluent/rest/auth/AuthUtil.java index 8567e62269..a826b22485 100644 --- a/core/src/main/java/io/confluent/rest/auth/AuthUtil.java +++ b/core/src/main/java/io/confluent/rest/auth/AuthUtil.java @@ -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(); diff --git a/core/src/test/java/io/confluent/rest/ApplicationServerTest.java b/core/src/test/java/io/confluent/rest/ApplicationServerTest.java index 4c48e0c7ab..4a4abcab73 100644 --- a/core/src/test/java/io/confluent/rest/ApplicationServerTest.java +++ b/core/src/test/java/io/confluent/rest/ApplicationServerTest.java @@ -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 { @@ -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 { diff --git a/core/src/test/java/io/confluent/rest/ApplicationTest.java b/core/src/test/java/io/confluent/rest/ApplicationTest.java index 1b0d416545..ca3e9c1045 100644 --- a/core/src/test/java/io/confluent/rest/ApplicationTest.java +++ b/core/src/test/java/io/confluent/rest/ApplicationTest.java @@ -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 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 diff --git a/core/src/test/java/io/confluent/rest/auth/AuthUtilTest.java b/core/src/test/java/io/confluent/rest/auth/AuthUtilTest.java index 57eac15400..adbb8ba0f1 100644 --- a/core/src/test/java/io/confluent/rest/auth/AuthUtilTest.java +++ b/core/src/test/java/io/confluent/rest/auth/AuthUtilTest.java @@ -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 @@ -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