This is an automated email from the ASF dual-hosted git repository. kgyrtkirk pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/hive.git
commit 94b43f40353216127742eaf1c3604479a36c660f Author: Kevin Risden <kris...@apache.org> AuthorDate: Tue Mar 17 17:01:17 2020 +0000 HIVE-22841: ThriftHttpServlet#getClientNameFromCookie should handle CookieSigner IllegalArgumentException on invalid cookie signature (Kevin Risden via Zoltan Haindrich) Signed-off-by: Zoltan Haindrich <k...@rxd.hu> --- .../auth/TestHttpCookieAuthenticationTest.java | 185 +++++++++++++++++ .../java/org/apache/hive/jdbc/HiveConnection.java | 2 +- .../hive/jdbc/HttpRequestInterceptorBase.java | 5 +- .../hive/service/cli/thrift/ThriftHttpServlet.java | 9 +- .../org/apache/hive/service/TestCookieSigner.java | 53 +++-- .../cli/thrift/ThriftCliServiceTestWithCookie.java | 231 --------------------- 6 files changed, 231 insertions(+), 254 deletions(-) diff --git a/itests/hive-unit/src/test/java/org/apache/hive/service/auth/TestHttpCookieAuthenticationTest.java b/itests/hive-unit/src/test/java/org/apache/hive/service/auth/TestHttpCookieAuthenticationTest.java new file mode 100644 index 0000000..827cc68 --- /dev/null +++ b/itests/hive-unit/src/test/java/org/apache/hive/service/auth/TestHttpCookieAuthenticationTest.java @@ -0,0 +1,185 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hive.service.auth; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +import java.lang.reflect.Field; +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.ResultSet; +import java.sql.Statement; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import org.apache.hadoop.hive.conf.HiveConf; +import org.apache.hive.jdbc.HiveConnection; +import org.apache.hive.jdbc.HiveDriver; +import org.apache.hive.jdbc.miniHS2.MiniHS2; +import org.apache.http.client.CookieStore; +import org.apache.http.client.HttpClient; +import org.apache.http.cookie.Cookie; +import org.apache.http.impl.cookie.BasicClientCookie; +import org.apache.thrift.transport.THttpClient; +import org.apache.thrift.transport.TTransport; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * TestHttpCookieAuthenticationTest. + */ +public class TestHttpCookieAuthenticationTest { + private static MiniHS2 miniHS2; + + @BeforeClass + public static void startServices() throws Exception { + miniHS2 = new MiniHS2.Builder().withHTTPTransport().build(); + + Map<String, String> configOverlay = new HashMap<>(); + configOverlay.put(HiveConf.ConfVars.HIVE_SUPPORT_CONCURRENCY.varname, Boolean.FALSE.toString()); + configOverlay.put(HiveConf.ConfVars.HIVE_SERVER2_THRIFT_HTTP_COOKIE_AUTH_ENABLED.varname, Boolean.TRUE.toString()); + miniHS2.start(configOverlay); + } + + @AfterClass + public static void stopServices() throws Exception { + if (miniHS2 != null && miniHS2.isStarted()) { + miniHS2.stop(); + miniHS2.cleanup(); + miniHS2 = null; + MiniHS2.cleanupLocalDir(); + } + } + + @Test + public void testHttpJdbcCookies() throws Exception { + String sqlQuery = "show tables"; + + Class.forName(HiveDriver.class.getCanonicalName()); + + String username = System.getProperty("user.name"); + try(Connection connection = DriverManager.getConnection(miniHS2.getJdbcURL(), username, "bar")) { + assertNotNull(connection); + + CookieStore cookieStore = getCookieStoreFromConnection(connection); + assertNotNull(cookieStore); + + // Test that basic cookies worked + List<Cookie> cookies1 = cookieStore.getCookies(); + assertEquals(1, cookies1.size()); + + try(Statement statement = connection.createStatement()) { + assertNotNull(statement); + try(ResultSet resultSet = statement.executeQuery(sqlQuery)) { + assertNotNull(resultSet); + } + } + + // Check that cookies worked and still the same after a statement + List<Cookie> cookies2 = cookieStore.getCookies(); + assertEquals(1, cookies2.size()); + assertEquals(cookies1, cookies2); + + // Empty out cookies to make sure same connection gets new cookies + cookieStore.clear(); + assertTrue(cookieStore.getCookies().isEmpty()); + + try(Statement statement = connection.createStatement()) { + assertNotNull(statement); + try(ResultSet resultSet = statement.executeQuery(sqlQuery)) { + assertNotNull(resultSet); + } + } + + // Check that cookies worked after clearing and got back new cookie + List<Cookie> cookies3 = cookieStore.getCookies(); + assertEquals(1, cookies3.size()); + assertNotEquals(cookies1, cookies3); + + + // Get original cookie to copy metadata + Cookie originalCookie = cookies3.get(0); + + // Put in a bad client side cookie - ensure HS2 authenticates and overwrites + BasicClientCookie badCookie = new BasicClientCookie("hive.server2.auth", "bad"); + badCookie.setDomain(originalCookie.getDomain()); + badCookie.setPath(originalCookie.getPath()); + badCookie.setExpiryDate(originalCookie.getExpiryDate()); + cookieStore.addCookie(badCookie); + + // Check that putting in the bad cookie overrode the original cookie + List<Cookie> cookies4 = cookieStore.getCookies(); + assertEquals(1, cookies4.size()); + assertTrue(cookies4.contains(badCookie)); + + try(Statement statement = connection.createStatement()) { + assertNotNull(statement); + try(ResultSet resultSet = statement.executeQuery(sqlQuery)) { + assertNotNull(resultSet); + } + } + + // Check that cookies worked and replaced the bad cookie + List<Cookie> cookies5 = cookieStore.getCookies(); + assertEquals(1, cookies5.size()); + assertNotEquals(cookies4, cookies5); + + try(Statement statement = connection.createStatement()) { + assertNotNull(statement); + try(ResultSet resultSet = statement.executeQuery(sqlQuery)) { + assertNotNull(resultSet); + } + } + + // Check that cookies worked and didn't get replaced + List<Cookie> cookies6 = cookieStore.getCookies(); + assertEquals(1, cookies6.size()); + assertEquals(cookies5, cookies6); + } + } + + // ((InternalHttpClient) ((THttpClient) ((HiveConnection) connection).transport).client).cookieStore.getCookies() + private CookieStore getCookieStoreFromConnection(Connection connection) throws Exception { + CookieStore cookieStore = null; + if (connection instanceof HiveConnection) { + HiveConnection hiveConnection = (HiveConnection) connection; + + Field transportField = hiveConnection.getClass().getDeclaredField("transport"); + transportField.setAccessible(true); + TTransport transport = (TTransport) transportField.get(hiveConnection); + + if(transport instanceof THttpClient) { + THttpClient httpTransport = (THttpClient) transport; + Field clientField = httpTransport.getClass().getDeclaredField("client"); + clientField.setAccessible(true); + HttpClient httpClient = (HttpClient) clientField.get(httpTransport); + + Field cookieStoreField = httpClient.getClass().getDeclaredField("cookieStore"); + cookieStoreField.setAccessible(true); + cookieStore = (CookieStore) cookieStoreField.get(httpClient); + } + } + return cookieStore; + } +} diff --git a/jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java b/jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java index d797d40..cbf6632 100644 --- a/jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java +++ b/jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java @@ -533,7 +533,7 @@ public class HiveConnection implements java.sql.Connection { if (isCookieEnabled) { // Create a http client with a retry mechanism when the server returns a status code of 401. httpClientBuilder = - HttpClients.custom().setServiceUnavailableRetryStrategy( + HttpClients.custom().setDefaultCookieStore(cookieStore).setServiceUnavailableRetryStrategy( new ServiceUnavailableRetryStrategy() { @Override public boolean retryRequest(final HttpResponse response, final int executionCount, diff --git a/jdbc/src/java/org/apache/hive/jdbc/HttpRequestInterceptorBase.java b/jdbc/src/java/org/apache/hive/jdbc/HttpRequestInterceptorBase.java index 1ef0ab1..bb1abd3 100644 --- a/jdbc/src/java/org/apache/hive/jdbc/HttpRequestInterceptorBase.java +++ b/jdbc/src/java/org/apache/hive/jdbc/HttpRequestInterceptorBase.java @@ -27,7 +27,6 @@ import org.apache.http.HttpException; import org.apache.http.HttpRequest; import org.apache.http.HttpRequestInterceptor; import org.apache.http.client.CookieStore; -import org.apache.http.client.protocol.ClientContext; import org.apache.http.protocol.HttpContext; public abstract class HttpRequestInterceptorBase implements HttpRequestInterceptor { @@ -60,9 +59,7 @@ public abstract class HttpRequestInterceptorBase implements HttpRequestIntercept // The necessary condition is either when there are no server side cookies in the // cookiestore which can be send back or when the server returns a 401 error code // indicating that the previous cookie has expired. - if (isCookieEnabled) { - httpContext.setAttribute(ClientContext.COOKIE_STORE, cookieStore); - } + // Generate the kerberos ticket under the following scenarios: // 1. Cookie Authentication is disabled OR // 2. The first time when the request is sent OR diff --git a/service/src/java/org/apache/hive/service/cli/thrift/ThriftHttpServlet.java b/service/src/java/org/apache/hive/service/cli/thrift/ThriftHttpServlet.java index 6eb2606..421aa5a 100644 --- a/service/src/java/org/apache/hive/service/cli/thrift/ThriftHttpServlet.java +++ b/service/src/java/org/apache/hive/service/cli/thrift/ThriftHttpServlet.java @@ -292,7 +292,14 @@ public class ThriftHttpServlet extends TServlet { // If we reached here, we have match for HS2 generated cookie currValue = currCookie.getValue(); // Validate the value. - currValue = signer.verifyAndExtract(currValue); + try { + currValue = signer.verifyAndExtract(currValue); + } catch (IllegalArgumentException e) { + if (LOG.isDebugEnabled()) { + LOG.debug("Invalid cookie", e); + } + currValue = null; + } // Retrieve the user name, do the final validation step. if (currValue != null) { String userName = HttpAuthUtils.getUserNameFromCookieToken(currValue); diff --git a/service/src/test/org/apache/hive/service/TestCookieSigner.java b/service/src/test/org/apache/hive/service/TestCookieSigner.java index aec6d47..54801d4 100644 --- a/service/src/test/org/apache/hive/service/TestCookieSigner.java +++ b/service/src/test/org/apache/hive/service/TestCookieSigner.java @@ -18,42 +18,61 @@ package org.apache.hive.service; -import java.util.Random; - +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import java.util.Random; -import org.junit.After; import org.junit.Before; import org.junit.Test; /** - * CLIServiceTest. + * TestCookieSigner. * */ public class TestCookieSigner { - - protected static CookieSigner cs; private static final Random RAN = new Random(); - /** - * @throws java.lang.Exception - */ + private CookieSigner cs; + @Before - public void setUp() throws Exception { + public void setUp() { cs = new CookieSigner(Long.toString(RAN.nextLong()).getBytes()); } - /** - * @throws java.lang.Exception - */ - @After - public void tearDown() throws Exception { + @Test + public void testVerifyAndExtract() { + String originalStr = "cu=scott"; + String signedStr = cs.signCookie(originalStr); + assertEquals(originalStr, cs.verifyAndExtract(signedStr)); + } + + @Test + public void testVerifyAndExtractNoSignature() { + String originalStr = "cu=scott"; + String signedStr = cs.signCookie(originalStr); + String modifedSignedStr = signedStr.replace("&s=", ""); + try { + cs.verifyAndExtract(modifedSignedStr); + } catch (IllegalArgumentException e) { + assertEquals("Invalid input sign: " + modifedSignedStr, e.getMessage()); + return; + } + fail("Expected IllegalArgumentException due to no signature"); } @Test - public void testVerifyAndExtract() throws Exception { + public void testVerifyAndExtractInvalidSignature() { String originalStr = "cu=scott"; String signedStr = cs.signCookie(originalStr); - assert(cs.verifyAndExtract(signedStr).equals(originalStr)); + String modifedSignedStr = signedStr.replace("&s=", "&s=abc"); + try { + cs.verifyAndExtract(modifedSignedStr); + } catch (IllegalArgumentException e) { + assertTrue(e.getMessage().startsWith("Invalid sign, original = ")); + return; + } + fail("Expected IllegalArgumentException checking signature"); } } diff --git a/service/src/test/org/apache/hive/service/cli/thrift/ThriftCliServiceTestWithCookie.java b/service/src/test/org/apache/hive/service/cli/thrift/ThriftCliServiceTestWithCookie.java deleted file mode 100644 index cbcaa97..0000000 --- a/service/src/test/org/apache/hive/service/cli/thrift/ThriftCliServiceTestWithCookie.java +++ /dev/null @@ -1,231 +0,0 @@ - - -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.apache.hive.service.cli.thrift; - -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.fail; - -import java.util.HashMap; -import java.util.Map; -import java.util.concurrent.TimeUnit; - -import org.apache.hadoop.hive.conf.HiveConf; -import org.apache.hadoop.hive.conf.HiveConf.ConfVars; -import org.apache.hadoop.hive.metastore.MetaStoreTestUtils; -import org.apache.hive.service.Service; -import org.apache.hive.service.auth.HiveAuthConstants; -import org.apache.hive.service.cli.OperationHandle; -import org.apache.hive.service.cli.OperationState; -import org.apache.hive.service.cli.OperationStatus; -import org.apache.hive.service.cli.SessionHandle; -import org.apache.hive.service.server.HiveServer2; -import org.junit.After; -import org.junit.AfterClass; -import org.junit.Before; -import org.junit.BeforeClass; -import org.junit.Test; - -/** - * ThriftCLIServiceTestWithCookie. - */ -public class ThriftCliServiceTestWithCookie { - - protected static int port; - protected static String host = "localhost"; - protected static HiveServer2 hiveServer2; - protected static ThriftCLIServiceClient client; - protected static HiveConf hiveConf; - protected static String USERNAME = "anonymous"; - protected static String PASSWORD = "anonymous"; - - /** - * @throws java.lang.Exception - */ - @BeforeClass - public static void setUpBeforeClass() throws Exception { - // Find a free port - port = MetaStoreTestUtils.findFreePort(); - hiveServer2 = new HiveServer2(); - hiveConf = new HiveConf(); - hiveConf.setBoolVar(ConfVars.HIVE_SERVER2_THRIFT_HTTP_COOKIE_AUTH_ENABLED, true); - // Set the cookie max age to a very low value so that - // the server sends 401 very frequently - hiveConf.setTimeVar(ConfVars.HIVE_SERVER2_THRIFT_HTTP_COOKIE_MAX_AGE, 1, TimeUnit.SECONDS); - hiveConf.setVar(ConfVars.HIVE_SERVER2_TRANSPORT_MODE, "http"); - hiveConf.setVar(ConfVars.HIVE_SERVER2_THRIFT_HTTP_PATH, "cliservice"); - - assertNotNull(port); - assertNotNull(hiveServer2); - assertNotNull(hiveConf); - - hiveConf.setBoolVar(ConfVars.HIVE_SERVER2_ENABLE_DOAS, false); - hiveConf.setVar(ConfVars.HIVE_SERVER2_THRIFT_BIND_HOST, host); - hiveConf.setIntVar(ConfVars.HIVE_SERVER2_THRIFT_HTTP_PORT, port); - hiveConf.setVar(ConfVars.HIVE_SERVER2_AUTHENTICATION, HiveAuthConstants.AuthTypes.NOSASL.toString()); - - startHiveServer2WithConf(hiveConf); - - client = getServiceClientInternal(); - } - - /** - * @throws java.lang.Exception - */ - @AfterClass - public static void tearDownAfterClass() throws Exception { - stopHiveServer2(); - } - - protected static void startHiveServer2WithConf(HiveConf hiveConf) throws Exception { - Exception HS2Exception = null; - boolean HS2Started = false; - for (int tryCount = 0; tryCount < MetaStoreTestUtils.RETRY_COUNT; tryCount++) { - try { - hiveServer2.init(hiveConf); - hiveServer2.start(); - HS2Started = true; - break; - } catch (Exception t) { - HS2Exception = t; - port = MetaStoreTestUtils.findFreePort(); - hiveConf.setIntVar(ConfVars.HIVE_SERVER2_THRIFT_HTTP_PORT, port); - hiveServer2 = new HiveServer2(); - } - } - - if (!HS2Started) { - HS2Exception.printStackTrace(); - fail(); - } - - // Wait for startup to complete - Thread.sleep(2000); - System.out.println("HiveServer2 started on port " + port); - } - - protected static void stopHiveServer2() throws Exception { - if (hiveServer2 != null) { - hiveServer2.stop(); - } - } - - protected static ThriftCLIServiceClient getServiceClientInternal() { - for (Service service : hiveServer2.getServices()) { - if (service instanceof ThriftBinaryCLIService) { - return new ThriftCLIServiceClient((ThriftBinaryCLIService) service); - } - if (service instanceof ThriftHttpCLIService) { - return new ThriftCLIServiceClient((ThriftHttpCLIService) service); - } - } - throw new IllegalStateException("HiveServer2 not running Thrift service"); - } - - /** - * @throws java.lang.Exception - */ - @Before - public void setUp() throws Exception { - } - - /** - * @throws java.lang.Exception - */ - @After - public void tearDown() throws Exception { - - } - - @Test - public void testOpenSession() throws Exception { - // Open a new client session - SessionHandle sessHandle = client.openSession(USERNAME, - PASSWORD, new HashMap<String, String>()); - // Session handle should not be null - assertNotNull("Session handle should not be null", sessHandle); - // Close client session - client.closeSession(sessHandle); - } - - @Test - public void testGetFunctions() throws Exception { - SessionHandle sessHandle = client.openSession(USERNAME, - PASSWORD, new HashMap<String, String>()); - assertNotNull("Session handle should not be null", sessHandle); - - String catalogName = null; - String schemaName = null; - String functionName = "*"; - - OperationHandle opHandle = client.getFunctions(sessHandle, catalogName, - schemaName, functionName); - - assertNotNull("Operation handle should not be null", opHandle); - - client.closeSession(sessHandle); - } - - /** - * Test synchronous query execution - * @throws Exception - */ - @Test - public void testExecuteStatement() throws Exception { - Map<String, String> opConf = new HashMap<String, String>(); - // Open a new client session - SessionHandle sessHandle = client.openSession(USERNAME, - PASSWORD, opConf); - // Session handle should not be null - assertNotNull("Session handle should not be null", sessHandle); - - // Change lock manager to embedded mode - String queryString = "SET hive.lock.manager=" + - "org.apache.hadoop.hive.ql.lockmgr.EmbeddedLockManager"; - client.executeStatement(sessHandle, queryString, opConf); - - // Drop the table if it exists - queryString = "DROP TABLE IF EXISTS TEST_EXEC_THRIFT"; - client.executeStatement(sessHandle, queryString, opConf); - - // Create a test table - queryString = "CREATE TABLE TEST_EXEC_THRIFT(ID STRING)"; - client.executeStatement(sessHandle, queryString, opConf); - - // Execute another query - queryString = "SELECT ID+1 FROM TEST_EXEC_THRIFT"; - OperationHandle opHandle = client.executeStatement(sessHandle, queryString, opConf); - assertNotNull(opHandle); - - OperationStatus opStatus = client.getOperationStatus(opHandle, false); - assertNotNull(opStatus); - - OperationState state = opStatus.getState(); - // Expect query to be completed now - assertEquals("Query should be finished", OperationState.FINISHED, state); - - // Cleanup - queryString = "DROP TABLE TEST_EXEC_THRIFT"; - client.executeStatement(sessHandle, queryString, opConf); - - client.closeSession(sessHandle); - } -} -