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.

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

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

Closed
Acconut opened this issue Jan 9, 2024 · 42 comments
Closed

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

Acconut opened this issue Jan 9, 2024 · 42 comments

Comments

@Acconut
Copy link

Acconut commented Jan 9, 2024

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.

go/src/net/http/transport.go

Lines 2328 to 2329 in 8db1310

num1xx := 0 // number of informational 1xx headers received
const max1xxResponses = 5 // arbitrary bound on number of informational responses

go/src/net/http/transport.go

Lines 2354 to 2357 in 8db1310

num1xx++
if num1xx > max1xxResponses {
return nil, errors.New("net/http: too many 1xx informational responses")
}

The code comments and the original commit (d88b137) 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
Copy link
Member

cc @neild

@royfielding
Copy link

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
Copy link

joakime commented Jan 16, 2024

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

@neild
Copy link
Contributor

neild commented Jan 16, 2024

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
Copy link
Author

Acconut commented Jan 23, 2024

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:

pc.readLimit = pc.maxHeaderResponseSize() // reset the limit

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.

@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Feb 6, 2024
@rsc rsc changed the title proposal: net/http: Customize limit on number of 1xx responses proposal: net/http: customize limit on number of 1xx responses Jun 12, 2024
@rsc
Copy link
Contributor

rsc commented Jun 12, 2024

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 rsc moved this from Incoming to Active in Proposals Jun 12, 2024
@rsc
Copy link
Contributor

rsc commented Jun 20, 2024

@neild do you still believe that #65035 (comment) 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
Copy link
Contributor

neild commented Jun 20, 2024

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
Copy link

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
Copy link
Contributor

neild commented Jun 20, 2024

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
Copy link

LPardue commented Jun 20, 2024

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
Copy link

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
Copy link
Contributor

neild commented Jun 21, 2024

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
Copy link
Contributor

rsc commented Jul 24, 2024

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

@neild
Copy link
Contributor

neild commented Aug 14, 2024

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
Copy link
Contributor

rsc commented Aug 14, 2024

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
Copy link
Contributor

rsc commented Aug 14, 2024

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

@neild
Copy link
Contributor

neild commented Aug 14, 2024

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:

  • Pass the callback in as a request context value. This is what we do with ClientTrace. This is still putting the callback in the Request, just with more layers of indirection, no type checking, and an inefficient lookup.

  • Put the callback on the Transport. Now we've got one callback for every request, and the user needs to map callback invocations back to the corresponding request. Ugly.

  • Add a new extended RoundTrip method, something along the lines of:

    type RoundTripOptser interface {
      RoundTripOpts(*Request, RoundTripOptions) (*Response, error)
    }
    type RoundTripOptions struct {
     On1xxResponse func(code int, header Header) error
    }
    

    Seems like a big step to take, but maybe something we should consider doing if there are other changes we'd like to make to RoundTrip?

  • Add the callback to Request as an exported field. Odd, as you point out.

  • Add the callback to Request as an unexported field. Add a function like NewRequestWithContext that sets the field. Doesn't really seem like an improvement over an exported field to me; more mechanism, no real difference.

  • Don't add the callback at all. Users can keep using httptrace, and maybe we can improve the ergonomics by guaranteeing that the Got1xxResponse trace hook is never called after RoundTrip has returned. We remember to revisit this if we ever do a net/http/v2.

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
Copy link
Contributor

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
Copy link
Contributor

neild commented Aug 15, 2024

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
Copy link
Author

Acconut commented Aug 23, 2024

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
Copy link
Contributor

rsc commented Aug 28, 2024

@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
Copy link
Contributor

neild commented Aug 28, 2024

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
Copy link
Contributor

rsc commented Sep 11, 2024

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
Copy link
Author

Acconut commented Sep 17, 2024

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
Copy link
Member

@bradfitz suggests that apply the size of 1xx responses toward the header limit (as suggested in #65035 (comment)) 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?

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/615255 mentions this issue: net/http: limit 1xx based on size, do not limit when delivered

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/615295 mentions this issue: http2: limit 1xx based on size, do not limit when delivered

@neild
Copy link
Contributor

neild commented Sep 23, 2024

Does that seem reasonable?

Dropping the limit when a Got1xxResponse hook is present seems reasonable.

@aclements
Copy link
Member

I believe the current proposal is for net/http to apply 1xx responses toward the overall header limit of Transport.MaxResponseHeaderBytes unless a Got1xxResponse hook is set. This isn't strictly an API change, but this behavior should be documented (the current behavior, I'll note, is not documented).

@aclements
Copy link
Member

Have all remaining concerns about this proposal been addressed?

net/http will to apply 1xx responses toward the overall header limit of Transport.MaxResponseHeaderBytes unless a Got1xxResponse hook is set.

@Acconut
Copy link
Author

Acconut commented Sep 26, 2024

