TLS protocol preferences in XMPPTCPConnection

[Georg_Lukas]

One of the regressions during the port from smack3 to smack4 was, that on a certain “Android” platform, no more connections could be established. Debugging has shown that connection attempts were made with only TLSv1.0 enabled to a TLSv1.2 server, and further digging has shown that smack4 doesn’t enforce protocol versions any more by default.

smack3 XMPPConnection does this:

List<String> supportedProtocols = java.util.Arrays.asList(((SSLSocket) socket).getSupportedProtocols());
List<String> enabledProtocols = new java.util.ArrayList<String>(3);
for (String p : new String[]{"TLSv1.2", "TLSv1.1", "TLSv1"}) {
    if (supportedProtocols.contains(p))
	enabledProtocols.add(p);
}
((SSLSocket) socket).setEnabledProtocols(enabledProtocols.toArray(new String[enabledProtocols.size()]));

However, this does not happen in Smack4 any more, at all. Instead, there is a helper function ConnectionConfiguration.Builder TLSUtil.setTLSOnly(ConnectionConfiguration.Builder) (and a less restrictive setSSLv3AndTLSOnly()) that needs to be called manually. I see multiple problems with this concept and implementation:

1. By default, the client will use the OS defaults, which might be horrible (this investigation was triggered by a “SSLv3”+“TLSv1” client). It would be better to have setTLSOnly being called in the CCB constructor and allow the user to override these defaults.
2. TLSUtil is the wrong place for the helper functions, they should be part of CCB.
3. The list is missing TLSv1.3, even if it isn’t supported on any real-world OS yet.
4. The implementation in TLSUtil.setEnabledProtocolsAndCiphers() is using set intersections, which will reorder the set. I haven’t investigated yet whether the order of protocols/suites plays a role during negotiation or not, but in case it does, this is not optimal.

[Flow]

@Georg_Lukas

By default, the client will use the OS defaults, which might be horrible

Or they may be better then the libraries defaults (as it is nicely demonstrated by Smack’s TLSUtil).

@Georg_Lukas

but in case it does, this is not optimal.

Right, in case it does, it is not optional. Interestingly the realted javadoc of SSLSocket also keeps quiet about that. It is probably something that should be proactively changed. Patches welcome.