Skip to content
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

port_default_packet_handler() needs to know when it should start with client validation #911

Open
Sashan opened this issue Nov 1, 2024 · 3 comments

Comments

@Sashan
Copy link

Sashan commented Nov 1, 2024

This ticket refers to draft pull request #25842. The code currently validate every client which attempts to connect to:

846     /*
847      * TODO: there should be some logic similar to accounting half-open
848      * states in TCP. If we reach certain threshold, then we want to
849      * validate clients.
850      */
851 #define VALIDATE_CLIENT 1
852     if (VALIDATE_CLIENT) {
853         if (hdr.token == NULL) {
854             port_send_retry(port, &e->peer, &hdr);
855             goto undesirable;
856         } else if (port_validate_token(&hdr, &odcid) == 0) {
857             goto undesirable;
858         } else {
859             port_bind_channel(port, &e->peer, &hdr.src_conn_id, &hdr.dst_conn_id,
860                               &odcid, &new_ch);
861         }
862     } else if (hdr.token == NULL || port_validate_token(&hdr, &odcid) == 1) {
863         /*
864          * client validation is optional. However if client presents
865          * token, then the token must be valid.
866          */
867         if (hdr.token != NULL) {
868             port_bind_channel(port, &e->peer, &hdr.src_conn_id, &hdr.dst_conn_id,
869                               &odcid, &new_ch);
870         } else {
871             /*
872              * Try to process this as a valid attempt to initiate a connection.
873              */
874             port_on_new_conn(port, &e->peer, &hdr.src_conn_id, &hdr.dst_conn_id,
875                              &new_ch);
876         }
877     }

Line 852 must be changed such there will be test which will determine it's time to validate client. The underlying logic should monitor number of half-opened sessions. Half-open session is every session which arrives with destination connection id which LCIDM (local connection ID manager) knows nothing about (function ossl_quic_lcidm_lookup() fails to find matching channel for such destination connection id. If this happens the half open counter must be bumped up.

As soon as SSL handshake completes (SSL handshake counts as client validation) the half open counter gets decremented. Once the counter reaches certain threshold the condition at line 852 trips and port starts to send retry packets to validate clients.

The devil is in detail: how application (or OpenSSL library) should define a threshold? In my opinion this is the hardest part to answer here.

Scope:

  • count half open connections
  • SSL_set_value - maximum half open connections
  • if limit exceeded start validating
  • Set some starting value
  • documentation
  • test
@t8m
Copy link
Member

t8m commented Nov 1, 2024

As soon as SSL handshake completes (SSL handshake counts as client validation) the half open counter gets decremented. Once the counter reaches certain threshold the condition at line 852 trips and port starts to send retry packets to validate clients.

Should the count be per-source-IP (or some masked value of it for IPv6)?

@Sashan
Copy link
Author

Sashan commented Dec 9, 2024

I think this is more or less related to issue discovered in interoperability tests. details are #948

@vavroch2010 vavroch2010 removed this from the 3.5.0 milestone Dec 9, 2024
@nhorman
Copy link
Contributor

nhorman commented Jan 12, 2025

actually, I think we can push this work to the client. If we implement the sending of NEW_TOKEN frames as per #929 , The we could just make the assumption that any inbound connection without a token value in it has to be validated, as clients that have already connected can just use the NEW_TOKEN provided token to bypass the extra round trip.

refinement note: Seems like we could just drop this to avoid the complexity of adding it, as NEW_TOKENS obviate the need to measure backlog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

No branches or pull requests

4 participants