Re: Support for cert auth in 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(at)postnewspapers(dot)com(dot)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(at)#$@ 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
>
>
>
>