On 01/03/2011 00:16, Filip Hanik - Dev Lists wrote: > On 2/28/2011 4:49 PM, Mark Thomas wrote: >> It isn't clear to me if you are voting -1 > on the above commit, and the following commits. r1074675
Understood and agree those commits are broken. I'll get those backed out shortly. > If you wish to do this, it should at least include: > 1. input filters need to check if they retrieved the entire body > if only partial, why even attempt a reneg and make your thread hang for > soTimeout while it fails. this is another DoS scenario. the system knows > if it read the entire body or not. it's part of the protocol itself, no > need to rely on timeouts for a reneg to fail. > > 2. don't change the names of all the flags, since it makes the diffs so > much harder to review. just change the lines pertinent to the change. > > 3. implement rehandshake as simple as possible, by using the > handshake(...) and using its return code > > 4. SSLAuthenticator should have a flag to fail directly without trying > to reneg if the connector is misconfigured to avoid reneg for clients > vulnerable to the man in the middle reneg attack > > 5. SSLAuthenticator should be able to find out if the cert truly was > client-auth or if it came from another source. otherwise, putting > httpd/mod_jk in front of it, and I can bypass client-auth as the > document states is required > > 6. And if you want the most performant solution, instead of opening a > selector on the same thread, just call sslEngine.beginHandshake, add the > connection to the poller, and return from the call all together. this > way, the worker thread is not in use during a handshake, and it's done > in the poller just like the initial hand shake. this protects you from > slow clients using up threads. this is of course more complicated, so I > would not expect it in the first iteration. > > I would say the other connectors would benefit from improvements in > 1,4,5 as well. I agree on all of those points (with a few questions - see below). My current thinking is approaching it in this order. Do 2 in a separate commit. The flag needs to be renamed to ease confusion but a separate commit that does just that should be easy to review. Address 3 for the NIO connector. That will bring it in line with BIO and APR. Fix 1 for all connectors. I don't understand what you mean in point 4. Could you try and expand on that. Fix 5. I may re-word the Javadoc again. Doing the client cert validation in httpd is valid. 6 is definitely more complicated. I did try this before but gave up. That was before I had anything working. It may well be easier to get there from a working solution. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
