andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Correctly report back when we fail to select the key material and ens… (#10610)

Browse Source 
Motivation:

We need to let openssl know that we failed to find the key material so it will produce an alert for the remote peer to consume. Beside this we also need to ensure we wrap(...) until we produced everything as otherwise the remote peer may see partial data when an alert is produced in multiple steps.

Modifications:

- Correctly throw if we could not find the keymaterial
- wrap until we produced everything
- Add test

Result:

Correctly handle the case when key material could not be found
This commit is contained in:
Norman Maurer 2020-09-26 08:56:16 +02:00
parent d92d92a923
commit ced5faa440
4 changed files with 152 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
handler/src/main/java/io/netty/handler/ssl/OpenSslKeyMaterialManager.java
View File

@ -16,11 +16,13 @@
package io.netty.handler.ssl;
import javax.net.ssl.SSLException;
import javax.net.ssl.SSLHandshakeException;
import javax.net.ssl.X509ExtendedKeyManager;
import javax.net.ssl.X509KeyManager;
import javax.security.auth.x500.X500Principal;
import java.security.PrivateKey;
import java.security.cert.X509Certificate;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
@ -68,8 +70,10 @@ final class OpenSslKeyMaterialManager {
void setKeyMaterialServerSide(ReferenceCountedOpenSslEngine engine) throws SSLException {
String[] authMethods = engine.authMethods();
if (authMethods.length == 0) {
return;
throw new SSLHandshakeException("Unable to find key material");
}
boolean matched = false;
// authMethods may contain duplicates but call chooseServerAlias(...) may be expensive. So let's ensure
// we filter out duplicates.
Set<String> authMethodsSet = new LinkedHashSet<>(authMethods.length);
@ -83,9 +87,14 @@ final class OpenSslKeyMaterialManager {
if (!setKeyMaterial(engine, alias)) {
return;
}
matched = true;
}
}
}
if (!matched) {
throw new SSLHandshakeException("Unable to find key material for auth method(s): "
+ Arrays.toString(authMethods));
}
}
void setKeyMaterialClientSide(ReferenceCountedOpenSslEngine engine, String[] keyTypes,

6
handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java
View File

@ -218,8 +218,12 @@ public final class ReferenceCountedOpenSslServerContext extends ReferenceCounted
// OpenJDK SSLEngineImpl does.
keyManagerHolder.setKeyMaterialServerSide(engine);
} catch (Throwable cause) {
logger.debug("Failed to set the server-side key material", cause);
engine.initHandshakeException(cause);
if (cause instanceof Exception) {
throw (Exception) cause;
}
throw new SSLException(cause);
}
}
}

7
handler/src/main/java/io/netty/handler/ssl/SslHandler.java
View File

@ -904,6 +904,13 @@ public class SslHandler extends ByteToMessageDecoder {
b = null;
}
finishWrap(ctx, b, p, inUnwrap, false);
// If we are expected to wrap again and we produced some data we need to ensure there
// is something in the queue to process as otherwise we will not try again before there
// was more added. Failing to do so may fail to produce an alert that can be
// consumed by the remote peer.
if (result.bytesProduced() > 0 && pendingUnencryptedWrites.isEmpty()) {
pendingUnencryptedWrites.add(Unpooled.EMPTY_BUFFER);
}
break;
}
case NEED_UNWRAP:

134
handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
View File

