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

server: enable H2C for HTTP listeners via CLI flag #2739

Merged
merged 2 commits into from Oct 1, 2020

Conversation

srenatus
Copy link
Contributor

This follows the docs provided by the github issue,
https://www.mailgun.com/blog/http-2-cleartext-h2c-client-example-go/

For manual testing, ensure that you have a curl version with the proper
"Features", as can be read off curl --version:

curl 7.72.0 (x86_64-apple-darwin19.5.0) libcurl/7.72.0 OpenSSL/1.1.1g zlib/1.2.11 brotli/1.0.9 zstd/1.4.5 c-ares/1.16.1 libssh2/1.9.0 nghttp2/1.41.0 librtmp/2.3
Release-Date: 2020-08-19
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtmp rtsp scp sftp smb smbs smtp smtps telnet tftp
Features: AsynchDNS brotli GSS-API HTTP2 HTTPS-proxy IPv6 Kerberos Largefile libz Metalink NTLM NTLM_WB SPNEGO SSL TLS-SRP UnixSockets zstd

As described in the mailgun blog post, curl-openssl on homebrew has it for osx.

With this change, and started with

./opa_darwin_amd64 run -s --h2c --diagnostic-addr :8182

Both the ALPN and the "prior knowledge" modes work against the insecure endpoints:

$ curl -v --http2 http://127.0.0.1:8181/metrics >/dev/null
*   Trying 127.0.0.1:8181...
> GET /metrics HTTP/1.1
> Host: 127.0.0.1:8181
> User-Agent: curl/7.72.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 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 == 250)!
< HTTP/2 200
< content-type: text/plain; version=0.0.4; charset=utf-8
< date: Wed, 30 Sep 2020 12:15:08 GMT
<
{ [4096 bytes data]
* Connection #0 to host 127.0.0.1 left intact
$ curl -v --http2-prior-knowledge http://127.0.0.1:8181/metrics >/dev/null
*   Trying 127.0.0.1:8181...
* 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
* Using Stream ID: 1 (easy handle 0x7f85c3814c00)
> GET /metrics HTTP/2
> Host: 127.0.0.1:8181
> user-agent: curl/7.72.0
> accept: */*
>
* Connection state changed (MAX_CONCURRENT_STREAMS == 250)!
< HTTP/2 200
< content-type: text/plain; version=0.0.4; charset=utf-8
< date: Wed, 30 Sep 2020 12:15:13 GMT
<
{ [4096 bytes data]
* Connection #0 to host 127.0.0.1 left intact

golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f h1:wMNYb4v58l5UBM7MYRLPG6ZhfOqbKu7X5eyFl8ZhKvA=
golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.0.0-20190423024810-112230192c58 h1:8gQV6CLnAEikrhgkHFbMAEhagSSnXWGV915qUMm9mrU=
golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sys v0.0.0-20180905080454-ebe1bf3edb33/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a h1:1BGLXjeY4akVXGgbC9HugT3Jv3hCI0z56oJR5vAMgBU=
golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20200323222414-85ca7c5b95cd h1:xhmwyvizuTgC2qz7ZlMluP20uW+C3Rm0FD/WLDX8884=
golang.org/x/sys v0.0.0-20200323222414-85ca7c5b95cd/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These blow the PR size out of proportion 🙄 No idea what to do about that, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dangers of adding new dependencies! 😆

@srenatus
Copy link
Contributor Author

Ugh Golang 1.13.7 doesn't know t.Cleanup(). I'll move some code. Let's get that updated, though 😄

Copy link
Contributor

@patrick-east patrick-east left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good! Just one comment on the test, but nothing major.

@@ -3219,6 +3229,78 @@ func newClient(t *testing.T, pool *x509.CertPool, clientKeyPair ...string) *http
return &c
}

func TestH2CHTTPListeners(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The majority of other tests in here, afaik, don't actually start the loops. They are mostly testing the HTTP handler directly by calling Handle() on it with a recorder and some request. I'd recommend moving this test into a new package in https://github.com/open-policy-agent/opa/tree/master/test/e2e where we have more tests that are like this one which are testing out the sort of full extent of the API service (plus you get a bunch of helpers over there for stuff like cleaning up, waiting for the server, getting the addresses, etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! I'll move it. 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. It's nicer indeed. I've got a test or two to move there for authentication with TLS and authorization, I think, but that'll be a follow-up.

golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f h1:wMNYb4v58l5UBM7MYRLPG6ZhfOqbKu7X5eyFl8ZhKvA=
golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.0.0-20190423024810-112230192c58 h1:8gQV6CLnAEikrhgkHFbMAEhagSSnXWGV915qUMm9mrU=
golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sys v0.0.0-20180905080454-ebe1bf3edb33/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a h1:1BGLXjeY4akVXGgbC9HugT3Jv3hCI0z56oJR5vAMgBU=
golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20200323222414-85ca7c5b95cd h1:xhmwyvizuTgC2qz7ZlMluP20uW+C3Rm0FD/WLDX8884=
golang.org/x/sys v0.0.0-20200323222414-85ca7c5b95cd/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dangers of adding new dependencies! 😆

This follows the docs provided by the [github issue](1),
https://www.mailgun.com/blog/http-2-cleartext-h2c-client-example-go/

For manual testing, ensure that you have a curl version with the proper
"Features", as can be read off `curl --version`:

    curl 7.72.0 (x86_64-apple-darwin19.5.0) libcurl/7.72.0 OpenSSL/1.1.1g zlib/1.2.11 brotli/1.0.9 zstd/1.4.5 c-ares/1.16.1 libssh2/1.9.0 nghttp2/1.41.0 librtmp/2.3
    Release-Date: 2020-08-19
    Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtmp rtsp scp sftp smb smbs smtp smtps telnet tftp
    Features: AsynchDNS brotli GSS-API HTTP2 HTTPS-proxy IPv6 Kerberos Largefile libz Metalink NTLM NTLM_WB SPNEGO SSL TLS-SRP UnixSockets zstd

As described in the mailgun blog post, curl-openssl on homebrew has it for osx.

With this change, and started with

    ./opa_darwin_amd64 run -s --h2c --diagnostic-addr :8182

Both the ALPN and the "prior knowledge" modes work against the insecure endpoints:

    $ curl -v --http2 http://127.0.0.1:8181/metrics >/dev/null
    *   Trying 127.0.0.1:8181...
    > GET /metrics HTTP/1.1
    > Host: 127.0.0.1:8181
    > User-Agent: curl/7.72.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 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 == 250)!
    < HTTP/2 200
    < content-type: text/plain; version=0.0.4; charset=utf-8
    < date: Wed, 30 Sep 2020 12:15:08 GMT
    <
    { [4096 bytes data]
    * Connection #0 to host 127.0.0.1 left intact
    $ curl -v --http2-prior-knowledge http://127.0.0.1:8181/metrics >/dev/null
    *   Trying 127.0.0.1:8181...
    * 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
    * Using Stream ID: 1 (easy handle 0x7f85c3814c00)
    > GET /metrics HTTP/2
    > Host: 127.0.0.1:8181
    > user-agent: curl/7.72.0
    > accept: */*
    >
    * Connection state changed (MAX_CONCURRENT_STREAMS == 250)!
    < HTTP/2 200
    < content-type: text/plain; version=0.0.4; charset=utf-8
    < date: Wed, 30 Sep 2020 12:15:13 GMT
    <
    { [4096 bytes data]
    * Connection #0 to host 127.0.0.1 left intact

[1]: open-policy-agent#2399

Signed-off-by: Stephan Renatus <srenatus@chef.io>
Signed-off-by: Stephan Renatus <srenatus@chef.io>
@patrick-east patrick-east merged commit 23b5987 into open-policy-agent:master Oct 1, 2020
@tsandall tsandall mentioned this pull request Oct 13, 2020
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