Skip to content

HBASE-28317: Expose client TLS certificate on RpcCallContext #5644

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bbeaudreault merged 3 commits into from
Jan 29, 2024
Merged

HBASE-28317: Expose client TLS certificate on RpcCallContext #5644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aea3966
Expose client TLS certificate on RpcCallContext
charlesconnell Jan 17, 2024
e956467
changes from PR feedback
charlesconnell Jan 24, 2024
93c7b4e
fix javadoc
charlesconnell Jan 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 28 additions & 3 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,12 @@
import java.io.InterruptedIOException;
import java.net.InetSocketAddress;
import java.net.SocketAddress;
import java.security.cert.Certificate;
import java.security.cert.X509Certificate;
import java.util.List;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.atomic.AtomicReference;
import javax.net.ssl.SSLPeerUnverifiedException;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hbase.HBaseInterfaceAudience;
import org.apache.hadoop.hbase.HBaseServerBase;
Expand Down Expand Up @@ -166,10 +169,10 @@ protected void initChannel(Channel ch) throws Exception {
ChannelPipeline pipeline = ch.pipeline();
FixedLengthFrameDecoder preambleDecoder = new FixedLengthFrameDecoder(6);
preambleDecoder.setSingleDecode(true);
NettyServerRpcConnection conn = createNettyServerRpcConnection(ch);
if (conf.getBoolean(HBASE_SERVER_NETTY_TLS_ENABLED, false)) {
initSSL(pipeline, conf.getBoolean(HBASE_SERVER_NETTY_TLS_SUPPORTPLAINTEXT, true));
initSSL(pipeline, conn, conf.getBoolean(HBASE_SERVER_NETTY_TLS_SUPPORTPLAINTEXT, true));
}
NettyServerRpcConnection conn = createNettyServerRpcConnection(ch);
pipeline.addLast(NettyRpcServerPreambleHandler.DECODER_NAME, preambleDecoder)
.addLast(new NettyRpcServerPreambleHandler(NettyRpcServer.this, conn))
// We need NettyRpcServerResponseEncoder here because NettyRpcServerPreambleHandler may
Expand Down Expand Up @@ -378,7 +381,7 @@ public int getNumOpenConnections() {
return allChannels.size();
}

private void initSSL(ChannelPipeline p, boolean supportPlaintext)
private void initSSL(ChannelPipeline p, NettyServerRpcConnection conn, boolean supportPlaintext)
throws X509Exception, IOException {
SslContext nettySslContext = getSslContext();

Expand Down Expand Up @@ -413,6 +416,28 @@ private void initSSL(ChannelPipeline p, boolean supportPlaintext)
sslHandler.setWrapDataSize(
conf.getInt(HBASE_SERVER_NETTY_TLS_WRAP_SIZE, DEFAULT_HBASE_SERVER_NETTY_TLS_WRAP_SIZE));

sslHandler.handshakeFuture().addListener(future -> {
Copy link
Contributor

@bbeaudreault bbeaudreault Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Apache9 do you think a listener is the right approach here? I'm wondering if we should instead pass the SSLHandler into NettyServerRpcConnection, and directly call handler.handshakeFuture().get() in setupHandlers(). That way it's more explicit that we have the certificate on the connection prior to handling any requests. With the listener approach, I'm not 100% sure we can guarantee that the listener is executed in the eventLoop prior to the channelRead0 in NettyRpcServerPreambleHandler.

Copy link
Contributor

@Apache9 Apache9 Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should never do future.get in event loop thread, as it has serious performance impact, or even dead lock...

Copy link
Contributor

@bbeaudreault bbeaudreault Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Do you think we should leave it as is?

Copy link
Contributor

@Apache9 Apache9 Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://stackoverflow.com/questions/70654915/order-of-invocation-of-listeners

Norman Maurer answered that the invocation order of listeners is the same with adding order. And I checked the code, if there are multiple listeners netty will call them sequentially, with the adding order. So here, since we are the first listener(at least first in the listeners added by our code), so I think we can guarantee that NettyRpcServerPreambleHandler.channelRead can see the certificates.

try {
Certificate[] certificates = sslHandler.engine().getSession().getPeerCertificates();
if (certificates != null && certificates.length > 0) {
conn.clientCertificateChain = (X509Certificate[]) certificates;
} else if (sslHandler.engine().getNeedClientAuth()) {
LOG.error(
"Could not get peer certificate on TLS connection from {}, although one is required",
remoteAddress);
}
} catch (SSLPeerUnverifiedException e) {
if (sslHandler.engine().getNeedClientAuth()) {
LOG.error(
"Could not get peer certificate on TLS connection from {}, although one is required",
remoteAddress, e);
}
} catch (Exception e) {
LOG.error("Unexpected error getting peer certificate for TLS connection from {}",
remoteAddress, e);
}
});

p.addLast("ssl", sslHandler);
LOG.debug("SSL handler added for channel: {}", p.channel());
}
Expand Down
8 changes: 8 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcCallContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package org.apache.hadoop.hbase.ipc;

import java.net.InetAddress;
import java.security.cert.X509Certificate;
import java.util.Optional;
import org.apache.hadoop.hbase.security.User;
import org.apache.yetus.audience.InterfaceAudience;
Expand Down Expand Up @@ -60,6 +61,13 @@ default Optional<String> getRequestUserName() {
return getRequestUser().map(User::getShortName);
}

/**
* Returns the TLS certificate(s) that the client presented to this HBase server when making its
* connection. TLS is orthogonal to Kerberos, so this is unrelated to
* {@link RpcCallContext#getRequestUser()}. Both, one, or neither may be present.
*/
Optional<X509Certificate[]> getClientCertificateChain();

/** Returns Address of remote client in this call */
InetAddress getRemoteAddress();

Expand Down
9 changes: 9 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerCall.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.io.IOException;
import java.net.InetAddress;
import java.nio.ByteBuffer;
import java.security.cert.X509Certificate;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -95,6 +96,7 @@ public abstract class ServerCall<T extends ServerRpcConnection> implements RpcCa

protected final User user;
protected final InetAddress remoteAddress;
protected final X509Certificate[] clientCertificateChain;
protected RpcCallback rpcCallback;

private long responseCellSize = 0;
Expand Down Expand Up @@ -134,9 +136,11 @@ public abstract class ServerCall<T extends ServerRpcConnection> implements RpcCa
if (connection != null) {
this.user = connection.user;
this.retryImmediatelySupported = connection.retryImmediatelySupported;
this.clientCertificateChain = connection.clientCertificateChain;
} else {
this.user = null;
this.retryImmediatelySupported = false;
this.clientCertificateChain = null;
}
this.remoteAddress = remoteAddress;
this.timeout = timeout;
Expand Down Expand Up @@ -498,6 +502,11 @@ public Optional<User> getRequestUser() {
return Optional.ofNullable(user);
}

@Override
public Optional<X509Certificate[]> getClientCertificateChain() {
return Optional.ofNullable(clientCertificateChain);
}

@Override
public InetAddress getRemoteAddress() {
return remoteAddress;
Expand Down
2 changes: 2 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerRpcConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.net.InetSocketAddress;
import java.nio.ByteBuffer;
import java.security.GeneralSecurityException;
import java.security.cert.X509Certificate;
import java.util.Collections;
import java.util.Map;
import java.util.Objects;
Expand Down Expand Up @@ -133,6 +134,7 @@ abstract class ServerRpcConnection implements Closeable {
protected User user = null;
protected UserGroupInformation ugi = null;
protected SaslServerAuthenticationProviders saslProviders = null;
protected X509Certificate[] clientCertificateChain = null;

public ServerRpcConnection(RpcServer rpcServer) {
this.rpcServer = rpcServer;
Expand Down
6 changes: 6 additions & 0 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/namequeues/TestNamedQueueRecorder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.net.InetAddress;
import java.security.PrivilegedAction;
import java.security.PrivilegedExceptionAction;
import java.security.cert.X509Certificate;
import java.util.Collections;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -814,6 +815,11 @@ public Optional<User> getRequestUser() {
return getUser(userName);
}

@Override
public Optional<X509Certificate[]> getClientCertificateChain() {
return Optional.empty();
}

@Override
public InetAddress getRemoteAddress() {
return null;
Expand Down
6 changes: 6 additions & 0 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/namequeues/TestRpcLogDetails.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.io.IOException;
import java.net.InetAddress;
import java.nio.ByteBuffer;
import java.security.cert.X509Certificate;
import java.util.Arrays;
import java.util.Collections;
import java.util.Map;
Expand Down Expand Up @@ -213,6 +214,11 @@ public Optional<User> getRequestUser() {
return null;
}

@Override
public Optional<X509Certificate[]> getClientCertificateChain() {
return Optional.empty();
}

@Override
public InetAddress getRemoteAddress() {
return null;
Expand Down
6 changes: 6 additions & 0 deletions ...c/test/java/org/apache/hadoop/hbase/procedure2/store/region/TestRegionProcedureStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

import java.io.IOException;
import java.net.InetAddress;
import java.security.cert.X509Certificate;
import java.util.HashSet;
import java.util.Map;
import java.util.Optional;
Expand Down Expand Up @@ -275,6 +276,11 @@ public Optional<User> getRequestUser() {
return Optional.empty();
}

@Override
public Optional<X509Certificate[]> getClientCertificateChain() {
return Optional.empty();
}

@Override
public InetAddress getRemoteAddress() {
return null;
Expand Down