Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SslHandler to fail handshake and pending writes if non-application write fails #9240

Merged
merged 1 commit into from Jun 16, 2019

Conversation

Projects
None yet
5 participants
@Scottmitch
Copy link
Member

commented Jun 13, 2019

Motivation:
SslHandler must generate control data as part of the TLS protocol, for example
to do handshakes. SslHandler doesn't capture the status of the future
corresponding to the writes when writing this control (aka non-application
data). If there is another handler before the SslHandler that wants to fail
these writes the SslHandler will not detect the failure and we must wait until
the handshake timeout to detect a failure.

Modifications:

  • SslHandler should detect if non application writes fail, tear down the
    channel, and clean up any pending state.

Result:
SslHandler detects non application write failures and cleans up immediately.

SslHandler to fail handshake and pending writes if non-application wr…
…ite fails

Motivation:
SslHandler must generate control data as part of the TLS protocol, for example
to do handshakes. SslHandler doesn't capture the status of the future
corresponding to the writes when writing this control (aka non-application
data). If there is another handler before the SslHandler that wants to fail
these writes the SslHandler will not detect the failure and we must wait until
the handshake timeout to detect a failure.

Modifications:
- SslHandler should detect if non application writes fail, tear down the
channel, and clean up any pending state.

Result:
SslHandler detects non application write failures and cleans up immediately.

@Scottmitch Scottmitch requested a review from normanmaurer Jun 13, 2019

@normanmaurer normanmaurer added this to the 4.1.37.Final milestone Jun 14, 2019

@@ -934,7 +934,15 @@ private boolean wrapNonAppData(ChannelHandlerContext ctx, boolean inUnwrap) thro
SSLEngineResult result = wrap(alloc, engine, Unpooled.EMPTY_BUFFER, out);

if (result.bytesProduced() > 0) {
ctx.write(out);
ctx.write(out).addListener(new ChannelFutureListener() {

This comment has been minimized.

Copy link
@johnou

johnou Jun 14, 2019

Contributor

will this add much overhead?

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Jun 14, 2019

Author Member

I didn't benchmark it but I don't anticipate a single future listener to add much. This is limited to "control" data and the impacts of keeping resources around until the handshake timeout fires (in the event the transport doesn't fail) are also non-zero.

@carl-mastrangelo
Copy link
Member

left a comment

LGTM

@normanmaurer normanmaurer merged commit 96feca1 into netty:4.1 Jun 16, 2019

3 checks passed

pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details
@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

@Scottmitch thanks!

normanmaurer added a commit that referenced this pull request Jun 16, 2019

SslHandler to fail handshake and pending writes if non-application wr…
…ite fails (#9240)

Motivation:
SslHandler must generate control data as part of the TLS protocol, for example
to do handshakes. SslHandler doesn't capture the status of the future
corresponding to the writes when writing this control (aka non-application
data). If there is another handler before the SslHandler that wants to fail
these writes the SslHandler will not detect the failure and we must wait until
the handshake timeout to detect a failure.

Modifications:
- SslHandler should detect if non application writes fail, tear down the
channel, and clean up any pending state.

Result:
SslHandler detects non application write failures and cleans up immediately.

@Scottmitch Scottmitch deleted the Scottmitch:ssl_handler_non_app_write_fail branch Jun 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.