You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by sc...@apache.org on 2015/02/19 18:45:35 UTC

svn commit: r1660953 - in /tomcat/tc8.0.x/trunk: ./ java/org/apache/catalina/connector/ java/org/apache/coyote/ajp/ java/org/apache/coyote/http11/ java/org/apache/tomcat/util/net/ java/org/apache/tomcat/util/net/jsse/ webapps/docs/

Author: schultz
Date: Thu Feb 19 17:45:34 2015
New Revision: 1660953

URL: http://svn.apache.org/r1660953
Log:
Back-port r1660924 to fix https://bz.apache.org/bugzilla/show_bug.cgi?id=57540
Expose TLS protocol via a request attribute.

Modified:
    tomcat/tc8.0.x/trunk/   (props changed)
    tomcat/tc8.0.x/trunk/java/org/apache/catalina/connector/Request.java
    tomcat/tc8.0.x/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java
    tomcat/tc8.0.x/trunk/java/org/apache/coyote/ajp/Constants.java
    tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java
    tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java
    tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11Processor.java
    tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/net/SSLSupport.java
    tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/net/jsse/JSSESupport.java
    tomcat/tc8.0.x/trunk/webapps/docs/changelog.xml

Propchange: tomcat/tc8.0.x/trunk/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Thu Feb 19 17:45:34 2015
@@ -1 +1 @@
-/tomcat/trunk:1636524,1637156,1637176,1637188,1637331,1637684,1637695,1638720-1638725,1639653,1640010,1640083-1640084,1640088,1640275,1640322,1640347,1640361,1640365,1640403,1640410,1640652,1640655-1640658,1640688,1640700-1640883,1640903,1640976,1640978,1641000,1641026,1641038-1641039,1641051-1641052,1641058,1641064,1641300,1641369,1641374,1641380,1641486,1641634,1641656-1641692,1641704,1641707-1641718,1641720-1641722,1641735,1641981,1642233,1642280,1642554,1642564,1642595,1642606,1642668,1642679,1642697,1642699,1642766,1643002,1643045,1643054-1643055,1643066,1643121,1643128,1643206,1643209-1643210,1643216,1643249,1643270,1643283,1643309-1643310,1643323,1643365-1643366,1643370-1643371,1643465,1643474,1643536,1643570,1643634,1643649,1643651,1643654,1643675,1643731,1643733-1643734,1643761,1643766,1643814,1643937,1643963,1644017,1644169,1644201-1644203,1644321,1644323,1644516,1644523,1644529,1644535,1644730,1644768,1644784-1644785,1644790,1644793,1644815,1644884,1644886,1644890,1644892
 ,1644910,1644924,1644929-1644930,1644935,1644989,1645011,1645247,1645355,1645357-1645358,1645455,1645465,1645469,1645471,1645473,1645475,1645486-1645488,1645626,1645641,1645685,1645743,1645763,1645951-1645953,1645955,1645993,1646098-1646106,1646178,1646220,1646302,1646304,1646420,1646470-1646471,1646476,1646559,1646717-1646723,1646773,1647026,1647042,1647530,1647655,1648304,1648815,1648907,1650081,1650365,1651116,1651120,1651280,1651470,1652938,1652970,1653041,1653471,1653550,1653574,1653797,1653815-1653816,1653819,1653840,1653857,1653888,1653972,1654013,1654030,1654050,1654123,1654148,1654159,1654513,1654515,1654517,1654522,1654524,1654725,1654735,1654766,1654785,1654851-1654852,1654978,1655122-1655124,1655126-1655127,1655129-1655130,1655132-1655133,1655312,1655438,1655441,1655454,1655558,1656087,1656299,1656319,1656331,1656345,1656350,1656590,1656648-1656650,1656657,1657041,1657054,1657374,1657492,1657510,1657565,1657580,1657584,1657586,1657589,1657592,1657607,1657609,1657682,1657
 907,1658207,1658734,1658781,1658790,1658799,1658802,1658804,1658833,1658840,1658966,1659043,1659053,1659059,1659188-1659189,1659216,1659263,1659293,1659304,1659306-1659307,1659382,1659384,1659428,1659471,1659486,1659505,1659516,1659521,1659524,1659559,1659562,1659803,1659806,1659814,1659833,1659862,1659919,1659967,1659983-1659984,1660060,1660074,1660077,1660133,1660168,1660331-1660332,1660353,1660358
