diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcConnection.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcConnection.java index eff4a13dde3..7d91fd9d6f0 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcConnection.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcConnection.java @@ -149,10 +149,6 @@ class NettyRpcConnection extends RpcConnection { if (error instanceof FallbackDisallowedException) { return; } - if (!provider.canRetry()) { - LOG.trace("SASL Provider does not support retries"); - return; - } synchronized (this) { if (reloginInProgress) { return; @@ -163,7 +159,9 @@ class NettyRpcConnection extends RpcConnection { @Override public void run() { try { - provider.relogin(); + if (provider.canRetry()) { + provider.relogin(); + } } catch (IOException e) { LOG.warn("Relogin failed", e); } diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/security/AbstractHBaseSaslRpcClient.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/security/AbstractHBaseSaslRpcClient.java index 26b3811648d..b1f0861e351 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/security/AbstractHBaseSaslRpcClient.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/security/AbstractHBaseSaslRpcClient.java @@ -88,24 +88,12 @@ public abstract class AbstractHBaseSaslRpcClient { } } - /** - * Computes the initial response a client sends to a server to begin the SASL - * challenge/response handshake. If the client's SASL mechanism does not require - * that an initial response is sent to begin the handshake, this method will return - * a null byte array, indicating no initial response needs to be sent by this client. - * - * It is unclear as to whether all SASL implementations will return a non-empty initial - * response, so this implementation is written such that this is allowed. All known - * SASL mechanism implementations in the JDK provide non-empty initial responses. - * - * @return The client's initial response to send the server (which may be empty), or null - * if this implementation does not require an initial response to be sent. - */ public byte[] getInitialResponse() throws SaslException { if (saslClient.hasInitialResponse()) { return saslClient.evaluateChallenge(EMPTY_TOKEN); + } else { + return EMPTY_TOKEN; } - return null; } public boolean isComplete() { diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/security/NettyHBaseSaslRpcClientHandler.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/security/NettyHBaseSaslRpcClientHandler.java index aff3993ff7c..e011cc612e5 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/security/NettyHBaseSaslRpcClientHandler.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/security/NettyHBaseSaslRpcClientHandler.java @@ -54,8 +54,6 @@ public class NettyHBaseSaslRpcClientHandler extends SimpleChannelInboundHandler< private final Configuration conf; - private final SaslClientAuthenticationProvider provider; - // flag to mark if Crypto AES encryption is enable private boolean needProcessConnectionHeader = false; @@ -70,7 +68,6 @@ public class NettyHBaseSaslRpcClientHandler extends SimpleChannelInboundHandler< this.saslPromise = saslPromise; this.ugi = ugi; this.conf = conf; - this.provider = provider; this.saslRpcClient = new NettyHBaseSaslRpcClient(conf, provider, token, serverAddr, securityInfo, fallbackAllowed, conf.get( "hbase.rpc.protection", SaslUtil.QualityOfProtection.AUTHENTICATION.name().toLowerCase())); @@ -87,11 +84,6 @@ public class NettyHBaseSaslRpcClientHandler extends SimpleChannelInboundHandler< return; } - // HBASE-23881 Clearly log when the client thinks that the SASL negotiation is complete. - if (LOG.isTraceEnabled()) { - LOG.trace("SASL negotiation for {} is complete", provider.getSaslAuthMethod().getName()); - } - saslRpcClient.setupSaslHandler(ctx.pipeline()); setCryptoAESOption(); @@ -121,19 +113,8 @@ public class NettyHBaseSaslRpcClientHandler extends SimpleChannelInboundHandler< }); if (initialResponse != null) { writeResponse(ctx, initialResponse); - } else { - LOG.trace("SASL initialResponse was null, not sending response to server."); } - // HBASE-23881 We do not want to check if the SaslClient thinks the handshake is - // complete as, at this point, we've not heard a back from the server with it's reply - // to our first challenge response. We should wait for at least one reply - // from the server before calling negotiation complete. - // - // Each SASL mechanism has its own handshake. Some mechanisms calculate a single client buffer - // to be sent to the server while others have multiple exchanges to negotiate authentication. GSSAPI(Kerberos) - // and DIGEST-MD5 both are examples of mechanisms which have multiple steps. Mechanisms which have multiple steps - // will not return true on `SaslClient#isComplete()` until the handshake has fully completed. Mechanisms which - // only send a single buffer may return true on `isComplete()` after that initial response is calculated. + tryComplete(ctx); } catch (Exception e) { // the exception thrown by handlerAdded will not be passed to the exceptionCaught below // because netty will remove a handler if handlerAdded throws an exception. @@ -165,8 +146,6 @@ public class NettyHBaseSaslRpcClientHandler extends SimpleChannelInboundHandler< }); if (response != null) { writeResponse(ctx, response); - } else { - LOG.trace("SASL challenge response was empty, not sending response to server."); } tryComplete(ctx); } diff --git a/hbase-examples/src/main/java/org/apache/hadoop/hbase/security/provider/example/ShadeSaslClientAuthenticationProvider.java b/hbase-examples/src/main/java/org/apache/hadoop/hbase/security/provider/example/ShadeSaslClientAuthenticationProvider.java index 761a2f60f34..7cda97b09d4 100644 --- a/hbase-examples/src/main/java/org/apache/hadoop/hbase/security/provider/example/ShadeSaslClientAuthenticationProvider.java +++ b/hbase-examples/src/main/java/org/apache/hadoop/hbase/security/provider/example/ShadeSaslClientAuthenticationProvider.java @@ -62,12 +62,6 @@ public class ShadeSaslClientAuthenticationProvider extends ShadeSaslAuthenticati return userInfoPB.build(); } - @Override - public boolean canRetry() { - // A static username/password either works or it doesn't. No kind of relogin/retry necessary. - return false; - } - static class ShadeSaslClientCallbackHandler implements CallbackHandler { private final String username; private final char[] password; diff --git a/hbase-examples/src/main/java/org/apache/hadoop/hbase/security/provider/example/ShadeSaslServerAuthenticationProvider.java b/hbase-examples/src/main/java/org/apache/hadoop/hbase/security/provider/example/ShadeSaslServerAuthenticationProvider.java index 8bba8d6f361..dc8d89b71d2 100644 --- a/hbase-examples/src/main/java/org/apache/hadoop/hbase/security/provider/example/ShadeSaslServerAuthenticationProvider.java +++ b/hbase-examples/src/main/java/org/apache/hadoop/hbase/security/provider/example/ShadeSaslServerAuthenticationProvider.java @@ -141,6 +141,7 @@ public class ShadeSaslServerAuthenticationProvider extends ShadeSaslAuthenticati @Override public void handle(Callback[] callbacks) throws InvalidToken, UnsupportedCallbackException { + LOG.info("SaslServerCallbackHandler called", new Exception()); NameCallback nc = null; PasswordCallback pc = null; AuthorizeCallback ac = null; diff --git a/hbase-examples/src/test/java/org/apache/hadoop/hbase/security/provider/example/TestShadeSaslAuthenticationProvider.java b/hbase-examples/src/test/java/org/apache/hadoop/hbase/security/provider/example/TestShadeSaslAuthenticationProvider.java index 79e8c57e7c1..001842f22c8 100644 --- a/hbase-examples/src/test/java/org/apache/hadoop/hbase/security/provider/example/TestShadeSaslAuthenticationProvider.java +++ b/hbase-examples/src/test/java/org/apache/hadoop/hbase/security/provider/example/TestShadeSaslAuthenticationProvider.java @@ -17,7 +17,6 @@ */ package org.apache.hadoop.hbase.security.provider.example; -import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; @@ -27,12 +26,8 @@ import java.io.BufferedWriter; import java.io.File; import java.io.IOException; import java.io.OutputStreamWriter; -import java.io.PrintWriter; -import java.io.StringWriter; import java.security.PrivilegedExceptionAction; -import java.util.ArrayList; import java.util.Collections; -import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -42,6 +37,7 @@ import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.Cell; import org.apache.hadoop.hbase.CellUtil; +import org.apache.hadoop.hbase.DoNotRetryIOException; import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.HBaseTestingUtility; import org.apache.hadoop.hbase.HConstants; @@ -52,14 +48,11 @@ import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder; import org.apache.hadoop.hbase.client.Connection; import org.apache.hadoop.hbase.client.ConnectionFactory; import org.apache.hadoop.hbase.client.Get; -import org.apache.hadoop.hbase.client.MasterRegistry; import org.apache.hadoop.hbase.client.Put; import org.apache.hadoop.hbase.client.Result; -import org.apache.hadoop.hbase.client.RetriesExhaustedException; import org.apache.hadoop.hbase.client.Table; import org.apache.hadoop.hbase.client.TableDescriptorBuilder; import org.apache.hadoop.hbase.coprocessor.CoprocessorHost; -import org.apache.hadoop.hbase.exceptions.MasterRegistryFetchException; import org.apache.hadoop.hbase.security.HBaseKerberosUtils; import org.apache.hadoop.hbase.security.provider.SaslClientAuthenticationProviders; import org.apache.hadoop.hbase.security.provider.SaslServerAuthenticationProviders; @@ -68,12 +61,8 @@ import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.testclassification.SecurityTests; import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.hbase.util.FSUtils; -import org.apache.hadoop.hbase.util.Pair; -import org.apache.hadoop.ipc.RemoteException; import org.apache.hadoop.minikdc.MiniKdc; import org.apache.hadoop.security.UserGroupInformation; -import org.apache.hadoop.security.token.SecretManager.InvalidToken; -import org.apache.hbase.thirdparty.com.google.common.base.Throwables; import org.junit.AfterClass; import org.junit.Before; import org.junit.BeforeClass; @@ -82,18 +71,14 @@ import org.junit.Rule; import org.junit.Test; import org.junit.experimental.categories.Category; import org.junit.rules.TestName; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; @Category({MediumTests.class, SecurityTests.class}) public class TestShadeSaslAuthenticationProvider { - private static final Logger LOG = LoggerFactory.getLogger(TestShadeSaslAuthenticationProvider.class); @ClassRule public static final HBaseClassTestRule CLASS_RULE = HBaseClassTestRule.forClass(TestShadeSaslAuthenticationProvider.class); - private static final char[] USER1_PASSWORD = "foobarbaz".toCharArray(); static LocalHBaseCluster createCluster(HBaseTestingUtility util, File keytabFile, @@ -235,84 +220,26 @@ public class TestShadeSaslAuthenticationProvider { } } - @Test + @Test(expected = DoNotRetryIOException.class) public void testNegativeAuthentication() throws Exception { - List>> params = new ArrayList<>(); - // Master-based connection will fail to ask the master its cluster ID - // as a part of creating the Connection. - params.add(new Pair>( - MasterRegistry.class.getName(), MasterRegistryFetchException.class)); - // ZK based connection will fail on the master RPC - params.add(new Pair>( - // ZKConnectionRegistry is package-private - HConstants.ZK_CONNECTION_REGISTRY_CLASS, RetriesExhaustedException.class)); - - params.forEach((pair) -> { - LOG.info("Running negative authentication test for client registry {}, expecting {}", - pair.getFirst(), pair.getSecond().getName()); - // Validate that we can read that record back out as the user with our custom auth'n - final Configuration clientConf = new Configuration(CONF); - clientConf.setInt(HConstants.HBASE_CLIENT_RETRIES_NUMBER, 3); - clientConf.set(HConstants.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY, pair.getFirst()); - try (Connection conn = ConnectionFactory.createConnection(clientConf)) { - UserGroupInformation user1 = UserGroupInformation.createUserForTesting( - "user1", new String[0]); - user1.addToken( - ShadeClientTokenUtil.obtainToken(conn, "user1", "not a real password".toCharArray())); - - LOG.info("Executing request to HBase Master which should fail"); - user1.doAs(new PrivilegedExceptionAction() { - @Override public Void run() throws Exception { - try (Connection conn = ConnectionFactory.createConnection(clientConf);) { - conn.getAdmin().listTableDescriptors(); - fail("Should not successfully authenticate with HBase"); - } catch (Exception e) { - LOG.info("Caught exception in negative Master connectivity test", e); - assertEquals("Found unexpected exception", pair.getSecond(), e.getClass()); - validateRootCause(Throwables.getRootCause(e)); - } + // Validate that we can read that record back out as the user with our custom auth'n + final Configuration clientConf = new Configuration(CONF); + clientConf.setInt(HConstants.HBASE_CLIENT_RETRIES_NUMBER, 3); + try (Connection conn = ConnectionFactory.createConnection(clientConf)) { + UserGroupInformation user1 = UserGroupInformation.createUserForTesting( + "user1", new String[0]); + user1.addToken( + ShadeClientTokenUtil.obtainToken(conn, "user1", "not a real password".toCharArray())); + user1.doAs(new PrivilegedExceptionAction() { + @Override public Void run() throws Exception { + try (Connection conn = ConnectionFactory.createConnection(clientConf); + Table t = conn.getTable(tableName)) { + t.get(new Get(Bytes.toBytes("r1"))); + fail("Should not successfully authenticate with HBase"); return null; } - }); - - LOG.info("Executing request to HBase RegionServer which should fail"); - user1.doAs(new PrivilegedExceptionAction() { - @Override public Void run() throws Exception { - // A little contrived because, with MasterRegistry, we'll still fail on talking - // to the HBase master before trying to talk to a RegionServer. - try (Connection conn = ConnectionFactory.createConnection(clientConf); - Table t = conn.getTable(tableName)) { - t.get(new Get(Bytes.toBytes("r1"))); - fail("Should not successfully authenticate with HBase"); - } catch (Exception e) { - LOG.info("Caught exception in negative RegionServer connectivity test", e); - assertEquals("Found unexpected exception", pair.getSecond(), e.getClass()); - validateRootCause(Throwables.getRootCause(e)); - } - return null; - } - }); - } catch (InterruptedException e) { - LOG.error("Caught interrupted exception", e); - Thread.currentThread().interrupt(); - return; - } catch (IOException e) { - throw new RuntimeException(e); - } - }); - } - - void validateRootCause(Throwable rootCause) { - LOG.info("Root cause was", rootCause); - if (rootCause instanceof RemoteException) { - RemoteException re = (RemoteException) rootCause; - IOException actualException = re.unwrapRemoteException(); - assertEquals(InvalidToken.class, actualException.getClass()); - } else { - StringWriter writer = new StringWriter(); - rootCause.printStackTrace(new PrintWriter(writer)); - String text = writer.toString(); - assertTrue("Message did not contain expected text", text.contains(InvalidToken.class.getName())); + } + }); } } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerRpcConnection.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerRpcConnection.java index e55254ebac5..d49a9043e9a 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerRpcConnection.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerRpcConnection.java @@ -373,7 +373,7 @@ abstract class ServerRpcConnection implements Closeable { String clientIP = this.toString(); // attempting user could be null RpcServer.AUDITLOG - .warn("{}{}: {}", RpcServer.AUTH_FAILED_FOR, clientIP, saslServer.getAttemptingUser()); + .warn("{} {}: {}", RpcServer.AUTH_FAILED_FOR, clientIP, saslServer.getAttemptingUser()); throw e; } if (replyToken != null) {