@ -58,6 +58,7 @@ import io.netty.util.concurrent.Promise;
import io.netty.util.internal.EmptyArrays;
import org.hamcrest.CoreMatchers;
import org.hamcrest.Matchers;
import org.junit.Assume;
import org.junit.Test;
import java.net.InetSocketAddress;
@ -67,6 +68,8 @@ import java.security.NoSuchAlgorithmException;
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
import java.util.Collections;
import java.util.Queue;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.CompletionException;
@ -89,10 +92,7 @@ import javax.net.ssl.SSLProtocolException;
import javax.net.ssl.X509ExtendedTrustManager;
import static io.netty.buffer.Unpooled.wrappedBuffer;
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.instanceOf;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.nullValue;
import static org.hamcrest.CoreMatchers.*;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
@ -1328,4 +1328,130 @@ public class SslHandlerTest {
group.shutdownGracefully();
}
}
@Test
public void testHandshakeFailureCipherMissmatchTLSv12Jdk() throws Exception {
testHandshakeFailureCipherMissmatch(SslProvider.JDK, false);
}
@Test
public void testHandshakeFailureCipherMissmatchTLSv13Jdk() throws Exception {
Assume.assumeTrue(SslProvider.isTlsv13Supported(SslProvider.JDK));
testHandshakeFailureCipherMissmatch(SslProvider.JDK, true);
}
@Test
public void testHandshakeFailureCipherMissmatchTLSv12OpenSsl() throws Exception {
Assume.assumeTrue(OpenSsl.isAvailable());
testHandshakeFailureCipherMissmatch(SslProvider.OPENSSL, false);
}
@Test
public void testHandshakeFailureCipherMissmatchTLSv13OpenSsl() throws Exception {
Assume.assumeTrue(OpenSsl.isAvailable());
Assume.assumeTrue(SslProvider.isTlsv13Supported(SslProvider.OPENSSL));
Assume.assumeFalse("BoringSSL does not support setting ciphers for TLSv1.3 explicit", OpenSsl.isBoringSSL());
testHandshakeFailureCipherMissmatch(SslProvider.OPENSSL, true);
}
private static void testHandshakeFailureCipherMissmatch(SslProvider provider, boolean tls13) throws Exception {
final String clientCipher;
final String serverCipher;
final String protocol;
if (tls13) {
clientCipher = "TLS_AES_128_GCM_SHA256";
serverCipher = "TLS_AES_256_GCM_SHA384";
protocol = SslUtils.PROTOCOL_TLS_V1_3;
} else {
clientCipher = "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256";
serverCipher = "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384";
protocol = SslUtils.PROTOCOL_TLS_V1_2;
}
final SslContext sslClientCtx = SslContextBuilder.forClient()
.trustManager(InsecureTrustManagerFactory.INSTANCE)
.protocols(protocol)
.ciphers(Collections.singleton(clientCipher))
.sslProvider(provider).build();
final SelfSignedCertificate cert = new SelfSignedCertificate();
final SslContext sslServerCtx = SslContextBuilder.forServer(cert.key(), cert.cert())
.protocols(protocol)
.ciphers(Collections.singleton(serverCipher))
.sslProvider(provider).build();
EventLoopGroup group = new MultithreadEventLoopGroup(NioHandler.newFactory());
Channel sc = null;
Channel cc = null;
final SslHandler clientSslHandler = sslClientCtx.newHandler(UnpooledByteBufAllocator.DEFAULT);
final SslHandler serverSslHandler = sslServerCtx.newHandler(UnpooledByteBufAllocator.DEFAULT);
class SslEventHandler implements ChannelHandler {
private final AtomicReference<SslHandshakeCompletionEvent> ref;
SslEventHandler(AtomicReference<SslHandshakeCompletionEvent> ref) {
this.ref = ref;
}
@Override
public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exception {
if (evt instanceof SslHandshakeCompletionEvent) {
ref.set((SslHandshakeCompletionEvent) evt);
}
ctx.fireUserEventTriggered(evt);
}
}
final AtomicReference<SslHandshakeCompletionEvent> clientEvent =
new AtomicReference<SslHandshakeCompletionEvent>();
final AtomicReference<SslHandshakeCompletionEvent> serverEvent =
new AtomicReference<SslHandshakeCompletionEvent>();
try {
sc = new ServerBootstrap()
.group(group)
.channel(NioServerSocketChannel.class)
.childHandler(new ChannelInitializer<Channel>() {
@Override
protected void initChannel(Channel ch) throws Exception {
ch.pipeline().addLast(serverSslHandler);
ch.pipeline().addLast(new SslEventHandler(serverEvent));
}
})
.bind(new InetSocketAddress(0)).syncUninterruptibly().channel();
ChannelFuture future = new Bootstrap()
.group(group)
.channel(NioSocketChannel.class)
.handler(new ChannelInitializer<Channel>() {
@Override
protected void initChannel(Channel ch) {
ch.pipeline().addLast(clientSslHandler);
ch.pipeline().addLast(new SslEventHandler(clientEvent));
}
}).connect(sc.localAddress());
cc = future.syncUninterruptibly().channel();
Throwable clientCause = clientSslHandler.handshakeFuture().await().cause();
assertThat(clientCause, CoreMatchers.<Throwable>instanceOf(SSLException.class));
assertThat(clientCause.getCause(), not(CoreMatchers.<Throwable>instanceOf(ClosedChannelException.class)));
Throwable serverCause = serverSslHandler.handshakeFuture().await().cause();
assertThat(serverCause, CoreMatchers.<Throwable>instanceOf(SSLException.class));
assertThat(serverCause.getCause(), not(CoreMatchers.<Throwable>instanceOf(ClosedChannelException.class)));
cc.close().syncUninterruptibly();
sc.close().syncUninterruptibly();
Throwable eventClientCause = clientEvent.get().cause();
assertThat(eventClientCause, CoreMatchers.<Throwable>instanceOf(SSLException.class));
assertThat(eventClientCause.getCause(),
not(CoreMatchers.<Throwable>instanceOf(ClosedChannelException.class)));
Throwable serverEventCause = serverEvent.get().cause();
assertThat(serverEventCause, CoreMatchers.<Throwable>instanceOf(SSLException.class));
assertThat(serverEventCause.getCause(),
not(CoreMatchers.<Throwable>instanceOf(ClosedChannelException.class)));
} finally {
group.shutdownGracefully();
ReferenceCountUtil.release(sslClientCtx);
}
}
}