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

The first attempt to solve #2006 #2083

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

biathlon3
Copy link
Contributor

It works, but I may have missed releasing some resources.

@biathlon3 biathlon3 linked an issue Mar 26, 2024 that may be closed by this pull request
3 tasks
fw/sock_clnt.c Outdated Show resolved Hide resolved
fw/sync_socket.h Outdated Show resolved Hide resolved
fw/tls.c Outdated
@@ -664,10 +665,10 @@ tfw_tls_conn_init(TfwConn *c)
goto err_cleanup;
}

if (TFW_FSM_TYPE(c->proto.type) == TFW_FSM_H2)
if (TFW_FSM_TYPE(c->proto.type) == TFW_FSM_H2 ||
TFW_FSM_TYPE(c->proto.type) == TFW_FSM_H2_HTTPS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another overhead for HTTP1, we initialize h2 context(including hpack) even for http1. Would be better to do it, when we know protocol.

@krizhanovsky Why do we init higher layer protocol context(http and http2) before finishing lower layer connection establishing, I mean TLS. Is there a reason or just implemented this way?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is kind of reversion of 3c97cb23d - we should initialize HTTP/2 context only for HTTP/2 connections. Note also #1422 - another HTTP/2 -related regression in HTTP/1.

@const-t Frankly, I don't remember why do we initialize HTTP/2 context before finishing TLS handshake. It makes sense to allocate memory necessary for HTTP/2 early to improve spacial locality and reduce number of allocator calls. But for now for me it seems that we can initialize the context later... Need to check commits history - I suppose this point should have been discussed at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another overhead for HTTP1, we initialize h2 context(including hpack) even for http1. Would be better to do it, when we know protocol.

I replace initialization h2 context for TFW_FSM_H2_HTTPS to tfw_http_msg_process

@biathlon3 biathlon3 force-pushed the ag_2006-handle-https1-and-http2-on-the-same-port branch 2 times, most recently from e3f4c4b to b2dd3d7 Compare April 26, 2024 14:07
@biathlon3 biathlon3 requested a review from const-t April 29, 2024 16:32
fw/gfsm.h Outdated
@@ -104,6 +104,7 @@ enum {
/* Security rules enforcement. */
TFW_FSM_FRANG_REQ,
TFW_FSM_FRANG_RESP,
TFW_FSM_H2_HTTPS = 11,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggesting another solution, please consider it:

  1. Postpone all initialization of http2/http1 proto after handshake completeness.
  2. Add to ss_proto_t flag negotiable or similar.
  3. By default always use TFW_FSM_H2 or TFW_FSM_HTTP/S proto type.
  4. If negotiable is true, at ALPN matching stage you just change the proto, to specified by ALPN.

With this approach we don't need TFW_FSM_H2_HTTPS dummy state.

@biathlon3 biathlon3 requested a review from const-t May 8, 2024 10:31
fw/tls.c Outdated

if (TFW_FSM_TYPE(sk_proto) == TFW_FSM_H2
&& alpn->id == TTLS_ALPN_ID_HTTP2)
&& alpn->id == TTLS_ALPN_ID_HTTP2) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do this way?

bool
tfw_tls_alpn_match(const TlsCtx *tls, const ttls_alpn_proto *alpn)
{
	int sk_proto = ((SsProto *)tls->sk->sk_user_data)->type;
	TfwConn *conn = (TfwConn*)tls->sk->sk_user_data;

	/* Downgrade from HTTP2 to HTTP1. */
	if (sk_proto & Conn_Negotiable && alpn->id == TTLS_ALPN_ID_HTTP1) {
		conn->proto.type = (conn->proto.type & ~TFW_GFSM_FSM_MASK) |
					TFW_FSM_HTTPS;
		return true;
	}

	if (TFW_FSM_TYPE(sk_proto) == TFW_FSM_H2
	    && alpn->id == TTLS_ALPN_ID_HTTP2)
		return true;

	if (TFW_FSM_TYPE(sk_proto) == TFW_FSM_HTTPS
	    && alpn->id == TTLS_ALPN_ID_HTTP1)
		return true;

	return false;
}

And I would prefer to move:

if (tfw_h2_context_init(tfw_h2_context(conn))) {
    T_ERR("cannot establish a new h2 connection\n");
    return false;
}

to tfw_tls_over() callback, yes we add another one if branch to check proto, but from my point of view we can sacrifice this branch for better code. In this case tfw_tls_alpn_match will have only one responsibility - match alpn. But the part about context initiating is up to you.

fw/tls.c Outdated
@@ -615,6 +615,9 @@ tfw_tls_conn_dtor(void *c)
if (TFW_FSM_TYPE(((TfwConn *)c)->proto.type) == TFW_FSM_H2)
tfw_h2_context_clear(tfw_h2_context(c));

if (((TfwConn *)c)->proto.type & Conn_Negotiable)
((TfwConn *)c)->proto.type = TFW_FSM_H2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this?

fw/sock_clnt.c Outdated
};

static const SsProto *
tfw_sock_clnt_proto(int type)
{
int i;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return this blank line please.

@@ -120,7 +120,10 @@ tfw_cli_conn_free(TfwCliConn *cli_conn)
tfw_connection_validate_cleanup((TfwConn *)cli_conn);
BUG_ON(!list_empty(&cli_conn->seq_queue));

kmem_cache_free(tfw_cli_cache(TFW_CONN_TYPE(cli_conn)), cli_conn);
if (!(TFW_CONN_TYPE(cli_conn) & Conn_Negotiable))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add comment to the code that describes why we have this condition.

@biathlon3 biathlon3 force-pushed the ag_2006-handle-https1-and-http2-on-the-same-port branch from 2588ecf to 336b43d Compare May 20, 2024 11:55
@biathlon3 biathlon3 requested a review from const-t May 21, 2024 05:15
Copy link
Contributor

@const-t const-t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please squash unnecessary commits.

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

Successfully merging this pull request may close these issues.

Handle HTTPS/1 and HTTP/2 on the same port
3 participants