Re: Support for cert auth in JDBC - Mailing list pgsql-jdbc

Hi all,

Following the recent discussions about SSL support, here is a bit of
feedback on this version.

To fix the "TODO: Delegate trust checks to system trustmanager if
no match in our own trustmanager?" is fairly easy: just pass null to
SSLContext.init(...).

I would do this in createTrustManager:

         KeyStore trustKs = loadKeyStore(trustStorePath, trustStorePass,
trustStoreType);
         if (trustKs != null) {
             TrustManagerFactory trustFactory =
TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
             trustFactory.init(trustKs);
             return trustFactory.getTrustManagers();
         } else {
             return null;
         }

and this in loadKeyStore (instead of throwing an exception):

         if (path == null || "".equals(path))
             return null;

The null default also works for the SecureRandom if anyone isn't
specifying it.

(I would also do it do avoid potential NPEs perhaps in createKeyManager,
although there is no actual default.)




Since using PKCS#11 was discussed a few days ago on the mailing list
[1], I would also suggest a couple of changes, similar to what I had
done for Apache Tomcat [2].

Essentially, not all keystores are file-based (e.g. PKCS#11, NSS, OSX
KeychainStore, Windows store). The "NONE" file-path should be used for
those:
http://download.oracle.com/javase/6/docs/technotes/guides/security/jsse/JSSERefGuide.html#Customization
In this case, use a null InputStream (not necessarily a FileInputStream).

For PKCS#11, it's also useful to specify the provider (if the PKCS#11
configuration is in the JRE security configuration. This could be done
as follows (with an extra provider name parameter, which would be null
by default):

     private static KeyStore loadKeyStoreOfType(String path, char[]
password, String type, String provider) throws IOException,
GeneralSecurityException{
         // Load the keystore with the specified keystore type.
         if (path == null || "".equals(path))
             return null;
         if (password == null)
             throw new IllegalArgumentException("Password is null");
         if (type == null)
             type = KeyStore.getDefaultType();
         InputStream fIn = null;
         try{
             KeyStore ks;
             if (provider != null) {
                 ks = KeyStore.getInstance(type, provider);
             } else {
                 ks = KeyStore.getInstance(type);
             }
             // Using "NONE" for keystore that are not file-based.
             //
http://download.oracle.com/javase/6/docs/technotes/guides/security/jsse/JSSERefGuide.html#Customization
             if (!"NONE".equals(path)) {
                 fIn = new FileInputStream(path);
             }
             ks.load(fIn, password);
             return ks;
         } finally{
             if (fIn != null) {
                 fIn.close();
             }
         }
     }


On 19/05/2011 08:27, Craig Ringer wrote:
> Thanks for those changes. Comments follow.
>
> I'm attaching a further-tweaked version of the code you just sent. I
> only had to change AbstractCertAuthFactory, so the other files remain
> unaltered. Changes are:
>
> - Only attempt to autodetect keystore type when null keystore type was
> passed, instead of always accepting but ignoring the keystore type
> argument.
>
> - Make string constants private so nobody lands up relying on them
> so they can't be changed without breaking backward compat.
>
> - Make "internal helper only" methods private. If they're protected,
> they're API not internal helpers.
>
> Reasonable?
>
>> I also added the code that allows to load both PKCS12 and JKS blindly. I
>> think that we can remove the property for specifying the type. I doubt
>> we have to worry about other types of keystores than those two.
>
> JECKS is also common.
>
> I don't love this part - it's making assumptions that may not prove true
> in the long run, and IOException is a very broad exception to rely on to
> detect a very specific thing like wrong keystore format. It could
> potentially result in confusing error messages, too. For example, a
> corrupt or damaged JKS keystore might fail to load with an error
> reporting that a "pkcs12 keystore could not be read", causing the user
> to wonder why the S@#$@ the code was trying to read it as PKCS#12 and
> not identify the real problem, that it tried JKS first and failed
> because the keystore was damaged.
>
> It seems OK to me to attempt to auto-detect the keystore type when no
> keystore type is given. The code you posted _ignores_ the keystore type
> argument. If a non-null keystore type is passed it should be honoured,
> but if a null keystore type is passed then I think it's fine to try to
> detect it rather than blindly assuming KeyStore.getDefaultType() .


I would personally prefer not to try to have the driver guess the
keystore type blindly. SSL errors can be obscure enough for users will
little SSL experience as it is (like "Could not find matching cipher
suites" sometimes when it's a problem with the key/cert).

Having the user specify the type or fall back on
KeyStore.getDefaultType() seems more in line with the rest of the
documentation in the JSSR ref. guide.



If this is of interest, I have done a bit of work on trying to make it
easier to configure SSLContexts while keeping default values in
jSSLutils [3].
I found that creating an SSLContextFactory interface/abstract class
tends to make this sort of configuration easier, as you don't need to
extend the SSLSocketFactory as much.
A similar approach could be used in the JDBC driver, but it would
require the creation of such an interface and modifications to
"MakeSSL". It might be too much.




Best wishes,

Bruno.

[1] http://archives.postgresql.org/pgsql-jdbc/2011-11/msg00015.php
[2] https://issues.apache.org/bugzilla/show_bug.cgi?id=43094
[3] http://www.jsslutils.org/