On 28/02/2011 22:50, Filip Hanik - Dev Lists wrote: > Ok, I've done some research. My conclusion is that I'm -1 for this change. > My suggestion on the other thread, that we should simply throw an > exception if the server is not configured to handle client-auth in the > connector still stands. > > Let me explain why: > > 1. The idea behind this code was that if the client is using SSL, the > SSL request must have been authenticated against the trust store, > meaning that the initial handshake did this. > 2. If the server was not configured for client auth [1], the current > implementation wants to do a renegotiation. > 3. The current implementation tries to swallow up as much body content > as possible, until maxPostSize has been filled up. > > This breaks in the following use case: > 1. Client Opens TCP Session > 2. Client Completes Initial Handshake > 3. Client Sends HTTP Headers and Body of Size N, where N>maxPostSize > > When the server in this case has read the HTTP headers, it's already too > late to renegotiate, since the body is already underway. The only time > we could renegotiate, would be after the request had been read in its > entirety. (There is a short window where this could happen, if the > client had sent a 100-Continue header before the body was even started, > but I'm excluding this use case for now.) > > Since we can't read the entire body without discarding it, we can't > implement this as the way it has been. Hence the -1.
It isn't clear to me if you are voting -1 on the recent NIO change or on how the BIO and APR connectors have implemented SSL renegotiation for years. It is an accepted limitation of SSL renegotiation that you need to buffer any request body and if the request body is too large renegotiation will fail. httpd does this exactly the same way. [1] The rare user for which the above is an issue has several workarounds available: 1. Increase maxSavePostSize (also increases risk of DOS but may be acceptable in some circumstances). 2. Set clientAuth="true" on the connector as you suggest below. > Here is the proposed solution: > 1. When SSLAuthenticator starts up, it should inspect configuration of > the connector to make sure there is at least one connector with > clientAuth="true" in it > Note: You need to decide what to do if there is only an AJP connector, > do you trust that the webserver did client-auth on your behalf? > > 2. If a request comes in, and the system is not configured for > client-auth, all a server can do is send a 400 Bad Request (or Not > Authorized) back. > Note: It should also send a Connection:close so that we can close the > connection without having to read the body. (See Rainer's email about > draining implications) > > 3. We should not implement server initiated renegotiation at this point > in time, since we can't do it in a way it would allow for the system to > handle the above use case. > > 4. We should not implement renegotiation period on the NIO connector at > this time, since it would still risk a vulnerability with clients that > are not updated. Such an approach would be a step backwards from what has been available for BIO as far back as Tomcat 4 [2] and has been available for APR in 6.0.21 and 5.5.29 [3]. If the issue is that the current NIO implementation is broken, I agree but I think it is fixable within the same limitations of the current renegotiation support for BIO and APR. Given that the current patch is heading in the right direction (hopefully) and that a fix shouldn't be too hard, I'd rather leave it in and fix it rather than pull it out. There is no huge rush at the moment for a 7.0.x release but I'd like to get the 7.0.10 started by the end of this week. If NIO renegotiation gets in the way then it can be reverted / commented out for now but I am hopeful of getting this to a working state this week. With the pointers you have provided on the current code, I think I can see how to get to a working (in the same way as BIO & APR) solution. It is actually taking longer to get the test cases working. If the issue is that you are -1 for the approach of buffering the request body prior to SSL renegotiation then I think that needs a wider discussion before any changes are made since that would involving removing functionality from the current connectors that is currently being used successfully by the user community. Mark [1] http://httpd.apache.org/docs/2.2/mod/mod_ssl.html#sslrenegbuffersize [2] http://svn.apache.org/viewvc/tomcat/archive/tc4.1.x/trunk/connectors/http11/src/java/org/apache/coyote/http11/Http11Processor.java?view=annotate (lines 1118 to 1138) [3] https://issues.apache.org/bugzilla/show_bug.cgi?id=46950 --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
