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

Scheme parsing fix (https) and extending (ws, wss) #1570

Merged
merged 6 commits into from
Feb 25, 2022
Merged

Conversation

ttaym
Copy link
Contributor

@ttaym ttaym commented Feb 22, 2022

Contributes to #755

Signed-off-by: Aleksey Mikhaylov aym@tempesta-tech.com

Signed-off-by: Aleksey Mikhaylov <aym@tempesta-tech.com>
@ttaym ttaym marked this pull request as ready for review February 22, 2022 14:09
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.

Let's move the new code to a new cold state and __FSM_JMP() onto it if c != '/'. It seems all the states Req_UriAuthorityStart, Req_UriAuthority, Req_UriAuthorityIPv6, Req_UriAuthorityResetHost, Req_UriAuthorityEnd, Req_UriPort are also must be cold it also makes sense to move them to after the "Improbable states" comment

fw/http_parser.c Outdated Show resolved Hide resolved
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.

LGTM with small cleanups

fw/http_parser.c Outdated Show resolved Hide resolved
fw/http_parser.c Outdated
@@ -4768,11 +4624,32 @@ Req_Method_1CharStep: __attribute__((cold))
__FSM_MOVE_nofixup_n(Req_MUSpace, 0);
}

__FSM_STATE(Req_UriRareForms, cold) {
/* TODO Support authority form as in RFC7230#section-5.3.3
* when CONNECT method will be added */
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't plan to support CONNECT and it seems we don't need it in terms of HTTP/1 for websockets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right. CONNECT method not needed for websockets. And maybe not needed for Tempesta FW at all, because it is not an explicit proxy. But i have seen in code mentions and TODOs of CONNECT:

root@devvps:~/tempesta# grep -r -I -w CONNECT
fw/t/unit/test_http_parser.c:   TEST_REQ_UNKNOWN(CONNECT);
fw/http_parser.c:        * DELETE, TRACE and CONNECT requests has no defined semantics  \
fw/http_parser.c:                * (Successful) response to a CONNECT request
fw/http_parser.c:        * in any 2xx (Successful) response to a CONNECT request.
fw/http_parser.c:                * when CONNECT method will be added */
fw/http_parser.c:                       /* TODO: Add (req == CONNECT && resp == 2xx) */
fw/http.h:/* TODO: When CONNECT will be added, add it to tfw_handle_validation_req()
fw/cache.c:     /* TODO: Add CONNECT */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be consistent with this intention i have added the comment for clarity and completeness. I'll change its content to reflect your note.

@ttaym ttaym merged commit 46dd34d into master Feb 25, 2022
@ttaym ttaym deleted the am-755-scheme-parsing branch February 25, 2022 06:59
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