golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
122.95k stars 17.53k forks source link

proposal: net/http: customize limit on number of 1xx responses #65035

Open Acconut opened 8 months ago

Acconut commented 8 months ago

Proposal Details

In HTTP, a server can send multiple responses for one requests: zero or more informational responses (1xx) and one final response (2xx, 3xx, 4xx, 5xx). Go's HTTP client is capable of receiving those informational responses and exposes them to users via net/http/httptrace.ClientTrace.Got1xxResponse. However, the client has a default limit of reading only up to 5 responses. Any additional 1xx response will trigger a net/http: too many 1xx informational responses error.

https://github.com/golang/go/blob/8db131082d08e497fd8e9383d0ff7715e1bef478/src/net/http/transport.go#L2328-L2329

https://github.com/golang/go/blob/8db131082d08e497fd8e9383d0ff7715e1bef478/src/net/http/transport.go#L2354-L2357

The code comments and the original commit (https://github.com/golang/go/commit/d88b13786d3f1645ee59c2be555cb18cf49fe2e5) mention that the limit of 5 responses is arbitrary. If the limit is reached, the entire request is stopped and the client cannot receive the final response (2xx etc) anymore. This is problematic for applications, where the server repeatedly sends informational responses. 5 is a sensible default value for nearly all applications, but it would be helpful if this limit could be customized to allow more or even an unlimited amount of responses.

One option for implementing this, would be to add another field to the net/http.Client struct. Setting it to a zero value keeps the current limit of 5 responses, while any other non-zero value sets the limit accordingly.

Background

In the HTTP working group of the IETF we are discussing a draft on resumable uploads. We are considering including a feature where the server can repeatedly send 1xx responses to inform the client about the upload progress. In these scenarios, the client sends data in the request body and repeatedly receives progress information in the 1xx responses. This progress information can be used to release data that is buffered on the client-side.

Example

Below you can find a brief program reproducing this behavior. The client sends a request to a server which responds with 10 1xx responses:

package main

import (
    "context"
    "fmt"
    "log"
    "net/http"
    "net/http/httptest"
    "net/http/httptrace"
    "net/textproto"
    "time"
)

func main() {
    ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        for i := 0; i < 10; i++ {
            w.Header().Set("X-Progress", fmt.Sprintf("%d%%", i*10))
            w.WriteHeader(150)
            <-time.After(100 * time.Millisecond)
        }

        w.WriteHeader(200)
    }))
    defer ts.Close()

    ctx := httptrace.WithClientTrace(context.Background(), &httptrace.ClientTrace{
        Got1xxResponse: func(code int, header textproto.MIMEHeader) error {
            fmt.Println("Progress:", header.Get("X-Progress"))

            return nil
        },
    })

    req, err := http.NewRequestWithContext(ctx, "GET", ts.URL, nil)
    if err != nil {
        log.Fatal(err)
    }

    client := ts.Client()
    res, err := client.Do(req)
    if err != nil {
        log.Fatal(err)
    }

    fmt.Println(res.Status)
}

The client receives the first 5 1xx responses, but then errors out. The final response is not received by the client.

$ go run clients/go/test/example.go 
Progress: 0%
Progress: 10%
Progress: 20%
Progress: 30%
Progress: 40%
2024/01/09 10:05:56 Get "http://127.0.0.1:52073": net/http: HTTP/1.x transport connection broken: net/http: too many 1xx informational responses
exit status 1

If the limit could be raised, the client could receive all informational and the final response without an error.

seankhliao commented 8 months ago

cc @neild

royfielding commented 8 months ago

This is a bug in the Go client. HTTP is expected to have many interim responses, depending on the nature of the request. There is no reasonable limit on number of valid interim responses. Setting a time limit overall might make sense for capacity reasons, but this default limit is nuts.

joakime commented 8 months ago

Wow! @royfielding commenting on an HTTP user-agent bug on github. Hard to get more authoritative about the HTTP spec.

neild commented 8 months ago

A configurable limit is too fiddly; we shouldn't require users to twiddle a knob to get reasonable behavior.

We generally consider hostile servers to be less of a concern than hostile clients, but no limit might be a bit much; a server shouldn't be able to send an arbitrary amount of data in response to a request without the client's knowledge.

Perhaps a good compromise might be something along the lines of: No more than N 1xx responses, resetting the counter after a time interval and after every response byte read. (So interleaving an arbitrary number of 1xx responses with response bytes is acceptable, as is sending 1xx responses with no data but below some rate.)

