From 90a1458ac9b81bd3bac443790242353bb701aadf Mon Sep 17 00:00:00 2001 From: Abhishek Agarwal <1477457+abhishekagarwal87@users.noreply.github.com> Date: Mon, 9 Oct 2023 20:45:10 +0530 Subject: [PATCH] Parse passwords containing colon correctly (#15109) --- .../BasicHTTPAuthenticator.java | 20 +++- .../BasicHTTPAuthenticatorTest.java | 109 ++++++++++-------- 2 files changed, 79 insertions(+), 50 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 600af931f031..85cc60d2e76b 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 @@ -182,15 +182,27 @@ public void doFilter(ServletRequest servletRequest, ServletResponse servletRespo return; } - String[] splits = decodedUserSecret.split(":"); - if (splits.length != 2) { + /* From https://www.rfc-editor.org/rfc/rfc7617.html, we can assume that userid won't include a colon but password + can. + + The user-id and password MUST NOT contain any control characters (see + "CTL" in Appendix B.1 of [RFC5234]). + + Furthermore, a user-id containing a colon character is invalid, as + the first colon in a user-pass string separates user-id and password + from one another; text after the first colon is part of the password. + User-ids containing colons cannot be encoded in user-pass strings. + + */ + int split = decodedUserSecret.indexOf(':'); + if (split < 0) { // The decoded user secret is not of the right format httpResp.sendError(HttpServletResponse.SC_UNAUTHORIZED); return; } - String user = splits[0]; - char[] password = splits[1].toCharArray(); + String user = decodedUserSecret.substring(0, split); + char[] password = decodedUserSecret.substring(split + 1).toCharArray(); // If any authentication error occurs we send a 401 response immediately and do not proceed further down the filter chain. // If the authentication result is null and skipOnFailure property is false, we send a 401 response and do not proceed 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 84bfdcf56b1c..bf0cf1778a14 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 @@ -112,55 +112,21 @@ public void testGoodPassword() throws IOException, ServletException } @Test - public void testGoodPasswordWithValidator() throws IOException, ServletException + public void testGoodNonEmptyPasswordWithValidator() throws IOException, ServletException { - CredentialsValidator validator = EasyMock.createMock(CredentialsValidator.class); - BasicHTTPAuthenticator authenticatorWithValidator = new BasicHTTPAuthenticator( - CACHE_MANAGER_PROVIDER, - "basic", - "basic", - null, - null, - false, - null, null, - false, - validator - ); - - String header = StringUtils.utf8Base64("userA:helloworld"); - header = StringUtils.format("Basic %s", header); - - EasyMock - .expect( - validator.validateCredentials(EasyMock.eq("basic"), EasyMock.eq("basic"), EasyMock.eq("userA"), EasyMock.aryEq("helloworld".toCharArray())) - ) - .andReturn( - new AuthenticationResult("userA", "basic", "basic", null) - ) - .times(1); - EasyMock.replay(validator); - - HttpServletRequest req = EasyMock.createMock(HttpServletRequest.class); - EasyMock.expect(req.getHeader("Authorization")).andReturn(header); - req.setAttribute( - AuthConfig.DRUID_AUTHENTICATION_RESULT, - new AuthenticationResult("userA", "basic", "basic", null) - ); - EasyMock.expectLastCall().times(1); - EasyMock.replay(req); - - HttpServletResponse resp = EasyMock.createMock(HttpServletResponse.class); - EasyMock.replay(resp); - - FilterChain filterChain = EasyMock.createMock(FilterChain.class); - filterChain.doFilter(req, resp); - EasyMock.expectLastCall().times(1); - EasyMock.replay(filterChain); + testGoodPasswordWithValidator("userA", "helloworld"); + } - Filter authenticatorFilter = authenticatorWithValidator.getFilter(); - authenticatorFilter.doFilter(req, resp, filterChain); + @Test + public void testGoodEmptyPasswordWithValidator() throws IOException, ServletException + { + testGoodPasswordWithValidator("userA", ""); + } - EasyMock.verify(req, resp, validator, filterChain); + @Test + public void testGoodColonInPasswordWithValidator() throws IOException, ServletException + { + testGoodPasswordWithValidator("userA", "hello:hello"); } @Test @@ -396,4 +362,55 @@ public void testMissingHeader() throws IOException, ServletException EasyMock.verify(req, resp, filterChain); } + + private void testGoodPasswordWithValidator(String username, String password) throws IOException, ServletException + { + CredentialsValidator validator = EasyMock.createMock(CredentialsValidator.class); + BasicHTTPAuthenticator authenticatorWithValidator = new BasicHTTPAuthenticator( + CACHE_MANAGER_PROVIDER, + "basic", + "basic", + null, + null, + false, + null, null, + false, + validator + ); + + String header = StringUtils.utf8Base64(username + ":" + password); + header = StringUtils.format("Basic %s", header); + + EasyMock + .expect( + validator.validateCredentials(EasyMock.eq("basic"), EasyMock.eq("basic"), EasyMock.eq(username), EasyMock.aryEq(password.toCharArray())) + ) + .andReturn( + new AuthenticationResult(username, "basic", "basic", null) + ) + .times(1); + EasyMock.replay(validator); + + HttpServletRequest req = EasyMock.createMock(HttpServletRequest.class); + EasyMock.expect(req.getHeader("Authorization")).andReturn(header); + req.setAttribute( + AuthConfig.DRUID_AUTHENTICATION_RESULT, + new AuthenticationResult(username, "basic", "basic", null) + ); + EasyMock.expectLastCall().times(1); + EasyMock.replay(req); + + HttpServletResponse resp = EasyMock.createMock(HttpServletResponse.class); + EasyMock.replay(resp); + + FilterChain filterChain = EasyMock.createMock(FilterChain.class); + filterChain.doFilter(req, resp); + EasyMock.expectLastCall().times(1); + EasyMock.replay(filterChain); + + Filter authenticatorFilter = authenticatorWithValidator.getFilter(); + authenticatorFilter.doFilter(req, resp, filterChain); + + EasyMock.verify(req, resp, validator, filterChain); + } }