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

That's good changes.
I'm not super keen on the idea of asking the user of providing the type.
But I'm not gonna fight over that :)

Now, would you please elaborate on those todos?

Also, methinks the @author tag at the top needs an extra line as follows:
@author Craig Ringer (craig@postnewspapers.com.au)

Marc-André Laverdière
Software Security Scientist
Innovation Labs, Tata Consultancy Services
Hyderabad, India

On Thursday 19 May 2011 12:57 PM, 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?
>
> 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
>
>
>
>