Whatever we do will need to be synchronized between the HTTP/1 and HTTP/2 paths, which both implement the same no-more-than-5 logic at this time.

Acconut commented 8 months ago

Thanks for the comments!

we shouldn't require users to twiddle a knob to get reasonable behavior.

This would be an ideal scenario, but I am not not sure if it's possible in this case.

No more than N 1xx responses, resetting the counter after a time interval

That would be an option. For example, with this approach we could allow 5 responses in 5 seconds?

after every response byte read. (So interleaving an arbitrary number of 1xx responses with response bytes is acceptable, as is sending 1xx responses with no data but below some rate.)

I am not sure this is possible. As far as I know interim responses and the final response cannot be interleaved. When the client starts reading the final response body, no more interim responses will be sent anyway. This should be the case for HTTP/1.1 and also HTTP/2 from reading https://httpwg.org/specs/rfc9113.html#HttpFraming.

a server shouldn't be able to send an arbitrary amount of data in response to a request without the client's knowledge.

Would it be possible to reuse net/http.Transport.MaxResponseHeaderBytes for this purpose? It could be the limit on the size of all response headers combined (i.e. interim response headers plus the final response header). The default value would allow some interim responses to be sent while protecting against too many. While in use cases like mine, we could increase the limit to allow more interim responses.

Right now MaxResponseHeaderBytes is applied on a per-response basis and is reset for every interim or final response: https://github.com/golang/go/blob/704401ffa06c60e059c9e6e4048045b4ff42530a/src/net/http/transport.go#L2358

Whatever we do will need to be synchronized between the HTTP/1 and HTTP/2 paths, which both implement the same no-more-than-5 logic at this time.

Yss, I agree on this.

rsc commented 3 months ago

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group

rsc commented 3 months ago

@neild do you still believe that https://github.com/golang/go/issues/65035#issuecomment-1894439403 is the right answer here?

Do we know what other HTTP client libraries do, especially ones that try to grapple with misbehaving servers, slowloris attacks, and so on?

neild commented 3 months ago

I spent a few minutes attempting to figure out what other clients do. I looked at Python's urllib3 and requests, Rust's hyper and reqwest, and libcurl. I have not managed to figure out from the documentation of any of them how you read 1xx responses, and I haven't tried delving into the code to see if there are any limits.

I'm still not enthused by a configurable knob for number of 1xx responses; you shouldn't need to twiddle settings to get good behavior. As @Acconut points out, 1xx responses can't be interleaved with data (I'd thought this was possible for HTTP/2, but no), so my suggestion above doesn't work, but we could change our rule to no more than N 1xx responses per second.

Another idea might be to permit unlimited 1xx responses, but only if the user is consuming them. Then if the user subscribes to 1xx responses, it's on them to figure out how many are too many.

Right now, the API for consuming 1xx responses is very clunky--you use net/http/httptrace and install a client trace handler for Got1xxResponse events. I've been thinking that we should have a more convenient API; perhaps we should add one (a new http.Request.On1xxResponse field containing a callback, perhaps?) and disable the 1xx limit for anyone using it, rather than changing the httptrace semantics. That's probably a separate proposal, though.

In the short term, I don't see a problem with changing the no-more-than-5 limit to no-more-than-5-per-second. (Or some number >5, it was chosen arbitrarily so far as I can tell.)

royfielding commented 3 months ago

I think you should step back a bit and consider what this limit is attempting to protect against.

