Skip to content

Commit

Permalink
Correctly handle optional client auth on the client (java-native-acce…
Browse files Browse the repository at this point in the history 
…ss#567)

Motivation:

When client auth is optional we must not fail the handshake on the
client side when no keymanager was configured on the client side.

Modifications:

- Fix handling on the client-side
- Add testcase
- Fix test

Result:

Fixes netty/netty-incubator-codec-quic#566
  • Loading branch information
@normanmaurer
normanmaurer authored Aug 22, 2023
1 parent b841ddd commit bd4fd4a
Show file tree
Hide file tree
Showing 3 changed files with 127 additions and 17 deletions.
9 changes: 7 additions & 2 deletions ...lasses-quic/src/main/java/io/netty/incubator/codec/quic/BoringSSLCertificateCallback.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ final class BoringSSLCertificateCallback {
KEY_TYPE_EC_RSA,
KEY_TYPE_EC_EC)));

// Directly returning this is safe as we never modify it within our JNI code.
private static final long[] NO_KEY_MATERIAL_CLIENT_SIDE = new long[] { 0, 0 };

private final QuicheQuicSslEngineMap engineMap;
private final X509ExtendedKeyManager keyManager;
private final String password;
Expand All @@ -102,7 +105,9 @@ long[] handle(long ssl, byte[] keyTypeBytes, byte[][] asn1DerEncodedPrincipals,

try {
if (keyManager == null) {
engineMap.remove(ssl);
if (engine.getUseClientMode()) {
return NO_KEY_MATERIAL_CLIENT_SIDE;
}
return null;
}
if (engine.getUseClientMode()) {
Expand Down Expand Up @@ -171,7 +176,7 @@ private long[] selectKeyMaterialClientSide(long ssl, QuicheQuicSslEngine engine,
if (alias != null) {
return selectMaterial(ssl, engine, alias) ;
}
return null;
return NO_KEY_MATERIAL_CLIENT_SIDE;
}

private long[] selectMaterial(long ssl, QuicheQuicSslEngine engine, String alias) {
Expand Down
5 changes: 5 additions & 0 deletions codec-native-quic/src/main/c/netty_quic_boringssl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,11 @@ static int quic_certificate_cb(SSL* ssl, void* arg) {
SSL_set_ex_data(ssl, sslTaskIdx, NULL);
netty_boringssl_ssl_task_free(e, ssl_task);

if (pkey == NULL && cchain == NULL) {
// No key material found.
return 1;
}

int numCerts = sk_CRYPTO_BUFFER_num(cchain);
if (numCerts == 0) {
goto done;
Expand Down
130 changes: 115 additions & 15 deletions codec-native-quic/src/test/java/io/netty/incubator/codec/quic/QuicChannelConnectTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
import io.netty.util.ReferenceCountUtil;
import io.netty.util.concurrent.Future;
import io.netty.util.concurrent.ImmediateEventExecutor;
import io.netty.util.concurrent.ImmediateExecutor;
import org.hamcrest.CoreMatchers;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.Timeout;
Expand All @@ -45,6 +44,7 @@
import javax.net.ssl.SSLEngineResult;
import javax.net.ssl.SSLException;
import javax.net.ssl.SSLHandshakeException;
import javax.net.ssl.X509ExtendedKeyManager;
import javax.net.ssl.X509ExtendedTrustManager;
import javax.net.ssl.X509TrustManager;
import java.io.File;
Expand All @@ -56,6 +56,8 @@
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.security.Principal;
import java.security.PrivateKey;
import java.security.Signature;
import java.security.SignatureException;
import java.security.cert.CertificateException;
Expand Down Expand Up @@ -930,23 +932,92 @@ InsecureQuicTokenHandler.INSTANCE, new ChannelInboundHandlerAdapter(),

@ParameterizedTest
@MethodSource("newSslTaskExecutors")
public void testConnectMutualAuthSuccess(Executor executor) throws Throwable {
public void testConnectMutualAuthRequiredSuccess(Executor executor) throws Throwable {
testConnectMutualAuthSuccess(executor, MutalAuthTestMode.REQUIRED);
}

@ParameterizedTest
@MethodSource("newSslTaskExecutors")
public void testConnectMutualAuthOptionalWithCertSuccess(Executor executor) throws Throwable {
testConnectMutualAuthSuccess(executor, MutalAuthTestMode.OPTIONAL_CERT);
}

@ParameterizedTest
@MethodSource("newSslTaskExecutors")
public void testConnectMutualAuthOptionalWithoutKeyManagerSuccess(Executor executor) throws Throwable {
testConnectMutualAuthSuccess(executor, MutalAuthTestMode.OPTIONAL_NO_KEYMANAGER);
}

@ParameterizedTest
@MethodSource("newSslTaskExecutors")
public void testConnectMutualAuthOptionalWithoutKeyInKeyManagerSuccess(Executor executor) throws Throwable {
testConnectMutualAuthSuccess(executor, MutalAuthTestMode.OPTIONAL_NO_KEY_IN_KEYMANAGER);
}

private void testConnectMutualAuthSuccess(Executor executor, MutalAuthTestMode mode) throws Throwable {
Channel server = QuicTestUtils.newServer(QuicTestUtils.newQuicServerBuilder(executor,
QuicSslContextBuilder.forServer(
QuicTestUtils.SELF_SIGNED_CERTIFICATE.privateKey(), null,
QuicTestUtils.SELF_SIGNED_CERTIFICATE.privateKey(), null,
QuicTestUtils.SELF_SIGNED_CERTIFICATE.certificate()).trustManager(
InsecureTrustManagerFactory.INSTANCE)
.applicationProtocols(QuicTestUtils.PROTOS).clientAuth(ClientAuth.REQUIRE).build()),
InsecureTrustManagerFactory.INSTANCE)
.applicationProtocols(QuicTestUtils.PROTOS)
.clientAuth(mode == MutalAuthTestMode.REQUIRED ?
ClientAuth.REQUIRE : ClientAuth.OPTIONAL).build()),
InsecureQuicTokenHandler.INSTANCE, new ChannelInboundHandlerAdapter(),
new ChannelInboundHandlerAdapter());
InetSocketAddress address = (InetSocketAddress) server.localAddress();

QuicSslContextBuilder clientSslCtxBuilder = QuicSslContextBuilder.forClient()
.trustManager(InsecureTrustManagerFactory.INSTANCE)
.applicationProtocols(QuicTestUtils.PROTOS);
switch (mode) {
case OPTIONAL_CERT:
case REQUIRED:
clientSslCtxBuilder.keyManager(
QuicTestUtils.SELF_SIGNED_CERTIFICATE.privateKey(), null,
QuicTestUtils.SELF_SIGNED_CERTIFICATE.certificate());
break;
case OPTIONAL_NO_KEY_IN_KEYMANAGER:
clientSslCtxBuilder.keyManager(new X509ExtendedKeyManager() {
@Override
public String[] getClientAliases(String keyType, Principal[] issuers) {
throw new UnsupportedOperationException();
}

@Override
public String chooseClientAlias(String[] keyType, Principal[] issuers, Socket socket) {
return null;
}

@Override
public String[] getServerAliases(String keyType, Principal[] issuers) {
throw new UnsupportedOperationException();
}

@Override
public String chooseServerAlias(String keyType, Principal[] issuers, Socket socket) {
throw new UnsupportedOperationException();
}

@Override
public X509Certificate[] getCertificateChain(String alias) {
throw new UnsupportedOperationException();
}

@Override
public PrivateKey getPrivateKey(String alias) {
throw new UnsupportedOperationException();
}
}, null);
break;
case OPTIONAL_NO_KEYMANAGER:
break;
default:
throw new IllegalStateException();
}

Channel channel = QuicTestUtils.newClient(QuicTestUtils.newQuicClientBuilder(executor,
QuicSslContextBuilder.forClient()
.trustManager(InsecureTrustManagerFactory.INSTANCE).keyManager(
QuicTestUtils.SELF_SIGNED_CERTIFICATE.privateKey(), null,
QuicTestUtils.SELF_SIGNED_CERTIFICATE.certificate())
.applicationProtocols(QuicTestUtils.PROTOS).build()));
clientSslCtxBuilder.build()));
try {
ChannelActiveVerifyHandler clientQuicChannelHandler = new ChannelActiveVerifyHandler();
QuicChannel quicChannel = QuicTestUtils.newQuicChannelBootstrap(channel)
Expand All @@ -968,32 +1039,61 @@ InsecureQuicTokenHandler.INSTANCE, new ChannelInboundHandlerAdapter(),
}
}

private enum MutalAuthTestMode {
REQUIRED,
OPTIONAL_CERT,
OPTIONAL_NO_KEYMANAGER,
OPTIONAL_NO_KEY_IN_KEYMANAGER
}

@ParameterizedTest
@MethodSource("newSslTaskExecutors")
public void testConnectMutualAuthFailsIfClientNotSendCertificate(Executor executor) throws Throwable {
CountDownLatch latch = new CountDownLatch(1);
AtomicReference<Throwable> causeRef = new AtomicReference<>();

Channel server = QuicTestUtils.newServer(QuicTestUtils.newQuicServerBuilder(executor,
QuicSslContextBuilder.forServer(
QuicTestUtils.SELF_SIGNED_CERTIFICATE.privateKey(), null,
QuicTestUtils.SELF_SIGNED_CERTIFICATE.certificate())
.trustManager(InsecureTrustManagerFactory.INSTANCE)
.applicationProtocols(QuicTestUtils.PROTOS).clientAuth(ClientAuth.REQUIRE).build()),
InsecureQuicTokenHandler.INSTANCE, new ChannelInboundHandlerAdapter(),
InsecureQuicTokenHandler.INSTANCE, new ChannelInboundHandlerAdapter() {
@Override
public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception {
causeRef.compareAndSet(null, cause);
latch.countDown();
ctx.close();
}
},
new ChannelInboundHandlerAdapter());
InetSocketAddress address = (InetSocketAddress) server.localAddress();
Channel channel = QuicTestUtils.newClient(QuicTestUtils.newQuicClientBuilder(executor,
QuicSslContextBuilder.forClient()
.trustManager(InsecureTrustManagerFactory.INSTANCE)
.applicationProtocols(QuicTestUtils.PROTOS).build()));
QuicChannel client = null;
try {
Throwable cause = QuicTestUtils.newQuicChannelBootstrap(channel)
.handler(new ChannelInboundHandlerAdapter())
client = QuicTestUtils.newQuicChannelBootstrap(channel)
.handler(new ChannelInboundHandlerAdapter() {
@Override
public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception {
cause.printStackTrace();
}
})
.streamHandler(new ChannelInboundHandlerAdapter())
.remoteAddress(address)
.connect()
.await().cause();
assertThat(cause, Matchers.instanceOf(SSLException.class));
.get();
latch.await();

assertThat(causeRef.get(), Matchers.instanceOf(SSLHandshakeException.class));
} finally {
server.close().sync();

if (client != null) {
client.close().sync();
}
// Close the parent Datagram channel as well.
channel.close().sync();

Expand Down

0 comments on commit bd4fd4a

Please sign in to comment.