Skip to content

Commit

Permalink
MINOR: Remove TLS renegotiation code
Browse files Browse the repository at this point in the history 
This has been disabled since the start and since
it's removed in TLS 1.3, there are no plans to
ever support it.

Author: Ismael Juma <[email protected]>

Reviewers: Rajini Sivaram <[email protected]>

Closes apache#4034 from ijuma/remove-tls-renegotiation-support
  • Loading branch information
@ijuma
ijuma committed Oct 27, 2017
1 parent 4e8ad90 commit f4e9c84
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 91 deletions.
47 changes: 23 additions & 24 deletions clients/src/main/java/org/apache/kafka/common/network/SslTransportLayer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ private enum State {
private final SSLEngine sslEngine;
private final SelectionKey key;
private final SocketChannel socketChannel;
private final boolean enableRenegotiation;

private HandshakeStatus handshakeStatus;
private SSLEngineResult handshakeResult;
Expand All @@ -69,25 +68,23 @@ private enum State {
private ByteBuffer emptyBuf = ByteBuffer.allocate(0);

public static SslTransportLayer create(String channelId, SelectionKey key, SSLEngine sslEngine) throws IOException {
// Disable renegotiation by default until we have fixed the known issues with the existing implementation
SslTransportLayer transportLayer = new SslTransportLayer(channelId, key, sslEngine, false);
SslTransportLayer transportLayer = new SslTransportLayer(channelId, key, sslEngine);
transportLayer.startHandshake();
return transportLayer;
}

// Prefer `create`, only use this in tests
SslTransportLayer(String channelId, SelectionKey key, SSLEngine sslEngine, boolean enableRenegotiation) throws IOException {
SslTransportLayer(String channelId, SelectionKey key, SSLEngine sslEngine) throws IOException {
this.channelId = channelId;
this.key = key;
this.socketChannel = (SocketChannel) key.channel();
this.sslEngine = sslEngine;
this.enableRenegotiation = enableRenegotiation;
}

/**
* starts sslEngine handshake process
*/
// Visible for testing
protected void startHandshake() throws IOException {
if (state != null)
throw new IllegalStateException("startHandshake() can only be called once, state " + state);

this.netReadBuffer = ByteBuffer.allocate(netReadBufferSize());
this.netWriteBuffer = ByteBuffer.allocate(netWriteBufferSize());
Expand Down Expand Up @@ -240,9 +237,10 @@ protected boolean flush(ByteBuffer buf) throws IOException {
*/
@Override
public void handshake() throws IOException {
// Reset state to support renegotiation. This can be removed if renegotiation support is removed.
if (state == State.READY)
state = State.HANDSHAKE;
throw renegotiationException();
if (state == State.CLOSING)
throw closingException();

int read = 0;
try {
Expand Down Expand Up @@ -369,12 +367,13 @@ private void doHandshake() throws IOException {
}
}

private void renegotiate() throws IOException {
if (!enableRenegotiation)
throw new SSLHandshakeException("Renegotiation is not supported");
handshake();
private SSLHandshakeException renegotiationException() throws IOException {
return new SSLHandshakeException("Renegotiation is not supported");
}

private IllegalStateException closingException() {
throw new IllegalStateException("Channel is in closing state");
}

/**
* Executes the SSLEngine tasks needed.
Expand Down Expand Up @@ -513,10 +512,10 @@ public int read(ByteBuffer dst) throws IOException {
netReadBuffer.compact();
// handle ssl renegotiation.
if (unwrapResult.getHandshakeStatus() != HandshakeStatus.NOT_HANDSHAKING && unwrapResult.getStatus() == Status.OK) {
log.trace("SSLChannel Read begin renegotiation channelId {}, appReadBuffer pos {}, netReadBuffer pos {}, netWriteBuffer pos {}",
channelId, appReadBuffer.position(), netReadBuffer.position(), netWriteBuffer.position());
renegotiate();
break;
log.trace("Renegotiation requested, but it is not supported, channelId {}, " +
"appReadBuffer pos {}, netReadBuffer pos {}, netWriteBuffer pos {}", channelId,
appReadBuffer.position(), netReadBuffer.position(), netWriteBuffer.position());
throw renegotiationException();
}

if (unwrapResult.getStatus() == Status.OK) {
Expand Down Expand Up @@ -615,8 +614,10 @@ public long read(ByteBuffer[] dsts, int offset, int length) throws IOException {
@Override
public int write(ByteBuffer src) throws IOException {
int written = 0;
if (state == State.CLOSING) throw new IllegalStateException("Channel is in closing state");
if (state != State.READY) return written;
if (state == State.CLOSING)
throw closingException();
if (state != State.READY)
return written;

if (!flush(netWriteBuffer))
return written;
Expand All @@ -626,10 +627,8 @@ public int write(ByteBuffer src) throws IOException {
netWriteBuffer.flip();

//handle ssl renegotiation
if (wrapResult.getHandshakeStatus() != HandshakeStatus.NOT_HANDSHAKING && wrapResult.getStatus() == Status.OK) {
renegotiate();
return written;
}
if (wrapResult.getHandshakeStatus() != HandshakeStatus.NOT_HANDSHAKING && wrapResult.getStatus() == Status.OK)
throw renegotiationException();

if (wrapResult.getStatus() == Status.OK) {
written = wrapResult.bytesConsumed();
Expand Down
68 changes: 2 additions & 66 deletions clients/src/test/java/org/apache/kafka/common/network/SslSelectorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import org.apache.kafka.common.memory.SimpleMemoryPool;
import org.apache.kafka.common.metrics.Metrics;
import org.apache.kafka.common.security.auth.SecurityProtocol;
import org.apache.kafka.common.security.ssl.SslFactory;
import org.apache.kafka.common.utils.LogContext;
import org.apache.kafka.common.utils.MockTime;
import org.apache.kafka.test.TestSslUtils;
Expand All @@ -31,7 +30,6 @@
import java.io.File;
import java.io.IOException;
import java.net.InetSocketAddress;
import java.nio.channels.SelectionKey;
import java.nio.channels.ServerSocketChannel;
import java.nio.channels.SocketChannel;
import java.util.ArrayList;
Expand Down Expand Up @@ -80,72 +78,10 @@ public SecurityProtocol securityProtocol() {
}

/**
* Tests that SSL renegotiation initiated by the server are handled correctly by the client
* @throws Exception
* Renegotiation is not supported since it is potentially unsafe and it has been removed in TLS 1.3
*/
@Test
public void testRenegotiation() throws Exception {
ChannelBuilder channelBuilder = new SslChannelBuilder(Mode.CLIENT) {
@Override
protected SslTransportLayer buildTransportLayer(SslFactory sslFactory, String id, SelectionKey key, String host) throws IOException {
SocketChannel socketChannel = (SocketChannel) key.channel();
SslTransportLayer transportLayer = new SslTransportLayer(id, key,
sslFactory.createSslEngine(host, socketChannel.socket().getPort()),
true);
transportLayer.startHandshake();
return transportLayer;
}
};
channelBuilder.configure(sslClientConfigs);
Selector selector = new Selector(5000, metrics, time, "MetricGroup2", channelBuilder, new LogContext());
try {
int reqs = 500;
String node = "0";
// create connections
InetSocketAddress addr = new InetSocketAddress("localhost", server.port);
selector.connect(node, addr, BUFFER_SIZE, BUFFER_SIZE);

// send echo requests and receive responses
int requests = 0;
int responses = 0;
int renegotiates = 0;
while (!selector.isChannelReady(node)) {
selector.poll(1000L);
}
selector.send(createSend(node, node + "-" + 0));
requests++;

// loop until we complete all requests
while (responses < reqs) {
selector.poll(0L);
if (responses >= 100 && renegotiates == 0) {
renegotiates++;
server.renegotiate();
}
assertEquals("No disconnects should have occurred.", 0, selector.disconnected().size());

// handle any responses we may have gotten
for (NetworkReceive receive : selector.completedReceives()) {
String[] pieces = asString(receive).split("-");
assertEquals("Should be in the form 'conn-counter'", 2, pieces.length);
assertEquals("Check the source", receive.source(), pieces[0]);
assertEquals("Check that the receive has kindly been rewound", 0, receive.payload().position());
assertEquals("Check the request counter", responses, Integer.parseInt(pieces[1]));
responses++;
}

// prepare new sends for the next round
for (int i = 0; i < selector.completedSends().size() && requests < reqs && selector.isChannelReady(node); i++, requests++) {
selector.send(createSend(node, node + "-" + requests));
}
}
} finally {
selector.close();
}
}

@Test
public void testDisabledRenegotiation() throws Exception {
public void testRenegotiationFails() throws Exception {
String node = "0";
// create connections
InetSocketAddress addr = new InetSocketAddress("localhost", server.port);
Expand Down
2 changes: 1 addition & 1 deletion clients/src/test/java/org/apache/kafka/common/network/SslTransportLayerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -817,7 +817,7 @@ class TestSslTransportLayer extends SslTransportLayer {
private final AtomicInteger numDelayedFlushesRemaining;

public TestSslTransportLayer(String channelId, SelectionKey key, SSLEngine sslEngine) throws IOException {
super(channelId, key, sslEngine, false);
super(channelId, key, sslEngine);
this.netReadBufSize = new ResizeableBufferSize(netReadBufSizeOverride);
this.netWriteBufSize = new ResizeableBufferSize(netWriteBufSizeOverride);
this.appBufSize = new ResizeableBufferSize(appBufSizeOverride);
Expand Down

0 comments on commit f4e9c84

Please sign in to comment.