We are talking about a self-imposed client limit. Nothing like slowloris (a drip feed attack on servers) applies in this context. The client has already made a request and is awaiting a reply from the server it chose. A nasty server is just as capable of sending a slow 2xx response, one character at a time, if that were ever a thing that needed protecting against. [In general, protecting a client from receiving the responses it requested is counter-productive. A client can terminate a connection at any time, for any reason, and if it doesn't get what it wants the first time it will usually repeat the same request.]

HTTP/1.1 clearly specifies that an expected response is zero or more intermediate 1xx response messages followed by a final 2xx-5xx response. So, what's the problem you are trying to prevent by limiting the number of 1xx responses? Limiting the number of 3xx redirects makes sense because it prevents infinite request loops. What does this limit do for the client?

A client might want a timeout for the initial response, and possibly for completion of the final response, but those are times (independent of the status code). When a request is known by the server to require a long response time, 1xx responses are used to maintain an active connection (across things like NAT) and let the client know that their request hasn't been dropped (yet). IOW, 1xx responses allow the client to implement a more forgiving per-response timeout while waiting for a slow final response.

Long response delays are rare for document retrieval requests, but fairly common for custom APIs (websockets, large queries, image processing, LLMs, etc.). The client has no control over how many 1xx messages will be received, nor their frequency over time.

A client might want to ignore all 1xx responses, because it doesn't have any means to process them, or choose to break on the first one received (because it hates them, for reasons unknown). That would make some sense. But "only process 4 of them" doesn't make any sense, in any context. I've never seen it before, at least not intentionally, and it's been 29 years since I introduced 1xx responses in HTTP/1.1.

neild commented 3 months ago

What should the client do if a server responds with an infinite stream of 1xx responses sent as quickly as possible? Consume them all, forever (or until the request timeout)? We're generally much less concerned about a malicious server's ability to cause resource consumption on the client than we are about the inverse, but that's not the same as not caring at all.

It's quite possible that the current limit on 1xx responses is much too low; I don't believe much thought was put into it. But I don't think a mode where the client silently reads bytes entirely without bound (including potentially expensive HTTP/2 header decoding) is right either.

I just tested curl on a server responding with an infinite stream of 102s, and got:

curl: (56) Too large response headers: 307204 > 307200

It looks like curl sets a limit in terms of total header bytes. Perhaps that's an option for us; we could drop the number-of-responses limit in favor of counting all response headers against Transport.MaxResponseHeaderBytes. This would effectively increase the 1xx limit significantly in the default case.

LPardue commented 3 months ago

Constraining on size of headers reminded me of the recent H2 CONTINUATION flood attack that only affected some implementations that didn't have such counting protections.

The thing to watch out for is that these are independent messages, and that you could could a long chain of 1xx and then blow the limit when you receive a final response that is more likely to contain more or larger headers.

royfielding commented 3 months ago

What should the client do if a server responds with an infinite stream of 1xx responses sent as quickly as possible? Consume them all, forever (or until the request timeout)? We're generally much less concerned about a malicious server's ability to cause resource consumption on the client than we are about the inverse, but that's not the same as not caring at all.

The reason we generally do not care about client resource consumption at the protocol level, at all, is because client/server protocols are intentionally 1:N related. Meaning, clients are designed for making a few parallel requests of their own choice while receiving a few parallel responses, whereas servers are designed for tens of thousands of parallel requests from an entire world of independent clients that they cannot control. They have totally different concerns with regard to performance and rate control. Where we do care about client resource consumption is at the next level up, mostly in regard to malicious redirects, evil content, privacy, or subsequent process control.

A user agent (specifically, the client code that actually knows what it is trying to do, not the client library that only knows HTTP) can stop whenever it likes based on the messages it receives. It might want to limit such numbers for its own sake if it knows a request is not supposed to be long-lived. Otherwise, UAs usually just rely on a timeout, or on a human selecting the cancel button.

1xx responses are not an attack vector because they have no content to execute. They supply information that may be useful to the client, because the immediate hop server wants that information to be sent, and are otherwise ignored. A server that wants to attack a client, for any reason, would send a single 2xx-5xx response containing evil content (code injection or NSFW stuff) or multiple 2xx responses in the hope of cache poisoning a pipelined GET.

I just tested curl on a server responding with an infinite stream of 102s, and got:

curl: (56) Too large response headers: 307204 > 307200

It looks like curl sets a limit in terms of total header bytes. Perhaps that's an option for us; we could drop the number-of-responses limit in favor of counting all response headers against Transport.MaxResponseHeaderBytes. This would effectively increase the 1xx limit significantly in the default case.

That sounds more like a bug in curl than a feature, like a per-response counter that isn't being reset, though I seriously doubt any non-test code would trigger it in real life. IOW, it's better than a limit on number of 1xx responses, since they rarely contain large header fields.

neild commented 3 months ago

Meaning, clients are designed for making a few parallel requests of their own choice while receiving a few parallel responses, whereas servers are designed for tens of thousands of parallel requests from an entire world of independent clients that they cannot control.

This is perhaps true if we limit ourselves to talking about web browsers. It's much less true if talking about a forwarding proxy, or a spider, where the client may make many requests in parallel, potentially to untrusted, malicious destinations.

1xx responses are not an attack vector because they have no content to execute.

I must respectfully disagree. We consider unbounded, invisible resource consumption to be an attack vector. (Although in this case, even absent a limit on the number of responses, we'd consider it a minor vector since client resource consumption is generally less exciting than server, and since the number of bytes consumed by the client is 1-1 with the number sent by the server. However in the HTTP/2 path, absent a limit, the server could cause an disproportionate amount of CPU consumption on the client in header decoding.)

(This isn't a defense of the specific current limit in net/http. I think there's general consensus that it's much too restrictive at the moment.)

rsc commented 1 month ago

@neild Do you have a concrete proposal for new API here?

neild commented 1 month ago

To summarize:

The net/http HTTP/1 and HTTP/2 clients currently limit the number of 1xx responses to 5. (This is implemented separately for each protocol, but the limit is the same for each.)

The limit of 5 is too small. There are legitimate reasons to return more responses. There is no natural maximum bound on the number of 1xx responses.

The client needs to limit the amount of 1xx responses that it will read in some way. A request to a malicious or misbehaving server that returns an unbounded sequence of 1xx responses should not continue to read forever.

The proposal in this issue is to keep a limit on the number of 1xx responses, but to add a knob allowing users to configure it. In general, we try to avoid adding configuration options, because each new option is a bit more cognitive load for users to deal with. I'd prefer to avoid a new configuration option here, especially since it isn't even clear that limiting the number of 1xx responses is the right way to defend against server misbehavior.

I propose that instead we drop the limit on the number of 1xx responses and instead use the existing Transport.MaxResponseHeaderBytes configuration option to limit the amount of 1xx response headers we are willing to read. Instead of tracking the number of 1xx responses, we will track the total size of response headers read and abort if it goes over the MaxResponseHeaderBytes limit. In the common case with the MaxResponseHeaderBytes default of 1MiB, this will substantially increase the number of 1xx responses we're willing to read.

This seems to match curl's behavior, and seems fairly intuitive to me. This change requires no API changes.

As a related issue, reading the content 1xx responses is currently fairly cumbersome: The only way to do it right now is using the httptrace package and installing a ClientTrace.Got1xxResponse hook. I think it would make sense to add support for receiving 1xx responses directly to net/http, perhaps along the lines of:

type Request struct {
  // On1xxResponse is called for each 1xx informational response
  // returned before the final non-1xx response.
  // If it returns an error, the request is aborted with that error.
  //
  // On1xxResponse is never called after RoundTrip returns.
  // It is not used for server requests.
  //
  // If On1xxResponse is not set, the total bytes for all response headers combined must be
  // lower than Transport.MaxResponseHeaderBytes.
  //
  // If On1xxResponse is set, the bytes for each individual response must be
  // lower than Transport.MaxResponseHeaderBytes, but there is no limit
  // on the combined total header size or the number of 1xx headers.
  On1xxResponse func(code Status, header http.Header) error
}

This would both provide an easy way to receive 1xx responses, and put control of the number of responses under user control. This would also address the issue raised in #65123 of ClientTrace.Got1xxResponse calls happening after RoundTrip returns.

I think we should make the change now to limit 1xx responses by total header size rather than number of responses. I'm less sure about Request.On1xxResponse; I'd really want to try implementing it before saying that it's the right API.

rsc commented 1 month ago

Adding a callback in Request would be almost the first callback, and certainly the first that's about the Response. That seems odd. Is there some other place it can go?

(The only other callback is GetBody, but that's about defining the request body, not processing the response.)

rsc commented 1 month ago

Perhaps by analogy with the redirect following, the 1xx callback should be in http.Client?

neild commented 1 month ago

If we put the callback in http.Client, we'd still need some way to plumb it back from the transport to the client. Since Client just wraps a RoundTripper, we'd still need some way to add the callback to the RoundTrip call.

Options I can see:

I'm not really thrilled by any of the options. Assuming we did want to add a callback somewhere, I can't think of anything better than a field on Request; it requires the smallest amount of new API. Perhaps someone else has a better idea.

ianlancetaylor commented 1 month ago

I'm not expert, but from a user point of view adding something to Client seems more natural than Request. If I want to do something special with a 1xx response, that seems like something at the client level rather than the individual request level.

If we do that, it seems to me that we could then add the callback to Request as an unexported field. The Client would take care of setting the unexported field for us.

Might still be too baroque, I admit--but wait, this is the net/http package, we've got plenty of baroque.

neild commented 1 month ago

I don't think it makes sense to add something user-visible to Client and not Transport.

This would be the first case (I think) of passing private data from the Client to the Transport. Currently, a Client operates on a RoundTripper, and anyone can implement RoundTripper. Adding a private backchannel would mean that only our own RoundTripper implementation can use this feature.

This actually would be a problem for our own implementations: The x/net/http2.Transport doesn't have access to unexported RoundTripper fields, and would have no way to call the 1xx hook. There's probably a way around this, but probably not a clean one.

Acconut commented 3 weeks ago

I'm not expert, but from a user point of view adding something to Client seems more natural than Request. If I want to do something special with a 1xx response, that seems like something at the client level rather than the individual request level.

I would disagree. For me the handling of a 1xx response fits into Request since a 1xx response belongs to one specific request only. A client may only be interested in receiving 1xx responses for a subset of all requests it sends and not all requests. Enabling the callback for individual requests is already possible with the current httptrace package and it would be helpful to keep this functionality. Personally, I like @neild's proposal of On1xxResponse, but I understand if this doesn't fit the principles of the http package.

rsc commented 3 weeks ago

@neild How would having On1xx in the http.Client be different from CheckRedirect in http.Client? It seems like the same kind of callback to me?

neild commented 3 weeks ago

http.Client implements the redirect behavior, so CheckRedirect is a Client field.

An http.RoundTripper is something that executes a single HTTP transaction--one request, one response.

An http.Transport is a RoundTripper implementation. A Transport implements HTTP and contains a connection pool.

An http.Client wraps a RoundTripper and adds support for cookies and redirects.

The only interaction between a Client and a Transport today is through the RoundTripper interface, which includes only a single method taking a Request and returning a (Response, error). A Client usually directly wraps an http.Transport, but it doesn't necessarily have to--users may add middleware layers wrapping the Transport, or use a Client backed by a golang.org/x/net/http2.Transport, or an HTTP/3 implementation. So we can't assume that we know the type of a Client's RoundTripper.

So if we add Client.On1xx, we have the question of how to plumb that through to the Transport, or how to plumb the 1xx responses back from the Transport to the client, and I don't see any great options.

Stepping back, however, I think that 1xx response hooks should be request-scoped rather than client- or transport-scoped. One of the motivating examples for this whole discussion is an upload progress report, in which a server sends periodic 104 responses as it reads data from a client. A client-side implementation of this protocol will want to associate those responses with a specific request.

rsc commented 2 weeks ago

Talking to @bradfitz, it does seem like we started down a path with ClientTrace (specifically, WithClientTrace) and should probably continue down that path (in fact it's already there as Got1xxResponse)? That would avoid making Request one word bigger for everyone even though no one uses it, and it would keep the many per-request hooks in the same place (ClientTrace).

It seems like we all agree about dropping the "five 1xx" limit and instead count those toward the header limit. Then at least we have only one limit instead of two. And then the very few people who want not just to not die but actually see the 1xx responses can use ClientTrace.

Thoughts?

Acconut commented 1 week ago

Continuing using ClientTrace is also a good option IMO.

rsc commented 1 week ago

Great. Based on the discussion then, it sounds like there is no new API and the only visible change is to stop doing the "no more than 5 1xx responses" - they are now unlimited in count - and instead subtract 1xx response sizes from the total headers allowed, so that if there are enough to use up all the header bytes, then the connection is killed. Do I have that right?

Acconut commented 2 days ago

subtract 1xx response sizes from the total headers allowed, so that if there are enough to use up all the header bytes, then the connection is killed.

With this approach, a user is only able to receive an unlimited amount of 1xx responses if they disable the overall header limit or set it to an incredibly high value. However, then there is also no effective limit on the size of an individual response. It would be useful if one avoid any limit on the number of 1xx responses and thereby shifting the responsibility of ensuring that the 1xx response don't overwhelm the application to the user.

Would it be possible to have a size limit for each individual response header (final or 1xx) and a combined limit for all response headers together? If a user wants to have no limit of 1xx responses from the HTTP client, they could disable the combined limit while keeping the individual limit intact. By doing this, the user also agrees to take care of mitigating any risk that is associated with a potential flood of 1xx responses.

aclements commented 1 day ago

@bradfitz suggests that apply the size of 1xx responses toward the header limit (as suggested in https://github.com/golang/go/issues/65035#issuecomment-2344257902) unless the user has set a Got1xxResponse hook set, in which case implementing any sort of limit is up to them. Does that seem reasonable?