Refactor alerts to make behavior clear #4019
Conversation
| uint8_t writer_alert_out; | ||
| uint8_t reader_alert_out; | ||
| uint8_t reader_warning_out; | ||
| bool alert_sent; |
There was a problem hiding this comment.
Should alert_sent go where close_notify_queued was, to save a byte?
There was a problem hiding this comment.
I opened an issue for why we shouldn't do that: #4026
It's arguable whether alert_sent falls into this category, but I believe it does. Our documentation doesn't currently say it, but I think we have to support calling s2n_shutdown_send while s2n_recv is still active.
| if (conn->reader_warning_out) { | ||
| POSIX_GUARD_RESULT(s2n_alerts_write_warning(conn)); | ||
| conn->reader_warning_out = 0; | ||
| POSIX_GUARD(s2n_flush(conn, blocked)); |
There was a problem hiding this comment.
Recursion?? You found an appropriate use of recursion??
There was a problem hiding this comment.
I mean... I found a use of recursion ;) It's a little cleaner than the previous goto.
lrstewart
force-pushed
the
alerts_refactor
branch
from
1306f85 to
2bea8c8
Compare
| @@ -35,8 +35,8 @@ int main(int argc, char **argv) | |||
| BEGIN_TEST(); | |||
|
|
|||
| const uint8_t close_notify_alert[] = { | |||
| 2 /* AlertLevel = fatal */, | |||
| 0 /* AlertDescription = close_notify */ | |||
| S2N_TLS_ALERT_LEVEL_WARNING, | |||
There was a problem hiding this comment.
Shouldn't this be alert level fatal?
There was a problem hiding this comment.
I couldn't find a line in the RFC stating that the level should be warning, but you can see the message on the relevant commit: b0b3871 It also just makes sense that it would be a warning. Most implementations (including ours) probably ignore the level completely, but it'd be an easy implementation mistake to treat it as an arbitrary fatal error instead of a close_notify if the level was set to fatal.
Resolved issues:
resolves #3941
resolves #3937
Description of changes:
Currently, the s2n-tls alerts / shutdown logic is difficult to follow and fairly deceptive. This refactor simplifies the flow.
There are three alert cases to consider:
Generic close_notify alerts
Described in #3941. In summary, s2n-tls only sends close_notify alerts from s2n_shutdown, so we can safely remove the logic that appears to send them from s2n_flush.
Specific error alerts
Described in #3937. In summary, the alerts can be sent from s2n_flush, but that skips self-service blinding.
We only queue specific fatal alerts in a handful of places in the code (unsupported version alerts, and handshake failure alerts), all in s2n_negotiate. In each case, we also immediately return a fatal error, causing the connection to be marked closed and s2n_negotiate to fail with a fatal error. s2n_negotiate DOES call s2n_flush even if marked closed. However, in order to trigger that behavior, an application would need to be deliberately calling s2n_negotiate again despite a fatal error. That's not impossible: you could get that behavior if you ignored the error type and only checked
blocked, but in that case it would also look like all your connections failed with S2N_ERR_T_CLOSED, regardless of the actual underlying error.Because of the limited scope (only a few cases where we use a specific alert) and the already not recommended application logic required (calling s2n_negotiate again after a fatal error), I think this is behavior we can safely change in order to ensure blinding is enforced and simplify the code.
Warnings
Warnings do still need to be sent from s2n_flush. They're the only alert that isn't blinded and shouldn't wait for s2n_shutdown.
Call-outs:
Testing:
Existing tests + new ones pass. I added a lot of new tests recently for s2n_shutdown / alerts, so there should be sufficient testing to catch any regressions.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.