On 12/08/2014 12:15, Mark Thomas wrote:
> On 12/08/2014 11:56, Konstantin Kolinko wrote:
>> 2014-08-12 14:41 GMT+04:00  <ma...@apache.org>:
>>> Author: markt
>>> Date: Tue Aug 12 10:41:49 2014
>>> New Revision: 1617445
>>>
>>> URL: http://svn.apache.org/r1617445
>>> Log:
>>> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=56825
>>> Enable pre-emptive authentication to work with the SSL authenticator. Based 
>>> on a patch by jlmonteiro.
>>>
>>> Modified:
>>>     
>>> tomcat/trunk/java/org/apache/catalina/authenticator/AuthenticatorBase.java
>>>     
>>> tomcat/trunk/java/org/apache/catalina/authenticator/SSLAuthenticator.java
>>>     tomcat/trunk/test/org/apache/tomcat/util/net/TestClientCert.java
>>>     tomcat/trunk/test/org/apache/tomcat/util/net/TesterSupport.java
>>>     tomcat/trunk/webapps/docs/changelog.xml
>>>
>>> Modified: 
>>> tomcat/trunk/java/org/apache/catalina/authenticator/AuthenticatorBase.java
>>> URL: 
>>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/authenticator/AuthenticatorBase.java?rev=1617445&r1=1617444&r2=1617445&view=diff
>>> ==============================================================================
>>> --- 
>>> tomcat/trunk/java/org/apache/catalina/authenticator/AuthenticatorBase.java 
>>> (original)
>>> +++ 
>>> tomcat/trunk/java/org/apache/catalina/authenticator/AuthenticatorBase.java 
>>> Tue Aug 12 10:41:49 2014
>>> @@ -41,6 +41,7 @@ import org.apache.catalina.connector.Req
>>>  import org.apache.catalina.connector.Response;
>>>  import org.apache.catalina.util.SessionIdGenerator;
>>>  import org.apache.catalina.valves.ValveBase;
>>> +import org.apache.coyote.ActionCode;
>>>  import org.apache.juli.logging.Log;
>>>  import org.apache.juli.logging.LogFactory;
>>>  import org.apache.tomcat.util.descriptor.web.LoginConfig;
>>> @@ -566,8 +567,7 @@ public abstract class AuthenticatorBase
>>>          }
>>>
>>>          if (!authRequired && context.getPreemptiveAuthentication()) {
>>> -            X509Certificate[] certs = (X509Certificate[]) 
>>> request.getAttribute(
>>> -                    Globals.CERTIFICATES_ATTR);
>>> +            X509Certificate[] certs = getRequestCertificates(request);
>>>              authRequired = certs != null && certs.length > 0;
>>>          }
>>>
>>> @@ -619,6 +619,35 @@ public abstract class AuthenticatorBase
>>>
>>>      // ------------------------------------------------------ Protected 
>>> Methods
>>>
>>> +    /**
>>> +     * Look for the X509 certificate chain in the Request under the key
>>> +     * <code>javax.servlet.request.X509Certificate</code>. If not found, 
>>> trigger
>>> +     * extracting the certificate chain from the Coyote request.
>>> +     *
>>> +     * @param request   Request to be processed
>>> +     *
>>> +     * @return          The X509 certificate chain if found, 
>>> <code>null</code>
>>> +     *                  otherwise.
>>> +     */
>>> +    protected X509Certificate[] getRequestCertificates(final Request 
>>> request)
>>> +            throws IllegalStateException {
>>> +
>>> +        X509Certificate certs[] =
>>> +                (X509Certificate[]) 
>>> request.getAttribute(Globals.CERTIFICATES_ATTR);
>>> +
>>> +        if ((certs == null) || (certs.length < 1)) {
>>> +            try {
>>> +                request.getCoyoteRequest().action 
>>> (ActionCode.REQ_SSL_CERTIFICATE, null);
>>
>> This ActionCode causes re-regotiation of HTTPS.
> 
> Ah. I didn't spot that. I need to look more closely at exactly what is
> going on where.
> 
>> It is understandable in case if webapp expects a certificate (in
>> SSLAuthenticator),  but in AuthenticatorBase this is generic code.
> 
> Indeed.
> 
>> Will it (erroneously) trigger re-authentication for webapps that do
>> not need it, e.g. protected by BASIC authentication?
> 
> Let me work through some examples. This might need changing / removing.
> 
>> I also wonder whether there is some check if renegotiation has already
>> been attempted once on this connection.
> 
> I'm not sure. Logically, there must be something stopping re-negotiation
> every time the certs are requested for an authenticated resource but I
> need look at the code in a bit more depth.
> 
> The other thing to keep in mind is that setting clientAuth="want" is
> probably the better way to handle this use case.
> 
> Thanks for the review.

Should be fixed now.

Mark


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Reply via email to