From 58f2e5116126fee868b39e268341db0e60c8425f Mon Sep 17 00:00:00 2001 From: Suneet Saldanha Date: Fri, 10 Jul 2020 12:19:10 -0700 Subject: [PATCH] Do not echo back username on auth failure (#10097) * Do not echo back username on auth failure * use bad username * Remove username from exception messages * fix tests * fix the tests * hopefully this time * this time the tests work * fixed this time * fix * upgrade to Jetty 9.4.30 * Unknown users echo back Unauthorized * fix --- .../BasicHTTPAuthenticator.java | 5 +-- .../validator/LDAPCredentialsValidator.java | 2 +- .../MetadataStoreCredentialsValidator.java | 2 +- .../BasicHTTPAuthenticatorTest.java | 6 ++-- .../validator/DBCredentialsValidatorTest.java | 2 +- .../ITBasicAuthConfigurationTest.java | 33 +++++++++++++++---- 6 files changed, 34 insertions(+), 16 deletions(-) diff --git a/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authentication/BasicHTTPAuthenticator.java b/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authentication/BasicHTTPAuthenticator.java index ee2a2ef990c..600af931f03 100644 --- a/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authentication/BasicHTTPAuthenticator.java +++ b/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authentication/BasicHTTPAuthenticator.java @@ -48,7 +48,6 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import java.io.IOException; import java.util.EnumSet; -import java.util.Locale; import java.util.Map; @JsonTypeName("basic") @@ -218,9 +217,7 @@ public class BasicHTTPAuthenticator implements Authenticator } catch (BasicSecurityAuthenticationException ex) { LOG.info("Exception authenticating user %s - %s", user, ex.getMessage()); - httpResp.sendError(HttpServletResponse.SC_UNAUTHORIZED, - String.format(Locale.getDefault(), - "User authentication failed username[%s].", user)); + httpResp.sendError(HttpServletResponse.SC_UNAUTHORIZED, "User authentication failed."); } } diff --git a/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authentication/validator/LDAPCredentialsValidator.java b/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authentication/validator/LDAPCredentialsValidator.java index 7a9bc0c0212..1db8799de38 100644 --- a/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authentication/validator/LDAPCredentialsValidator.java +++ b/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authentication/validator/LDAPCredentialsValidator.java @@ -165,7 +165,7 @@ public class LDAPCredentialsValidator implements CredentialsValidator if (!validatePassword(this.ldapConfig, userDn, password)) { LOG.debug("Password incorrect for LDAP user %s", username); - throw new BasicSecurityAuthenticationException("User LDAP authentication failed username[%s].", userDn.toString()); + throw new BasicSecurityAuthenticationException("User LDAP authentication failed."); } byte[] salt = BasicAuthUtils.generateSalt(); diff --git a/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authentication/validator/MetadataStoreCredentialsValidator.java b/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authentication/validator/MetadataStoreCredentialsValidator.java index 9f86f0d50a3..7d75d0c6805 100644 --- a/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authentication/validator/MetadataStoreCredentialsValidator.java +++ b/extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authentication/validator/MetadataStoreCredentialsValidator.java @@ -83,7 +83,7 @@ public class MetadataStoreCredentialsValidator implements CredentialsValidator return new AuthenticationResult(username, authorizerName, authenticatorName, null); } else { LOG.debug("Password incorrect for metadata store user %s", username); - throw new BasicSecurityAuthenticationException("User metadata store authentication failed username[%s].", username); + throw new BasicSecurityAuthenticationException("User metadata store authentication failed."); } } } diff --git a/extensions-core/druid-basic-security/src/test/java/org/apache/druid/security/authentication/BasicHTTPAuthenticatorTest.java b/extensions-core/druid-basic-security/src/test/java/org/apache/druid/security/authentication/BasicHTTPAuthenticatorTest.java index 52c6356d0bf..84bfdcf56b1 100644 --- a/extensions-core/druid-basic-security/src/test/java/org/apache/druid/security/authentication/BasicHTTPAuthenticatorTest.java +++ b/extensions-core/druid-basic-security/src/test/java/org/apache/druid/security/authentication/BasicHTTPAuthenticatorTest.java @@ -174,7 +174,7 @@ public class BasicHTTPAuthenticatorTest EasyMock.replay(req); HttpServletResponse resp = EasyMock.createMock(HttpServletResponse.class); - resp.sendError(HttpServletResponse.SC_UNAUTHORIZED, "User authentication failed username[userA]."); + resp.sendError(HttpServletResponse.SC_UNAUTHORIZED, "User authentication failed."); EasyMock.expectLastCall().times(1); EasyMock.replay(resp); @@ -210,7 +210,7 @@ public class BasicHTTPAuthenticatorTest validator.validateCredentials(EasyMock.eq("basic"), EasyMock.eq("basic"), EasyMock.eq("userA"), EasyMock.aryEq("badpassword".toCharArray())) ) .andThrow( - new BasicSecurityAuthenticationException("User authentication failed username[%s].", "userA") + new BasicSecurityAuthenticationException("User authentication failed.") ) .times(1); EasyMock.replay(validator); @@ -220,7 +220,7 @@ public class BasicHTTPAuthenticatorTest EasyMock.replay(req); HttpServletResponse resp = EasyMock.createMock(HttpServletResponse.class); - resp.sendError(HttpServletResponse.SC_UNAUTHORIZED, "User authentication failed username[userA]."); + resp.sendError(HttpServletResponse.SC_UNAUTHORIZED, "User authentication failed."); EasyMock.expectLastCall().times(1); EasyMock.replay(resp); diff --git a/extensions-core/druid-basic-security/src/test/java/org/apache/druid/security/authentication/validator/DBCredentialsValidatorTest.java b/extensions-core/druid-basic-security/src/test/java/org/apache/druid/security/authentication/validator/DBCredentialsValidatorTest.java index b8e49426ce9..a981b52f936 100644 --- a/extensions-core/druid-basic-security/src/test/java/org/apache/druid/security/authentication/validator/DBCredentialsValidatorTest.java +++ b/extensions-core/druid-basic-security/src/test/java/org/apache/druid/security/authentication/validator/DBCredentialsValidatorTest.java @@ -143,7 +143,7 @@ public class DBCredentialsValidatorTest String password = "badpassword"; expectedException.expect(BasicSecurityAuthenticationException.class); - expectedException.expectMessage("User metadata store authentication failed username[userA]."); + expectedException.expectMessage("User metadata store authentication failed."); validator.validateCredentials(authenticatorName, authorizerName, username, password.toCharArray()); } } diff --git a/integration-tests/src/test/java/org/apache/druid/tests/security/ITBasicAuthConfigurationTest.java b/integration-tests/src/test/java/org/apache/druid/tests/security/ITBasicAuthConfigurationTest.java index d276acba42a..2a8506f4651 100644 --- a/integration-tests/src/test/java/org/apache/druid/tests/security/ITBasicAuthConfigurationTest.java +++ b/integration-tests/src/test/java/org/apache/druid/tests/security/ITBasicAuthConfigurationTest.java @@ -291,8 +291,8 @@ public class ITBasicAuthConfigurationTest datasourceOnlyUserClient, SYS_SCHEMA_TASKS_QUERY, adminTasks.stream() - .filter((taskEntry) -> "auth_test".equals(taskEntry.get("datasource"))) - .collect(Collectors.toList()) + .filter((taskEntry) -> "auth_test".equals(taskEntry.get("datasource"))) + .collect(Collectors.toList()) ); // as user that can read auth_test and STATE @@ -317,7 +317,8 @@ public class ITBasicAuthConfigurationTest datasourceWithStateUserClient, SYS_SCHEMA_SERVER_SEGMENTS_QUERY, adminServerSegments.stream() - .filter((serverSegmentEntry) -> ((String) serverSegmentEntry.get("segment_id")).contains("auth_test")) + .filter((serverSegmentEntry) -> ((String) serverSegmentEntry.get("segment_id")).contains( + "auth_test")) .collect(Collectors.toList()) ); @@ -326,8 +327,8 @@ public class ITBasicAuthConfigurationTest datasourceWithStateUserClient, SYS_SCHEMA_TASKS_QUERY, adminTasks.stream() - .filter((taskEntry) -> "auth_test".equals(taskEntry.get("datasource"))) - .collect(Collectors.toList()) + .filter((taskEntry) -> "auth_test".equals(taskEntry.get("datasource"))) + .collect(Collectors.toList()) ); // as user that can only read STATE @@ -471,6 +472,26 @@ public class ITBasicAuthConfigurationTest testOptionsRequests(adminClient); } + @Test + public void testMaliciousUser() + { + String maliciousUsername = ""; + HttpClient maliciousClient = new CredentialedHttpClient( + new BasicCredentials(maliciousUsername, "noPass"), + httpClient + ); + StatusResponseHolder responseHolder = HttpUtil.makeRequestWithExpectedStatus( + maliciousClient, + HttpMethod.GET, + config.getBrokerUrl() + "/status", + null, + HttpResponseStatus.UNAUTHORIZED + ); + String responseContent = responseHolder.getContent(); + Assert.assertTrue(responseContent.contains("MESSAGE:Unauthorized")); + Assert.assertFalse(responseContent.contains(maliciousUsername)); + } + private void testOptionsRequests(HttpClient httpClient) { HttpUtil.makeRequest(httpClient, HttpMethod.OPTIONS, config.getCoordinatorUrl() + "/status", null); @@ -522,7 +543,7 @@ public class ITBasicAuthConfigurationTest catch (AvaticaSqlException ase) { Assert.assertEquals( ase.getErrorMessage(), - "Error while executing SQL \"SELECT * FROM INFORMATION_SCHEMA.COLUMNS\": Remote driver error: BasicSecurityAuthenticationException: User metadata store authentication failed username[admin]." + "Error while executing SQL \"SELECT * FROM INFORMATION_SCHEMA.COLUMNS\": Remote driver error: BasicSecurityAuthenticationException: User metadata store authentication failed." ); return; }