Skip to content

Commit

Permalink
SslHandler should call beginHanshake once for the initial handshake
Browse files Browse the repository at this point in the history
Motivation:
Not all SSLEngine implementations permit beginHandshake being called while a handshake is in progress during the initial handshake. We should ensure we only go through the initial handshake code once to prevent unexpected exceptions from being thrown.

Modifications:
- Only call beginHandshake if there is not currently a handshake in progress

Result:
SslHandler's handshake method is compatible with OpenSSLEngineImpl in Android 5.0+ and 6.0+.
Fixes #4718
  • Loading branch information
Scottmitch authored and normanmaurer committed Jan 28, 2016
1 parent af39cb6 commit e1d34ef
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 14 deletions.
26 changes: 12 additions & 14 deletions handler/src/main/java/io/netty/handler/ssl/OpenSslEngine.java
Expand Up @@ -148,7 +148,6 @@ private enum HandshakeState {

private HandshakeState handshakeState = HandshakeState.NOT_STARTED;
private boolean receivedShutdown;
@SuppressWarnings("UnusedDeclaration")
private volatile int destroyed;

private volatile ClientAuth clientAuth = ClientAuth.NONE;
Expand Down Expand Up @@ -226,13 +225,18 @@ public OpenSslEngine(long sslCtx, ByteBufAllocator alloc,
}

@Override
public SSLSession getHandshakeSession() {
if (handshakeState != HandshakeState.NOT_STARTED) {
// handshake started we are able to return the session.
public synchronized SSLSession getHandshakeSession() {
// Javadocs state return value should be:
// null if this instance is not currently handshaking, or if the current handshake has not
// progressed far enough to create a basic SSLSession. Otherwise, this method returns the
// SSLSession currently being negotiated.
switch(handshakeState) {
case NOT_STARTED:
case FINISHED:
return null;
default:
return session;
}
// As stated by the javadocs of getHandshakeSession() we should return null if the handshake not started yet.
return null;
}

/**
Expand Down Expand Up @@ -1250,18 +1254,12 @@ private SSLEngineResult.HandshakeStatus mayFinishHandshake(SSLEngineResult.Hands
@Override
public synchronized SSLEngineResult.HandshakeStatus getHandshakeStatus() {
// Check if we are in the initial handshake phase or shutdown phase
if (needPendingStatus()) {
return pendingStatus(SSL.pendingWrittenBytesInBIO(networkBIO));
}
return NOT_HANDSHAKING;
return needPendingStatus() ? pendingStatus(SSL.pendingWrittenBytesInBIO(networkBIO)) : NOT_HANDSHAKING;
}

private SSLEngineResult.HandshakeStatus getHandshakeStatus(int pending) {
// Check if we are in the initial handshake phase or shutdown phase
if (needPendingStatus()) {
return pendingStatus(pending);
}
return NOT_HANDSHAKING;
return needPendingStatus() ? pendingStatus(pending) : NOT_HANDSHAKING;
}

private boolean needPendingStatus() {
Expand Down
4 changes: 4 additions & 0 deletions handler/src/main/java/io/netty/handler/ssl/SslHandler.java
Expand Up @@ -1351,6 +1351,10 @@ public void operationComplete(Future<Channel> future) throws Exception {
}

handshakePromise = p = newHandshakePromise;
} else if (engine.getHandshakeStatus() != HandshakeStatus.NOT_HANDSHAKING) {
// Not all SSLEngine implementations support calling beginHandshake multiple times while a handshake
// is in progress. See https://github.com/netty/netty/issues/4718.
return;
} else {
// Forced to reuse the old handshake.
p = handshakePromise;
Expand Down

0 comments on commit e1d34ef

Please sign in to comment.