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

openssl: do not log excess "TLS app data" lines for TLS 1.3 #3281

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@Lekensteyn
Member

Lekensteyn commented Nov 16, 2018

The SSL_CTX_set_msg_callback callback is not just called for the
Handshake or Alert protocols, but also for the raw record header
(SSL3_RT_HEADER) and the decrypted inner record type
(SSL3_RT_INNER_CONTENT_TYPE). Be sure to ignore the latter to avoid
excess debug spam when using curl -v against a TLSv1.3-enabled server:

* TLSv1.3 (IN), TLS app data, [no content] (0):

(Following this message, another callback for the decrypted
handshake/alert messages will be be present anyway.)


Note that SSL3_RT_HEADER is only defined since 2012 via openssl/openssl@36b5bb6, otherwise the ssl_ver check could have been replaced by content_type != SSL3_RT_HEADER.

In #2403 (comment), @jay already questioned why this message needs to be logged. For symmetry with ignoring raw record headers, I think dropping the message is a good idea.

openssl: do not log excess "TLS app data" lines for TLS 1.3
The SSL_CTX_set_msg_callback callback is not just called for the
Handshake or Alert protocols, but also for the raw record header
(SSL3_RT_HEADER) and the decrypted inner record type
(SSL3_RT_INNER_CONTENT_TYPE). Be sure to ignore the latter to avoid
excess debug spam when using `curl -v` against a TLSv1.3-enabled server:

    * TLSv1.3 (IN), TLS app data, [no content] (0):

(Following this message, another callback for the decrypted
handshake/alert messages will be be present anyway.)

@Lekensteyn Lekensteyn added the SSL/TLS label Nov 16, 2018

@Lekensteyn Lekensteyn requested a review from jay Nov 16, 2018

@jay

This comment has been minimized.

Member

jay commented Nov 16, 2018

I think dropping the message is a good idea.

I agree, 'No content' messages did not prove to be useful. We are still showing the follow up messages so it should be fine. Thanks

@jay jay closed this in 27e4ac2 Nov 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment