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

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?

See further comments inline below:

On 05/19/2011 02:41 PM, Marc-André Laverdière wrote:

 > There was a tiny bug in SysPropCertAuthFactory whereas a path was sent
 > instead of a password. I'm attaching the fix.

Whoops. Thanks.

> Thanks for the changes. It is a good step forward in terms of
> extensibility. That being said, I think we could just put a lot of the
> loading code in AbstractCertAuthFactory, so that it is more reusable.

I thought it was, as much as already possible. What more would you move
into AbstractCertAuthFactory ? Or do you want to create a single helper
method that takes something like:

initFactory(
   String ksPath, char[] ksPass, char[] keyPass,
   String tsPath, char[] tsPass)

to reduce verbosity, one that uses the existing buildSSLSocketFactory(),
createKeyManagers() and createTrustManagers() behind the scenes?

To me, it seems a pretty small thing to have to call createKeyManagers
and createTrustManagers and pass the args to buildSSLSocketFactory(...).

I did intentionally move the checks for null/empty keystore path and
truststore path out of AbstractCertAuthFactory and into
SysPropCertAuthFactory. It's already possible to say "use system
default" by passing null to buildSSLSocketFactory(...), so I don't think
createKeyManagers(..) or createTrustManagers() should be making that
decision implicitly for the caller. In security, it might be quite
undesirable to have the app fail silently back to the system truststore
- you might have good reasons to have specified your own.

I can see that it's necessary to treat a missing key as "use default"
when using system properties as a configuration mechanism, but don't
think that should be an assumption that AbstractCertAuthFactory makes
for all users of that code.

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

--
Craig Ringer