[Bug 56555] Multiple connection headers for status 400 when keep-alive is specified
https://issues.apache.org/bugzilla/show_bug.cgi?id=56555 Mark Thomas ma...@apache.org changed: What|Removed |Added Status|REOPENED|RESOLVED Resolution|--- |INVALID --- Comment #12 from Mark Thomas ma...@apache.org --- Fixes reverted for trunk and 7.0x. as discussed. Closing this issue as INVALID. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56555] Multiple connection headers for status 400 when keep-alive is specified
https://issues.apache.org/bugzilla/show_bug.cgi?id=56555 --- Comment #9 from Mark Thomas ma...@apache.org --- (In reply to Konstantin Kolinko from comment #8) I am -1 to r1598007 See how Connection header is defined in RFC2616 section 14.10 and processing of close token in section 8.1.2.1. The Connection header is a multi-value header that declares names of header fields that are for this connection only and are not forwarded by proxies. If you drop the Connection header you must drop any other headers that are referenced in it. There are no grounds to do that. I understand (and agree with) your objections to r1598007. What isn't clear is what you think Tomcat should do here. Is your position that this issue should simply have been resolved as invalid as multiple connection headers are allowed and the presence of a connection: close will always close the connection? -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56555] Multiple connection headers for status 400 when keep-alive is specified
https://issues.apache.org/bugzilla/show_bug.cgi?id=56555 --- Comment #10 from Remy Maucherat r...@apache.org --- It seems enough to me, +1 for a simple revert. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56555] Multiple connection headers for status 400 when keep-alive is specified
https://issues.apache.org/bugzilla/show_bug.cgi?id=56555 --- Comment #11 from Konstantin Kolinko knst.koli...@gmail.com --- multiple connection headers are allowed and the presence of a connection: close will always close the connection? Yes. (and multiple tokens in the same Connection header are allowed). If you strive for cleanness then a) remove keep-alive token only, b) remove Keep-Alive: header if there is any (a header defined by RFC2068). I do not think that it is worth pursuing. As for OP, I see no sense why they explicitly add a keep-alive header. First, container takes care of that (and will use keep-alive whenever possible, taking into account the protocol version and how many keep-alive requests have already been processed). Second, HTTP/1.1 connections are keep-alive by default. No header needed. Third, as this is a header for the current hop/connection only, it further makes no sense to rely on it in an application. Thus I think that the original claim is INVALID. Regarding the original code of AbstractHttp11Processor: I think Tomcat adds keep-alive header in some cases when it should not. Per the above, it does not matter, as the close header wins. The issues are the following: 1) isConnectionClose(headers) call happens in one branch of an if/else only. It shall be called in both cases. 2) If isConnectionClose(headers) returns true, then keepAlive flag shall be set to false and no header added. 3) isConnectionClose( ) method looks for the value of the first header only. It does not enumerate all Connection headers, nor it looks for multiple tokens in the same header. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56555] Multiple connection headers for status 400 when keep-alive is specified
https://issues.apache.org/bugzilla/show_bug.cgi?id=56555 --- Comment #7 from Christopher Schultz ch...@christopherschultz.net --- (In reply to Mark Thomas from comment #5) The correct response code for invalid credentials should be 401. Use of 401 is only appropriate when using WWW-Authenticate, as RFC2616 says 401 RFC-MUST include a WWW-Authenticate header. I don't think this happens with OAuth. If it's just a lack of (other) credentials, I think 403 is more appropriate. Tomcat has no way of telling which component set the 400 response status and therefore no way of distinguishing between a correct use of a 400 where there has been a syntax error and the connection needs to be closed and any other use of a 400 where it is safe to leave the connection open. The presense (or not) of the connection header may provide a hint but it is not reliable indicator. You are not going to like it but the only safe option for Tomcat with a 400 response is to close the connection (and yes we need to up the connection header handling when this happens). What about using some kind of Tomcat-specific request attribute that says Don't close this connection; I know what I'm doing? -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56555] Multiple connection headers for status 400 when keep-alive is specified
https://issues.apache.org/bugzilla/show_bug.cgi?id=56555 Konstantin Kolinko knst.koli...@gmail.com changed: What|Removed |Added Status|RESOLVED|REOPENED Resolution|FIXED |--- --- Comment #8 from Konstantin Kolinko knst.koli...@gmail.com --- I am -1 to r1598007 See how Connection header is defined in RFC2616 section 14.10 and processing of close token in section 8.1.2.1. The Connection header is a multi-value header that declares names of header fields that are for this connection only and are not forwarded by proxies. If you drop the Connection header you must drop any other headers that are referenced in it. There are no grounds to do that. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56555] Multiple connection headers for status 400 when keep-alive is specified
https://issues.apache.org/bugzilla/show_bug.cgi?id=56555 --- Comment #5 from Mark Thomas ma...@apache.org --- Having re-read RFC2616 the use of a 400 response code in this case looks wrong. 400 is meant to be used to indicate a syntax error in the request which is why Tomcat closes the connection. Tomcat can not be sure that it has correctly identified the end of the faulty request and continuing to process the connection could lead to security problems. The correct response code for invalid credentials should be 401. I do not recall anyone raising this as an issue previously. Tomcat has no way of telling which component set the 400 response status and therefore no way of distinguishing between a correct use of a 400 where there has been a syntax error and the connection needs to be closed and any other use of a 400 where it is safe to leave the connection open. The presense (or not) of the connection header may provide a hint but it is not reliable indicator. You are not going to like it but the only safe option for Tomcat with a 400 response is to close the connection (and yes we need to up the connection header handling when this happens). -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56555] Multiple connection headers for status 400 when keep-alive is specified
https://issues.apache.org/bugzilla/show_bug.cgi?id=56555 Mark Thomas ma...@apache.org changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #6 from Mark Thomas ma...@apache.org --- The multiple connection headers aspect of this has been fixed in 8.0.x for 8.0.9 onwards and in 7.0.x for 7.0.55 onwards. I'm marking this as RESOLVED on that basis. I recognise that this doesn't address the connection being closed part of the problem. I don't see an easy solution to that. Suggestions welcome and the dev list is probably the place to start that discussion. Alternatively, you can open a BZ enahncement request for a solution to the connection being closed in this case but, without a concrete proposal, the enhancement request is likely to make little/no progress. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56555] Multiple connection headers for status 400 when keep-alive is specified
https://issues.apache.org/bugzilla/show_bug.cgi?id=56555
Mark Thomas ma...@apache.org changed:
What|Removed |Added
OS||All
--- Comment #1 from Mark Thomas ma...@apache.org ---
(In reply to Brett from comment #0)
Should the code from line 1518 ('if
(!connectionClosePresent)') not also check for the presence of a Connection
header to avoid adding multiple conflicting entries, not just duplicate
entries?
Most likely yes, but the end result is still going to be a Connection: close
header for a 400 response code.
--
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56555] Multiple connection headers for status 400 when keep-alive is specified
https://issues.apache.org/bugzilla/show_bug.cgi?id=56555 --- Comment #2 from Brett lee.br...@gmail.com --- Thanks for the quick reply. So is it expected behavior, even in cases where Connection: keep-alive is set, for the container to force it to Connection: close? I would think that if the response header is already set by the application, that the container should honor it (unless maxKeepAlive is exceeded, or persistent connections are disabled). I haven't seen anything in the HTTP 1.1 spec saying that a status of 400 requires that the connection be closed. The only case where a server is required to close a connection is when the request includes Connection: close but other than that, it appears to be optional (with the default being persistent connections). Am I missing something? -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56555] Multiple connection headers for status 400 when keep-alive is specified
https://issues.apache.org/bugzilla/show_bug.cgi?id=56555 --- Comment #3 from Christopher Schultz ch...@christopherschultz.net --- (In reply to Brett from comment #0) Background/Expected Behavior: Our application is a RESTful web service, we return error responses with status code 400 in situations, like for example when a POST to access a request token contains a valid username but invalid password. Shouldn't this be 403? (In reply to Brett from comment #2) Thanks for the quick reply. So is it expected behavior, even in cases where Connection: keep-alive is set, for the container to force it to Connection: close? Not generally, but 400 Bad Request is kind of a killer. Tomcat defines a few deal-breaker status codes that will close the connection regardless of the wishes of the application. You can find them in the statusDropsConnection method called by the code you already posted. Here's the meat of the method: return status == 400 /* SC_BAD_REQUEST */ || status == 408 /* SC_REQUEST_TIMEOUT */ || status == 411 /* SC_LENGTH_REQUIRED */ || status == 413 /* SC_REQUEST_ENTITY_TOO_LARGE */ || status == 414 /* SC_REQUEST_URI_TOO_LONG */ || status == 500 /* SC_INTERNAL_SERVER_ERROR */ || status == 503 /* SC_SERVICE_UNAVAILABLE */ || status == 501 /* SC_NOT_IMPLEMENTED */; Basically, any of those status codes should close the connection. The code docs say that Apache httpd will do the same thing for those status codes (i.e. drop the connection), but I see no reference to any spec that requires such behavior. One could make the argument that the above list covers too much ground (e.g. NOT IMPLEMENTED or LENGTH_REQUIRED -- why kill the connection in those cases?) but for the case of 400 Bad Request, the server (application, actually) is saying that the request itself is garbled, indicating that something horrible has happened to the connection (or the client). Closing the connection is appropriate in this case. I think if you use a 403 response code (which really makes much more sense in the case presented above, IMO), then you should avoid this problem and go back to being able to use keepalives. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[Bug 56555] Multiple connection headers for status 400 when keep-alive is specified
https://issues.apache.org/bugzilla/show_bug.cgi?id=56555 --- Comment #4 from Brett lee.br...@gmail.com --- Thanks for the info, but the conflict here is the OAuth2 specification requires a status code 400 in this case. From http://tools.ietf.org/html/rfc6749#section-5.2 : The authorization server responds with an HTTP 400 (Bad Request) status code (unless specified otherwise)... and goes on to list the only exception as invalid_client wherein [t]he authorization server MAY return an HTTP 401 (Unauthorized) status code to indicate which HTTP authentication schemes are supported. Our particular error condition in this case is invalid_grant which according to the spec appears to get the default 400 status code. This is how we've implemented it, and this is how our clients are expecting it :/ I am puzzled because we can't be the only ones that have encountered this issue. However, I have not found anything in the last two days online where anyone has even brought this up as an issue before. -- You are receiving this mail because: You are the assignee for the bug. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
