Close the connection after sending tls alerts in the queue #1323
Conversation
avbelov23
force-pushed
the
avb-1308
branch
3 times, most recently
from
9d3944d to
ee55cd2
Compare
There was a problem hiding this comment.
Reviewed once more and I see no issues with the code. However, I didn't review for locks and reference counting, only TCP stuff. So another review with deep attention to locking and reference counting is required.
Also please make sure that all functional tests pass and there are no deadlocks, stalled sockets and other anomalies in dmesg.
A backport to 0.6 is required.
There was a problem hiding this comment.
Reviewed once more and deeper. I paid attention to locking and reference counting this time and didn't find any issues there. However, there are other issues. Some of them we're discussed today.
The difference between ss_close() and ss_linkerror() is only in callback called - connection_error or connection_drop. Both the callbacks are the same for client side (but this might change in future). But with the change we break server connection failovering: for ss_tcp_data_ready() change if there is some error and ss_tcp_process_data() returns bad code, then we don't try to reestablish connection to a server. Similarly, for ss_tcp_state_change() change if a server closes connection (e.g. for system restart) we won't reconnect it.
We do send alert with SS_F_CONN_CLOSE: ttls_send_alert() -> ttls_write_record() -> __ttls_send_record() -> ttls_send_cb() -> tfw_tls_send() sets SS_F_CONN_CLOSE for ss_send(). Not all the allerts must lead to connection closing, but according to RFC 5246 7.2.2 transmission of a fatal alert must be followed by connection closing.
The same chapter says
Upon transmission or receipt of a fatal alert message, both parties immediately close the connection.
Chapter 7.2.1 says (see also TLS truncation attack discussion)
each party is required to send a close_notify alert before closing the write side of the connection. The other party MUST respond with a close_notify alert of its own and close down the connection immediately, discarding any pending writes. It is not required for the initiator of the close to wait for the responding close_notify alert before closing the read side of the connection.
This means that we do not complain to the RFC since ttls_handle_alert() doesn't reply with close_notify message. We should set Conn_Stop on receiving such alerts. This must be fixed and a functional test for connection closing, ensuring that we do send close_notify in response to fatal and close_notify alerts, must be added to tempesta-test.
RFC allows us to set Conn_Stop with sending fatal or close_notify alert - there is no sense to read next records if we failed on processing current one. In this case we just send responses for previous records, send the alert and close TCP connection.
Also don't forget to enable tests from tempesta-test/tests_disabled.json marked by #1308.
avbelov23
force-pushed
the
avb-1308
branch
2 times, most recently
from
5050fa9 to
adf359b
Compare
|
Just saw kernel oops on CI http://93.115.28.191:4010/#/builders/5/builds/810/steps/21/logs/stdio: |
It seems like it doesn't related to this PR, I've got the same on master branch, #1283 (comment) |
There was a problem hiding this comment.
For me passing callback function into ss_close() function looks ugly. This callback is stored inside TfwConnection, but you pass it second time via ss_close(). It becomes very unclear what is the proper way to close the connection: call the hook or pass a function as argument and call it.
If the desire is to control, which callback should be used, at the ss_close() step, may be we should introduce some state of the connection and leave only one connection_error() callback? And this callback would be responsible to check the connection state and chose drop/error routines?
tls/ttls.c
Outdated
| @@ -1334,7 +1334,21 @@ ttls_send_alert(TlsCtx *tls, unsigned char lvl, unsigned char msg) | |||
| io->alert[0] = lvl; | |||
| io->alert[1] = msg; | |||
|
|
|||
| if (msg == TTLS_ALERT_MSG_CLOSE_NOTIFY) | |||
| if (msg == TTLS_ALERT_MSG_UNEXPECTED_MESSAGE || | |||
There was a problem hiding this comment.
Why not switch-case statement here?
tempesta_fw/sock.c
Outdated
| do { \ | ||
| ss_do_close(sk); \ | ||
| bh_unlock_sock(sk); \ | ||
| SS_CALL_GUARD_EXIT(connection_drop, sk); \ |
There was a problem hiding this comment.
What is the reason to drop call of SS_CALL_GUARD_EXIT?
There was a problem hiding this comment.
Yes. Did not have time to push fix
avbelov23
force-pushed
the
avb-1308
branch
3 times, most recently
from
46fb8ab to
3728df5
Compare
avbelov23
force-pushed
the
avb-1308
branch
4 times, most recently
from
30c1546 to
a274cd7
Compare
| static void | ||
| tfw_sock_clnt_error(struct sock *sk) | ||
| { | ||
| tfw_sock_clnt_do_drop(sk, "connection error"); |
There was a problem hiding this comment.
tfw_sock_clnt_do_drop() function was introduced to call the same function on connection_error and connection_drop callbacks, but only one of them exists now and we don't need special tfw_sock_clnt_do_drop() function
There was a problem hiding this comment.
From the current PR state I can say that this was addressed.
tls/ttls.c
Outdated
| close = true; | ||
| break; |
There was a problem hiding this comment.
Good practice is to provide default branch even there is no actions required.
tempesta_fw/http.c
Outdated
| @@ -1962,6 +1962,7 @@ tfw_http_conn_init(TfwConn *conn) | |||
| static int | |||
| tfw_http_conn_close(TfwConn *conn, bool sync) | |||
| { | |||
| SS_CONN_TYPE(conn->sk) |= Conn_Shutdown; | |||
There was a problem hiding this comment.
Why this flag is issued for every tfw_http_conn_close() (tfw_connection_close() () call? The flag is not needed for the client connections, only for server ones to highlight that the server connections to be dropped and re-establishing it is not needed.
We have discussed the flag in private chat and it looked much better than the passing connection_error|connection_drop pointers as arguments into ss_close. But now the flag Conn_Shutdown duplicates the meaning of the TFW_CONN_B_DEL flag. But implementation of that flags is completely different and some collisions are possible.
tempesta_fw/tls.c
Outdated
| flags |= SS_F_ENCRYPT; | ||
| TFW_CONN_TYPE(&conn->cli_conn) |= Conn_Stop; | ||
| } | ||
| if (ttls_xfrm_ready(tls) && !was_encrypt) |
There was a problem hiding this comment.
Having both ttls_xfrm_ready() and was_encrypt looks redundant and wrong.
We already know which records we have to encrypt and which not. In the T_FSM_STATE(TTLS_SERVER_FINISHED) we change tls->state before __ttls_send_record() is called, so here tls->state is a bit off. But order can be changed. Here is what I'm talking about:
diff --git a/tls/tls_srv.c b/tls/tls_srv.c
index 58699482..b9e2cb7f 100644
--- a/tls/tls_srv.c
+++ b/tls/tls_srv.c
@@ -2262,19 +2262,20 @@ ttls_handshake_finished(TlsCtx *tls)
}
T_FSM_STATE(TTLS_SERVER_FINISHED) {
if ((r = ttls_write_finished(tls, &sgt, &p)))
return r;
CHECK_STATE(TLS_HEADER_SIZE + TTLS_HS_FINISHED_BODY_LEN);
+ sg_mark_end(&sgt.sgl[sgt.nents - 1]);
+ r = __ttls_send_record(tls, &sgt, false, false);
/*
* In case of session resuming, invert the client and server
* ChangeCipherSpec messages order.
*/
tls->state = tls->hs->resume
? TTLS_CLIENT_CHANGE_CIPHER_SPEC
: TTLS_HANDSHAKE_WRAPUP;
- sg_mark_end(&sgt.sgl[sgt.nents - 1]);
- return __ttls_send_record(tls, &sgt, false, true);
+ return r;
}
}
T_FSM_FINISH(r, tls->state);
/* If we exit here, then something went wrong. */
diff --git a/tls/ttls.c b/tls/ttls.c
index 5041ce56..1b0c6e89 100644
--- a/tls/ttls.c
+++ b/tls/ttls.c
@@ -242,11 +242,12 @@ EXPORT_SYMBOL(ttls_register_bio);
*/
bool
ttls_xfrm_ready(TlsCtx *tls)
{
return tls->state >= TTLS_CLIENT_FINISHED
- && tls->state != TTLS_SERVER_CHANGE_CIPHER_SPEC;
+ && tls->state != TTLS_SERVER_CHANGE_CIPHER_SPEC
+ && tls->state != TTLS_SERVER_FINISHED;
}
EXPORT_SYMBOL(ttls_xfrm_ready);
#if defined(TTLS_CLI_C)
static intI think, it should work. And then was_encrypt can be removed altogether.
There was a problem hiding this comment.
There was a problem hiding this comment.
Well, maybe. But anyway, tls->state should be enough to figure out if a record needs to be encrypted. Perhaps we'll need a new predicate for that.
avbelov23
force-pushed
the
avb-1308
branch
2 times, most recently
from
fe28860 to
5582ff7
Compare
#1308
Close the socket after sending all pending data on TCP_CLOSE_WAIT and internal errors.
Close the connection after sending fatal tls alerts and close_notify alert in the queue
Not encryption for TTLS_SERVER_FINISHED, because encryption is done in
ttls_write_finished().Sending close_notify alert on close_notify alert.