net/http will to apply 1xx responses toward the overall header limit of Transport.MaxResponseHeaderBytes unless a Got1xxResponse hook is set.

I agree that this is helpful.

One question is remaining for me: If Got1xxResponse is set, can the user effectively limit the size of an individual 1xx response? If a rouge server is sending a never ending 1xx response, the user's Got1xxResponse is never called because the 1xx response is not completed yet. At the same time, buffers are filling up storing the received 1xx headers. This only ends once a request context is cancelled. Having two separate limits for individual response size and overall response size could help here. Judging from #65035 (comment), protecting clients from such servers does concern the Go team.

@aclements
Copy link
Member

@Acconut , that's an interesting point. Perhaps if Got1xxResponse is set, Transport.MaxResponseHeaderBytes should still apply to an individual response, just not the cumulative responses? In other words, if Got1xxResponse, net/http would still track and limit the bytes in the response, it would just reset that count at the end of the response.

@Acconut
Copy link
Author

Acconut commented Oct 2, 2024

Perhaps if Got1xxResponse is set, Transport.MaxResponseHeaderBytes should still apply to an individual response, just not the cumulative responses?

Yes, that should work as well 👍

@neild
Copy link
Contributor

neild commented Oct 2, 2024

Yes, MaxResponseHeaderBytes always limits the size of any individual response (1xx or otherwise). When no Got1xxResponse hook is set we'll apply that limit to the sum of all headers across all responses to a request, and when a hook is set we will apply it only to each individual response.

@aclements aclements moved this from Active to Likely Accept in Proposals Oct 4, 2024
@aclements
Copy link
Member

Based on the discussion above, this proposal seems like a likely accept.

net/http will to apply 1xx responses toward the overall header limit of Transport.MaxResponseHeaderBytes unless a Got1xxResponse hook is set. Transport.MaxResponseHeaderBytes will continue to apply to individual 1xx responses even if Got1xxResponse is set.

gopherbot pushed a commit to golang/net that referenced this issue Oct 21, 2024
Replace Transport's limit of 5 1xx responses with a limit based
on the maximum header size: The total size of all 1xx response
headers must not exceed the limit we use on the size of the
final response headers.

(This differs slightly from the corresponding HTTP/1 change,
which imposes a limit on all 1xx response headers *plus* the
final response headers. The difference isn't substantial,
and this implementation fits better with the HTTP/2 framer.)

When the user is reading 1xx responses using a Got1xxResponse
client trace hook, disable the limit: Each 1xx response is
individually limited by the header size limit, but there
is no limit on the total number of responses. The user is
responsible for imposing a limit if they want one.

For golang/go#65035

Change-Id: I9c19dbf068e0f580789d952f63113b3d21ad86fc
Reviewed-on: https://go-review.googlesource.com/c/net/+/615295
Reviewed-by: Cherry Mui <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Damien Neil <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>
@aclements
Copy link
Member

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.

net/http will to apply 1xx responses toward the overall header limit of Transport.MaxResponseHeaderBytes unless a Got1xxResponse hook is set. Transport.MaxResponseHeaderBytes will continue to apply to individual 1xx responses even if Got1xxResponse is set.

@aclements aclements moved this from Likely Accept to Accepted in Proposals Oct 23, 2024
@aclements aclements changed the title proposal: net/http: customize limit on number of 1xx responses net/http: customize limit on number of 1xx responses Oct 23, 2024
@aclements aclements modified the milestones: Proposal, Backlog Oct 23, 2024
gopherbot pushed a commit that referenced this issue Oct 24, 2024
Replace Transport's limit of 5 1xx responses with a limit based
on MaxResponseHeaderBytes: The total number of responses
(including 1xx reponses and the final response) must not exceed
this value.

When the user is reading 1xx responses using a Got1xxResponse
client trace hook, disable the limit: Each 1xx response is
individually limited by MaxResponseHeaderBytes, but there
is no limit on the total number of responses. The user is
responsible for imposing a limit if they want one.

For #65035

Change-Id: If4bbbbb0b808cb5016701d50963c89f0ce1229f8
Reviewed-on: https://go-review.googlesource.com/c/go/+/615255
Reviewed-by: David Chase <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/622335 mentions this issue: doc: document new http.Transport limit on 1xx responses

@Acconut
Copy link
Author

Acconut commented Oct 28, 2024

Thank you for such a quick implementation! For reference, the changes seem to be (since I went looking for them):

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/625195 mentions this issue: net/http: handle new HTTP/2 error for 1xx limit exceeded

gopherbot pushed a commit that referenced this issue Nov 6, 2024
CL 615295 changed the error message produced by the HTTP/2
implementation when a server sends more 1xx headers than expected.
Update a test that checks for this error.

For #65035

Change-Id: I57e22f6a880412e3a448e58693127540806d5ddb
Reviewed-on: https://go-review.googlesource.com/c/go/+/625195
Reviewed-by: David Chase <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests

10 participants