From b3e0c60d7bf70756145cc649f951f3be1199860a Mon Sep 17 00:00:00 2001 From: Nihal Jain Date: Wed, 25 Oct 2023 13:47:27 +0530 Subject: [PATCH 1/3] HBASE-24179 Backport fix for "Netty SASL implementation does not wait for challenge response" to branch-2.x (#5472) - Backport HBASE-23881 "Netty SASL implementation does not wait for challenge response" - Backport HBASE-24263 "TestDelegationToken is broken" - Fix assertion to check for InvalidToken.class.getName() to ensure bug is fixed --- .../security/AbstractHBaseSaslRpcClient.java | 10 +++++-- .../NettyHBaseSaslRpcClientHandler.java | 26 ++++++++++++++++--- ...ShadeSaslClientAuthenticationProvider.java | 6 +++++ ...ShadeSaslServerAuthenticationProvider.java | 1 - .../TestShadeSaslAuthenticationProvider.java | 2 +- 5 files changed, 37 insertions(+), 8 deletions(-) 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 b2bf6a4f5363..87b2287a6014 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 @@ -83,12 +83,18 @@ protected AbstractHBaseSaslRpcClient(Configuration conf, } } + /** + * 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 have an initial response, an empty token + * will be returned without querying the evaluateChallenge method, as an authentication processing + * must be started by client. + * @return The client's initial response to send the server (which may be empty). + */ public byte[] getInitialResponse() throws SaslException { if (saslClient.hasInitialResponse()) { return saslClient.evaluateChallenge(EMPTY_TOKEN); - } else { - return EMPTY_TOKEN; } + return EMPTY_TOKEN; } 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 525a78d0ae8d..e57ca56390c4 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 @@ -53,6 +53,8 @@ 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; @@ -67,6 +69,7 @@ public NettyHBaseSaslRpcClientHandler(Promise saslPromise, UserGroupInf 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())); @@ -83,6 +86,10 @@ private void tryComplete(ChannelHandlerContext ctx) { 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(); @@ -110,10 +117,19 @@ public byte[] run() throws Exception { return saslRpcClient.getInitialResponse(); } }); - if (initialResponse != null) { - writeResponse(ctx, initialResponse); - } - tryComplete(ctx); + assert initialResponse != null; + writeResponse(ctx, initialResponse); + // 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. } 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. @@ -145,6 +161,8 @@ public byte[] run() throws Exception { }); 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 45bf5f015bf2..d0930a0f3148 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 @@ -60,6 +60,12 @@ public UserInformation getUserInfo(User user) { 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 3454bc30a1d0..9f0e70133db5 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 @@ -139,7 +139,6 @@ public ShadeSaslServerCallbackHandler(AtomicReference atte @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 75894d69b0a0..4ee753b1d26e 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 @@ -305,7 +305,7 @@ void validateRootCause(Throwable rootCause) { rootCause.printStackTrace(new PrintWriter(writer)); String text = writer.toString(); assertTrue("Message did not contain expected text", - text.contains("Connection reset by peer")); + text.contains(InvalidToken.class.getName())); } } } From 76cca565fb5f187356cd8b7ffd9863dc735c3361 Mon Sep 17 00:00:00 2001 From: "Jain, Nihal" Date: Fri, 3 Nov 2023 02:46:53 +0530 Subject: [PATCH 2/3] Add a dummy change. Should revert before commit! --- pom.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/pom.xml b/pom.xml index d8d48024a025..7632926390e2 100644 --- a/pom.xml +++ b/pom.xml @@ -748,6 +748,7 @@ none 1.2.0 + From 60e721b63bfb90475262b243154d7c463f84b602 Mon Sep 17 00:00:00 2001 From: "Jain, Nihal" Date: Fri, 3 Nov 2023 18:18:16 +0530 Subject: [PATCH 3/3] Revert dummy change --- pom.xml | 1 - 1 file changed, 1 deletion(-) diff --git a/pom.xml b/pom.xml index 7632926390e2..d8d48024a025 100644 --- a/pom.xml +++ b/pom.xml @@ -748,7 +748,6 @@ none 1.2.0 -