Allow for Secure IBM i connections to use a custom SSLSocketFactory

[pjyoung-ibm]

Adapted from old SF bug 457.

Currently in SocketContainerJSSE, the default SSLSocketFactory is retrieved before using it to create the secure socket to whichever host server it is attempting to connect to. There is no way to provide a custom SSLSocketFactory for JTOpen to use. Unless there was a good reason to only use the default that I'm just not aware of, a way to provide a custom SSLSocketFactory would be nice, as it would allow users of JTOpen to manage multiple SSLContexts and provide it with the factory it should use to create the socket, without needing to override the JVM default context with their own version. It doesn't appear that JTOpen cares about the underlying factory itself, just so long as it produces a valid socket to use.

The original bug reporter cited needing this because their company uses an Internal CA that the default Java SSLContext would reject because its root certificate wouldn't be in the global truststore. My use case is that my application might have two different truststores it will need to use, based on the action it is performing. Currently, I would need to set the truststore to be used for Secure IBM i connections as the default SSLContext, then have my other truststore as a separate SSLContext to pass around to the needed areas. This is doable but less than ideal, as I would like to avoid changing the default JVM context if I can help it.

[MarcelRomijn]

Hello,

I'm the original reporter of bug 457 on SourceForge.

At the time, we 'solved' the secure connection requirement by using JTOpen's sources, add our modifications and publish the modified JTOpen publicly on GitHub.
As we wanted to pickup the latest version of JTOpen, and make the same modifications, we saw that JTOpen has gone forward by moving the sources to GitHub.

Since the original report, we also added changes to allow for a secure JDBC connections.
We do this by putting a prepared SSLSocketFactory into the 'Properties' that are passed on with the DriverManager.getConnection(jdbcUrl, jdbcProperties) call.

I tried applying our modifications to the current JTOpen code base. That worked without much problems.
There are some differences in AS400JDBCDriver since I last looked, but no problems there as well.

For us, it would be beneficial to have our modifications be part of the original JTOpen code-base. And we think the modifications would be a useful addition for JTOpen as well 😊
I can add the modifications to the JTOpen code-base, if you want.
I would only need some steps on how to actually do that, like where to create a branch, how to submit a PR, etc...

Kind regards,
Marcel Romijn

[ThePrez]

Hi, @MarcelRomijn ! We would welcome this pull request (and in my opinion it is a feature that is a bit overdue).

Here's a sample article I've used as reference for getting started with pull requests: https://www.freecodecamp.org/news/how-to-make-your-first-pull-request-on-github-3/
You'll want to fork the repository so it'll exist at https://github.com/MarcelRomijn/JTOpen , create a branch there (optional), commit your changes, and send a PR. Let me know if you need help with any of the process and I'll do my best.

[MarcelRomijn]

Hi @ThePrez, thanks for the instructions for creating the PR!

I'm on PTO after today, but I will pick this up when I get back to work, in the week of Aug 26th.

Thanks!

[MarcelRomijn]

Hi @ThePrez, I followed the instructions and created a Pull Request: #200

I created another GitHub repository that contains tester code to test/demonstrate the security enhancement: https://github.com/MarcelRomijn/JTOpen_security_test/tree/feature/security

Unfortunately, the Pull Request has a failed check about the 'DCO'.
I was not aware of this check. Will this cause a problem?

Thanks,
Marcel

[trknz]

Hi @MarcelRomijn,
Would it be possible, if it’s not too late, to add a 'pre-cooked' SSLSocketFactory class version that would bypass SSL certificate validation i.e. accept any certificate provided by the server? I believe this is a common use case, and including such a factory class in the package would save users from having to write their own implementation.

Thank you for considering this.

Example (not tested):

import javax.net.ssl.*;
import java.io.IOException;
import java.net.Socket;
import java.security.KeyManagementException;
import java.security.NoSuchAlgorithmException;
import java.security.cert.X509Certificate;

public class BypassSSLSocketFactory extends SSLSocketFactory {

    private SSLSocketFactory sslSocketFactory;

    public BypassSSLSocketFactory() {
        try {
            // Create a trust manager that does not validate certificate chains
            TrustManager[] trustAllCerts = new TrustManager[]{
                new X509TrustManager() {
                    public X509Certificate[] getAcceptedIssuers() {
                        return null;
                    }

                    public void checkClientTrusted(X509Certificate[] certs, String authType) {
                    }

                    public void checkServerTrusted(X509Certificate[] certs, String authType) {
                    }
                }
            };

            // Install the all-trusting trust manager
            SSLContext sc = SSLContext.getInstance("TLS");
            sc.init(null, trustAllCerts, new java.security.SecureRandom());

            sslSocketFactory = sc.getSocketFactory();
        } catch (NoSuchAlgorithmException e) {
            e.printStackTrace();
        } catch (KeyManagementException e) {
            e.printStackTrace();
        }
    }

