Uploaded image for project: 'Traffic Server'
  1. Traffic Server
  2. TS-1125

POST's with Expect: 100-continue are slowed by delayed 100 response.

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0.2
    • Fix Version/s: 5.0.0
    • Component/s: HTTP
    • Labels:
    • Environment:

      TS 3.0.2 going to Apache 2.2 web server

      Description

      Sending a post like:
      POST / HTTP/1.1
      Host: www.example.com
      Content-Length: 10
      Expect: 100-continue

      directly to the web server immediately sends back:
      HTTP/1.1 100 Continue

      And then when the post data is sent, a status 200 response comes back.
      But when going through ATS the "HTTP/1.1 100 Continue" is not sent immediately, and instead is sent after the POST data has been received. This is legal, but it makes clients that are hoping for a 100 continue to wait a little while hoping to get that, ATS should forward that response through immediately.

      Note: I see curl using "Expect: 100-continue" with > 1024 bytes of post data, but web searching indicates that some Microsoft products also use it.

      1. ts1125.diff
        11 kB
        Feifei Cai
      2. ts1125.diff
        11 kB
        Bryan Call
      3. ts1125.diff
        11 kB
        Bryan Call
      4. TS-1125.diff
        11 kB
        Feifei Cai
      5. TS-1125.diff
        9 kB
        Feifei Cai

        Issue Links

          Activity

          Hide
          zwoop Leif Hedstrom added a comment -

          Moving all unassigned bugs out to 3.3.0. Move back and assign as necessary.

          Show
          zwoop Leif Hedstrom added a comment - Moving all unassigned bugs out to 3.3.0. Move back and assign as necessary.
          Hide
          janfrode Jan-Frode Myklebust added a comment -

          It seems to be waiting more than a little while. We see a full 2 second delay for POST with large responses from origin server, on ATS 3.2.3.

          Show
          janfrode Jan-Frode Myklebust added a comment - It seems to be waiting more than a little while. We see a full 2 second delay for POST with large responses from origin server, on ATS 3.2.3.
          Hide
          zwoop Leif Hedstrom added a comment -

          Moving to 3.3.2.

          Show
          zwoop Leif Hedstrom added a comment - Moving to 3.3.2.
          Hide
          zwoop Leif Hedstrom added a comment -

          William: Any updates on this? Patches?

          Show
          zwoop Leif Hedstrom added a comment - William: Any updates on this? Patches?
          Hide
          wbardwel William Bardwell added a comment -

          I haven't worked on this.

          Show
          wbardwel William Bardwell added a comment - I haven't worked on this.
          Hide
          ushachar Uri Shachar added a comment -

          AFAIK – no browsers actually use "Expect: 100-Continue", but many other clients do (for example, cURL, .NET clients, adobe downloaders when not configured with explicit proxy...)

          It's quite hard to implement support for this correctly in a proxy perspective (Squid didn't support this until 3.2) - If you return a 100-Continue immediately (regardless of upstream), you run into issues if the upstream refuses the request....

          Note that according to the RFC, clients which have seen a 100-Continue response from a particular upstream are allowed to hang indefinitely while waiting for the response – if they haven't seen such a response in the past they "SHOULD NOT"...

          Show
          ushachar Uri Shachar added a comment - AFAIK – no browsers actually use "Expect: 100-Continue", but many other clients do (for example, cURL, .NET clients, adobe downloaders when not configured with explicit proxy...) It's quite hard to implement support for this correctly in a proxy perspective (Squid didn't support this until 3.2) - If you return a 100-Continue immediately (regardless of upstream), you run into issues if the upstream refuses the request.... Note that according to the RFC, clients which have seen a 100-Continue response from a particular upstream are allowed to hang indefinitely while waiting for the response – if they haven't seen such a response in the past they "SHOULD NOT"...
          Hide
          janfrode Jan-Frode Myklebust added a comment -

          Squid has a knob to ignore it: http://www.squid-cache.org/Doc/config/ignore_expect_100/

          Wouldn't it be better to fake it – provide a knob to return 100 continue regardless of if the backend will accept the request, than the current behaviour that doesn't return 100 continue before data is already posted ?

          Show
          janfrode Jan-Frode Myklebust added a comment - Squid has a knob to ignore it: http://www.squid-cache.org/Doc/config/ignore_expect_100/ Wouldn't it be better to fake it – provide a knob to return 100 continue regardless of if the backend will accept the request, than the current behaviour that doesn't return 100 continue before data is already posted ?
          Hide
          ushachar Uri Shachar added a comment -

          Sure – adding a knob would be better then what we currently have.

          Show
          ushachar Uri Shachar added a comment - Sure – adding a knob would be better then what we currently have.
          Show
          zwoop Leif Hedstrom added a comment - Moving to 4.2.0 as per https://cwiki.apache.org/confluence/display/TS/New+Release+Processes
          Hide
          ffcai Feifei Cai added a comment -

          Force ATS to return a “100 Continue” response to client as soon as it receives the "Expect: 100-continue" request from client, and remove this header, not relay to original server.

          This is a quick fix. It does not strictly follows RFC-2616's requirements.

          Show
          ffcai Feifei Cai added a comment - Force ATS to return a “100 Continue” response to client as soon as it receives the "Expect: 100-continue" request from client, and remove this header, not relay to original server. This is a quick fix. It does not strictly follows RFC-2616's requirements.
          Hide
          bcall Bryan Call added a comment -

          Feifei Cai,

          Can you explain why the VC_EVENT_WRITE_COMPLETE event action had to change. Section of the patch:

             case VC_EVENT_WRITE_COMPLETE:
          -    // mark the vc as no longer in tunnel
          -    //   so we don't get hosed if the ua abort before
          -    //   real response header is received
          -    ua_entry->in_tunnel = false;
          +    if (t_state.txn_conf->send_100_continue_response == 0) {
          +       // mark the vc as no longer in tunnel
          +       //   so we don't get hosed if the ua abort before
          +       //   real response header is received
          +       ua_entry->in_tunnel = false;
          +    }
               c->write_success = true;
          
          Show
          bcall Bryan Call added a comment - Feifei Cai , Can you explain why the VC_EVENT_WRITE_COMPLETE event action had to change. Section of the patch: case VC_EVENT_WRITE_COMPLETE: - // mark the vc as no longer in tunnel - // so we don't get hosed if the ua abort before - // real response header is received - ua_entry->in_tunnel = false ; + if (t_state.txn_conf->send_100_continue_response == 0) { + // mark the vc as no longer in tunnel + // so we don't get hosed if the ua abort before + // real response header is received + ua_entry->in_tunnel = false ; + } c->write_success = true ;
          Hide
          bcall Bryan Call added a comment -

          Talked to Sudheer and we agreed that the code in VC_EVENT_WRITE_COMPLETE isn't needed with the way the 100-continue is now implemented.

          Show
          bcall Bryan Call added a comment - Talked to Sudheer and we agreed that the code in VC_EVENT_WRITE_COMPLETE isn't needed with the way the 100-continue is now implemented.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 9da123014382c00bccb1869782a4f2502229a459 in trafficserver's branch refs/heads/master from Bryan Call
          [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=9da1230 ]

          TS-1125: POST's with Expect: 100-continue are slowed by delayed 100 response
          Additional Authors:
          Feifei Cai <ffcai@yahoo-inc.com>
          Sudheer Vinukonda <sudheerv@yahoo-inc.com>

          Show
          jira-bot ASF subversion and git services added a comment - Commit 9da123014382c00bccb1869782a4f2502229a459 in trafficserver's branch refs/heads/master from Bryan Call [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=9da1230 ] TS-1125 : POST's with Expect: 100-continue are slowed by delayed 100 response Additional Authors: Feifei Cai <ffcai@yahoo-inc.com> Sudheer Vinukonda <sudheerv@yahoo-inc.com>
          Hide
          bcall Bryan Call added a comment -

          Made changes to the patch to use existing ssl write function, set the header values in the WKS, and got rid of the tunnel code for the VC_EVENT_WRITE_COMPLETE event

          Show
          bcall Bryan Call added a comment - Made changes to the patch to use existing ssl write function, set the header values in the WKS, and got rid of the tunnel code for the VC_EVENT_WRITE_COMPLETE event
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit bdcf937732aaa2004640bd4fc6fb3e20cc4ec008 in trafficserver's branch refs/heads/master from Bryan Call
          [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=bdcf937 ]

          Added TS-2650, TS-1125, TS-2548, TS-2710, TS-2704 to CHANGES

          Show
          jira-bot ASF subversion and git services added a comment - Commit bdcf937732aaa2004640bd4fc6fb3e20cc4ec008 in trafficserver's branch refs/heads/master from Bryan Call [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=bdcf937 ] Added TS-2650 , TS-1125 , TS-2548 , TS-2710 , TS-2704 to CHANGES
          Hide
          jamespeach James Peach added a comment -

          This change crashes traffic_manager and traffic_server on startup every time.

          Show
          jamespeach James Peach added a comment - This change crashes traffic_manager and traffic_server on startup every time.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit bcfd36abfa423fd84afe008f90205068da6642ac in trafficserver's branch refs/heads/master from James Peach
          [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=bcfd36a ]

          Revert "TS-1125: POST's with Expect: 100-continue are slowed by delayed 100 response"

          This reverts commit 9da123014382c00bccb1869782a4f2502229a459 because
          it breaks traffic_server and traffic_manager sta startup:

          oarfish:trafficserver.git jpeach$ sudo /tmp/ats/bin/traffic_server -T hdr_token
          [TrafficServer] using root directory '/tmp/ats'
          [Apr 17 21:07:16.489] Server

          {0x7fff745dd310}

          DEBUG: (hdr_token) Did not find a WKS for '100-continue'
          NOTE: Traffic Server received Kernel Sig 11, Reason: 1
          /tmp/ats/bin/traffic_server - STACK TRACE:
          0 libsystem_platform.dylib 0x00007fff849a85aa _sigtramp + 26
          1 ??? 0x00007fff00000001 0x0 + 140733193388033
          2 traffic_server 0x0000000101b1ad25 main + 2453
          3 libdyld.dylib 0x00007fff890b25fd start + 1

          Show
          jira-bot ASF subversion and git services added a comment - Commit bcfd36abfa423fd84afe008f90205068da6642ac in trafficserver's branch refs/heads/master from James Peach [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=bcfd36a ] Revert " TS-1125 : POST's with Expect: 100-continue are slowed by delayed 100 response" This reverts commit 9da123014382c00bccb1869782a4f2502229a459 because it breaks traffic_server and traffic_manager sta startup: oarfish:trafficserver.git jpeach$ sudo /tmp/ats/bin/traffic_server -T hdr_token [TrafficServer] using root directory '/tmp/ats' [Apr 17 21:07:16.489] Server {0x7fff745dd310} DEBUG: (hdr_token) Did not find a WKS for '100-continue' NOTE: Traffic Server received Kernel Sig 11, Reason: 1 /tmp/ats/bin/traffic_server - STACK TRACE: 0 libsystem_platform.dylib 0x00007fff849a85aa _sigtramp + 26 1 ??? 0x00007fff00000001 0x0 + 140733193388033 2 traffic_server 0x0000000101b1ad25 main + 2453 3 libdyld.dylib 0x00007fff890b25fd start + 1
          Hide
          bcall Bryan Call added a comment -

          Updated with strings defined in HdrToken.cc

          Show
          bcall Bryan Call added a comment - Updated with strings defined in HdrToken.cc
          Hide
          zwoop Leif Hedstrom added a comment -

          Couple of comments on the latest patch:

          1. It seems the new configuration is an overridable configuration, but there is no support for that in ts/ts.h, InkAPI.cc nor are there any regression tests. If this is not intended to be overridable, it should be moved out of the overridable struct IMO.

          2. The new WKS string is added before existing WKS strings, I think? Didn't we agree to append new WKS's to the end of the list? Otherwise, you also have to bump the cache version (at least the minor version), and force it to re-read the WKS on every object. It seems better to append the new WKS. At a minimum, you'd have to bump the cache minor version, to avoid that same crash problem (but my vote is to append to the WKSs).

          3. The "raw" write on the socket seems, unorthodox. Is there no way here to use the normal VIO APIs ?

          Cheers,

          – Leif

          Show
          zwoop Leif Hedstrom added a comment - Couple of comments on the latest patch: 1. It seems the new configuration is an overridable configuration, but there is no support for that in ts/ts.h, InkAPI.cc nor are there any regression tests. If this is not intended to be overridable, it should be moved out of the overridable struct IMO. 2. The new WKS string is added before existing WKS strings, I think? Didn't we agree to append new WKS's to the end of the list? Otherwise, you also have to bump the cache version (at least the minor version), and force it to re-read the WKS on every object. It seems better to append the new WKS. At a minimum, you'd have to bump the cache minor version, to avoid that same crash problem (but my vote is to append to the WKSs). 3. The "raw" write on the socket seems, unorthodox. Is there no way here to use the normal VIO APIs ? Cheers, – Leif
          Hide
          bcall Bryan Call added a comment -

          Moved the new WKS to the end of the array.

          Show
          bcall Bryan Call added a comment - Moved the new WKS to the end of the array.
          Hide
          ffcai Feifei Cai added a comment -

          Correct the little mistake of comma.

          Show
          ffcai Feifei Cai added a comment - Correct the little mistake of comma.
          Hide
          ffcai Feifei Cai added a comment -

          Hi Leif Hedstrom,
          Thanks for your comments.
          1. Overridable configuration
          After further check and referring to docs, I found overridable configuration is not needed. I'll move it out of overridable struct. Thanks for your advice.
          2. Append new WKS to end of the list.
          Thanks to Bryan Call's help. The latest patch has done this.
          3. Use VIO APIs.
          The patch with "raw" write has been verified in yahoo's servers. I'll try with normal VIO APIs, and update a new patch after it's verified.

          Show
          ffcai Feifei Cai added a comment - Hi Leif Hedstrom , Thanks for your comments. 1. Overridable configuration After further check and referring to docs , I found overridable configuration is not needed. I'll move it out of overridable struct. Thanks for your advice. 2. Append new WKS to end of the list. Thanks to Bryan Call 's help. The latest patch has done this. 3. Use VIO APIs. The patch with "raw" write has been verified in yahoo's servers. I'll try with normal VIO APIs, and update a new patch after it's verified.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit df0e9bb0537b460b418544cb67d57b20e63d1cd7 in trafficserver's branch refs/heads/master from Bryan Call
          [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=df0e9bb ]

          updated changes to add TS-2762 and remove TS-1125

          Show
          jira-bot ASF subversion and git services added a comment - Commit df0e9bb0537b460b418544cb67d57b20e63d1cd7 in trafficserver's branch refs/heads/master from Bryan Call [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=df0e9bb ] updated changes to add TS-2762 and remove TS-1125
          Hide
          ffcai Feifei Cai added a comment -

          Some changes of the new patch:
          1. Change the configuration option for 100 continue response, not using overridable configuration.
          2. Use normal VIO APIs, instead of the initial fix of "raw" write.

          Show
          ffcai Feifei Cai added a comment - Some changes of the new patch: 1. Change the configuration option for 100 continue response, not using overridable configuration. 2. Use normal VIO APIs, instead of the initial fix of "raw" write.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit e5da353c51fc6b1bc5cc150db4a5f4f4e334fd6c in trafficserver's branch refs/heads/master from Feifei Cai
          [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=e5da353 ]

          TS-1125: POST's with Expect: 100-continue are slowed by delayed 100 response

          Show
          jira-bot ASF subversion and git services added a comment - Commit e5da353c51fc6b1bc5cc150db4a5f4f4e334fd6c in trafficserver's branch refs/heads/master from Feifei Cai [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=e5da353 ] TS-1125 : POST's with Expect: 100-continue are slowed by delayed 100 response
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 95afef8b28705e48938b31ca069b73770f17dd27 in trafficserver's branch refs/heads/master from Bryan Call
          [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=95afef8 ]

          Added TS-1125 to changes

          Show
          jira-bot ASF subversion and git services added a comment - Commit 95afef8b28705e48938b31ca069b73770f17dd27 in trafficserver's branch refs/heads/master from Bryan Call [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=95afef8 ] Added TS-1125 to changes
          Hide
          bcall Bryan Call added a comment -

          Final patch looks good. I set the 100-continue response body variables to be static (scoped to the file). Test and it works...

          Show
          bcall Bryan Call added a comment - Final patch looks good. I set the 100-continue response body variables to be static (scoped to the file). Test and it works...
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user jacksontj opened a pull request:

          https://github.com/apache/trafficserver/pull/153

          Document feature from TS-1125

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/jacksontj/trafficserver master_TS-1125

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/trafficserver/pull/153.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 #153


          commit 3bda48678862f065e9b879ca28b6ee4a6d622260
          Author: Thomas Jackson <jacksontj.89@gmail.com>
          Date: 2014-12-09T23:12:17Z

          Document feature from TS-1125


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user jacksontj opened a pull request: https://github.com/apache/trafficserver/pull/153 Document feature from TS-1125 You can merge this pull request into a Git repository by running: $ git pull https://github.com/jacksontj/trafficserver master_ TS-1125 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/153.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 #153 commit 3bda48678862f065e9b879ca28b6ee4a6d622260 Author: Thomas Jackson <jacksontj.89@gmail.com> Date: 2014-12-09T23:12:17Z Document feature from TS-1125
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 665b442dad51d89cc9f42f5394f3099d75519ec2 in trafficserver's branch refs/heads/master from Thomas Jackson
          [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=665b442 ]

          Document feature from TS-1125

          This closes #153

          Show
          jira-bot ASF subversion and git services added a comment - Commit 665b442dad51d89cc9f42f5394f3099d75519ec2 in trafficserver's branch refs/heads/master from Thomas Jackson [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=665b442 ] Document feature from TS-1125 This closes #153
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/trafficserver/pull/153

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/trafficserver/pull/153
          Hide
          githubbot ASF GitHub Bot added a comment -

          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 <ffcai@yahoo-inc.com>
          Date: 2015-06-10T07:06:24Z

          handle Expect: 100-continue header in state_read_client_request_header


          Show
          githubbot ASF GitHub Bot added a comment - 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 <ffcai@yahoo-inc.com> Date: 2015-06-10T07:06:24Z handle Expect: 100-continue header in state_read_client_request_header
          Hide
          githubbot ASF GitHub Bot added a comment -

          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?

          Show
          githubbot ASF GitHub Bot added a comment - 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?
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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).

              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.
              
          Show
          githubbot ASF GitHub Bot added a comment - 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). 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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?

          Show
          githubbot ASF GitHub Bot added a comment - 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 ?
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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

          Show
          githubbot ASF GitHub Bot added a comment - 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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

          Show
          githubbot ASF GitHub Bot added a comment - 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          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).

          Show
          githubbot ASF GitHub Bot added a comment - 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).
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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?

          Show
          githubbot ASF GitHub Bot added a comment - 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?

            People

            • Assignee:
              bcall Bryan Call
              Reporter:
              wbardwel William Bardwell
            • Votes:
              1 Vote for this issue
              Watchers:
              13 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development