+/tomcat/trunk:1636524,1637156,1637176,1637188,1637331,1637684,1637695,1638720-1638725,1639653,1640010,1640083-1640084,1640088,1640275,1640322,1640347,1640361,1640365,1640403,1640410,1640652,1640655-1640658,1640688,1640700-1640883,1640903,1640976,1640978,1641000,1641026,1641038-1641039,1641051-1641052,1641058,1641064,1641300,1641369,1641374,1641380,1641486,1641634,1641656-1641692,1641704,1641707-1641718,1641720-1641722,1641735,1641981,1642233,1642280,1642554,1642564,1642595,1642606,1642668,1642679,1642697,1642699,1642766,1643002,1643045,1643054-1643055,1643066,1643121,1643128,1643206,1643209-1643210,1643216,1643249,1643270,1643283,1643309-1643310,1643323,1643365-1643366,1643370-1643371,1643465,1643474,1643536,1643570,1643634,1643649,1643651,1643654,1643675,1643731,1643733-1643734,1643761,1643766,1643814,1643937,1643963,1644017,1644169,1644201-1644203,1644321,1644323,1644516,1644523,1644529,1644535,1644730,1644768,1644784-1644785,1644790,1644793,1644815,1644884,1644886,1644890,1644892
 ,1644910,1644924,1644929-1644930,1644935,1644989,1645011,1645247,1645355,1645357-1645358,1645455,1645465,1645469,1645471,1645473,1645475,1645486-1645488,1645626,1645641,1645685,1645743,1645763,1645951-1645953,1645955,1645993,1646098-1646106,1646178,1646220,1646302,1646304,1646420,1646470-1646471,1646476,1646559,1646717-1646723,1646773,1647026,1647042,1647530,1647655,1648304,1648815,1648907,1650081,1650365,1651116,1651120,1651280,1651470,1652938,1652970,1653041,1653471,1653550,1653574,1653797,1653815-1653816,1653819,1653840,1653857,1653888,1653972,1654013,1654030,1654050,1654123,1654148,1654159,1654513,1654515,1654517,1654522,1654524,1654725,1654735,1654766,1654785,1654851-1654852,1654978,1655122-1655124,1655126-1655127,1655129-1655130,1655132-1655133,1655312,1655438,1655441,1655454,1655558,1656087,1656299,1656319,1656331,1656345,1656350,1656590,1656648-1656650,1656657,1657041,1657054,1657374,1657492,1657510,1657565,1657580,1657584,1657586,1657589,1657592,1657607,1657609,1657682,1657
 907,1658207,1658734,1658781,1658790,1658799,1658802,1658804,1658833,1658840,1658966,1659043,1659053,1659059,1659188-1659189,1659216,1659263,1659293,1659304,1659306-1659307,1659382,1659384,1659428,1659471,1659486,1659505,1659516,1659521,1659524,1659559,1659562,1659803,1659806,1659814,1659833,1659862,1659919,1659967,1659983-1659984,1660060,1660074,1660077,1660133,1660168,1660331-1660332,1660353,1660358,1660924

Modified: tomcat/tc8.0.x/trunk/java/org/apache/catalina/connector/Request.java
URL: http://svn.apache.org/viewvc/tomcat/tc8.0.x/trunk/java/org/apache/catalina/connector/Request.java?rev=1660953&r1=1660952&r2=1660953&view=diff
==============================================================================
--- tomcat/tc8.0.x/trunk/java/org/apache/catalina/connector/Request.java (original)
+++ tomcat/tc8.0.x/trunk/java/org/apache/catalina/connector/Request.java Thu Feb 19 17:45:34 2015
@@ -98,6 +98,7 @@ import org.apache.tomcat.util.http.fileu
 import org.apache.tomcat.util.http.fileupload.servlet.ServletFileUpload;
 import org.apache.tomcat.util.http.fileupload.servlet.ServletRequestContext;
 import org.apache.tomcat.util.http.parser.AcceptLanguage;
+import org.apache.tomcat.util.net.SSLSupport;
 import org.apache.tomcat.util.res.StringManager;
 import org.ietf.jgss.GSSCredential;
 import org.ietf.jgss.GSSException;
