- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.1k
Propagate error from DotNetty TLS handshake failure to Akka.Remote #7824
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
Propagate error from DotNetty TLS handshake failure to Akka.Remote #7824
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self review
| public override void UserEventTriggered(IChannelHandlerContext context, object evt) | ||
| { | ||
| if (evt is TlsHandshakeCompletionEvent { IsSuccessful: false } tlsEvent) | ||
| { | ||
| var ex = tlsEvent.Exception ?? new Exception("TLS handshake failed."); | ||
| Log.Error(ex, "TLS handshake failed. Channel [{0}->{1}](Id={2})", | ||
| context.Channel.LocalAddress, context.Channel.RemoteAddress, context.Channel.Id); | ||
|  | ||
| // Best-effort surface to higher layers if listener already registered | ||
| NotifyListener(new UnderlyingTransportError(ex, | ||
| $"TLS handshake failed on channel [{context.Channel.LocalAddress}->{context.Channel.RemoteAddress}](Id={context.Channel.Id})")); | ||
|  | ||
| context.CloseAsync(); | ||
| return; // don't pass to next handlers | ||
| } | ||
|  | ||
| base.UserEventTriggered(context, evt); | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Server side fix, listen for TLS handshake failure event and log/propagate it.
| public override void UserEventTriggered(IChannelHandlerContext context, object evt) | ||
| { | ||
| if (evt is TlsHandshakeCompletionEvent tlsEvent) | ||
| { | ||
| if (tlsEvent.IsSuccessful) | ||
| { | ||
| _tlsHandshakePromise.TrySetResult(true); | ||
| } | ||
| else | ||
| { | ||
| var ex = tlsEvent.Exception ?? new Exception("TLS handshake failed."); | ||
| _tlsHandshakePromise.TrySetException(ex); | ||
| } | ||
| } | ||
|  | ||
| base.UserEventTriggered(context, evt); | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Client side fix, listen for TLS handshake failure and log/propagate it.
| Server side now logs the error appropriately: The client side logs the error appropriately:  | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #7823
Supercedes #7821
Summary
Changes
TlsHandshakeCompletionEventinTcpHandlers.UserEventTriggered:UnderlyingTransportErrorwhen possible.AssociateInternal)InvalidAssociationExceptionwith the TLS failure as inner exception on error.DotNettyTlsHandshakeFailureSpecto assert that server logs “TLS handshake failed …” at ERROR when cert lacks a private key and a client connects.Behavioral impact
Associatenow fails fast withInvalidAssociationExceptioncontaining the TLS error.