Conversation
ed5b7a0 to
398cb16
Compare
|
|
||
| fprintf(stderr, "Error reading from connection: '%s' %d\n", s2n_strerror(s2n_errno, "EN"), s2n_connection_get_alert(conn)); |
There was a problem hiding this comment.
s2n_connection_get_alert actually returns an error (setting s2n_errno) if called when there isn't an alert. So we should only call it for S2N_ERR_T_ALERT, and we definitely shouldn't call it before s2n_strerror, because calling it might change the output of s2n_strerror. The previous code here seemed to be compiled so that s2n_connection_get_alert executed before s2n_strerror. This led to a S2N_ERR_CLOSED being reported as S2N_ERR_NO_ALERT.
|
|
||
| fprintf(stderr, "Error reading from connection: '%s' %d\n", s2n_strerror(s2n_errno, "EN"), s2n_connection_get_alert(conn)); |
| # Give the server one last message to send without a corresponding send_marker. | ||
| # The message will never be sent, but waiting to send it will prevent the server | ||
| # from closing the connection before the client can. | ||
| if mode is Provider.ServerMode: |
There was a problem hiding this comment.
How does this change relate to End of Data behavior?
There was a problem hiding this comment.
s_server closes its socket in response to us closing stdin: https://github.com/openssl/openssl/blob/master/apps/s_server.c#L2680-L2687 It doesn't send a close_notify in that case :(
We close stdin once all messages are sent, which is what I was trying to explain in this comment. Any suggestions to make the comment more useful?
| assert results.exit_code != 0 | ||
| assert not renegotiate_was_started(results) | ||
| assert to_bytes("TLS alert received") in results.stderr | ||
| assert to_bytes("Received alert: 40") in results.stderr |
There was a problem hiding this comment.
Did we ever set up any way of letting the user know what alert 40 means 🤔 ?
There was a problem hiding this comment.
Nope :) We just return alert codes. You'd have to look up their meaning in the RFC.
| /* Don't propagate the error if we already read some bytes. | ||
| * We'll report S2N_ERR_CLOSED on the next call. | ||
| */ | ||
| if (s2n_errno == S2N_ERR_CLOSED && bytes_read) { |
There was a problem hiding this comment.
Do you need to set the blocked status for this case?
There was a problem hiding this comment.
That's a good question.
I think returning S2N_BLOCKED_ON_READ (like we do if we don't change it here) is correct. If this code is triggered, that means that we attempted another read, which means that the caller requested more data than we're returning with bytes_read. And we do really want them to call again in that case, because we want to give them the S2N_ERR_CLOSED error.
Co-authored-by: Sam Clark <3758302+goatgoose@users.noreply.github.com>
Resolved issues:
resolves #3936
Description of changes:
s2n-tls was reporting "end of data" to the application even if the peer closed the transport unexpectedly. Unless we receive a close_notify, we should not indicate end-of-data.
Testing:
This is another bug I found while writing s2n_alerts_protocol_test, so more tests in that file and updates to correct expectations :)
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.