@@ -882,7 +883,7 @@ public class Request
         if(attr != null) {
             return attr;
         }
-        if( isSSLAttribute(name) ) {
+        if( isSSLAttribute(name) || name.equals(SSLSupport.PROTOCOL_VERSION_KEY)) {
             coyoteRequest.action(ActionCode.REQ_SSL_ATTRIBUTE,
                                  coyoteRequest);
             attr = coyoteRequest.getAttribute(Globals.CERTIFICATES_ATTR);
@@ -905,6 +906,10 @@ public class Request
             if(attr != null) {
                 attributes.put(Globals.SSL_SESSION_MGR_ATTR, attr);
             }
+            attr = coyoteRequest.getAttribute(SSLSupport.PROTOCOL_VERSION_KEY);
+            if(attr != null) {
+                attributes.put(SSLSupport.PROTOCOL_VERSION_KEY, attr);
+            }
             attr = attributes.get(name);
             sslAttributesParsed = true;
         }

Modified: tomcat/tc8.0.x/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java
URL: http://svn.apache.org/viewvc/tomcat/tc8.0.x/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java?rev=1660953&r1=1660952&r2=1660953&view=diff
==============================================================================
--- tomcat/tc8.0.x/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java (original)
+++ tomcat/tc8.0.x/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java Thu Feb 19 17:45:34 2015
@@ -1249,6 +1249,8 @@ public abstract class AbstractAjpProcess
                     } catch (NumberFormatException nfe) {
                         // Ignore invalid value
                     }
+                } else if(n.equals(Constants.SC_A_SSL_PROTOCOL)) {
+                    request.setAttribute(SSLSupport.PROTOCOL_VERSION_KEY, v);
                 } else {
                     request.setAttribute(n, v );
                 }

Modified: tomcat/tc8.0.x/trunk/java/org/apache/coyote/ajp/Constants.java
URL: http://svn.apache.org/viewvc/tomcat/tc8.0.x/trunk/java/org/apache/coyote/ajp/Constants.java?rev=1660953&r1=1660952&r2=1660953&view=diff
==============================================================================
--- tomcat/tc8.0.x/trunk/java/org/apache/coyote/ajp/Constants.java (original)
+++ tomcat/tc8.0.x/trunk/java/org/apache/coyote/ajp/Constants.java Thu Feb 19 17:45:34 2015
@@ -83,6 +83,7 @@ public final class Constants {
      */
     public static final String SC_A_REQ_LOCAL_ADDR  = "AJP_LOCAL_ADDR";
     public static final String SC_A_REQ_REMOTE_PORT = "AJP_REMOTE_PORT";
+    public static final String SC_A_SSL_PROTOCOL    = "AJP_SSL_PROTOCOL";
 
     // Terminates list of attributes
     public static final byte SC_A_ARE_DONE      = (byte)0xFF;

Modified: tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java
URL: http://svn.apache.org/viewvc/tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java?rev=1660953&r1=1660952&r2=1660953&view=diff
==============================================================================
--- tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java (original)
+++ tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java Thu Feb 19 17:45:34 2015
@@ -409,6 +409,11 @@ public class Http11AprProcessor extends
                     if (sslO != null) {
                         request.setAttribute(SSLSupport.SESSION_ID_KEY, sslO);
                     }
+                    sslO = SSLSocket.getInfoS(socketRef, SSL.SSL_INFO_PROTOCOL);
+                    if (sslO != null) {
+                        request.setAttribute
+                        (SSLSupport.PROTOCOL_VERSION_KEY, sslO);
+                    }
                     //TODO provide a hook to enable the SSL session to be
                     // invalidated. Set AprEndpoint.SESSION_MGR req attr
                 } catch (Exception e) {

Modified: tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java
URL: http://svn.apache.org/viewvc/tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java?rev=1660953&r1=1660952&r2=1660953&view=diff
==============================================================================
--- tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java (original)
+++ tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java Thu Feb 19 17:45:34 2015
@@ -425,6 +425,11 @@ public class Http11NioProcessor extends
                         request.setAttribute
                             (SSLSupport.SESSION_ID_KEY, sslO);
                     }
