You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by ffcai <gi...@git.apache.org> on 2015/06/10 09:19:47 UTC
[GitHub] trafficserver pull request: TS-1125: handle "Expect: 100-continue"...
GitHub user ffcai opened a pull request:
https://github.com/apache/trafficserver/pull/216
TS-1125: handle "Expect: 100-continue" header in state_read_client_request_header
Currently, ATS handles `"Expect: 100-continue"` header in `HttpSM::state_send_server_request_header`. In intercept plugin case, ATS may have no chance to run into this logic, it handles the header in a later point - `HttpSM::state_send_server_request_header`. I did not take this into account when I wrote the first patch. Now we have an intercept plugin use case in yahoo, and I think we need to move the handle logic some earlier, right after finish parsing the client request header.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ffcai/trafficserver expect_100_continue
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/216.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #216
----
commit 34d6a4203d7399aa85daeeb88abafe5ee85f21b5
Author: Feifei Cai <ff...@yahoo-inc.com>
Date: 2015-06-10T07:06:24Z
handle Expect: 100-continue header in state_read_client_request_header
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-1125: handle "Expect: 100-continue"...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/216#issuecomment-110773332
Ok, then moving forward should be fine and your patch looks good. I'll get this merged up.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3693: Move 100-continue logic to re...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/216#issuecomment-116437793
@jacksontj sounds like it should be fine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-1125: handle "Expect: 100-continue"...
Posted by sudheerv <gi...@git.apache.org>.
Github user sudheerv commented on the pull request:
https://github.com/apache/trafficserver/pull/216#issuecomment-110774240
@ffcai is right about how the config <send_100_continue_response> works. Basically, when enabled, ATS would send a dummy "100 CONT" to the clients on receiving POST requests. Below's a clarification on why this feature was needed/added (afaik) -
*) ATS core has bugs/limitations in the handling of "100 CONT" message (apparently, ATS either stalls indefinitely on receiving the message from the origin or relays it back to the client too late (after POST body) both of which are not ideal from a client's perspective).
*) Some legacy clients always wait for a fixed duration before sending the POST body, if a "100 CONT" is not received.
The solution to address both concerns above was to basically send a dummy "100 CONT" from ATS and strip off the *Expect* header in the server request (TS-1125).
In any case, even with the previous patch, sending of "100 CONT" was being done in *HttpSM::state_send_server_request_header* which only means the origin connection has been made but, not really after the origin gave a go-ahead to send the POST body (there's no response from origin yet in that state, afaict).
{code}
The purpose of the 100 (Continue) status (see section 10.1.1) is to allow a client that is sending a request message with a request body to determine if the origin server is willing to accept the request (based on the request headers) before the client sends the request body. In some cases, it might either be inappropriate or highly inefficient for the client to send the body if the server will reject the message without looking at the body.
{code}
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3693: Move 100-continue logic to re...
Posted by ffcai <gi...@git.apache.org>.
Github user ffcai commented on the pull request:
https://github.com/apache/trafficserver/pull/216#issuecomment-112654624
Thanks @bryancall ! It's needed for server intercept.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-1125: handle "Expect: 100-continue"...
Posted by sudheerv <gi...@git.apache.org>.
Github user sudheerv commented on the pull request:
https://github.com/apache/trafficserver/pull/216#issuecomment-111157498
@ffcai : I think you should delay the sending of "100 CONT" to at least until after calling *is_request_valid()*, which occurs in *HttpTransact::HandleRequest*. It doesn't make sense to send back a "100 CONT" for a request that may end up returning an error (due to http header validation failures).
At that point, you have all the information (e.g transfer-encoding, or content-length etc) that is needed to preserve the conditional checks as they existed before.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-1125: handle "Expect: 100-continue"...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/216#issuecomment-110739883
I thought the reason sending the 100-continue is later was to make sure there was communication with the origin server (or access from the cache) before telling the client to go ahead and send the post data. So the client doesn't waste it's time and network bandwidth sending large post bodies.
If you move the send 100 earlier, in to the read request header, all you have guaranteed is that the client successfully communicated with ATS.
Perhaps a better option is to provide a call to the intercept plugin to send off the 100-continue later?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-1125: handle "Expect: 100-continue"...
Posted by ffcai <gi...@git.apache.org>.
Github user ffcai commented on the pull request:
https://github.com/apache/trafficserver/pull/216#issuecomment-110763755
You are right...However, here the configuration send_100_continue_response is introduced to keep compatible with some old origin server (e.g. HTTP 1.0 server). When it's enabled, ATS would send back 100 continue response to tell the client to go ahead, in case the origin server does not know the header and do nothing with it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-1125: handle "Expect: 100-continue"...
Posted by sudheerv <gi...@git.apache.org>.
Github user sudheerv commented on the pull request:
https://github.com/apache/trafficserver/pull/216#issuecomment-110775146
I need to review this more - Shouldn't the patch *at least* check that the method is *POST*?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-1125: handle "Expect: 100-continue"...
Posted by sudheerv <gi...@git.apache.org>.
Github user sudheerv commented on the pull request:
https://github.com/apache/trafficserver/pull/216#issuecomment-111531431
@ffcai: I'm a little concerned about this change - this would mean that requests that would otherwise return an error would always return a "100 CONT" first. It seems quite odd that, a request would get a "100 CONT" followed by a "404 - Not found on Accelerator", for example (or even a "403 - Forbidden", for e.g with *quick_filter*).
The current implementation of the "100 CONT" is already a hack (and not inline with the spec), but, at least, it ensures that the requests pass the proxy checks/validations.
Making this change now to send a "100 CONT" immediately after seeing (and basic parsing of) the request, to all cases (not just the cases where a intercept plugin is being used) seems pretty bad to me. It may even open up a vulnerability that someone can exploit (e.g. keep pounding the box with a POST request with Expect header).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-1125: handle "Expect: 100-continue"...
Posted by ffcai <gi...@git.apache.org>.
Github user ffcai commented on the pull request:
https://github.com/apache/trafficserver/pull/216#issuecomment-111380014
@sudheerv , as discussed some earlier, in transaction interception case, ATS would not have chance to handle the *"Expect: 100-continue"* header. Our sherpa plugin intercepts in the *TS_HTTP_READ_REQUEST_HDR_HOOK* point, whereas ATS handles "Expect" header in *HttpSM::state_send_server_request_header* call (old patch), which would not have chance to execute in interception case.
*HttpTransact::HandleRequest* is called after *RemapRequest*, which is a later point of *TS_HTTP_READ_REQUEST_HDR_HOOK*. I think here we have to send the 100 continue response in *HttpSM::state_read_client_request_header*.
https://trafficserver.readthedocs.org/en/latest/sdk/http-hooks-and-transactions.en.html#http-txn-state-diagram
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-1125: handle "Expect: 100-continue"...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/216#issuecomment-110776224
I suppose it could check the method. As it stands if someone put a "Except 100 continue" in a GET request header it would return a 100-continue which I guess would be out of spec(?). Not a huge deal either way in my opinion.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-1125: handle "Expect: 100-continue"...
Posted by adityaumrani <gi...@git.apache.org>.
Github user adityaumrani commented on the pull request:
https://github.com/apache/trafficserver/pull/216#issuecomment-111023984
You can check for either chunked encoding or content length > 0.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3693: Move 100-continue logic to re...
Posted by ffcai <gi...@git.apache.org>.
Github user ffcai closed the pull request at:
https://github.com/apache/trafficserver/pull/216
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-1125: handle "Expect: 100-continue"...
Posted by ffcai <gi...@git.apache.org>.
Github user ffcai commented on the pull request:
https://github.com/apache/trafficserver/pull/216#issuecomment-111045489
Hi @adityaumrani , for *POST/PUT* method, `Content-Length` should present or use Chunked Transfer Encoding (present `Transfer-Encoding: chunked`). And, ATS has a dedicated function - `HttpTransact::check_request_validity` for this check.
```cpp
// Require Content-Length/Transfer-Encoding for POST/PUSH/PUT
if ((scheme == URL_WKSIDX_HTTP || scheme == URL_WKSIDX_HTTPS) &&
(method == HTTP_WKSIDX_POST || method == HTTP_WKSIDX_PUSH || method == HTTP_WKSIDX_PUT) &&
s->client_info.transfer_encoding != CHUNKED_ENCODING) {
if ((s->txn_conf->post_check_content_length_enabled) && !incoming_hdr->presence(MIME_PRESENCE_CONTENT_LENGTH)) {
return NO_POST_CONTENT_LENGTH;
}
if (HTTP_UNDEFINED_CL == s->hdr_info.request_content_length) {
return INVALID_POST_CONTENT_LENGTH;
}
}
}
```
https://github.com/apache/trafficserver/blob/master/proxy/http/HttpTransact.cc#L5282
I think we can just skip doing this here and let the function check it some later.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-1125: handle "Expect: 100-continue"...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on the pull request:
https://github.com/apache/trafficserver/pull/216#issuecomment-112223076
There should be a new Jira ticket created for this and not try to use the perviously closed ticket TS-1125.
Is this needed for intercept or server intercept?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-1125: handle "Expect: 100-continue"...
Posted by ffcai <gi...@git.apache.org>.
Github user ffcai commented on the pull request:
https://github.com/apache/trafficserver/pull/216#issuecomment-111015398
Hi @sudheerv , I add the check for *HTTP/1.1* and *POST/PUT* method.
As to *Content-Length*, it may not present when using *Chunked Transfer Encoding*, so I think we do not check for this header. How do you think?
http://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-1125: handle "Expect: 100-continue"...
Posted by ffcai <gi...@git.apache.org>.
Github user ffcai commented on the pull request:
https://github.com/apache/trafficserver/pull/216#issuecomment-111710928
OK, looking forward to @zwoop 's comments. If this is not accept, we'd have to ask some intercept plugin for adding another "100 continue" hack in its own logic.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-3693: Move 100-continue logic to re...
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj commented on the pull request:
https://github.com/apache/trafficserver/pull/216#issuecomment-115726377
@bgaff Does this play nice with our double plugin execution fix?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-1125: handle "Expect: 100-continue"...
Posted by sudheerv <gi...@git.apache.org>.
Github user sudheerv commented on the pull request:
https://github.com/apache/trafficserver/pull/216#issuecomment-110778191
IIRC, the solution to TS-1125 was to check for some request headers (e.g *Content-Length* > 0) before sending the dummy "100 CONT". I don't think that logic should be lost when trying to move things around.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---