Bug 57540

4) furthermore, I suggest to add in org.apache.tomcat.util.net.jsse.openssl.Protocol

    TLSv1_1("TLSv1.1")

The key needs to be in the org.apache.tomcat name space, not the javax.servlet space since the Servlet name space is reserved.

thanks Mark, so  1a) should rather be:

"org.apache.tomcat.util.net.secure_protocol_version"

(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.

(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.

Created attachment 32437 [details]
apache.tomcat.tls.protocol.57540.patch

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.

(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. :(

Created attachment 32486 [details]
apache.tomcat.tls.protocol.57540c9.patch

hopefully followed all the instructions by Mark and Christopher - please committ

I'm evaluating this. The change for AprSSLSupport doesn't seem that onerous. Why didn't you implement that?

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.

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.

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.

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.

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.

I've got an updated patch with AJP support that I'm testing now.

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 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]

JkEnvVar AJP_SSL_PROTOCOL

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.

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.

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.

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.

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.

Fixed in Tomcat 7 in r1660966. Will be in Tomcat 7.0.60.

I do not intend to back-port this to Tomcat 6.

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.

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.