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.

Sign up for GitHub

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

Recreate upgrade headers for websocket request #1592

Merged
merged 4 commits into from
Mar 18, 2022
Merged

Recreate upgrade headers for websocket request #1592

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b3e04fe
Recreate upgrade headers for websocket request
ttaym Mar 5, 2022
23d3e9b
fixup! Recreate upgrade headers for websocket request
ttaym Mar 17, 2022
73f894c
fixup! Recreate upgrade headers for websocket request
ttaym Mar 17, 2022
1ce0b5d
Update fw/http.c
ttaym Mar 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 86 additions & 9 deletions fw/http.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2572,6 +2572,44 @@ tfw_http_set_hdr_date(TfwHttpMsg *hm)
return r;
}

/*
* Add 'Upgrade:' header for websocket upgrade messages
*/
static int
tfw_http_set_hdr_upgrade(TfwHttpMsg *hm, bool is_resp)
{
int r = 0;

if (test_bit(TFW_HTTP_B_UPGRADE_WEBSOCKET, hm->flags)) {
/*
* RFC7230#section-6.7:
* A server that sends a 101 (Switching Protocols) response
* MUST send an Upgrade header field to indicate the new
* protocol(s) to * which the connection is being switched; if
ttaym marked this conversation as resolved.
Show resolved Hide resolved
* multiple protocol layers are being switched, the sender MUST
* list the protocols in layer-ascending order.
*
* We do expect neither upgrades besides 'websocket' nor
* multilayer upgrades. So we consider extra options as error.
*/
if (is_resp && ((TfwHttpResp *)hm)->status == 101
&& test_bit(TFW_HTTP_B_UPGRADE_EXTRA, hm->flags))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if I understand this. It seems this is about https://datatracker.ietf.org/doc/html/rfc7230#section-6.7

A server that sends a 101 (Switching Protocols) response MUST send an
Upgrade header field to indicate the new protocol(s) to which the
connection is being switched; if multiple protocol layers are being
switched, the sender MUST list the protocols in layer-ascending
order. A server MUST NOT switch to a protocol that was not indicated
by the client in the corresponding request's Upgrade header field.

This piece of code is for response with something extra plus to websocket in Upgrader header. Since we can only generate Upgrade: websocket in adjusted client request and server MUST NOT add anything non-requested, then what could be in the response for the extra? Can't we just always send Upgrade: websocket from the server to the client?

If I'm wrong, then a good comment with the RFC cite is required in the place.

Copy link
Contributor Author

@ttaym ttaym Mar 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

My intention was as follows:

We always strip upgrade header from response and re-add it when needed. So we do not just pass it through tempesta as is. But when we recreate headers would be beneficially to do simple sane check because we essentially hide backend and act on behalf on it.

Imaging erroneous backend that we on our discretion silently transform into compliant backend. That would be destructive if you ask me.

But this intention is, i understand, subtle and subjective. It is up to you if we need just alway send Upgrade: websocket.

{
T_ERR("Unable to add uncompliant 'Upgrade:' header "
"to msg [%p]\n", hm);
return -EINVAL;
}
r = tfw_http_msg_hdr_xfrm(hm, "upgrade", SLEN("upgrade"),
"websocket", SLEN("websocket"),
TFW_HTTP_HDR_UPGRADE, 0);
if (r)
T_ERR("Unable to add Upgrade: header to msg [%p]\n", hm);
else
T_DBG2("Added Upgrade: header to msg [%p]\n", hm);
krizhanovsky marked this conversation as resolved.
Show resolved Hide resolved
}
return r;
}

/*
* Expand HTTP response with 'Date:' header field.
*/
Expand Down Expand Up @@ -2677,23 +2715,54 @@ tfw_http_expand_hbh(TfwHttpResp *resp, unsigned short status)
static int
tfw_http_set_hdr_connection(TfwHttpMsg *hm, unsigned long conn_flg)
{
int r;
BUILD_BUG_ON(BIT_WORD(__TFW_HTTP_MSG_M_CONN) != 0);
if (((hm->flags[0] & __TFW_HTTP_MSG_M_CONN) == conn_flg)
&& (!TFW_STR_EMPTY(&hm->h_tbl->tbl[TFW_HTTP_HDR_CONNECTION]))
&& !test_bit(TFW_HTTP_B_CONN_EXTRA, hm->flags))
&& !test_bit(TFW_HTTP_B_CONN_EXTRA, hm->flags)
&& !test_bit(TFW_HTTP_B_CONN_UPGRADE, hm->flags))
{
return 0;
}

