On 07/11/2012 06:38 AM, Amos Jeffries wrote: > * why is is FATAL: error to have old ssl_bump allow/deny values in the > config? > can we auto-convert the "allow" to client-first and deny to bump-none > with very loud ERROR: messages instead? we can calculate what the > implicit negate would have been and add it explicitly with another loud > warning.
I have struggled with this decision. Yes, we could auto-convert old configs (using client-first for the implicit allow rule, if any), but then some folks will start using a mixture of old and new keywords or would expect a simple 's/allow/server-first/;s/deny/none/' to work. I think it is safer to force a manual intervention here, especially since SslBump config should not be taken lightly, and client-first is really not the best default. If the above arguments did not convince you, or others insist on auto-conversion, we will add it. We would still reject a mixture of old and new keywords though. Do you still think auto-conversion is the best approach here? > * Why are error pages delayed until after bumping? Because many browsers do not display error responses to CONNECT requests (browser developers do not want to implement a special "proxy" context that securely handling such responses requires). > have you investigated > how this interacts with deny_info redirection to 4xx and 5xx pages? > (particularly 403 and 511 "web login required" splash pages) I do not think we have, but perhaps those who requested this feature have. I will ask around, and we will "investigate" if nobody did. > src/url.cc: > * this breaks the canonical URI "cache" when URL parsing is changing the > request URL pieces. Please revert. It does not. The cache is now invalidated in HttpRequest::setHost(), a call to which follows the removed explicit cache invalidation line. The whole canonical cache API is bad, of course, but we are not trying to fix it in this project. I can revert if you insist, but it will just result in us trying to free the canonical cache twice. The other comments probably require changes/development so I will address them with a new version of the patch. Thank you, Alex.
