You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by GitBox <gi...@apache.org> on 2020/04/20 17:02:36 UTC
[GitHub] [tomcat] arkanovicz opened a new pull request #277: Filter invalid HTTP 2.0 headers from response
arkanovicz opened a new pull request #277:
URL: https://github.com/apache/tomcat/pull/277
Connection headers like `Connection: keep-alive` are invalid in HTTP/2.0, and some clients (like Safari or curl) are very touchy about it.
When an application component adds the typical HTTP/1.x "Connection: keep-alive" header to the response, despite the component's good intention, the header is faulty in HTTP/2.0 and SHOULD always be filtered.
The current implementation emits a warning in the logs only once per instance.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] arkanovicz commented on a change in pull request #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
arkanovicz commented on a change in pull request #277:
URL: https://github.com/apache/tomcat/pull/277#discussion_r412030387
##########
File path: java/org/apache/coyote/Response.java
##########
@@ -61,6 +62,11 @@
*/
private static final Locale DEFAULT_LOCALE = Locale.getDefault();
+ /**
+ * Helper to log the invalid HTTP/2.0 header error only once per instance
+ */
+ private static AtomicBoolean invalidHeaderWarningEmitted = new AtomicBoolean(false);
Review comment:
@martin-g It's a langage abuse. Per instance of running tomcat, sorry. Anyhow, @rmaucher suggestion to just throw the exception is simpler, so the AtomicBoolean is deprecated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on a change in pull request #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #277:
URL: https://github.com/apache/tomcat/pull/277#discussion_r412098476
##########
File path: java/org/apache/coyote/Response.java
##########
@@ -435,6 +435,20 @@ private boolean checkSpecialHeader( String name, String value) {
return false;
}
}
+
+ if (outputBuffer instanceof Http2OutputBuffer && name.equalsIgnoreCase("Connection") ) {
+
+ /*
+ * Connection headers are invalid in HTTP/2.0, and some clients (like Safari or curl)
+ * are very touchy about it. Most probably, an application component has added the
+ * typical HTTP/1.x "Connection: keep-alive" header, but despide the component's
+ * good intention, the header is faulty in HTTP/2.0 and *should* be refused.
+ * .
+ * @see https: *tools.ietf.org/html/rfc7540#section-8.1.2.2
Review comment:
URL is broken
##########
File path: java/org/apache/coyote/Response.java
##########
@@ -435,6 +435,20 @@ private boolean checkSpecialHeader( String name, String value) {
return false;
}
}
+
+ if (outputBuffer instanceof Http2OutputBuffer && name.equalsIgnoreCase("Connection") ) {
+
+ /*
+ * Connection headers are invalid in HTTP/2.0, and some clients (like Safari or curl)
Review comment:
Please to HTTP/2. There is not HTTP/2.0.
##########
File path: java/org/apache/coyote/Response.java
##########
@@ -435,6 +435,20 @@ private boolean checkSpecialHeader( String name, String value) {
return false;
}
}
+
+ if (outputBuffer instanceof Http2OutputBuffer && name.equalsIgnoreCase("Connection") ) {
+
+ /*
+ * Connection headers are invalid in HTTP/2.0, and some clients (like Safari or curl)
+ * are very touchy about it. Most probably, an application component has added the
+ * typical HTTP/1.x "Connection: keep-alive" header, but despide the component's
Review comment:
despite
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] michael-o edited a comment on issue #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
michael-o edited a comment on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-616852929
> I see. But at that point, you do not know who set up the header, so the stack trace won't be informative.
Can you retry with a vanilla Tomcat and the most recent curl?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on issue #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
michael-o commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-616789924
Why can't I reproduce it although I have an h2c reponse?!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] arkanovicz commented on issue #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
arkanovicz commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-616715639
Of course, you will need to actually *add* the faulty header to the response to see this error.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] arkanovicz commented on issue #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
arkanovicz commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-616714936
@michael-o curl example:
$ curl --http2 -Nv "https://some.http2.url/"
* Trying 127.0.0.1:8833...
* TCP_NODELAY set
* Connected to some.http2.url (127.0.0.1) port 8833 (#0)
> GET / HTTP/1.1
> Host: some.http2.url:8833
> User-Agent: curl/7.65.3
> Connection: Upgrade, HTTP2-Settings
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAARAAAAAAAIAAAAA
> Accept:text/plain
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 101
< Connection: Upgrade
< Upgrade: h2c
< Date: Mon, 20 Apr 2020 17:48:26 GMT
* 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=15
* Connection state changed (MAX_CONCURRENT_STREAMS == 100)!
* http2 error: Invalid HTTP header field was received: frame type: 1, stream: 1, name: [connection], value: [keep-alive]
* HTTP/2 stream 0 was not closed cleanly: PROTOCOL_ERROR (err 1)
* stopped the pause stream!
* Connection #0 to host some.http2.url left intact
curl: (92) HTTP/2 stream 0 was not closed cleanly: PROTOCOL_ERROR (err 1)
Safari will display the following error in the javascript console:
[Error] Failed to load resource: The operation couldnt be completed. Protocol error
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] arkanovicz commented on issue #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
arkanovicz commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-616850925
I see. But at that point, you do not know who set up the header, so the stack trace won't be informative.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on issue #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
markt-asf commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-616767803
That would also allow some clean up in the current code that sets the header and has to take account of any value that may have been set by the application.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on issue #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
michael-o commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-616754168
I fail to see this issue:
Pure:
```
$ curl --http2 -NvIsq http://localhost:8080
HTTP/1.1 101
Connection: Upgrade
Upgrade: h2c
Date: Mon, 20 Apr 2020 19:10:48 GMT
HTTP/2 200
content-type: text/html;charset=UTF-8
date: Mon, 20 Apr 2020 19:10:48 GMT
* Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8080 (#0)
> HEAD / HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.63.0
> Accept: */*
> Connection: Upgrade, HTTP2-Settings
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAARAAAAAAAIAAAAA
>
< HTTP/1.1 101
< Connection: Upgrade
< Upgrade: h2c
< Date: Mon, 20 Apr 2020 19:10:48 GMT
* 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
< content-type: text/html;charset=UTF-8
< date: Mon, 20 Apr 2020 19:10:48 GMT
<
* Connection #0 to host localhost left intact
```
with explicit headers:
```
$ curl --http2 -NvIsq http://localhost:8080 -H "Connection: keep-alive"
HTTP/1.1 101
Connection: Upgrade, keep-alive
Upgrade: h2c
Date: Mon, 20 Apr 2020 19:11:45 GMT
Keep-Alive: timeout=20
HTTP/2 200
content-type: text/html;charset=UTF-8
date: Mon, 20 Apr 2020 19:11:45 GMT
* Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8080 (#0)
> HEAD / HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.63.0
> Accept: */*
> Connection: Upgrade, HTTP2-Settings
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAARAAAAAAAIAAAAA
> Connection: keep-alive
>
< HTTP/1.1 101
< Connection: Upgrade, keep-alive
< Upgrade: h2c
< Date: Mon, 20 Apr 2020 19:11:45 GMT
< Keep-Alive: timeout=20
* 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
< content-type: text/html;charset=UTF-8
< date: Mon, 20 Apr 2020 19:11:45 GMT
<
* Connection #0 to host localhost left intact
```
Running Tomcat 9.0.34 and
```
$ curl --version
curl 7.63.0 (x86_64-w64-mingw32) libcurl/7.63.0 OpenSSL/1.1.1a (WinSSL) zlib/1.2.11 libidn2/2.0.5 nghttp2/1.35.1
Release-Date: 2018-12-12
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtsp smtp smtps telnet tftp
Features: AsynchDNS IDN IPv6 Largefile SSPI Kerberos SPNEGO NTLM SSL libz TLS-SRP HTTP2 HTTPS-proxy MultiSSL Metalink
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] pirateskipper commented on a change in pull request #277: Refuse adding invalid HTTP 2.0 headers
Posted by GitBox <gi...@apache.org>.
pirateskipper commented on a change in pull request #277:
URL: https://github.com/apache/tomcat/pull/277#discussion_r694075997
##########
File path: java/org/apache/coyote/Response.java
##########
@@ -435,6 +435,20 @@ private boolean checkSpecialHeader( String name, String value) {
return false;
}
}
+
+ if (outputBuffer instanceof Http2OutputBuffer && name.equalsIgnoreCase("Connection") ) {
Review comment:
`if (outputBuffer instanceof Http2OutputBuffer && "Connection".equalsIgnoreCase(name) ) {`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] arkanovicz commented on issue #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
arkanovicz commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-616897939
> Can you retry with a vanilla Tomcat and the most recent curl?
I've spent enough time digging for this bug (to end up finding it in a SSE dependency), including step by step debugging in 9.0.33 sources (then checking it is still present with 9.0.34) *and* in the specs and the browser sources to not really need to retry anything to persuade myself of the pertinenece of this patch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on issue #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
michael-o commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-616689787
Can you show your sample curl input/output?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on issue #277: Refuse adding invalid HTTP 2.0 headers
Posted by GitBox <gi...@apache.org>.
markt-asf commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-618453474
I've been reading the HTTP/2 RFC and there is more to this than simply blocking the `connection` header.
1. What the HTTP/2 and HTTP/1.1 specs suggest we should be doing in parsing an attempt to set the `connection` header and then blocking that header *and* and connection level headers it specifies whether set previously or not.
1. There is the general question of whether we should be targeting just HTTP/2 or whether we should be preventing applications doing this regardless of protocol.
We need to figure out what we actually want to do first.
I'm currently leaning towards introducing logging of attempts to set connection level headers with a warning that a future version will block the attempt. Probably with `UserDataHelper` to keep log volumes down even though this isn't really user data.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] rmaucher commented on issue #277: Refuse adding invalid HTTP 2.0 headers
Posted by GitBox <gi...@apache.org>.
rmaucher commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-617758397
Still, -1, again for your patch. In addition to being ugly, there's no provision in the Servlet spec to throw an exception on random header names, especially common ones, so failing, fast or slow, is wrong.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on issue #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
markt-asf commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-616991103
@arkanovicz
You can ignore requests to retest this. I was able to recreate the issue with curl using the steps you describe.
Could you expand the SSE acronym please. I want to make sure I understand you correctly.
Code format issues are opening braces `{` should not be on a new line and multi-line comments either use `//` before every line or, if the `/* ... */` style is used each intermediate line starts with an aligned `*`
More generally...
It would be worth reviewing the HTTP/2 spec to check if there are any other headers that are invalid for HTTP/2.
The global blocking off applications setting Connection headers seems reasonable at first consideration but needs more thought/review in case there are use cases where it is arguably valid / necessary to do so.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] rmaucher commented on issue #277: Refuse adding invalid HTTP 2.0 headers
Posted by GitBox <gi...@apache.org>.
rmaucher commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-618470184
Yes, it is accurate if there's a "connection: foobar" header, then there could be a "foobar" header and in that case it's tied to the connection header.
Note about my earlier "proposal" for HTTP/1.1, the connection header is used by the websocket.server.UpgradeUtil helper class to allow upgrade through the API. So it's not possible to filter it and be done, this would have to be fixed as well [not sure how]. Servlet apps can upgrade through the proper API and would not be affected.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on issue #277: Refuse adding invalid HTTP 2.0 headers
Posted by GitBox <gi...@apache.org>.
michael-o commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-618504729
Please note that there may be client components which may explicitly close connections with `Connection: close`. E.g., in `SpnegoAuthenticator`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] arkanovicz commented on issue #277: Refuse adding invalid HTTP 2.0 headers
Posted by GitBox <gi...@apache.org>.
arkanovicz commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-617753041
@rmaucher The spec authors justify themselves of this strict policy by saying that otherwise it's a vector of attack. I don't know the whereabouts, here.
About where to put the filtering, in Response.addHeader() or StreamProcessor.prepareHeaders(), I understand your point of view (which is, I suppose, to gather all protocol stuff together) but targeting StreamProcessor doesn't respect the Fail Fast paradigm, which prevails IMHO.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] rmaucher commented on issue #277: Refuse adding invalid HTTP 2.0 headers
Posted by GitBox <gi...@apache.org>.
rmaucher commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-617239514
I will maintain my -1
For starters, any such HTTP/2 specific nonsense safety nets need to be added to StreamProcessor.prepareHeaders instead of other random locations.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] martin-g commented on a change in pull request #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #277:
URL: https://github.com/apache/tomcat/pull/277#discussion_r411969053
##########
File path: java/org/apache/coyote/Response.java
##########
@@ -61,6 +62,11 @@
*/
private static final Locale DEFAULT_LOCALE = Locale.getDefault();
+ /**
+ * Helper to log the invalid HTTP/2.0 header error only once per instance
+ */
+ private static AtomicBoolean invalidHeaderWarningEmitted = new AtomicBoolean(false);
Review comment:
`per instance` of what ? Because you use `static` here, so it is on class loader level.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] arkanovicz commented on issue #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
arkanovicz commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-617075551
> Could you expand the SSE acronym please. I want to make sure I understand you correctly.
[Server-Side Events](https://html.spec.whatwg.org/multipage/server-sent-events.html)
> Code format issues are opening braces `{` should not be on a new line and multi-line comments either use `//` before every line or, if the `/* ... */` style is used each intermediate line starts with an aligned `*`
Ok, noted.
> More generally...
>
> It would be worth reviewing the HTTP/2 spec to check if there are any other headers that are invalid for HTTP/2.
To my knowledge, only the Connection headers.
> The global blocking off applications setting Connection headers seems reasonable at first consideration but needs more thought/review in case there are use cases where it is arguably valid / necessary to do so.
At best, if the faulty header doesn't provoke an error (required by the specs), it will be ignored. Here's what the spec says:
> Intermediaries that process HTTP requests or responses (i.e., any intermediary not acting as a tunnel) MUST NOT forward a malformed request or response. Malformed requests or responses that are detected MUST be treated as a stream error (Section 5.4.2) of type PROTOCOL_ERROR.
> For malformed requests, a server MAY send an HTTP response prior to closing or resetting the stream. Clients MUST NOT accept a malformed response. Note that these requirements are intended to protect against several types of common attacks against HTTP; they are deliberately strict because being permissive can expose implementations to these vulnerabilities.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on issue #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
michael-o commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-616852929
> I see. But at that point, you do not know who set up the header, so the stack
> I see. But at that point, you do not know who set up the header, so the stack trace won't be informative.
Can you retry with a vanilla Tomcat and the most recent curl?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] michael-o edited a comment on issue #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
michael-o edited a comment on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-616754168
I fail to see this issue:
Pure:
```
$ curl --http2 -NvIsq http://localhost:8080
HTTP/1.1 101
Connection: Upgrade
Upgrade: h2c
Date: Mon, 20 Apr 2020 19:10:48 GMT
HTTP/2 200
content-type: text/html;charset=UTF-8
date: Mon, 20 Apr 2020 19:10:48 GMT
* Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8080 (#0)
> HEAD / HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.63.0
> Accept: */*
> Connection: Upgrade, HTTP2-Settings
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAARAAAAAAAIAAAAA
>
< HTTP/1.1 101
< Connection: Upgrade
< Upgrade: h2c
< Date: Mon, 20 Apr 2020 19:10:48 GMT
* 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
< content-type: text/html;charset=UTF-8
< date: Mon, 20 Apr 2020 19:10:48 GMT
<
* Connection #0 to host localhost left intact
```
with explicit headers:
```
$ curl --http2 -NvIsq http://localhost:8080 -H "Connection: keep-alive"
HTTP/1.1 101
Connection: Upgrade, keep-alive
Upgrade: h2c
Date: Mon, 20 Apr 2020 19:11:45 GMT
Keep-Alive: timeout=20
HTTP/2 200
content-type: text/html;charset=UTF-8
date: Mon, 20 Apr 2020 19:11:45 GMT
* Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8080 (#0)
> HEAD / HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.63.0
> Accept: */*
> Connection: Upgrade, HTTP2-Settings
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAARAAAAAAAIAAAAA
> Connection: keep-alive
>
< HTTP/1.1 101
< Connection: Upgrade, keep-alive
< Upgrade: h2c
< Date: Mon, 20 Apr 2020 19:11:45 GMT
< Keep-Alive: timeout=20
* 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
< content-type: text/html;charset=UTF-8
< date: Mon, 20 Apr 2020 19:11:45 GMT
<
* Connection #0 to host localhost left intact
```
Running Tomcat 9.0.34 and
```
$ curl --version
curl 7.63.0 (x86_64-w64-mingw32) libcurl/7.63.0 OpenSSL/1.1.1a (WinSSL) zlib/1.2.11 libidn2/2.0.5 nghttp2/1.35.1
Release-Date: 2018-12-12
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtsp smtp smtps telnet tftp
Features: AsynchDNS IDN IPv6 Largefile SSPI Kerberos SPNEGO NTLM SSL libz TLS-SRP HTTP2 HTTPS-proxy MultiSSL Metalink
```
Even with
```
C:\Users\mosipov>curl --version
curl 7.69.1 (x86_64-pc-win32) libcurl/7.69.1 OpenSSL/1.1.1f (Schannel) zlib/1.2.11 brotli/1.0.7 WinIDN libssh2/1.9.0 nghttp2/1.40.0
Release-Date: 2020-03-11
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp
Features: AsynchDNS HTTP2 HTTPS-proxy IDN IPv6 Kerberos Largefile MultiSSL NTLM SPNEGO SSL SSPI TLS-SRP brotli libz
```
Please ugrade your curl first!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] rmaucher commented on pull request #277: Refuse adding invalid HTTP 2.0 headers
Posted by GitBox <gi...@apache.org>.
rmaucher commented on pull request #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-619026541
Indeed, there are two very legacy looking workarounds for brokeness in spnego. One of them can be removed since using Java 8u40 is not reasonable anymore. What about that connection close one ? IMO it's useless since there is no default, so an admin would have to figure out whatever broken clients are out there, then populate it. Not reasonable. If the feature is useful, then it should have a default with all the known broken clients.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on pull request #277: Refuse adding invalid HTTP 2.0 headers
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-619041883
@rmaucher The question is when is it justified for component or webapp code to close the connection? Should a container solely decide to close the connection?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] michael-o edited a comment on issue #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
michael-o edited a comment on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-616789924
Why can't I reproduce it although I have an h2c response?!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] arkanovicz commented on issue #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
arkanovicz commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-616771727
@markt-asf This happens for instance in SSE components (tomcat does *not* provide such components), or more generally in *any* J2EE filters or servlets, coded before HTTP/2.0, which want to ensure that the connection is kept alive.
@rmaucher It means that you choose tomcat to let buggy server-side components break HTTP/2.0 streams. Other user agents do *always* ignore this faulty header, so what's the point in sending it in the first place? Helping to find bad client code by breaking more often? Of course, it's a strategy, but considering the vast amount of HTTP/1.1-specific code in the nature that is supposed to migrate to HTTP/2.0, it's better to just log an informative warning and get things straight. Or directly throw.
@michael-o I'm using a recent curl. There is a misunderstanding, here, I'm talking about a header sent by the server in the HTTP response.
> Since it's now 2020, shouldn't it be doable to block any attempt to set the connection header by the application ?
> That would also allow some clean up in the current code that sets the header and has to take account of any value that may have been set by the application.
@rmaucher That would mean to throw the exception right away, it's enough to remove the try catch in this patch. Yes, certainly simpler.
> If applied (and I'm not convinced it should be) the formatting of the PR needs fixing to be consistent with the Tomcat code.
@markt-asf Can you give me some pointers on the needed fixes?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] rmaucher commented on issue #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
rmaucher commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-616745834
-1
Tomcat covers the most egregious cases, but this is probably not one of them. Even in HTTP/1.1, what the application does setting this connection header is nonsense and will never help. You will need to fix the application.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] rmaucher commented on issue #277: Refuse adding invalid HTTP 2.0 headers
Posted by GitBox <gi...@apache.org>.
rmaucher commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-617661025
The initial post says SHOULD, but after actually checking the spec it is a MUST.
https://tools.ietf.org/html/rfc7540#section-8.1.2.2
It is really odd the specification made such a choice given how many applications toy with that header in an attempt to do something useful, and given they have no idea if they're using HTTP/1.1 or HTTP/2. So this looks to me like a spec mistake [for compatibility, they should have said it has to be ignored and can be freely removed, whatever], and it is not very surprising most HTTP/2 clients would not check this requirement.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on issue #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
markt-asf commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-616745000
I'm curious why applications think they need to set the connection header. I'd expect the container to handle this. Further, applications that want to set this header can/should use `ServletRequest.getProtocol()` first to check it is appropriate to do so.
If applied (and I'm not convinced it should be) the formatting of the PR needs fixing to be consistent with the Tomcat code. I'd also consider using UserDataHelper for the logging even though this isn't strictly a user data issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on issue #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
michael-o commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-616794864
We are talking about this code block: https://github.com/apache/tomcat/blob/65bc61a466fdc4ccc0ee281bd411fca7cb11adb0/java/org/apache/coyote/http11/Http11Processor.java#L932-L956
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] rmaucher commented on issue #277: Filter invalid HTTP 2.0 headers from response
Posted by GitBox <gi...@apache.org>.
rmaucher commented on issue #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-616764169
Since it's now 2020, shouldn't it be doable to block any attempt to set the connection header by the application ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on pull request #277: Refuse adding invalid HTTP 2.0 headers
Posted by GitBox <gi...@apache.org>.
markt-asf commented on pull request #277:
URL: https://github.com/apache/tomcat/pull/277#issuecomment-619147196
In an ideal world, anything in the Connection header should only be the concern of the container. Historically that hasn't been possible due to broken clients. The question is, are we at the point where it would be possible? Logging a warning when an application attempts this is one way to help us find out. Users will complain about the log message. Whether they are able to fix the app and/or client in question will tell us how safe it would be for us to move to a ban.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org