iostream: downgrade the SSL handshake-error log to INFO#3676
Open
HrachShah wants to merge 1 commit into
Open
Conversation
The SSL handshake-error branch in SSLIOStream._do_ssl_handshake fires
on a variety of remote conditions that the application cannot
control:
- a client that fails to validate the server's certificate sends a
tls alert, and the server's handshake then raises and logs here
- a client that drops the connection before speaking TLS
(SSL_ERROR_EOF / SSL_ERROR_ZERO_RETURN) which lands in the
SSL_ERROR_SSL branch via SSL_ERROR_SYSCALL
- premature closes from port scanners and other uninvited probes
None of these are a problem with the server. They were logged at
WARNING, which suggests to the operator that something is wrong on
their end, but the only thing the operator can do is configure
certificate validation more strictly, which does not change the
volume of these messages. Drop the level to INFO so the application
can opt in to the messages if it cares, and the default operator
view is not polluted by clients making bad requests.
Three existing tests expected the old WARNING level via ExpectLog;
update them to pass level=logging.INFO so the message is still
captured (ExpectLog without an explicit level is becoming a
DeprecationWarning match in Tornado 7.0 anyway).
Fixes tornadoweb#3347.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3347.
The SSLIOStream handshake-error log was emitted at WARNING level, but
the events that cause it are almost always out of the server's control:
and fails validation. The TLS alert the client sends back then
causes the server's handshake to fail with SSL_ERROR_SSL or
SSL_ERROR_SYSCALL, which is what gets logged. The application
cannot prevent the client from doing this.
the socket before do_handshake completes.
In all of these cases the operator does not need to be warned; the
event is a normal occurrence on a server that is exposed to the
internet. Downgrading the log to INFO matches the level already
used for similar client-driven noise such as "Unsatisfiable read,
closing connection" and "Connect error". Applications that want
to surface a warning can install their own handler at the INFO
level.
The server-side error path still closes the connection and surfaces
the underlying exception via close(exc_info=err), so callers that
do care about a specific failure have not lost any information.
Tests:
TestIOStreamStartTLS.test_check_hostname: now pass level=
logging.INFO on their ExpectLog blocks because the SSL Error
message is emitted at INFO instead of WARNING.
certificate" ExpectLog on the server side is now expected at
INFO.