Revert "HBASE-23381 Ensure Netty client receives at least one response before considering SASL negotiation complete"

Incorrect jira issue number

This reverts commit 78eecd3a7d.

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 {
+            if (provider.canRetry()) {
               provider.relogin();
+            }
           } catch (IOException e) {
             LOG.warn("Relogin failed", e);
           }

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() {

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
+      tryComplete(ctx);
-      // 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.
     } 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);
   }

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;

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;

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<Pair<String, Class<? extends Exception>>> 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<String, Class<? extends Exception>>(
-        MasterRegistry.class.getName(), MasterRegistryFetchException.class));
-    // ZK based connection will fail on the master RPC
-    params.add(new Pair<String, Class<? extends Exception>>(
-        // 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<Void>() {
         @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));
-            }
-            return null;
-          }
-        });
-
-        LOG.info("Executing request to HBase RegionServer which should fail");
-        user1.doAs(new PrivilegedExceptionAction<Void>() {
-          @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()));
-    }
   }
 }