Skip to content
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

Jump to bottom

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

Merged
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 1 commit
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
21 changes: 18 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,6 +27,8 @@
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;
Expand Down Expand Up @@ -166,10 +168,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 +380,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 +415,19 @@ 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

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

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

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

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.length > 0) {
conn.clientCertificate = (X509Certificate) certificates[0];
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should inspect the sslHandler.engine().getNeedClientAuth() and throw an exception if no certificate is found here and/or in the exception below. See https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/NettyServerCnxnFactory.java#L418-L463 for how zookeeper handles hits.

Another thing I notice in zookeeper is that they set the entire Certificate[] onto the connection, so that someone can inspect the whole certificate chain if they want. I think we should probably do that in order to avoid having to break this interface in the future if someone wants more than the head of the chain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed the interface to use the certificate chain. I'm also logging when getNeedClientAuth() is true but no certificate is found. I don't think throwing an exception in this listener is any more useful than logging, it wouldn't block a connection from forming. I also don't necessarily think that it's the job of this code to verify that connections that require mutual auth are doing it. Netty is already enforcing that.

LOG.debug("No client certificate found for peer {}", remoteAddress);
}
} catch (Exception e) {
LOG.debug("Failure getting peer certificate for {}", 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 that the client presented to this HBase server when making its
* connection. TLS is orthogonal to Kerberos, so this is unrelated to
* {@link this#getRequestUser()}. Both, one, or neither, may be present.
*/
Optional<X509Certificate> getClientCertificate();

/** 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 clientCertificate;
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.clientCertificate = connection.clientCertificate;
} else {
this.user = null;
this.retryImmediatelySupported = false;
this.clientCertificate = 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> getClientCertificate() {
return Optional.ofNullable(clientCertificate);
}

@Override
public InetAddress getRemoteAddress() {
return remoteAddress;
Expand Down
3 changes: 3 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,8 @@ abstract class ServerRpcConnection implements Closeable {
protected User user = null;
protected UserGroupInformation ugi = null;
protected SaslServerAuthenticationProviders saslProviders = null;
// volatile because this gets set after this object is constructed, when TLS handshake finishes
protected volatile X509Certificate clientCertificate = 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> getClientCertificate() {
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> getClientCertificate() {
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> getClientCertificate() {
return Optional.empty();
}

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