switch (conn_flg) {
case BIT(TFW_HTTP_B_CONN_CLOSE):
/*
* We can see `TFW_HTTP_B_CONN_CLOSE` here only in case of 4XX
* response with 'Connection: close' option.
*
* For requests conn_flg by default is TFW_HTTP_B_CONN_KA.
*/
if (unlikely(conn_flg == BIT(TFW_HTTP_B_CONN_CLOSE)))
return TFW_HTTP_MSG_HDR_XFRM(hm, "Connection", "close",
TFW_HTTP_HDR_CONNECTION, 0);
case BIT(TFW_HTTP_B_CONN_KA):
return TFW_HTTP_MSG_HDR_XFRM(hm, "Connection", "keep-alive",
TFW_HTTP_HDR_CONNECTION, 0);
default:
return TFW_HTTP_MSG_HDR_DEL(hm, "Connection",
TFW_HTTP_HDR_CONNECTION);

if (conn_flg == BIT(TFW_HTTP_B_CONN_KA)) {
if (test_bit(TFW_HTTP_B_UPGRADE_WEBSOCKET, hm->flags)
&& test_bit(TFW_HTTP_B_CONN_UPGRADE, hm->flags))
{
r = TFW_HTTP_MSG_HDR_XFRM(hm, "Connection",
"keep-alive, upgrade",
TFW_HTTP_HDR_CONNECTION, 0);
}
else {
r = TFW_HTTP_MSG_HDR_XFRM(hm, "Connection",
"keep-alive",
TFW_HTTP_HDR_CONNECTION, 0);
}
} else {
if (test_bit(TFW_HTTP_B_UPGRADE_WEBSOCKET, hm->flags)
&& test_bit(TFW_HTTP_B_CONN_UPGRADE, hm->flags))
{
r = TFW_HTTP_MSG_HDR_XFRM(hm, "Connection",
"upgrade",
TFW_HTTP_HDR_CONNECTION, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we have Connection: close, upgrade, then it seems we should send just Connection: close. Also could you please check whether a connection is considered keep-alive by default (there is neither close nor keep-alive)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default passed 0 in the function for responses, BIT(TFW_HTTP_B_CONN_KA) passed to function for responses only if paired request has TFW_HTTP_B_CONN_KA bit.

But for requests by default passed BIT(TFW_HTTP_B_CONN_KA).

And semantically connection is keep-alive by default for HTTP/1.1 and later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we have Connection: close, upgrade in response than now result depends on status. If status is 4XX than connection is closed without upgrade. If 101 than upgrade is finished and connection state than depends on actual closing on no-closing from backend.

May be that is right enough.

}
else {
r = TFW_HTTP_MSG_HDR_DEL(hm, "Connection",
TFW_HTTP_HDR_CONNECTION);
}
}

return r;
}

/**
Expand Down Expand Up @@ -3072,6 +3141,10 @@ tfw_h1_adjust_req(TfwHttpReq *req)
if (r < 0)
return r;

r = tfw_http_set_hdr_upgrade(hm, false);
if (r < 0)
return r;

r = tfw_h1_set_loc_hdrs(hm, false, false);
if (r < 0)
return r;
Expand Down Expand Up @@ -3642,6 +3715,10 @@ tfw_http_adjust_resp(TfwHttpResp *resp)
if (r < 0)
return r;

r = tfw_http_set_hdr_upgrade(hm, true);
if (r < 0)
return r;

r = tfw_http_set_hdr_keep_alive(hm, conn_flg);
if (r < 0)
return r;
Expand Down
4 changes: 3 additions & 1 deletion fw/http.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,8 +242,10 @@ enum {
TFW_HTTP_B_CONN_KA,
TFW_HTTP_B_CONN_UPGRADE,
TFW_HTTP_B_CONN_EXTRA,
/* Request is a websocket upgrade request */
/* Message is a websocket upgrade request */
TFW_HTTP_B_UPGRADE_WEBSOCKET,
/* Message upgrade header contains extra fields */
TFW_HTTP_B_UPGRADE_EXTRA,
/* Chunked is last transfer encoding. */
TFW_HTTP_B_CHUNKED,
/* Chunked in the middle of applied transfer encodings. */
Expand Down
35 changes: 31 additions & 4 deletions fw/http_parser.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3348,7 +3348,7 @@ STACK_FRAME_NON_STANDARD(__parse_pragma);
* __FSM_I_MOVE_fixup()/__FSM_I_MATCH_fixup()/TRY_STR_fixup() everywhere.
*/
static int
__req_parse_upgrade(TfwHttpMsg *hm, unsigned char *data, size_t len)
__parse_upgrade(TfwHttpMsg *hm, unsigned char *data, size_t len)
{
int r = CSTR_NEQ;
__FSM_DECLARE_VARS(hm);
Expand Down Expand Up @@ -3382,6 +3382,8 @@ __req_parse_upgrade(TfwHttpMsg *hm, unsigned char *data, size_t len)
*/
return CSTR_NEQ;
}

__set_bit(TFW_HTTP_B_UPGRADE_EXTRA, msg->flags);
__FSM_I_JMP(I_UpgradeProtocolEnd);
}

