From 1f1c4ae5ada586b6d45fccaf905c20280b9bfa21 Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 26 Jan 2021 09:53:31 +0100 Subject: [PATCH 1/3] Test to reproduce #5909 Test to reproduce #5909 --- .../jetty/security/ConstraintTest.java | 189 +++++++++++------- 1 file changed, 121 insertions(+), 68 deletions(-) diff --git a/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java b/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java index 5353eef9e887..02488e8cff7d 100644 --- a/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java +++ b/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java @@ -31,6 +31,7 @@ import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.function.Consumer; +import java.util.logging.Logger; import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Stream; @@ -74,6 +75,7 @@ import static java.nio.charset.StandardCharsets.ISO_8859_1; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.in; import static org.hamcrest.Matchers.is; @@ -92,9 +94,17 @@ public class ConstraintTest private LocalConnector _connector; private ConstraintSecurityHandler _security; private HttpConfiguration _config; + private Constraint _forbidConstraint; + private Constraint _authAnyRoleConstraint; + private Constraint _authAdminConstraint; + private Constraint _relaxConstraint; + private Constraint _loginPageConstraint; + private Constraint _allowPostConstraint; + private Constraint _confidentialDataConstraint; + private Constraint _anyUserAuthConstraint; @BeforeEach - public void startServer() + public void setupServer() { _server = new Server(); _connector = new LocalConnector(_server); @@ -143,98 +153,80 @@ public Set getKnownRoles() private List getConstraintMappings() { - Constraint constraint0 = new Constraint(); - constraint0.setAuthenticate(true); - constraint0.setName("forbid"); + _forbidConstraint = new Constraint(); + _forbidConstraint.setAuthenticate(true); + _forbidConstraint.setName("forbid"); ConstraintMapping mapping0 = new ConstraintMapping(); mapping0.setPathSpec("/forbid/*"); - mapping0.setConstraint(constraint0); + mapping0.setConstraint(_forbidConstraint); - Constraint constraint1 = new Constraint(); - constraint1.setAuthenticate(true); - constraint1.setName("auth"); - constraint1.setRoles(new String[]{Constraint.ANY_ROLE}); + _authAnyRoleConstraint = new Constraint(); + _authAnyRoleConstraint.setAuthenticate(true); + _authAnyRoleConstraint.setName("auth"); + _authAnyRoleConstraint.setRoles(new String[]{Constraint.ANY_ROLE}); ConstraintMapping mapping1 = new ConstraintMapping(); mapping1.setPathSpec("/auth/*"); - mapping1.setConstraint(constraint1); + mapping1.setConstraint(_authAnyRoleConstraint); - Constraint constraint2 = new Constraint(); - constraint2.setAuthenticate(true); - constraint2.setName("admin"); - constraint2.setRoles(new String[]{"administrator"}); + _authAdminConstraint = new Constraint(); + _authAdminConstraint.setAuthenticate(true); + _authAdminConstraint.setName("admin"); + _authAdminConstraint.setRoles(new String[]{"administrator"}); ConstraintMapping mapping2 = new ConstraintMapping(); mapping2.setPathSpec("/admin/*"); - mapping2.setConstraint(constraint2); + mapping2.setConstraint(_authAdminConstraint); mapping2.setMethod("GET"); - - Constraint constraint3 = new Constraint(); - constraint3.setAuthenticate(false); - constraint3.setName("relax"); + ConstraintMapping mapping2o = new ConstraintMapping(); + mapping2o.setPathSpec("/admin/*"); + mapping2o.setConstraint(_forbidConstraint); + mapping2o.setMethodOmissions(new String[]{"GET"}); + + _relaxConstraint = new Constraint(); + _relaxConstraint.setAuthenticate(false); + _relaxConstraint.setName("relax"); ConstraintMapping mapping3 = new ConstraintMapping(); mapping3.setPathSpec("/admin/relax/*"); - mapping3.setConstraint(constraint3); + mapping3.setConstraint(_relaxConstraint); - Constraint constraint4 = new Constraint(); - constraint4.setAuthenticate(true); - constraint4.setName("loginpage"); - constraint4.setRoles(new String[]{"administrator"}); + _loginPageConstraint = new Constraint(); + _loginPageConstraint.setAuthenticate(true); + _loginPageConstraint.setName("loginpage"); + _loginPageConstraint.setRoles(new String[]{"administrator"}); ConstraintMapping mapping4 = new ConstraintMapping(); mapping4.setPathSpec("/testLoginPage"); - mapping4.setConstraint(constraint4); + mapping4.setConstraint(_loginPageConstraint); - Constraint constraint5 = new Constraint(); - constraint5.setAuthenticate(false); - constraint5.setName("allow forbidden POST"); + _allowPostConstraint = new Constraint(); + _allowPostConstraint.setAuthenticate(false); + _allowPostConstraint.setName("allow forbidden POST"); ConstraintMapping mapping5 = new ConstraintMapping(); mapping5.setPathSpec("/forbid/post"); - mapping5.setConstraint(constraint5); + mapping5.setConstraint(_allowPostConstraint); mapping5.setMethod("POST"); - - Constraint constraint6 = new Constraint(); - constraint6.setAuthenticate(false); - constraint6.setName("data constraint"); - constraint6.setDataConstraint(2); + ConstraintMapping mapping5o = new ConstraintMapping(); + mapping5o.setPathSpec("/forbid/post"); + mapping5o.setConstraint(_forbidConstraint); + mapping5o.setMethodOmissions(new String[]{"POST"}); + + _confidentialDataConstraint = new Constraint(); + _confidentialDataConstraint.setAuthenticate(false); + _confidentialDataConstraint.setName("data constraint"); + _confidentialDataConstraint.setDataConstraint(Constraint.DC_CONFIDENTIAL); ConstraintMapping mapping6 = new ConstraintMapping(); mapping6.setPathSpec("/data/*"); - mapping6.setConstraint(constraint6); + mapping6.setConstraint(_confidentialDataConstraint); - Constraint constraint7 = new Constraint(); - constraint7.setAuthenticate(true); - constraint7.setName("** constraint"); - constraint7.setRoles(new String[]{ + _anyUserAuthConstraint = new Constraint(); + _anyUserAuthConstraint.setAuthenticate(true); + _anyUserAuthConstraint.setName("** constraint"); + _anyUserAuthConstraint.setRoles(new String[]{ Constraint.ANY_AUTH, "user" }); //the "user" role is superfluous once ** has been defined ConstraintMapping mapping7 = new ConstraintMapping(); mapping7.setPathSpec("/starstar/*"); - mapping7.setConstraint(constraint7); + mapping7.setConstraint(_anyUserAuthConstraint); - return Arrays.asList(mapping0, mapping1, mapping2, mapping3, mapping4, mapping5, mapping6, mapping7); - } - - @Test - public void testConstraints() throws Exception - { - List mappings = new ArrayList<>(_security.getConstraintMappings()); - - assertTrue(mappings.get(0).getConstraint().isForbidden()); - assertFalse(mappings.get(1).getConstraint().isForbidden()); - assertFalse(mappings.get(2).getConstraint().isForbidden()); - assertFalse(mappings.get(3).getConstraint().isForbidden()); - - assertFalse(mappings.get(0).getConstraint().isAnyRole()); - assertTrue(mappings.get(1).getConstraint().isAnyRole()); - assertFalse(mappings.get(2).getConstraint().isAnyRole()); - assertFalse(mappings.get(3).getConstraint().isAnyRole()); - - assertFalse(mappings.get(0).getConstraint().hasRole("administrator")); - assertTrue(mappings.get(1).getConstraint().hasRole("administrator")); - assertTrue(mappings.get(2).getConstraint().hasRole("administrator")); - assertFalse(mappings.get(3).getConstraint().hasRole("administrator")); - - assertTrue(mappings.get(0).getConstraint().getAuthenticate()); - assertTrue(mappings.get(1).getConstraint().getAuthenticate()); - assertTrue(mappings.get(2).getConstraint().getAuthenticate()); - assertFalse(mappings.get(3).getConstraint().getAuthenticate()); + return Arrays.asList(mapping0, mapping1, mapping2, mapping2o, mapping3, mapping4, mapping5, mapping5o, mapping6, mapping7); } /** @@ -1798,7 +1790,68 @@ public void testRelaxedMethod() throws Exception assertThat(response, startsWith("HTTP/1.1 200 ")); response = _connector.getResponse("GET /ctx/forbid/post HTTP/1.0\r\n\r\n"); - assertThat(response, startsWith("HTTP/1.1 200 ")); // This is so stupid, but it is the S P E C + assertThat(response, startsWith("HTTP/1.1 403 ")); + } + + @Test + public void testUncoveredMethod() throws Exception + { + ConstraintMapping specificMethod = new ConstraintMapping(); + specificMethod.setMethod("GET"); + specificMethod.setPathSpec("/specific/method"); + specificMethod.setConstraint(_forbidConstraint); + _security.addConstraintMapping(specificMethod); + _security.setAuthenticator(new BasicAuthenticator()); + Logger.getAnonymousLogger().info("Uncovered method for /specific/method is expected"); + _server.start(); + + assertThat(_security.getPathsWithUncoveredHttpMethods(), contains("/specific/method")); + + String response; + response = _connector.getResponse("GET /ctx/specific/method HTTP/1.0\r\n\r\n"); + assertThat(response, startsWith("HTTP/1.1 403 ")); + + response = _connector.getResponse("POST /ctx/specific/method HTTP/1.0\r\n\r\n"); + assertThat(response, startsWith("HTTP/1.1 200 ")); // This is so stupid, but it is the S P E C + } + + @Test + public void testForbidTraceAndOptions() throws Exception + { + ConstraintMapping forbidTrace = new ConstraintMapping(); + forbidTrace.setMethod("TRACE"); + forbidTrace.setPathSpec("/"); + forbidTrace.setConstraint(_forbidConstraint); + ConstraintMapping allowOmitTrace = new ConstraintMapping(); + allowOmitTrace.setMethodOmissions(new String[] {"TRACE"}); + allowOmitTrace.setPathSpec("/"); + allowOmitTrace.setConstraint(_relaxConstraint); + + ConstraintMapping forbidOptions = new ConstraintMapping(); + forbidOptions.setMethod("OPTIONS"); + forbidOptions.setPathSpec("/"); + forbidOptions.setConstraint(_forbidConstraint); + ConstraintMapping allowOmitOptions = new ConstraintMapping(); + allowOmitOptions.setMethodOmissions(new String[] {"OPTIONS"}); + allowOmitOptions.setPathSpec("/"); + allowOmitOptions.setConstraint(_relaxConstraint); + + _security.setConstraintMappings(new ConstraintMapping[] {forbidTrace, allowOmitTrace, forbidOptions, allowOmitOptions}); + + _security.setAuthenticator(new BasicAuthenticator()); + _server.start(); + + assertThat(_security.getPathsWithUncoveredHttpMethods(), Matchers.empty()); + + String response; + response = _connector.getResponse("TRACE /ctx/some/path HTTP/1.0\r\n\r\n"); + assertThat(response, startsWith("HTTP/1.1 403 ")); + + response = _connector.getResponse("OPTIONS /ctx/some/path HTTP/1.0\r\n\r\n"); + assertThat(response, startsWith("HTTP/1.1 403 ")); + + response = _connector.getResponse("GET /ctx/some/path HTTP/1.0\r\n\r\n"); + assertThat(response, startsWith("HTTP/1.1 200 ")); } private static String authBase64(String authorization) From cc4b6ec400cf5a1320a8fa0d63f43f4a7ab8c0df Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 26 Jan 2021 10:41:06 +0100 Subject: [PATCH 2/3] Fix #5909 Better handle merged RoleInfo Fix #5909 Better handle merged RoleInfo --- .../org/eclipse/jetty/security/RoleInfo.java | 30 ++++++++++--------- .../jetty/security/ConstraintTest.java | 22 ++++++++++---- 2 files changed, 32 insertions(+), 20 deletions(-) diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/RoleInfo.java b/jetty-security/src/main/java/org/eclipse/jetty/security/RoleInfo.java index f8ba12f23ac4..2b774283d924 100644 --- a/jetty-security/src/main/java/org/eclipse/jetty/security/RoleInfo.java +++ b/jetty-security/src/main/java/org/eclipse/jetty/security/RoleInfo.java @@ -41,7 +41,7 @@ public class RoleInfo /** * List of permitted roles */ - private final Set _roles = new CopyOnWriteArraySet(); + private final Set _roles = new CopyOnWriteArraySet<>(); public RoleInfo() { @@ -140,26 +140,28 @@ public void combine(RoleInfo other) { if (other._forbidden) setForbidden(true); - else if (!other._checked) // TODO is this the right way around??? - setChecked(true); - else if (other._isAnyRole) - setAnyRole(true); - else if (other._isAnyAuth) - setAnyAuth(true); - else if (!_isAnyRole) + else if (other._checked) { - for (String r : other._roles) - { - _roles.add(r); - } + setChecked(true); + if (other._isAnyRole) + setAnyRole(true); + else if (other._isAnyAuth) + setAnyAuth(true); + else if (!_isAnyRole) + _roles.addAll(other._roles); } - setUserDataConstraint(other._userDataConstraint); } @Override public String toString() { - return "{RoleInfo" + (_forbidden ? ",F" : "") + (_checked ? ",C" : "") + (_isAnyRole ? ",*" : _roles) + (_userDataConstraint != null ? "," + _userDataConstraint : "") + "}"; + return String.format("RoleInfo@%x{%s%s%s%s,%s}", + hashCode(), + (_forbidden ? "Forbidden," : ""), + (_checked ? "Checked," : ""), + (_isAnyAuth ? "AnyAuth," : ""), + (_isAnyRole ? "*" : _roles), + _userDataConstraint); } } diff --git a/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java b/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java index 02488e8cff7d..a686581f3e48 100644 --- a/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java +++ b/jetty-security/src/test/java/org/eclipse/jetty/security/ConstraintTest.java @@ -99,7 +99,7 @@ public class ConstraintTest private Constraint _authAdminConstraint; private Constraint _relaxConstraint; private Constraint _loginPageConstraint; - private Constraint _allowPostConstraint; + private Constraint _noAuthConstraint; private Constraint _confidentialDataConstraint; private Constraint _anyUserAuthConstraint; @@ -196,12 +196,12 @@ private List getConstraintMappings() mapping4.setPathSpec("/testLoginPage"); mapping4.setConstraint(_loginPageConstraint); - _allowPostConstraint = new Constraint(); - _allowPostConstraint.setAuthenticate(false); - _allowPostConstraint.setName("allow forbidden POST"); + _noAuthConstraint = new Constraint(); + _noAuthConstraint.setAuthenticate(false); + _noAuthConstraint.setName("allow forbidden"); ConstraintMapping mapping5 = new ConstraintMapping(); mapping5.setPathSpec("/forbid/post"); - mapping5.setConstraint(_allowPostConstraint); + mapping5.setConstraint(_noAuthConstraint); mapping5.setMethod("POST"); ConstraintMapping mapping5o = new ConstraintMapping(); mapping5o.setPathSpec("/forbid/post"); @@ -1836,7 +1836,11 @@ public void testForbidTraceAndOptions() throws Exception allowOmitOptions.setPathSpec("/"); allowOmitOptions.setConstraint(_relaxConstraint); - _security.setConstraintMappings(new ConstraintMapping[] {forbidTrace, allowOmitTrace, forbidOptions, allowOmitOptions}); + ConstraintMapping someConstraint = new ConstraintMapping(); + someConstraint.setPathSpec("/some/constaint/*"); + someConstraint.setConstraint(_noAuthConstraint); + + _security.setConstraintMappings(new ConstraintMapping[] {forbidTrace, allowOmitTrace, forbidOptions, allowOmitOptions, someConstraint}); _security.setAuthenticator(new BasicAuthenticator()); _server.start(); @@ -1852,6 +1856,12 @@ public void testForbidTraceAndOptions() throws Exception response = _connector.getResponse("GET /ctx/some/path HTTP/1.0\r\n\r\n"); assertThat(response, startsWith("HTTP/1.1 200 ")); + + response = _connector.getResponse("GET /ctx/some/constraint/info HTTP/1.0\r\n\r\n"); + assertThat(response, startsWith("HTTP/1.1 200 ")); + + response = _connector.getResponse("OPTIONS /ctx/some/constraint/info HTTP/1.0\r\n\r\n"); + assertThat(response, startsWith("HTTP/1.1 403 ")); } private static String authBase64(String authorization) From 2c412aca34f6cc83ede3ea4670c293f725e19b01 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Tue, 2 Feb 2021 17:38:41 +0100 Subject: [PATCH 3/3] Issue #5909 Update after review Signed-off-by: Jan Bartel --- .../main/java/org/eclipse/jetty/security/RoleInfo.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/RoleInfo.java b/jetty-security/src/main/java/org/eclipse/jetty/security/RoleInfo.java index 2b774283d924..eba7b981c6e3 100644 --- a/jetty-security/src/main/java/org/eclipse/jetty/security/RoleInfo.java +++ b/jetty-security/src/main/java/org/eclipse/jetty/security/RoleInfo.java @@ -143,12 +143,12 @@ public void combine(RoleInfo other) else if (other._checked) { setChecked(true); + if (other._isAnyAuth) + setAnyAuth(true); if (other._isAnyRole) setAnyRole(true); - else if (other._isAnyAuth) - setAnyAuth(true); - else if (!_isAnyRole) - _roles.addAll(other._roles); + + _roles.addAll(other._roles); } setUserDataConstraint(other._userDataConstraint); }