+                    sslO = sslSupport.getProtocol();
+                    if (sslO != null) {
+                        request.setAttribute
+                        (SSLSupport.PROTOCOL_VERSION_KEY, sslO);
+                    }
                     request.setAttribute(SSLSupport.SESSION_MGR, sslSupport);
                 }
             } catch (Exception e) {

Modified: tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11Processor.java
URL: http://svn.apache.org/viewvc/tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11Processor.java?rev=1660953&r1=1660952&r2=1660953&view=diff
==============================================================================
--- tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11Processor.java (original)
+++ tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11Processor.java Thu Feb 19 17:45:34 2015
@@ -256,6 +256,11 @@ public class Http11Processor extends Abs
                     if (sslO != null)
                         request.setAttribute
                             (SSLSupport.SESSION_ID_KEY, sslO);
+                    sslO = sslSupport.getProtocol();
+                    if (sslO != null) {
+                        request.setAttribute
+                            (SSLSupport.PROTOCOL_VERSION_KEY, sslO);
+                    }
                     request.setAttribute(SSLSupport.SESSION_MGR, sslSupport);
                 }
             } catch (Exception e) {

Modified: tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/net/SSLSupport.java
URL: http://svn.apache.org/viewvc/tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/net/SSLSupport.java?rev=1660953&r1=1660952&r2=1660953&view=diff
==============================================================================
--- tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/net/SSLSupport.java (original)
+++ tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/net/SSLSupport.java Thu Feb 19 17:45:34 2015
@@ -55,6 +55,12 @@ public interface SSLSupport {
     public static final String SESSION_MGR =
             "javax.servlet.request.ssl_session_mgr";
 
+    /**
+     * The request attribute key under which the String indicating the protocol
+     * that created the SSL socket is recorded - e.g. TLSv1 or TLSv1.2 etc.
+     */
+    public static final String PROTOCOL_VERSION_KEY =
+            "org.apache.tomcat.util.net.secure_protocol_version";
 
     /**
      * The cipher suite being used on this connection.
@@ -91,5 +97,11 @@ public interface SSLSupport {
      * The current session Id.
      */
     public String getSessionId() throws IOException;
+
+    /**
+     * @return the protocol String indicating how the SSL socket was created
+     *  e.g. TLSv1 or TLSv1.2 etc.
+     */
+    public String getProtocol() throws IOException;
 }
 

Modified: tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/net/jsse/JSSESupport.java
URL: http://svn.apache.org/viewvc/tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/net/jsse/JSSESupport.java?rev=1660953&r1=1660952&r2=1660953&view=diff
==============================================================================
--- tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/net/jsse/JSSESupport.java (original)
+++ tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/net/jsse/JSSESupport.java Thu Feb 19 17:45:34 2015
@@ -270,5 +270,13 @@ class JSSESupport implements SSLSupport,
     public void invalidateSession() {
         session.invalidate();
     }
+
+    @Override
+    public String getProtocol() throws IOException {
+        if (session == null) {
+           return null;
+        }
+       return session.getProtocol();
+   }
 }
 

Modified: tomcat/tc8.0.x/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/tc8.0.x/trunk/webapps/docs/changelog.xml?rev=1660953&r1=1660952&r2=1660953&view=diff
==============================================================================
--- tomcat/tc8.0.x/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/tc8.0.x/trunk/webapps/docs/changelog.xml Thu Feb 19 17:45:34 2015
@@ -45,6 +45,20 @@
   issues to not "pop up" wrt. others).
 -->
 <section name="Tomcat 8.0.21 (markt)" rtext="in development">
+  <subsection name="Coyote">
+    <changelog>
+      <add>
+        <bug>57540</bug>: Make TLS/SSL protocol available in a new request
+        attribute
+        (<code>org.apache.tomcat.util.net.secure_protocol_version</code>).
+        (Note that AJP connectors will require <tt>mod_jk</tt> 1.2.41 or later,
+        or an as-yet-unknown version of mod_proxy_ajp, or configure the proxy
+        to send the AJP_SSL_PROTOCOL request attribute to Tomcat. Please see
+        the bug comments for details.)
+        Based upon a patch provided by Ralf Hauser. (schultz)
+      </add>
+    </changelog>
+  </subsection>
   <subsection name="Jasper">
     <changelog>
       <fix>



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


Re: svn commit: r1660953 - in /tomcat/tc8.0.x/trunk: ./ java/org/apache/catalina/connector/ java/org/apache/coyote/ajp/ java/org/apache/coyote/http11/ java/org/apache/tomcat/util/net/ java/org/apache/tomcat/util/net/jsse/ webapps/docs/

Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,

On 2/24/15 12:59 PM, Mark Thomas wrote:
> On 19/02/2015 19:31, Christopher Schultz wrote:
>> Mark,
>>
>> On 2/19/15 1:41 PM, Mark Thomas wrote:
>>> On 19/02/2015 17:45, schultz@apache.org wrote:
>>>> Author: schultz
>>>> Date: Thu Feb 19 17:45:34 2015
>>>> New Revision: 1660953
>>>>
>>>> URL: http://svn.apache.org/r1660953
>>>> Log:
>>>> Back-port r1660924 to fix https://bz.apache.org/bugzilla/show_bug.cgi?id=57540
>>>> Expose TLS protocol via a request attribute.
>>>
>>> Sorry for the delayed feedback.
>>>
>>> <snip/>
>>>
>>>> @@ -882,7 +883,7 @@ public class Request
>>>>          if(attr != null) {
>>>>              return attr;
>>>>          }
>>>> -        if( isSSLAttribute(name) ) {
>>>> +        if( isSSLAttribute(name) || name.equals(SSLSupport.PROTOCOL_VERSION_KEY)) {
>>>
>>> This should be part of the isSSLAttribute() test. I'd it to Globals to
>>> for consistency with the other attributes.
>>
>> I avoided adding it to isSSLAttribute because of this Javadoc for that
>> method:
>>
>>     /**
>>      * Test if a given name is one of the special Servlet-spec SSL
>> attributes.
>>      */
>>
>> Since this isn't in the spec, I kept is separate. I think it's a
>> reasonable thing to add to the spec: any chance you'd be willing to
>> bring it to their attention?
> 
> If you raise the Jira I can comment positively on it. Better for two
> people to request it than one.

Created:
https://java.net/jira/browse/SERVLET_SPEC-130

Comments and votes appreciated.

Thanks,
-chris


Re: svn commit: r1660953 - in /tomcat/tc8.0.x/trunk: ./ java/org/apache/catalina/connector/ java/org/apache/coyote/ajp/ java/org/apache/coyote/http11/ java/org/apache/tomcat/util/net/ java/org/apache/tomcat/util/net/jsse/ webapps/docs/

Posted by Mark Thomas <ma...@apache.org>.
On 19/02/2015 19:31, Christopher Schultz wrote:
> Mark,
> 
> On 2/19/15 1:41 PM, Mark Thomas wrote:
>> On 19/02/2015 17:45, schultz@apache.org wrote:
>>> Author: schultz
>>> Date: Thu Feb 19 17:45:34 2015
>>> New Revision: 1660953
>>>
>>> URL: http://svn.apache.org/r1660953
>>> Log:
>>> Back-port r1660924 to fix https://bz.apache.org/bugzilla/show_bug.cgi?id=57540
>>> Expose TLS protocol via a request attribute.
>>
>> Sorry for the delayed feedback.
>>
>> <snip/>
>>
>>> @@ -882,7 +883,7 @@ public class Request
>>>          if(attr != null) {
>>>              return attr;
>>>          }
>>> -        if( isSSLAttribute(name) ) {
>>> +        if( isSSLAttribute(name) || name.equals(SSLSupport.PROTOCOL_VERSION_KEY)) {
>>
>> This should be part of the isSSLAttribute() test. I'd it to Globals to
>> for consistency with the other attributes.
> 
> I avoided adding it to isSSLAttribute because of this Javadoc for that
> method:
> 
>     /**
>      * Test if a given name is one of the special Servlet-spec SSL
> attributes.
>      */
> 
> Since this isn't in the spec, I kept is separate. I think it's a
> reasonable thing to add to the spec: any chance you'd be willing to
> bring it to their attention?

If you raise the Jira I can comment positively on it. Better for two
people to request it than one.

My commendation is to change the comment to Servlet spec + Tomcat
specific SSL attributes.

>> Hmm. That does mean we end up with multiple definitions. That seems wrong.
>>
>> I haven't checked how reasonable the following is...
>> How about for trunk drop the Globals constants and use the ones from
>> SSLSupport. For the back-port, deprecate the the Globals ones and define
>> them in terms of SSLSupport.
> 
> SSLSupport has varying levels of, er, support, from Tomcat 7 onward.
> Your work with refactoring everything in trunk was great.. back-porting
> required duplications of lots of that stuff -- feel free to look at the
> diffs I committed.
> 
> I'll wait for it to roll-around in your head a bit more; I trust your
> architectural view and I'm not sure my opinion would be well-informed.
> Once you decide, I can make any changes required.

We definitely should only be defining constants once as far as possible.
On that basis my suggestion above stands but if you can see a better way
go for it.

Mark


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


Re: svn commit: r1660953 - in /tomcat/tc8.0.x/trunk: ./ java/org/apache/catalina/connector/ java/org/apache/coyote/ajp/ java/org/apache/coyote/http11/ java/org/apache/tomcat/util/net/ java/org/apache/tomcat/util/net/jsse/ webapps/docs/

Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,

On 2/19/15 1:41 PM, Mark Thomas wrote:
> On 19/02/2015 17:45, schultz@apache.org wrote:
>> Author: schultz
>> Date: Thu Feb 19 17:45:34 2015
>> New Revision: 1660953
>>
>> URL: http://svn.apache.org/r1660953
>> Log:
>> Back-port r1660924 to fix https://bz.apache.org/bugzilla/show_bug.cgi?id=57540
>> Expose TLS protocol via a request attribute.
> 
> Sorry for the delayed feedback.
> 
> <snip/>
> 
>> @@ -882,7 +883,7 @@ public class Request
>>          if(attr != null) {
>>              return attr;
>>          }
>> -        if( isSSLAttribute(name) ) {
>> +        if( isSSLAttribute(name) || name.equals(SSLSupport.PROTOCOL_VERSION_KEY)) {
> 
> This should be part of the isSSLAttribute() test. I'd it to Globals to
> for consistency with the other attributes.

I avoided adding it to isSSLAttribute because of this Javadoc for that
method:

    /**
     * Test if a given name is one of the special Servlet-spec SSL
attributes.
     */

Since this isn't in the spec, I kept is separate. I think it's a
reasonable thing to add to the spec: any chance you'd be willing to
bring it to their attention?

> Hmm. That does mean we end up with multiple definitions. That seems wrong.
> 
> I haven't checked how reasonable the following is...
> How about for trunk drop the Globals constants and use the ones from
> SSLSupport. For the back-port, deprecate the the Globals ones and define
> them in terms of SSLSupport.

SSLSupport has varying levels of, er, support, from Tomcat 7 onward.
Your work with refactoring everything in trunk was great.. back-porting
required duplications of lots of that stuff -- feel free to look at the
diffs I committed.

I'll wait for it to roll-around in your head a bit more; I trust your
architectural view and I'm not sure my opinion would be well-informed.
Once you decide, I can make any changes required.

Thanks,
-chris


Re: svn commit: r1660953 - in /tomcat/tc8.0.x/trunk: ./ java/org/apache/catalina/connector/ java/org/apache/coyote/ajp/ java/org/apache/coyote/http11/ java/org/apache/tomcat/util/net/ java/org/apache/tomcat/util/net/jsse/ webapps/docs/

Posted by Mark Thomas <ma...@apache.org>.
On 19/02/2015 17:45, schultz@apache.org wrote:
> Author: schultz
> Date: Thu Feb 19 17:45:34 2015
> New Revision: 1660953
> 
> URL: http://svn.apache.org/r1660953
> Log:
> Back-port r1660924 to fix https://bz.apache.org/bugzilla/show_bug.cgi?id=57540
> Expose TLS protocol via a request attribute.

Sorry for the delayed feedback.

<snip/>

> @@ -882,7 +883,7 @@ public class Request
>          if(attr != null) {
>              return attr;
>          }
> -        if( isSSLAttribute(name) ) {
> +        if( isSSLAttribute(name) || name.equals(SSLSupport.PROTOCOL_VERSION_KEY)) {

This should be part of the isSSLAttribute() test. I'd it to Globals to
for consistency with the other attributes.

Hmm. That does mean we end up with multiple definitions. That seems wrong.

I haven't checked how reasonable the following is...
How about for trunk drop the Globals constants and use the ones from
SSLSupport. For the back-port, deprecate the the Globals ones and define
them in terms of SSLSupport.

Mark

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