Expand All @@ -3398,6 +3400,8 @@ __req_parse_upgrade(TfwHttpMsg *hm, unsigned char *data, size_t len)
__set_bit(TFW_HTTP_B_UPGRADE_WEBSOCKET,
msg->flags);
}
} else {
__set_bit(TFW_HTTP_B_UPGRADE_EXTRA, msg->flags);
}

__FSM_I_JMP(I_UpgradeProtocolEnd);
Expand Down Expand Up @@ -3437,7 +3441,6 @@ __req_parse_upgrade(TfwHttpMsg *hm, unsigned char *data, size_t len)
* At this state we know that we saw at least one character in
* protocol version and now we can pass zero length token.
*/

__FSM_STATE(I_UpgradeVersion) {
__FSM_I_MATCH_MOVE_fixup(token, I_UpgradeVersion,
TFW_STR_VALUE);
Expand Down Expand Up @@ -3477,7 +3480,7 @@ __req_parse_upgrade(TfwHttpMsg *hm, unsigned char *data, size_t len)
done:
return r;
}
STACK_FRAME_NON_STANDARD(__req_parse_upgrade);
STACK_FRAME_NON_STANDARD(__parse_upgrade);

static int
__req_parse_user_agent(TfwHttpMsg *hm, unsigned char *data, size_t len)
Expand Down Expand Up @@ -4479,7 +4482,7 @@ tfw_http_parse_req(void *req_data, unsigned char *data, size_t len,
TFW_HTTP_HDR_USER_AGENT);

/* 'Upgrade:*OWS' is read, process field-value. */
__TFW_HTTP_PARSE_SPECHDR_VAL(Req_HdrUpgradeV, msg,__req_parse_upgrade,
__TFW_HTTP_PARSE_SPECHDR_VAL(Req_HdrUpgradeV, msg, __parse_upgrade,
TFW_HTTP_HDR_UPGRADE, 0);

/* 'Cookie:*OWS' is read, process field-value. */
Expand Down Expand Up @@ -10057,6 +10060,17 @@ tfw_http_parse_resp(void *resp_data, unsigned char *data, size_t len,
__FSM_MOVE_hdr_fixup(RGen_LWS, 1);
}
__FSM_MOVE(Resp_HdrT);
case 'u':
if (likely(__data_available(p, 8)
&& C4_INT_LCM(p, 'u', 'p', 'g', 'r')
&& C4_INT3_LCM(p + 4, 'a', 'd', 'e', ':')))
{
__msg_hdr_chunk_fixup(data, __data_off(p + 7));
parser->_i_st = &&Resp_HdrUpgradeV;
p += 7;
__FSM_MOVE_hdr_fixup(RGen_LWS, 1);
}
__FSM_MOVE(Resp_HdrU);
case 'v':
if (likely(__data_available(p, 5)
&& C4_INT3_LCM(p + 1, 'a', 'r', 'y', ':')))
Expand Down Expand Up @@ -10229,6 +10243,10 @@ tfw_http_parse_resp(void *resp_data, unsigned char *data, size_t len,
/* 'Pragma:*OWS' is read, process field-value. */
__TFW_HTTP_PARSE_RAWHDR_VAL(Resp_HdrPragmaV, msg, __parse_pragma, 0);

/* 'Upgrade:*OWS' is read, process field-value. */
__TFW_HTTP_PARSE_SPECHDR_VAL(Resp_HdrUpgradeV, msg, __parse_upgrade,
TFW_HTTP_HDR_UPGRADE, 0);

/* 'Server:*OWS' is read, process field-value. */
TFW_HTTP_PARSE_SPECHDR_VAL(Resp_HdrServerV, resp, __resp_parse_server,
TFW_HTTP_HDR_SERVER);
Expand Down Expand Up @@ -10744,6 +10762,15 @@ tfw_http_parse_resp(void *resp_data, unsigned char *data, size_t len,
__FSM_TX_AF(Resp_HdrWWW_Authenticat, 'e', Resp_HdrWWW_Authenticate);
__FSM_TX_AF_OWS_HP(Resp_HdrWWW_Authenticate, RGen_HdrOtherV, 61);

/* Upgrade header processing. */
__FSM_TX_AF(Resp_HdrU, 'p', Resp_HdrUp);
__FSM_TX_AF(Resp_HdrUp, 'g', Resp_HdrUpg);
__FSM_TX_AF(Resp_HdrUpg, 'r', Resp_HdrUpgr);
__FSM_TX_AF(Resp_HdrUpgr, 'a', Resp_HdrUpgra);
__FSM_TX_AF(Resp_HdrUpgra, 'd', Resp_HdrUpgrad);
__FSM_TX_AF(Resp_HdrUpgrad, 'e', Resp_HdrUpgrade);
__FSM_TX_AF_OWS(Resp_HdrUpgrade, Resp_HdrUpgradeV);

__FSM_FINISH(resp);

return r;
Expand Down