HTTP/2 vs HTTPS handling #1643
Merged
HTTP/2 vs HTTPS handling #1643
Conversation
This file contains 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
krizhanovsky
force-pushed
the
ak-h2-vs-https
branch
from
c42b120 to
980187c
Compare
|
Just caught crash on simple Apparently |
1 task
krizhanovsky
force-pushed
the
ak-h2-vs-https
branch
from
d0f5d9b to
1839dbb
Compare
|
Caught crash, which is fixed in 70daa64 |
krizhanovsky
force-pushed
the
ak-h2-vs-https
branch
from
e2a1dc7 to
271d1f5
Compare
This was referenced Aug 17, 2022
Closed
Closed
const-t
reviewed
Aug 31, 2022
s0nx
reviewed
Sep 1, 2022
|
BTW, there are some other cases of freeing the list member without explicit |
established sockets from HTTPS. Now we can use more efficient TFW_FSM_TYPE(conn->proto.type) == TFW_FSM_H2 instead of TFW_CONN_H2(conn) and initialize HTTP/2 connection descriptor only for HTTP/2 connections, not HTTPS as previously. Add a new alpn_match_cb hook for TLS, which validates client ALPN matching one of ours ALPN against type of the socket, so if client sends up ALPN "h2" on a TCP connection established on a socket with listen ... proto=https; , then the connection won't be established.
Disable annoying warning on inability to send TLS alerts on connections with handshakes in progress. Add comments for ss_active_guard_enter() about the need for the double checking. Add more information about connections counters state to the warning message about pending client connections.
1. remove stress.[ch] which never been used and we still don't know whether we need then for the QoS. 2. Remove classifier - there is only one module (http_limits) using it and there is no sense to play with RCU and indirect calls 3. Replace SS_CALL_GUARD_EXIT() with an (inlined) function and remove unused SS_CALL_GUARD_ENTER(). Add assertion that a socket is locked, when it enters frang_conn_limit() from tcp_v4_syn_recv_sock() (the socket is locked by tcp_create_openreq_child() -> inet_csk_clone_lock().
new listening port to the bitmap before moving the socket to listening state - this way we guarantee that HTTP limiting is called on the initialized accounting descriptor. The problem appears on constant system restart under 2000 HTTPS connections from wrk.
to static functions. Instead, just store and restore the list in the particular test (we need this since there is a session test, which initializes the session module in the list). Remove unnecessary exports.
and immediately after that tfw_client_stop() closes the clients database, so during ss_synchronize() client connections closing callback tfw_sock_clnt_drop() removes client connection from the dead linked list. This patch count each sock.c users (they must set TfwMod->sock_user) and calls ss_synchronize() exactly when last of them finishes (including interrupted start procedure due to an error in some module).
need to save and restore FPU context before calling syncronous sockets.
connection close alert, which is just normal.
tcp_write_xmit() calls tfw_tls_encrypt(). However, the may never happen if the peer didn't announce sufficient receive window. In this case Tempesta shutdown may find live connections and rise "pending active connections for 5s" and leave the connections objects in the memory cache. ss_synchronize() now returns a success value and we abort all the client connections in case of failure and recheck all the counters with ss_synchronize() again. To abort all the connections we extend ss_do_send() with __SS_F_RST and send RST on aborted connections. The connections aborting (TCP RST) is also useful on security events: when we see any malicious activity we should reset connection instead of issuing normal TCP connections closing. For this purpose conn_abort callback was introduced for TfwConnHooks and ss_tcp_data_ready() normally closes or aborts a TCP connection depending on ss_tcp_process_data() return value (SS_BAD and SS_DROP correspondinglY), so several innocent places returning T_DROP/SS_DROP were adjusted to return T_BAD/SS_BAD, most likely not all the places though. tfw_peer_del_conn() were updated to use 1-depth nesting to be called as a callback from tfw_peer_for_each_conn() (the ability isn't used by the current version of connections aborting). Also: - several comments are updated/fixed; - couple of macros replaced with inlined functions - better messaging in ss_synchronize() - some other minor code cleanups
chunk, to tfw_str_collect_cmp(). Firstly check chunk == end in tfw_str_collect_cmp() and only after than make an assertion that chunk is a plain string.
may free the conn and we should not dereference the pointer even for the assertion.
it is mainly used on connection freeing since WS(S) is upgraded from HTTP(S).
If one of sockets can't start listening in tfw_sock_clnt_start we just breaks the initialization loop and clenup some data, but already opened sockets won't be released, since module has not been started yet and tfw_sock_clnt_stop must not be called for such modules. Now has been added realising of sockets in tfw_listen_sock_del_all which must be called from cleanup callback even on tfw_sock_clnt_start failure. Also tfw_listen_sock_del_all now frees tfw_listen_socks list
krizhanovsky
force-pushed
the
ak-h2-vs-https
branch
from
8e089ff to
06b2157
Compare
tfw_conn_hook_call() or tfw_h2_context_init(): call TLS connection destructor on yet not fully initialized TLS connection handler. Nullify TfwHPackDTbl pool pointer after destruction to let tfw_hpack_clean() call on the handler without double-frees.
connection drops.
|
Just caught a crash on the current version of the branch on The problem is caused by the race between Fixed in e8cfb48 |
tfw_sock_clnt_start(): while a new child socket is created and initialized by the listening socket, the listening socket, whcih was removed from the reloaded configuration, is freed during the reconfiguration. There actually was no requirement to use the listening socket in the child initialization. The patch removes `listener` from SsProto as doubling TfwListenSock->sk, so all connections will use smaller memory. Also remove the unused synchronous sockets tests.
1. all our sockets have sk->sk_allocation == GFP_ATOMIC, so remove setting of the allocation class and use sk_uid to differentiate client and server sockets in ss_tcp_state_change(). 2. ss_tcp_state_change() uses SsHooks from SsProto, which is supposed to be in sk->sk_user_data to call connection_new hooks, so we do need to keep full SsProto in sk_user_data for listening sockets. I used statically allocates SsProtos to make child sockets independent from parent to avoid the race, attempted to fix in previous commit. 3. remove ss_wait_newconn() call from tfw_sock_clnt_start() - this was just a garbage from invalid way to fix the race in previous commit. 4. remove t/sync_sockets from the Makefile
|
The tests are broken. On CI I http://tempesta-vm.cloud.cherryservers.net:8080/job/tempesta-test%20PR/211/console do see these tests failed: The both pass in my VM with Ubuntu 22 and tests master at 8efb2353a2b89a542de4e1783f9913fc32b924a7 . The last test throws though this warning: However, on my system these tests fail: The first test is about tempesta-tech/tempesta-test#285 , the TLS one is tempesta-tech/tempesta-test#302 . I reran the failed CI tests on my VM and they do pass: |
keshonok
reviewed
Sep 5, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fix #1624 with explicit ALPN verification against the type of the listening socket, inherited by all established sockets. Manually tested with tempesta-tech/tempesta-test#275 (review)
Fix
pending active connections for 5sissue (#1377 (comment)) and related 2nd checkbox from #1422 .Fix #861: now we send (or at least able to send) TCP RST on security events.
Linked tests pull request tempesta-tech/tempesta-test#288
The pull request also requires tempesta-tech/linux-5.10.35-tfw@19bbfd1