    @Override
    public String[] getDefaultCipherSuites() {
        return sslSocketFactory.getDefaultCipherSuites();
    }

    @Override
    public String[] getSupportedCipherSuites() {
        return sslSocketFactory.getSupportedCipherSuites();
    }

    @Override
    public Socket createSocket(Socket s, String host, int port, boolean autoClose) throws IOException {
        return sslSocketFactory.createSocket(s, host, port, autoClose);
    }

    @Override
    public Socket createSocket(String host, int port) throws IOException {
        return sslSocketFactory.createSocket(host, port);
    }

    @Override
    public Socket createSocket(String host, int port, java.net.InetAddress localHost, int localPort) throws IOException {
        return sslSocketFactory.createSocket(host, port, localHost, localPort);
    }

    @Override
    public Socket createSocket(java.net.InetAddress host, int port) throws IOException {
        return sslSocketFactory.createSocket(host, port);
    }

    @Override
    public Socket createSocket(java.net.InetAddress address, int port, java.net.InetAddress localAddress, int localPort) throws IOException {
        return sslSocketFactory.createSocket(address, port, localAddress, localPort);
    }
}

[MarcelRomijn]

Hi @trknz,

I have two thoughts about such an implementation...

Security:

In our company, we are steering away from any implementation that allows for a 'trust-all' type of secure connection.
The obvious reason is that this allows for a man-in-the-middle attack.
But also because we want our customers to be aware of what it means to use secure connections.

It's easy to install a server- and/or client application, click the 'trust-all' checkbox during installation and get something working.
But then, because 'it works', no thought is given to the actual security implication of the 'trust-all' configuration and applications are taken into production with it.
That's why, where secure connections are necessary, our company pushes a 'secure by default' policy. If a customer does not have their PKI in place, you cannot install or configure our products.

JTOpen:

The proposed BypassSSLSocketFactory is basically unrelated to the core functionality of JTOpen. It could be in any 3rd party library (or a company's local library).
I'm more than happy to include such a class in the Pull Request I created for JTOpen, but I want the JTOpen team to make the decision if they want such a class in their JTOpen code base.

Thanks,
Marcel

[trknz]

Hi @trknz,

I have two thoughts about such an implementation...

I understand your point, but I believe it's up to the user to set their own security policies. Personally, I dislike when others decide what's best for me, as there's no one-size-fits-all solution. I have two examples:

1. Test Environments: Sometimes, flexibility is needed in testing scenarios.
2. Third-Party Dependencies: For instance, dealing with a legacy IBM i site where the admins are not well-versed in certificate management.

Overall, I would prefer that the package include some common implementations of SocketFactory like BypassSSLSocketFactory. This would make it easier to use for testing or proof-of-concept work and help us be more agile.

[MarcelRomijn]

Hi, @trknz,

Thanks for you feedback!

Everyone is entitled to view of the way security should be implemented differently of course.
Our company's policy comes from experience with our customers. Other companies can have other experiences.
So please regard my first point as information only.

But I stand by my second point that it is up to the JTOpen team to decide whether a bypass SSLSocketFactory should be added to the JTOpen code base.

Thanks,
Marcel

[william-xiang]

Hi @ThePrez, I followed the instructions and created a Pull Request: #200

I created another GitHub repository that contains tester code to test/demonstrate the security enhancement: https://github.com/MarcelRomijn/JTOpen_security_test/tree/feature/security

Unfortunately, the Pull Request has a failed check about the 'DCO'. I was not aware of this check. Will this cause a problem?

Thanks, Marcel

Hi @MarcelRomijn,

From my experience, the failure of DCO checking is not a blocker for merging the PR. However, if you prefer, you can follow the instructions below to resolve this issue.

In your local branch, run: git rebase HEAD~1 --signoff
Force push your changes to overwrite the branch: git push --force-with-lease origin feature/security

[nadiramra]

To pass DCO checking, you have to sign your commits.

[MarcelRomijn]

Hi all,

I saw that there were new commits to the JTOpen repository, so my PR had conflicts that needed to be resolved first.
In the commit (after the merge) I included the Signed-off-by and it looks like all check now pass (including the DCO check).

Thanks,
Marcel

[trknz]

@jeber-ibm, can we speed up the review and merge of the commit? It’s been waiting for approval for over two weeks. Thanks.

[trknz]

Hi @MarcelRomijn,

I think it would make sense to apply the change to the AS400ConnectionPool class too, since it's often used to manage connections. What do you think?

[MarcelRomijn]

Hi @trknz,

At the moment, my original PR has not been merged. There are doubts whether this is the right way to go.
See comments on the actual PR: #200

Once/if the PR is merged, we can think about other enhancements, including the BypassSSLSocketFactory and adding similar functionality to AS400ConnectionPool.
All with the consent of the JTOpen team, of course.

Thanks,
Marcel