Bug 57540 - report TLS protocol version
Summary: report TLS protocol version
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Connectors (show other bugs)
Version: trunk
Hardware: All Linux
: P2 enhancement (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
Depends on:
Reported: 2015-02-05 11:25 UTC by Ralf Hauser
Modified: 2015-02-24 18:36 UTC (History)
1 user (show)

apache.tomcat.tls.protocol.57540.patch (3.25 KB, patch)
2015-02-05 20:24 UTC, Ralf Hauser
Details | Diff
apache.tomcat.tls.protocol.57540c9.patch (2.88 KB, patch)
2015-02-17 15:22 UTC, Ralf Hauser
Details | Diff
Updated patch (4.59 KB, patch)
2015-02-17 20:51 UTC, Christopher Schultz
Details | Diff
Updated patch (5.79 KB, patch)
2015-02-18 14:31 UTC, Christopher Schultz
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ralf Hauser 2015-02-05 11:25:22 UTC
There is org.apache.tomcat.util.net.SSLSupport.CIPHER_SUITE_KEY and with 

request.getAttribute(SSLSupport.CIPHER_SUITE_KEY) , one can find out with cipher suite has been used between the client and the tomcat.

However, it doesn't seem possible to do the same on the TLS version, therefore my suggestions:

1) in SSLSuport create
  a) public static final String PROTOCOL_VERSION_KEY =
  b) public String getProtocol() throws IOException;

2) in org.apache.tomcat.util.net.jsse.JSSESupport add

   public String getProtocol() throws IOException {
         if (session == null)
            return null;
        return session.getProtocol();


3) in  org.apache.coyote.http11.Http11Processor.action() add

                    sslO = sslSupport.getProtocol();
                    if (sslO != null) {
                            (SSLSupport.PROTOCOL_VERSION_KEY, sslO);
Comment 1 Ralf Hauser 2015-02-05 11:40:04 UTC
4) furthermore, I suggest to add in org.apache.tomcat.util.net.jsse.openssl.Protocol

Comment 2 Mark Thomas 2015-02-05 12:45:05 UTC
The key needs to be in the org.apache.tomcat name space, not the javax.servlet space since the Servlet name space is reserved.
Comment 3 Ralf Hauser 2015-02-05 14:24:51 UTC
thanks Mark, so  1a) should rather be:

Comment 4 Christopher Schultz 2015-02-05 19:47:58 UTC
(In reply to Ralf Hauser from comment #1)
> 4) furthermore, I suggest to add in
> org.apache.tomcat.util.net.jsse.openssl.Protocol
>     TLSv1_1("TLSv1.1")

And "TLSv1.0" and/or "TLSv1".

Would you care to prepare a patch? You've filed this against Tomcat 7, but if you can do it against the trunk ("9.0") if would be easier to deal with.
Comment 5 Mark Thomas 2015-02-05 19:52:35 UTC
(In reply to Christopher Schultz from comment #4)
> (In reply to Ralf Hauser from comment #1)
> > 4) furthermore, I suggest to add in
> > org.apache.tomcat.util.net.jsse.openssl.Protocol
> > 
> >     TLSv1_1("TLSv1.1")
> And "TLSv1.0" and/or "TLSv1".
> Would you care to prepare a patch? You've filed this against Tomcat 7, but
> if you can do it against the trunk ("9.0") if would be easier to deal with.

Please don't waste your time on either of these changes. They will not be applied as they will never be used. I recommend reading the Javadoc for that class to understand why.
Comment 6 Ralf Hauser 2015-02-05 20:24:21 UTC
Created attachment 32437 [details]
Comment 7 Mark Thomas 2015-02-11 19:55:13 UTC
Reviewing the proposed patch:

1. The changes to tomcat/util/net/jsse/openssl/Protocol.java need to be removed

2. There looks to be the right hooks in tc-native to obtain the protocol so do that rather than throw an IOE for APR.

3. Comment lines should wrap at 80 chars.

4. The Javadoc comments starting "as per..." should not refer to this bug report but fully describe the elements they are documenting. I'd expect the description for the attribute name to be different to that for the method.
Comment 8 Christopher Schultz 2015-02-12 05:11:16 UTC
(In reply to Ralf Hauser from comment #6)
> Created attachment 32437 [details]
> apache.tomcat.tls.protocol.57540.patch

Is IOException the most natural exception type for SSLSupport.getProtocol? Under what circumstances do you expect an exception to be thrown (other than APR, which will probably not throw an IOException when properly-implemented)?

If you aren't going to implement it yet for APR, you could throw UnsupportedOperationException.... except that this code is called unconditionally and without any try/catch in Http11Protocol, so the patch as it stands will break requests coming from an APR connector. :(
Comment 9 Ralf Hauser 2015-02-17 15:22:06 UTC
Created attachment 32486 [details]

hopefully followed all the instructions by Mark and Christopher - please committ
Comment 10 Christopher Schultz 2015-02-17 19:00:29 UTC
I'm evaluating this. The change for AprSSLSupport doesn't seem that onerous. Why didn't you implement that?
Comment 11 Christopher Schultz 2015-02-17 20:05:34 UTC
Something is missing, here. I can't get the NIO connector to give me the protocol value. I changed the code to use "(unknown)" when the value returned by SSLSupport is null, and I'm not seeing that, either.

I think something might be wrong with my test case.

One more note: AJP isn't supported by this patch, and I think it should be.
Comment 12 Christopher Schultz 2015-02-17 20:13:02 UTC
Rats... looks like to support AJP, there need to be additional data sent by the proxy. There is no pre-defined field for SSL_PROTOCOL (similar to o.a.coyote.ajp.Constants.SC_A_SSL_CIPHER) so that'll need to wait.
Comment 13 Christopher Schultz 2015-02-17 20:41:41 UTC
Okay, the patch doesn't work as presented.

If you request the SSL protocol before any of the other SSL attributes, then the protocol comes back as null. That's because o.a.c.connector.Request.getAttribtue does some magic to trigger the loading of the SSL variables from the (physical) request into the request attributes.

I'm working on an update to the patch that includes hooks for this magic.
Comment 14 Christopher Schultz 2015-02-17 20:51:42 UTC
Created attachment 32487 [details]
Updated patch

This patch supports APR-based connectors and also works when the SSL protocol is the first SSL attribute fetched from the request attributes.

I'm not sure about how many places we like to define these key constants, so I'm asking for a review before I commit.
Comment 15 Rainer Jung 2015-02-18 03:26:16 UTC
I added a proprietary request attribute named "AJP_SSL_PROTOCOL" to mod_jk in r1660504. It could be mapped to a uniform attribute name, e.g. "org.apache.tomcat.util.net.secure_protocol_version" or whatever is the final name from this patch here in the Tomcat connector.

If this featur here gets applied, I'll add the same extension to mod_proxy_ajp (httpd trunk) and will propose for backport to httpd 2.4/2.2.
Comment 16 Christopher Schultz 2015-02-18 14:05:31 UTC
I've got an updated patch with AJP support that I'm testing now.
Comment 17 Christopher Schultz 2015-02-18 14:31:25 UTC
Created attachment 32493 [details]
Updated patch

Updated patch to support AJP connections (only with mod_jk 1.2.41 and higher, and with an as-yet-unspecified version of mod_proxy_ajp).
Comment 18 Rainer Jung 2015-02-18 16:37:33 UTC
Comment on attachment 32493 [details]
Updated patch

The part for java/org/apache/coyote/ajp looks fine to me.
The rest also, but I didn't inspect it very thoroughly.

For testing with stable mod_jk (without the recent addition), something like

SSLOptions StdEnvVars

RewriteEngine On
RewriteCond %{SSL:SSL_PROTOCOL} (.+)
RewriteRule . - [ENV=AJP_SSL_PROTOCOL:%1]


should simulate the behavior.

You can check the httpd side of this by adding %{SSL_PROTOCOL}e and %{AJP_SSL_PROTOCOL}e to the LogFormat of your access log. Only if these two vars show useful values in the access log, has the Tomcat side (using your patch) a chance of working for AJP.
Comment 19 Christopher Schultz 2015-02-18 19:54:29 UTC
I have a question about your implementation in mod_jk: why are you passing the SSL_PROTOCOL as a "SC_A_REQ_ATTRIBUTE" instead of a first-class piece of information, like SC_A_SSL_CIPHER is done?

Would that represent a change to the protocol, since SC_A_SSL_CIPHER is a constant defined by the protocol and SSL_PROTOCOL has nothing yet defined?

I'm going to commit this patch to trunk and then work on a proposal for a back-port, since a lot has changed between Tomcat 8 and the trunk at this point.
Comment 20 Rainer Jung 2015-02-18 20:48:57 UTC
Some attributes are "known" in the AJP 1.3 protocol and their names are marshalled on the wire with hex abbreviations. Those must be known by the receiver as well otherwise it is a protocol violation. So new attributes can't simply get new hex abbreviations because then we would have a compativbility problem with old receivers.

For general (not "known") attributes there's the option to send their name as clear text. That's what we do here.
Comment 21 Christopher Schultz 2015-02-19 16:45:54 UTC
Fixed in trunk in r1660924.

I'm working on back-porting this to Tomcat 8, since a lot has changed between Tomcat 8 and Tomcat 9/trunk.
Comment 22 Christopher Schultz 2015-02-19 17:51:19 UTC
Fixed in Tomcat 8 in r1660953. Will be in Tomcat 8.0.21.

I'll see about back-porting to Tomcat 7. It should be easy at this point.
Comment 23 Christopher Schultz 2015-02-19 18:39:41 UTC
Fixed in Tomcat 7 in r1660966. Will be in Tomcat 7.0.60.

I do not intend to back-port this to Tomcat 6.
Comment 24 Rainer Jung 2015-02-20 07:34:01 UTC
Support to forward the info via AJP has been added to mod_proxy_ajp in httpd trunk in r1661067. The feature has been proposed for addition to mod_proxy_ajp in httpd 2.4.
Comment 25 Rainer Jung 2015-02-24 18:36:06 UTC
The support in mod_proxy_ajp was ported to Apache 2.4 today as r1662076.
It will be part of the next Apache HTTP server release 2.4.13.