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

hyper: does not request connection upgrade via headers when --http2 flag is set (test 1800) #8605

Closed
ndd7xv opened this issue Mar 18, 2022 · 9 comments

Comments

@ndd7xv
Copy link

ndd7xv commented Mar 18, 2022

I did this

I ran runtests.pl 1800 -f (HTTP/2 upgrade refused) when curl is built with hyper, though this error is reproducible whenever hyper is the backend, --http2 is set, and curl tries to connect with http (so, this can be seen with curl -v --http2 http://google.com).

https attempts to negotiate in the TLS handshake (by default), and behaves correctly.

I expected the following

As per the expected test output, curl should attempt to negotiate HTTP/2 and the following headers should appear in the request:

Connection: Upgrade, HTTP2-Settings
Upgrade: h2c
HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA

I'm not 100% sure on the fix, but I'm guessing that Curl_http in c-hyper.c should check for --http2 and by some means add the corresponding headers to the request.

curl/libcurl version

curl 7.83.0-DEV (x86_64-pc-linux-gnu) libcurl/7.83.0-DEV OpenSSL/1.1.1l-fips zlib/1.2.11 zstd/1.5.2 Hyper/0.14.17
Release-Date: [unreleased]
Protocols: dict file ftp ftps gopher gophers http https imap imaps mqtt pop3 pop3s smb smbs smtp smtps telnet tftp 
Features: alt-svc AsynchDNS Debug HSTS HTTP2 HTTPS-proxy IPv6 Largefile libz NTLM NTLM_WB SSL TLS-SRP TrackMemory UnixSockets zstd

operating system

Linux ndd7xv 5.16.12-200.fc35.x86_64 #1 SMP PREEMPT Wed Mar 2 19:06:17 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

@ndd7xv
Copy link
Author

ndd7xv commented Mar 18, 2022

I wrote this issue following this article by seanmonstar and in my infinite wisdom (/s) think I misinterpreted what he meant by

We’re so close to having it work! With hundreds of tests working, there’s only a dozen or so tests left to fix. I’ve inlined the test number and brief description here, but the actual file will be more up-to-date.

I see that 1800 was not included in this list... and what was meant is probably that in the future there will be even less on that list. I'll try and get around to actually helping soon, but I apologize in advance if this problem was already known!

@bagder
Copy link
Member

bagder commented Mar 18, 2022

I think Sean's list was cut off early? The full list I made is in the hyper wiki page and does include 1800 (and the file DISABLED in git has it disabled, which gives it away too). I think your conclusion about what's lacking there is correct and matches my thinking.

@bagder
Copy link
Member

bagder commented Mar 18, 2022

I presume hyper has some way to help us get the HTTP2-Settings: header made?

@ndd7xv
Copy link
Author

ndd7xv commented Mar 19, 2022

@seanmonstar can correct me if I'm wrong, but I'm assuming that the fact that 1800 wasn't on the list in his article indicates that they already know the root cause of this problem. I think (but am not 100% sure) that there isn't currently logic to create the HTTP2-Settings header from hyper (should I make an issue for hyper?).

That being said, if there is a way to get the HTTP2-Settings with hyper (or when there is a way), perhaps we could add something like the following after all of the current header logic in Curl_http (forgive me for my lack of knowledge in C):

if(!h2 && (data->state.httpwant == CURL_HTTP_VERSION_2_0)) {
  result = Curl_hyper_header(data, headers,
                             "Connection: Upgrade, HTTP2-Settings");
  if(result)
    goto error;

  result = Curl_hyper_header(data, headers, "Upgrade: h2c");
  if(result)
    goto error;

  /* Eventual callback to some HTTP2-Settings Header Generation? */
  if(result)
    goto error;
}

@seanmonstar
Copy link
Contributor

I didn't include it in the list because this one we do know the root cause of: hyper doesn't have http1-to-http2 upgrade support. Specifically, it's h2 internal dependency doesn't know how to start a stream with just the response side being http2.

@bagder
Copy link
Member

bagder commented Mar 19, 2022

I didn't know that...

@bagder
Copy link
Member

bagder commented Mar 19, 2022

Steps forward for this are likely:

  1. document this as another known restriction when using hyper for HTTP.
  2. libcurl needs to return a suitable error for h2 enabled when using plain http with hyper
  3. test case 1800 (and some of the 17xx ones ?) need to be disabled in the test case itself. We should probably make them require feature h2c and make runtests.pl set the feature depending on what backend it detects is in use

@ndd7xv
Copy link
Author

ndd7xv commented Mar 19, 2022

Would this also pose a problem for tests 358 and 359 (which are on the list of unknown root causes)? I'm not sure if it's necessarily the root cause (I don't know too much about alternative services/if & how hyper deals with them), but both tests have commands that set the --http2 option and expect output that indicates a protocol switch with h2c, which hyper doesn't support.

@bagder
Copy link
Member

bagder commented Mar 19, 2022

Would this also pose a problem for tests 358 and 359

Yes. Even if those tests are strictly speaking for testing alt-svc, they do check for an eventual upgrade to h2 with --http2 and thus this won't work with hyper as it works now. It is unfortunate to disable the alt-svc tests for hyper builds, but it will take a larger fix to work around that test setup - like by providing a "real" h2 HTTPS server to the curl test quite that would agree to h2 with alpn. We currently use stunnel for all the HTTPS'ing there and it doesn't do alpn.

bagder added a commit that referenced this issue Mar 19, 2022
Make tests require h2c feature present to run, and only set h2c if
nghttp2 is used in the build. Hyper does not support it.

Remove those tests from DISABLED

Ref: #8605
@bagder bagder closed this as completed in 7e145dd Mar 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants