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

warp-tls and connection keep-alive header error #707

Closed
k-bx opened this Issue Aug 15, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@k-bx

k-bx commented Aug 15, 2018

So, I was trying to add a Connection: keep-alive header to my application, which is served in both, HTTP and HTTPS (via warp-tls). Surprisingly, once I've added the header, curl started showing an error curl: (92) HTTP/2 stream 1 was not closed cleanly: PROTOCOL_ERROR (err 1). I've re-created the minimal code here: https://github.com/k-bx/warp-keepalive

Without the header I get:

➜  warp-keepalive git:(master) ✗ curl --insecure -i https://localhost:3000
HTTP/2 200
date: Wed, 15 Aug 2018 17:41:17 GMT
server: Warp/3.2.23

Hello World%

Adding header gives:

➜  warp-keepalive git:(master) ✗ curl --insecure -i https://localhost:3000
curl: (92) HTTP/2 stream 1 was not closed cleanly: PROTOCOL_ERROR (err 1)

I'm not an expert in HTTP/2, but from quick googling, I've found MDN saying "Also, Connection and Keep-Alive are ignored in HTTP/2", but some answer on SO have said: "So HTTP/2 doesn't negate need for keep-alives. It negates need for multiple connections (amongst other things)."

@alexanderkjeldaas

This comment has been minimized.

Contributor

alexanderkjeldaas commented Sep 6, 2018

Seems related to #703 - does Safari set this header for HTTP/2 unlike other browsers?

@kazu-yamamoto kazu-yamamoto self-assigned this Sep 6, 2018

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Sep 7, 2018

https://tools.ietf.org/html/rfc7540#section-8.1.2.2 says:

HTTP/2 does not use the Connection header field to indicate
connection-specific header fields; in this protocol, connection-
specific metadata is conveyed by other means. An endpoint MUST NOT
generate an HTTP/2 message containing connection-specific header
fields; any message containing connection-specific header fields MUST
be treated as malformed

This means that an intermediary transforming an HTTP/1.x message to
HTTP/2 will need to remove any header fields nominated by the
Connection header field, along with the Connection header field
itself. Such intermediaries SHOULD also remove other connection-
specific header fields, such as Keep-Alive, Proxy-Connection,
Transfer-Encoding, and Upgrade, even if they are not nominated by the
Connection header field.

I will make a patch to remove Connection:, Keep-Alive, Proxy-Connection, Transfer-Encoding, and Upgrade in HTTP/2.

@kazu-yamamoto kazu-yamamoto added the http2 label Sep 7, 2018

kazu-yamamoto added a commit to kazu-yamamoto/wai that referenced this issue Sep 7, 2018

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Sep 7, 2018

I could reproduce this and confirmed that the patch above fixed this issue.

@k-bx Would you try to test master?

@k-bx

This comment has been minimized.

k-bx commented Sep 7, 2018

@kazu-yamamoto yes, it works, thank you!

➜  warp-keepalive git:(master) ✗ curl --insecure -i https://localhost:3000
HTTP/2 200
date: Fri, 07 Sep 2018 16:45:48 GMT
server: Warp/3.2.24

@k-bx k-bx closed this Sep 7, 2018

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Sep 10, 2018

A new version of Warp has been released.
Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment