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

Websocket upgrade directive implementation #1571

Merged
merged 2 commits into from
Mar 16, 2022

Conversation

ttaym
Copy link
Contributor

@ttaym ttaym commented Feb 24, 2022

Contributes to #755

Considerations.

It seems that to confrom to RFC7230#section-6.6 we have to be prepared to process requests with Connection: upgrade and Expect: 100-continue set at the same time and respond with 100 before going further with the request. But for websocket upgrade case it is obscure and rare (if ever) seen. For now implemented logic does not have that.

It seems that standard does not explicitly forbid Upgrade header in request trailer field. But for now implementation does not support that scenario.

Tempesta FW MUST block requests with Upgrade header but without upgrade option in Connection header. Tempesta FW MUST ignore Upgrade header for HTTP version less then HTTP/1.1. See RFC7230#section-6.1:

When Upgrade is sent, the sender MUST also send a Connection header
field (Section 6.1) that contains an "upgrade" connection option, in
order to prevent Upgrade from being accidentally forwarded by
intermediaries that might not implement the listed protocols. A
server MUST ignore an Upgrade header field that is received in an
HTTP/1.0 request.

Signed-off-by: Aleksey Mikhaylov [email protected]

@ttaym ttaym force-pushed the am-755-websocket-upgrade-directive branch 2 times, most recently from 482d49d to 8d93200 Compare March 1, 2022 14:22
Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

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

Just several notes how to make the parser move efficient

fw/http.c Outdated Show resolved Hide resolved
fw/http_parser.c Outdated Show resolved Hide resolved
fw/http_parser.c Outdated Show resolved Hide resolved
fw/http_parser.c Outdated Show resolved Hide resolved
fw/http_parser.c Show resolved Hide resolved
fw/http_parser.c Show resolved Hide resolved
fw/http_parser.c Show resolved Hide resolved
fw/http_parser.c Show resolved Hide resolved
fw/http_parser.c Outdated Show resolved Hide resolved
@ttaym ttaym force-pushed the am-755-websocket-upgrade-directive branch from ebda32a to 4734569 Compare March 4, 2022 12:31
@ttaym ttaym marked this pull request as ready for review March 4, 2022 12:36
@ttaym ttaym requested a review from krizhanovsky March 4, 2022 12:36
Contributes to #755

Signed-off-by: Aleksey Mikhaylov <[email protected]>
@ttaym ttaym force-pushed the am-755-websocket-upgrade-directive branch from 9b5c4aa to 198aadf Compare March 5, 2022 06:49
Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

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

Good to merge after the couple of cleanups

fw/http_limits.c Show resolved Hide resolved
fw/http_limits.c Outdated Show resolved Hide resolved
fw/http_limits.c Outdated Show resolved Hide resolved
fw/http_parser.c Outdated Show resolved Hide resolved
@ttaym ttaym merged commit 3aef6a3 into master Mar 16, 2022
@ttaym ttaym deleted the am-755-websocket-upgrade-directive branch March 16, 2022 13:47
@ttaym ttaym restored the am-755-websocket-upgrade-directive branch March 16, 2022 13:59
@ttaym ttaym deleted the am-755-websocket-upgrade-directive branch March 16, 2022 14:00
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.

2 participants