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

FIX HTTP/2 disabled support not working as intended #697

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

alexvec
Copy link

@alexvec alexvec commented Jun 25, 2023

Description

The parameter -usehttp2 is disabled by default, however the ffuf client doesn't actually disable HTTP/2 requests when that parameter is set to false.

As it is officially documented

Starting with Go 1.6, the http package has transparent support for the HTTP/2 protocol when using HTTPS. Programs that must disable HTTP/2 can do so by setting Transport.TLSNextProto (for clients) or Server.TLSNextProto (for servers) to a non-nil, empty map.

However, this isn't being done in the runner client,

ForceAttemptHTTP2: conf.Http2,

Not forcing HTTP/2 is not the same as disabling HTTP/2.

I have modified the client code to disable HTTP/2 requests completely if the parameter -usehttp2 , by setting an empty map under TLSNextProto:
transport.TLSNextProto = map[string]func(string, *tls.Conn) http.RoundTripper{}

@alexvec alexvec changed the title Disable HTTP/2 support when not enabled FIX Disable HTTP/2 support when not enabled Jun 25, 2023
@alexvec alexvec changed the title FIX Disable HTTP/2 support when not enabled FIX HTTP/2 disabled support not working as intended Jun 25, 2023
@joohoi
Copy link
Member

joohoi commented Jul 3, 2023

Thanks for the PR!

I think we should maintain sensible defaults, and to me opportunistic use of HTTP/2 would be a sensible default. I would love to see the behavior being:

  • Default: opportunistic use of HTTP/2
  • A flag to force HTTP/2 (I would assume rare, but still realistic use case), like -usehttp2 is atm
  • A flag to disable HTTP/2, something like -nohttp2

What do you think? And would you be up to making the change?

@alexvec
Copy link
Author

alexvec commented Jul 3, 2023

Sure, I like the idea. I will make the changes and update it.

- Introduces opportunistic behaviour for HTTP/2 under default behaviours.
- Adds a new flag, nohttp2, which will effectively disable HTTP/2 from being used in requests. By default this is set to false.
@alexvec
Copy link
Author

alexvec commented Oct 18, 2023

Hi, sorry for the delay.

I have a proposed idea which might make for a better implementation:

As it currently stands, http2 flag will enable HTTP/2 to make opportunistic use of HTTP/2. At the moment, by default that flag is set to false, which means that HTTP/2 is currently not being used for any request under the default behaviour.

There are several options:

  1. Introduce default behaviour to use HTTP/2 opportunistically, if the server provides support for it, otherwise it would fallback to HTTP/1. Therefore, we can remove http2 flag, add a new flag called nohttp2 which is set to false by default. When said flag is set to true will mean all requests are never going to be handled by HTTP/2, rather by HTTP/1. (This is in fact, what currently happens when running ffuf under the default settings.
  2. Leave default behaviour as is, HTTP/2 will not be used, unless http2 flag is set. (At the moment, this works, however official Go documentation also mentions to set TLSNextProto to a non-nil, empty map, which is a small change that can be introduced).
  3. Do option 1, and moreover, introduce a flag forcehttp2 which forces exclusive use of HTTP/2 requests, or else errors.

For the time being, I consider option 1 to be the simplest, and I think enabling HTTP/2 opportunistically as a default behaviour is a good idea. I have pushed changes to introduce option 1. Nevertheless, please let me know what you think, and we can change things over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants