-
Notifications
You must be signed in to change notification settings - Fork 103
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
Conversation
Signed-off-by: Aleksey Mikhaylov <aym@tempesta-tech.com>
d192289
to
1636b2f
Compare
There was a problem hiding this 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
There was a problem hiding this 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
@@ -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 */ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 */
There was a problem hiding this comment.
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.
Contributes to #755
Signed-off-by: Aleksey Mikhaylov aym@tempesta-tech.com