Recreate upgrade headers for websocket request #1592
Conversation
Signed-off-by: Aleksey Mikhaylov <[email protected]>
ttaym
force-pushed
the
am-755-upgrade-adjust-request
branch
from
011fd2c
to
b3e04fe
Compare
|
|
||
| if (test_bit(TFW_HTTP_B_UPGRADE_WEBSOCKET, hm->flags)) { | ||
| if (is_resp && ((TfwHttpResp *)hm)->status == 101 | ||
| && test_bit(TFW_HTTP_B_UPGRADE_EXTRA, hm->flags)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
fw/http.c
Outdated
| 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, 1); |
There was a problem hiding this comment.
Headers modification is a heavy operation, probably involving memory allocations and skb frarmentation (see #1103), so it'd be more beneficial to do bit more complex condition and call do only one transformation call.
There was a problem hiding this comment.
That make sense. I'll fix it.
| { | ||
| r = TFW_HTTP_MSG_HDR_XFRM(hm, "Connection", | ||
| "upgrade", | ||
| TFW_HTTP_HDR_CONNECTION, 0); |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Co-authored-by: Alexander Krizhanovsky <[email protected]>
Contributes to #755
Signed-off-by: Aleksey Mikhaylov [email protected]