Skip to content

Commit 1681a12

Browse files
rzo1rzo1
committed
Fix TLS client auth socket configuration and modernize certificate handling
 - The SSLServerSocket setup called setWantClientAuth() after setNeedClientAuth(), which per the JSSE specification clears the needClientAuth flag. Remove the redundant setWantClientAuth() call so the configured client-auth mode is applied correctly. - Replace deprecated javax.security.cert.X509Certificate / getPeerCertificateChain() with java.security.cert.X509Certificate / getPeerCertificates() - Make TTlsWrapProcessor aware of the client-auth configuration so it can distinguish encryption-only from mutual-auth mode when handling peer certificate lookup failures - Add regression test for mutual-auth TLS handshake enforcement
1 parent 81b5b80 commit 1681a12

3 files changed

Lines changed: 101 additions & 10 deletions

File tree

‎storm-client/src/jvm/org/apache/storm/security/auth/tls/ReloadableTsslTransportFactory.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,6 @@ private static TServerSocket createServerSocket(SSLServerSocketFactory factory,
6464

6565
serverSocket.setSoTimeout(timeout);
6666
serverSocket.setNeedClientAuth(clientAuth);
67-
serverSocket.setWantClientAuth(clientAuth);
6867
return new TServerSocket(new TServerSocket.ServerSocketTransportArgs().serverSocket(serverSocket).clientTimeout(timeout));
6968
} catch (Exception e) {
7069
throw new TTransportException("Could not bind to port " + port, e);

‎storm-client/src/jvm/org/apache/storm/security/auth/tls/TlsTransportPlugin.java‎

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,8 @@
1616
import java.io.UnsupportedEncodingException;
1717
import java.net.InetAddress;
1818
import java.net.ServerSocket;
19-
import java.security.Principal;
19+
import java.security.cert.Certificate;
20+
import java.security.cert.X509Certificate;
2021
import java.util.Map;
2122
import java.util.concurrent.ArrayBlockingQueue;
2223
import java.util.concurrent.BlockingQueue;
@@ -26,7 +27,6 @@
2627
import javax.net.ssl.SSLPeerUnverifiedException;
2728
import javax.net.ssl.SSLSocket;
2829
import javax.security.auth.Subject;
29-
import javax.security.cert.X509Certificate;
3030
import org.apache.storm.security.auth.ITransportPlugin;
3131
import org.apache.storm.security.auth.ReqContext;
3232
import org.apache.storm.security.auth.SingleUserPrincipal;
@@ -97,7 +97,7 @@ public TServer getServer(TProcessor processor) throws IOException, TTransportExc
9797
Integer queueSize = type.getQueueSize(conf);
9898

9999
TThreadPoolServer.Args serverArgs = new TThreadPoolServer.Args(serverTransport)
100-
.processor(new TTlsWrapProcessor(processor))
100+
.processor(new TTlsWrapProcessor(processor, type.isClientAuthRequired(conf)))
101101
.minWorkerThreads(numWorkerThreads)
102102
.maxWorkerThreads(numWorkerThreads)
103103
.protocolFactory(new TBinaryProtocol.Factory(false, true));
@@ -130,9 +130,11 @@ public boolean areWorkerTokensSupported() {
130130

131131
private static class TTlsWrapProcessor implements TProcessor {
132132
final TProcessor wrapped;
133+
final boolean clientAuthRequired;
133134

134-
TTlsWrapProcessor(TProcessor wrapped) {
135+
TTlsWrapProcessor(TProcessor wrapped, boolean clientAuthRequired) {
135136
this.wrapped = wrapped;
137+
this.clientAuthRequired = clientAuthRequired;
136138
}
137139

138140
@Override
@@ -144,13 +146,22 @@ public void process(final TProtocol inProt, final TProtocol outProt) throws TExc
144146

145147
String principalName = ANONYMOUS_PRINCIPAL_NAME;
146148
try {
147-
for (X509Certificate cert: socket.getSession().getPeerCertificateChain()) {
148-
Principal principal = cert.getSubjectDN();
149-
principalName = principal.getName();
150-
break;
149+
Certificate[] peers = socket.getSession().getPeerCertificates();
150+
if (peers.length > 0 && peers[0] instanceof X509Certificate) {
151+
principalName = ((X509Certificate) peers[0]).getSubjectX500Principal().getName();
152+
} else if (clientAuthRequired) {
153+
throw new TException("TLS peer presented no X.509 certificate");
151154
}
152155
} catch (SSLPeerUnverifiedException e) {
153-
LOG.warn("Client cert is not verified. Set principalName={}.", principalName, e);
156+
// Encryption-only mode (clientAuthRequired=false): fall through with CN=ANONYMOUS.
157+
// When client auth IS required this branch is belt-and-suspenders against a
158+
// misconfigured server socket — fail closed rather than assign a default identity.
159+
if (clientAuthRequired) {
160+
LOG.warn("Rejecting TLS connection from {}: peer certificate not verified",
161+
socket.getInetAddress());
162+
throw new TException("TLS peer not verified", e);
163+
}
164+
LOG.debug("Client cert not presented; clientAuthRequired=false, using {}", principalName);
154165
}
155166
LOG.debug("principalName : {} ", principalName);
156167
ReqContext reqContext = ReqContext.context();

‎storm-client/test/jvm/org/apache/storm/security/auth/TlsTransportPluginTest.java‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,30 @@
1313
package org.apache.storm.security.auth;
1414

1515

16+
import java.io.IOException;
17+
import java.io.InputStream;
18+
import java.net.InetAddress;
19+
import java.nio.file.Files;
20+
import java.nio.file.Paths;
21+
import java.security.KeyStore;
22+
import java.security.SecureRandom;
1623
import java.util.HashMap;
1724
import java.util.Map;
25+
import java.util.concurrent.atomic.AtomicReference;
26+
import javax.net.ssl.SSLContext;
27+
import javax.net.ssl.SSLException;
28+
import javax.net.ssl.SSLHandshakeException;
29+
import javax.net.ssl.SSLServerSocket;
30+
import javax.net.ssl.SSLSocket;
31+
import javax.net.ssl.TrustManagerFactory;
1832
import org.apache.storm.Config;
1933
import org.apache.storm.generated.Nimbus;
34+
import org.apache.storm.security.auth.tls.ReloadableTsslTransportFactory;
2035
import org.apache.storm.security.auth.tls.TlsTransportPlugin;
36+
import org.apache.storm.thrift.transport.TServerSocket;
2137
import static org.junit.jupiter.api.Assertions.assertEquals;
2238
import static org.junit.jupiter.api.Assertions.assertThrows;
39+
import static org.junit.jupiter.api.Assertions.assertTrue;
2340
import static org.junit.jupiter.api.Assertions.fail;
2441
import org.junit.jupiter.api.BeforeEach;
2542
import org.junit.jupiter.api.Test;
@@ -35,6 +52,7 @@ class TlsTransportPluginTest {
3552
@BeforeEach
3653
void setUp() {
3754
tlsTransportPlugin = new TlsTransportPlugin();
55+
conf.clear();
3856
conf.put(Config.STORM_THRIFT_TRANSPORT_PLUGIN, TlsTransportPlugin.class.getName());
3957
handler = mock(Nimbus.Iface.class);
4058
}
@@ -65,4 +83,67 @@ void testValidTlsSetup() {
6583
fail("TLS setup failed: " + e.getMessage());
6684
}
6785
}
86+
87+
/**
88+
* Regression test for VULN-14. With {@code nimbus.thrift.tls.client.auth.required=true} the
89+
* factory must leave the SSLServerSocket in true "need client auth" mode, so that JSSE fails
90+
* the handshake closed when the client presents no certificate. The {@code CN=ANONYMOUS}
91+
* fallback branch in {@link TlsTransportPlugin} must therefore be unreachable.
92+
*/
93+
@Test
94+
void testClientAuthRequiredRejectsUnauthenticatedClient() throws Exception {
95+
conf.put(Config.NIMBUS_THRIFT_TLS_PORT, 0);
96+
conf.put(Config.STORM_THRIFT_TLS_SOCKET_TIMEOUT_MS, 5000);
97+
conf.put(Config.NIMBUS_THRIFT_TLS_SERVER_KEYSTORE_PATH, testDataPath + "testKeyStore.jks");
98+
conf.put(Config.NIMBUS_THRIFT_TLS_SERVER_KEYSTORE_PASSWORD, "testpass");
99+
conf.put(Config.NIMBUS_THRIFT_TLS_SERVER_TRUSTSTORE_PATH, testDataPath + "testTrustStore.jks");
100+
conf.put(Config.NIMBUS_THRIFT_TLS_SERVER_TRUSTSTORE_PASSWORD, "testpass");
101+
conf.put(Config.NIMBUS_THRIFT_TLS_CLIENT_AUTH_REQUIRED, true);
102+
103+
TServerSocket serverTransport = ReloadableTsslTransportFactory.getServerSocket(
104+
0, 5000, InetAddress.getLoopbackAddress(), type, conf);
105+
SSLServerSocket serverSocket = (SSLServerSocket) serverTransport.getServerSocket();
106+
try {
107+
assertTrue(serverSocket.getNeedClientAuth(),
108+
"Factory must leave needClientAuth=true so JSSE fails the handshake closed");
109+
110+
int port = serverSocket.getLocalPort();
111+
AtomicReference<Throwable> serverError = new AtomicReference<>();
112+
Thread acceptor = new Thread(() -> {
113+
try (SSLSocket accepted = (SSLSocket) serverSocket.accept()) {
114+
accepted.setSoTimeout(5000);
115+
accepted.startHandshake();
116+
} catch (Throwable t) {
117+
serverError.set(t);
118+
}
119+
}, "tls-test-acceptor");
120+
acceptor.setDaemon(true);
121+
acceptor.start();
122+
123+
// Trust-only client context: no client keystore, so the client cannot present a cert.
124+
KeyStore ts = KeyStore.getInstance("JKS");
125+
try (InputStream in = Files.newInputStream(Paths.get(testDataPath + "testTrustStore.jks"))) {
126+
ts.load(in, "testpass".toCharArray());
127+
}
128+
TrustManagerFactory tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
129+
tmf.init(ts);
130+
SSLContext sslContext = SSLContext.getInstance("TLSv1.2");
131+
sslContext.init(null, tmf.getTrustManagers(), new SecureRandom());
132+
133+
try (SSLSocket client = (SSLSocket) sslContext.getSocketFactory()
134+
.createSocket(InetAddress.getLoopbackAddress(), port)) {
135+
client.setSoTimeout(5000);
136+
// Depending on timing, JSSE surfaces the server-side need-client-auth rejection
137+
// as SSLException or as SocketException ("Connection reset") — both prove the
138+
// handshake failed closed.
139+
assertThrows(IOException.class, client::startHandshake);
140+
}
141+
142+
acceptor.join(5000);
143+
assertTrue(serverError.get() instanceof IOException,
144+
"Server-side handshake must fail; got: " + serverError.get());
145+
} finally {
146+
serverTransport.close();
147+
}
148+
}
68149
}

0 commit comments

Comments
 (0)