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

HTTP/2 (h2c) upgrade in clear-text problem #7633

Closed
tk-xiong opened this issue Aug 26, 2021 · 24 comments
Closed

HTTP/2 (h2c) upgrade in clear-text problem #7633

tk-xiong opened this issue Aug 26, 2021 · 24 comments
Labels

Comments

@tk-xiong
Copy link

@tk-xiong tk-xiong commented Aug 26, 2021

I did this

I use curl and Golang HttpClient to call my service.
Golang's HttpClient is working fine.

curl:
This does not work if --http2 is used.
This works if use --http2-prior-knowledge.

I expected the following

both use --http2 and --http2-prior-knowledge working fine.

curl/libcurl version

Windows:
curl 7.78.0 (x86_64-pc-win32) libcurl/7.78.0 OpenSSL/1.1.1l (Schannel) zlib/1.2.11 brotli/1.0.9 zstd/1.5.0 libidn2/2.3.2 libssh2/1.9.0 nghttp2/1.44.0 libgsasl/1.10.0
Release-Date: 2021-07-21
Protocols: dict file ftp ftps gopher gophers http https imap imaps ldap ldaps mqtt pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp
Features: alt-svc AsynchDNS brotli gsasl HSTS HTTP2 HTTPS-proxy IDN IPv6 Kerberos Largefile libz MultiSSL NTLM SPNEGO SSL SSPI TLS-SRP UnixSockets zstd

Linux:
curl 7.78.0 (x86_64-pc-linux-gnu) libcurl/7.78.0 zlib/1.2.7 nghttp2/1.44.0
Release-Date: 2021-07-21
Protocols: dict file ftp gopher http imap mqtt pop3 rtsp smtp telnet tftp
Features: alt-svc AsynchDNS HTTP2 IPv6 Largefile libz UnixSockets

[curl -V output]

*   Trying 192.168.0.11:10001...
* Connected to 192.168.0.11 (192.168.0.11) port 10001 (#0)
> Get /hello/ HTTP/1.1
> Host: 192.168.0.11:10001
> User-Agent: curl/7.78.0
> Accept: */*
> Connection: Upgrade, HTTP2-Settings
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 101 Switching Protocols
HTTP/1.1 101 Switching Protocols
< Connection: Upgrade
Connection: Upgrade
< Upgrade: h2c
Upgrade: h2c
* Received 101
* Using HTTP2, server supports multiplexing
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0

operating system

Linux 3.10.0-1160.15.2.el7.x86_64 #1 SMP Wed Feb 3 15:06:38 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

@tk-xiong
Copy link
Author

@tk-xiong tk-xiong commented Aug 26, 2021

Upon receiving the 101 response, the client MUST send a connection
preface (Section 3.5), which includes a SETTINGS frame.

@tk-xiong
Copy link
Author

@tk-xiong tk-xiong commented Aug 26, 2021

Use Wireshark,i can't find connection preface
image

@tk-xiong
Copy link
Author

@tk-xiong tk-xiong commented Aug 26, 2021

and read golang src, golang.org/x/net/http2/h2c/h2c.go line 192.

	// A conforming client will now send an H2 client preface which need to drain
	// since we already sent this.
	if err := drainClientPreface(rw); err != nil {
		return nil, err
	}

@bagder bagder added the HTTP/2 label Aug 26, 2021
@bagder
Copy link
Member

@bagder bagder commented Aug 26, 2021

$ curl --http2 localhost -v
*   Trying ::1:80...
* Connected to localhost (::1) port 80 (#0)
> GET / HTTP/1.1
> Host: localhost
> User-Agent: curl/7.74.0
> Accept: */*
> Connection: Upgrade, HTTP2-Settings
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 101 Switching Protocols
< Upgrade: h2c
< Connection: Upgrade
* Received 101
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Connection state changed (MAX_CONCURRENT_STREAMS == 100)!
< HTTP/2 200 
< date: Thu, 01 Jan 1970 00:00:00 GMT
< server: Apache/2.4.48 (Debian)
< last-modified: Sat, 31 May 2014 22:05:33 GMT
< etag: W/"d-4fab95c704b1c"
< accept-ranges: bytes
< content-length: 13
< content-type: text/html
< 
inken tinken
* Connection #0 to host localhost left intact

@tk-xiong
Copy link
Author

@tk-xiong tk-xiong commented Aug 26, 2021

Can you use some tools to capture the package into file and show it to me.

i use wireshark on windows and not find client send this -> " PRI *HTTP/2.0\r\n\r\nSM\r\n\r\n"

maybe nghttp2 is not Compatible with golang server.

@tk-xiong
Copy link
Author

@tk-xiong tk-xiong commented Aug 26, 2021

Please teach me whether the client should send a Connection preface message after receiving the Upgrade message.

i think client need, and read golang server, it also drainClientPreface.

but i use wireshark, i can't find package Client Preface(this -> " PRI *HTTP/2.0\r\n\r\nSM\r\n\r\n").

@tk-xiong
Copy link
Author

@tk-xiong tk-xiong commented Aug 26, 2021

Yes! I Found!
If annotation this code, it run:

golang.org/x/net/http2/h2c/h2c.go line 192 to line 196.

// A conforming client will now send an H2 client preface which need to drain
// since we already sent this.
if err := drainClientPreface(rw); err != nil {
return nil, err
}

@tk-xiong
Copy link
Author

@tk-xiong tk-xiong commented Aug 26, 2021

But I don't think it was Golang's fault.

In RFC7540 (Section 3.2):
Upon receiving the 101 response, the client MUST send a connection
preface (Section 3.5), which includes a SETTINGS frame.

I think Curl or NGHTTP2 does not follow the RFC specification.

@bagder
Copy link
Member

@bagder bagder commented Aug 26, 2021

I've failed to reproduce this problem myself, so can I ask you to provide us with more details on how to go ahead and repeat this. Preferably with a command line or a stand-alone program we can run from our ends against a public URL to trigger the problem?

@tk-xiong
Copy link
Author

@tk-xiong tk-xiong commented Aug 26, 2021

I Send e-mail to you. Please do not disclose this IP address, thank you.

@bagder
Copy link
Member

@bagder bagder commented Aug 26, 2021

You then assume that nobody else than me wants and can debug or help out here?

@tk-xiong
Copy link
Author

@tk-xiong tk-xiong commented Aug 26, 2021

I'm Sorry,My server is not secure, so I dare not expose the public network.
please try this url:
curl -v --http2 http://121.37.5.232:10000/hello/

@bagder
Copy link
Member

@bagder bagder commented Aug 26, 2021

Reverting 455a63c fixes this problem for me. Presumably it would however reopen #7036...

@starrify, any ideas?

@tk-xiong
Copy link
Author

@tk-xiong tk-xiong commented Aug 27, 2021

I try revert 455a63c
Maybe I think too little, I can't understand #7036

$ ./curl -vI --http2 http://192.168.0.11:10001/hello/
*   Trying 192.168.0.11:10001...
* Connected to 192.168.0.11 (192.168.0.11) port 10001 (#0)
> HEAD /hello/ HTTP/1.1
> Host: 192.168.0.11:10001
> User-Agent: curl/7.78.0
> Accept: */*
> Connection: Upgrade, HTTP2-Settings
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 101 Switching Protocols
HTTP/1.1 101 Switching Protocols
< Connection: Upgrade
Connection: Upgrade
< Upgrade: h2c
Upgrade: h2c
* Received 101
* Using HTTP2, server supports multiplexing
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0

* Connection state changed (MAX_CONCURRENT_STREAMS == 250)!
* HTTP/2 stream 0 was not closed cleanly: PROTOCOL_ERROR (err 1)
* stopped the pause stream!
* Connection #0 to host 192.168.0.11 left intact
curl: (92) HTTP/2 stream 0 was not closed cleanly: PROTOCOL_ERROR (err 1)

@bagder
Copy link
Member

@bagder bagder commented Aug 30, 2021

I can't repro that error. With the reverted commit I get this (debug build):

$ ./src/curl -v --http2 http://121.37.5.232:10000/hello/
* STATE: INIT => CONNECT handle 0x55e7b84e20a8; line 1780 (connection #-5000)
* Added connection 0. The cache now contains 1 members
* family0 == v4, family1 == v6
*   Trying 121.37.5.232:10000...
* STATE: CONNECT => CONNECTING handle 0x55e7b84e20a8; line 1841 (connection #0)
* Connected to 121.37.5.232 (121.37.5.232) port 10000 (#0)
* STATE: CONNECTING => PROTOCONNECT handle 0x55e7b84e20a8; line 1971 (connection #0)
* STATE: PROTOCONNECT => DO handle 0x55e7b84e20a8; line 1994 (connection #0)
> GET /hello/ HTTP/1.1
> Host: 121.37.5.232:10000
> User-Agent: curl/7.79.0-DEV
> Accept: */*
> Connection: Upgrade, HTTP2-Settings
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA
> 
* STATE: DO => DID handle 0x55e7b84e20a8; line 2068 (connection #0)
* STATE: DID => PERFORMING handle 0x55e7b84e20a8; line 2187 (connection #0)
* Mark bundle as not supporting multiuse
* HTTP/2 found, allow multiplexing
< HTTP/1.1 101 Switching Protocols
< Connection: Upgrade
< Upgrade: h2c
* Received 101
* Using HTTP2, server supports multiplexing
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* multi changed, check CONNECT_PEND queue!
* Connection state changed (MAX_CONCURRENT_STREAMS == 250)!
* multi changed, check CONNECT_PEND queue!
* HTTP/2 found, allow multiplexing
< HTTP/2 200 
< content-type: text/plain; charset=utf-8
< content-length: 6
< date: Mon, 30 Aug 2021 10:54:30 GMT
< 
* nread == 0, stream closed, bailing
* STATE: PERFORMING => DONE handle 0x55e7b84e20a8; line 2386 (connection #0)
* multi_done
* Connection #0 to host 121.37.5.232 left intact
* Expire cleared (transfer 0x55e7b84e20a8)

bagder added a commit that referenced this issue Aug 30, 2021
…witch"

This reverts commit 455a63c.

Reported-by: Tk Xiong
Fixes #7633
@tk-xiong
Copy link
Author

@tk-xiong tk-xiong commented Aug 30, 2021

I revert 455a63c
It returns the correct results, but I still think there are some problems.

$ ./curl -v --http2 -g "121.37.5.232:10000/hello/"

Trying 121.37.5.232:10000...
Connected to 121.37.5.232 (121.37.5.232) port 10000 (#0)
GET /hello/ HTTP/1.1
Host: 121.37.5.232:10000
User-Agent: curl/7.78.0
Accept: /
Connection: Upgrade, HTTP2-Settings
Upgrade: h2c
HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA

Mark bundle as not supporting multiuse
< HTTP/1.1 101 Switching Protocols
< Connection: Upgrade
< Upgrade: h2c
Received 101
Using HTTP2, server supports multiplexing
Connection state changed (HTTP/2 confirmed)
Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
Connection state changed (MAX_CONCURRENT_STREAMS == 250)!
< HTTP/2 200
< content-type: text/plain; charset=utf-8
< content-length: 6
< date: Mon, 30 Aug 2021 12:04:46 GMT
<
Connection #0 to host 121.37.5.232 left intact
Hello.
$ ./curl -vI --http2 -g "121.37.5.232:10000/hello/"

Trying 121.37.5.232:10000...
Connected to 121.37.5.232 (121.37.5.232) port 10000 (#0)
HEAD /hello/ HTTP/1.1
Host: 121.37.5.232:10000
User-Agent: curl/7.78.0
Accept: /
Connection: Upgrade, HTTP2-Settings
Upgrade: h2c
HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA

Mark bundle as not supporting multiuse
< HTTP/1.1 101 Switching Protocols
HTTP/1.1 101 Switching Protocols
< Connection: Upgrade
Connection: Upgrade
< Upgrade: h2c
Upgrade: h2c

Received 101

Using HTTP2, server supports multiplexing

Connection state changed (HTTP/2 confirmed)

Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0

Connection state changed (MAX_CONCURRENT_STREAMS == 250)!

HTTP/2 stream 0 was not closed cleanly: PROTOCOL_ERROR (err 1)

stopped the pause stream!

Connection #0 to host 121.37.5.232 left intact
curl: (92) HTTP/2 stream 0 was not closed cleanly: PROTOCOL_ERROR (err 1)

curl: (92) HTTP/2 stream 0 was not closed cleanly: PROTOCOL_ERROR (err 1)
Does this need to be fixed?

@bagder
Copy link
Member

@bagder bagder commented Aug 30, 2021

Does this need to be fixed?

Probably, but as I've said: I cannot reproduce that.

@bagder bagder changed the title curl --http2 Not Work HTTP/2 (h2c) upgrade in clear-text problem Aug 30, 2021
@tk-xiong
Copy link
Author

@tk-xiong tk-xiong commented Aug 30, 2021

I Use git clone master branch and revert code.

git clone https://github.com/curl/curl.git
 
autoreconf -fi
 
# change configure file:
want_h2_path="$withval/lib/pkgconfig" to want_h2_path="$withval/lib64/pkgconfig"
 
# revert code.
... ... 
 
# build
./configure --prefix=/data/tkxiong/curl/release \
    --with-nghttp2=/data/tkxiong/nghttp2/release \
    --disable-shared \
    --without-ssl
 
make && make install
$ ./curl -vI -g --http2 "http://121.37.5.232:10000/hello/"
*   Trying 121.37.5.232:10000...
* Connected to 121.37.5.232 (121.37.5.232) port 10000 (#0)
> HEAD /hello/ HTTP/1.1
> Host: 121.37.5.232:10000
> User-Agent: curl/7.79.0-DEV
> Accept: */*
> Connection: Upgrade, HTTP2-Settings
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 101 Switching Protocols
HTTP/1.1 101 Switching Protocols
< Connection: Upgrade
Connection: Upgrade
< Upgrade: h2c
Upgrade: h2c
* Received 101
* Using HTTP2, server supports multiplexing
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0

* Connection state changed (MAX_CONCURRENT_STREAMS == 250)!
* HTTP/2 stream 0 was not closed cleanly: PROTOCOL_ERROR (err 1)
* stopped the pause stream!
* Connection #0 to host 121.37.5.232 left intact
curl: (92) HTTP/2 stream 0 was not closed cleanly: PROTOCOL_ERROR (err 1)
$ ./curl -v -g --http2 "http://121.37.5.232:10000/hello/"
*   Trying 121.37.5.232:10000...
* Connected to 121.37.5.232 (121.37.5.232) port 10000 (#0)
> GET /hello/ HTTP/1.1
> Host: 121.37.5.232:10000
> User-Agent: curl/7.79.0-DEV
> Accept: */*
> Connection: Upgrade, HTTP2-Settings
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 101 Switching Protocols
< Connection: Upgrade
< Upgrade: h2c
* Received 101
* Using HTTP2, server supports multiplexing
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Connection state changed (MAX_CONCURRENT_STREAMS == 250)!
< HTTP/2 200 
< content-type: text/plain; charset=utf-8
< content-length: 6
< date: Mon, 30 Aug 2021 13:17:41 GMT
< 
* Connection #0 to host 121.37.5.232 left intact
Hello.

@tk-xiong
Copy link
Author

@tk-xiong tk-xiong commented Aug 30, 2021

$ ./curl --version
curl 7.79.0-DEV (x86_64-unknown-linux-gnu) libcurl/7.79.0-DEV zlib/1.2.7 nghttp2/1.44.0
Release-Date: [unreleased]
Protocols: dict file ftp gopher http imap mqtt pop3 rtsp smtp telnet tftp 
Features: alt-svc AsynchDNS HTTP2 IPv6 Largefile libz UnixSockets

@bagder
Copy link
Member

@bagder bagder commented Aug 30, 2021

I believe the HTTP/2 stream 0 was not closed cleanly: PROTOCOL_ERROR error is unrelated and a different thing.

Your server sends a RST_STREAM with this error when curl sends a HEAD request, but not when a GET is sent.

@tk-xiong
Copy link
Author

@tk-xiong tk-xiong commented Aug 30, 2021

Ok, thank you very much. I'll try to learn the difference between HEAD and GET requests.
close the issue.

@tk-xiong tk-xiong closed this Aug 30, 2021
@bagder
Copy link
Member

@bagder bagder commented Aug 30, 2021

The revert is still needed so this issue is still real and thus you closed it prematurely.

bagder added a commit that referenced this issue Aug 30, 2021
…witch"

This reverts commit 455a63c.

Reported-by: Tk Xiong
Fixes #7633
Closes #7648
@tk-xiong
Copy link
Author

@tk-xiong tk-xiong commented Aug 31, 2021

Sorry, I will reopen it, so when should I close this issue?

@tk-xiong tk-xiong reopened this Aug 31, 2021
@bagder
Copy link
Member

@bagder bagder commented Aug 31, 2021

When we merged the fix, which was done already.

@bagder bagder closed this Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants