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.

Sign up for GitHub

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

Jump to bottom

Proxy does not handle empty request bodies correctly (except for HEAD and GET?) #447

Closed
briansmith opened this issue Feb 24, 2018 · 1 comment · Fixed by #457
Closed

Proxy does not handle empty request bodies correctly (except for HEAD and GET?) #447

briansmith opened this issue Feb 24, 2018 · 1 comment · Fixed by #457
Assignees
@seanmonstar
Labels
area/proxy priority/P0 Release Blocker
Milestone
0.3.1

Comments

@briansmith
Copy link
Contributor

This is a follow-up to #402 to track the real solution to the problem that the proxy doesn't handle requests without a body correctly. The PR for #402 only partially resolves the issue. Really we should fix the real issue. I very vaguely understand that to require some changes to the Hyper API or some other more drastic solution.

In particular, DELETE will almost never have a request body, so many REST APis would be affected by this.

/cc @olix0r @seanmonstar
@briansmith briansmith added the bug label Feb 24, 2018
@seanmonstar
Copy link
Contributor

The bug here is a combination of a few things:

  • hyper's Request holds an optional body. If not set, then no body is assumed. Otherwise, it assumes a body.
  • The body is generic over a futures Stream, so there is no way to ask anything about it before the headers are written.
  • If the body is set, and there is no content-length header, it inserts Transfer-Encoding chunked, since the length cannot be calculated.
  • The proxy maps bodies from both hyper and h2, with the result that client requests always have the body set, even if internally the stream will be empty.

In the long term, hyper is going to change it's body type to a trait with more methods that it can ask about if the body is empty.

In the short term, I think a small addition in hyper to expose that server bodies are empty, and then a modification in the proxy to check that, can allow optionally setting the body in hyper client requests.

@seanmonstar seanmonstar self-assigned this Feb 24, 2018
@seanmonstar seanmonstar added this to the Conduit 0.3.1 milestone Feb 24, 2018
seanmonstar added a commit that referenced this issue Feb 26, 2018
@seanmonstar
proxy: prevent body-less transfer-encoding in http1
22cbd19 
Closes #447

Signed-off-by: Sean McArthur <sean@seanmonstar.com>
@seanmonstar seanmonstar mentioned this issue Feb 26, 2018
Merged
@seanmonstar seanmonstar added the review/ready Issue has a reviewable PR label Feb 26, 2018
seanmonstar added a commit that referenced this issue Feb 28, 2018
@seanmonstar
proxy: prevent body-less transfer-encoding in http1
6b2f033 
Closes #447

Signed-off-by: Sean McArthur <sean@seanmonstar.com>
seanmonstar added a commit that referenced this issue Mar 1, 2018
@seanmonstar
proxy: prevent body-less transfer-encoding in http1
bb1f60c 
Closes #447

Signed-off-by: Sean McArthur <sean@seanmonstar.com>
seanmonstar added a commit that referenced this issue Mar 2, 2018
@seanmonstar
proxy: prevent body-less transfer-encoding in http1
5c170dd 
Closes #447

Signed-off-by: Sean McArthur <sean@seanmonstar.com>
seanmonstar added a commit that referenced this issue Mar 5, 2018
@seanmonstar
proxy: preserve body headers in http1
2e106aa 
As a goal of being a transparent proxy, we want to proxy requests and
responses with as little modification as possible. Basically, servers
and clients should see messages that look the same whether the proxy was
injected or not.

With that goal in mind, we want to make sure that body headers (things
like `Content-Length`, `Transfer-Encoding`, etc) are left alone. Prior
to this commit, we at times were changing behavior. Sometimes
`Transfer-Encoding` was added to requests, or `Content-Length: 0` may
have been removed. While RC 7230 defines that differences are
semantically the same, implementations may not handle them correctly.

Now, we've added some fixes to prevent any of these header changes
from occurring, along with tests to make sure library updates don't
regress.

For requests:

- With no message body, `Transfer-Encoding: chunked` should no longer be
added.
- With `Content-Length: 0`, the header is forwarded untouched.

For responses:

- Tests were added that responses not allowed to have bodies (to HEAD
requests, 204, 304) did not have `Transfer-Encoding` added.
- Tests that `Content-Length: 0` is preserved.
- Tests that HTTP/1.0 responses with no body headers do not have
`Transfer-Encoding` added.
- Tests that `HEAD` responses forward `Content-Length` headers (but not
an actual body).

Closes #447

Signed-off-by: Sean McArthur <sean@seanmonstar.com>
seanmonstar added a commit that referenced this issue Mar 6, 2018
@seanmonstar
proxy: preserve body headers in http1
f6b49ae 
As a goal of being a transparent proxy, we want to proxy requests and
responses with as little modification as possible. Basically, servers
and clients should see messages that look the same whether the proxy was
injected or not.

With that goal in mind, we want to make sure that body headers (things
like `Content-Length`, `Transfer-Encoding`, etc) are left alone. Prior
to this commit, we at times were changing behavior. Sometimes
`Transfer-Encoding` was added to requests, or `Content-Length: 0` may
have been removed. While RC 7230 defines that differences are
semantically the same, implementations may not handle them correctly.

Now, we've added some fixes to prevent any of these header changes
from occurring, along with tests to make sure library updates don't
regress.

For requests:

- With no message body, `Transfer-Encoding: chunked` should no longer be
added.
- With `Content-Length: 0`, the header is forwarded untouched.

For responses:

- Tests were added that responses not allowed to have bodies (to HEAD
requests, 204, 304) did not have `Transfer-Encoding` added.
- Tests that `Content-Length: 0` is preserved.
- Tests that HTTP/1.0 responses with no body headers do not have
`Transfer-Encoding` added.
- Tests that `HEAD` responses forward `Content-Length` headers (but not
an actual body).

Closes #447

Signed-off-by: Sean McArthur <sean@seanmonstar.com>
seanmonstar added a commit that referenced this issue Mar 6, 2018
@seanmonstar
proxy: preserve body headers in http1 (#457)
c278228 
As a goal of being a transparent proxy, we want to proxy requests and
responses with as little modification as possible. Basically, servers
and clients should see messages that look the same whether the proxy was
injected or not.

With that goal in mind, we want to make sure that body headers (things
like `Content-Length`, `Transfer-Encoding`, etc) are left alone. Prior
to this commit, we at times were changing behavior. Sometimes
`Transfer-Encoding` was added to requests, or `Content-Length: 0` may
have been removed. While RC 7230 defines that differences are
semantically the same, implementations may not handle them correctly.

Now, we've added some fixes to prevent any of these header changes
from occurring, along with tests to make sure library updates don't
regress.

For requests:

- With no message body, `Transfer-Encoding: chunked` should no longer be
added.
- With `Content-Length: 0`, the header is forwarded untouched.

For responses:

- Tests were added that responses not allowed to have bodies (to HEAD
requests, 204, 304) did not have `Transfer-Encoding` added.
- Tests that `Content-Length: 0` is preserved.
- Tests that HTTP/1.0 responses with no body headers do not have
`Transfer-Encoding` added.
- Tests that `HEAD` responses forward `Content-Length` headers (but not
an actual body).

Closes #447

Signed-off-by: Sean McArthur <sean@seanmonstar.com>
@olix0r olix0r removed the review/ready Issue has a reviewable PR label Mar 6, 2018
hawkw pushed a commit that referenced this issue Mar 6, 2018
@seanmonstar @hawkw
proxy: prevent body-less transfer-encoding in http1
b5b2d34 
Closes #447

Signed-off-by: Sean McArthur <sean@seanmonstar.com>
khappucino pushed a commit to Nordstrom/linkerd2 that referenced this issue Mar 5, 2019
@seanmonstar
proxy: preserve body headers in http1 (linkerd#457)
e07700b 
As a goal of being a transparent proxy, we want to proxy requests and
responses with as little modification as possible. Basically, servers
and clients should see messages that look the same whether the proxy was
injected or not.

With that goal in mind, we want to make sure that body headers (things
like `Content-Length`, `Transfer-Encoding`, etc) are left alone. Prior
to this commit, we at times were changing behavior. Sometimes
`Transfer-Encoding` was added to requests, or `Content-Length: 0` may
have been removed. While RC 7230 defines that differences are
semantically the same, implementations may not handle them correctly.

Now, we've added some fixes to prevent any of these header changes
from occurring, along with tests to make sure library updates don't
regress.

For requests:

- With no message body, `Transfer-Encoding: chunked` should no longer be
added.
- With `Content-Length: 0`, the header is forwarded untouched.

For responses:

- Tests were added that responses not allowed to have bodies (to HEAD
requests, 204, 304) did not have `Transfer-Encoding` added.
- Tests that `Content-Length: 0` is preserved.
- Tests that HTTP/1.0 responses with no body headers do not have
`Transfer-Encoding` added.
- Tests that `HEAD` responses forward `Content-Length` headers (but not
an actual body).

Closes linkerd#447

Signed-off-by: Sean McArthur <sean@seanmonstar.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@seanmonstar seanmonstar

Labels
area/proxy priority/P0 Release Blocker
Projects
None yet
Milestone
0.3.1
Development

Successfully merging a pull request may close this issue.

proxy: preserve body headers in http1 linkerd/linkerd2
3 participants
@briansmith @seanmonstar @olix0r