Re: SSL patch

Hi all,

Sorry I haven't been able to find the time to try your patch Andras, but
I've been able to read some of the code.
It looks good and it's a good idea to have a more complete set of
parameters that matches the behaviour of libpq as closely as possible.
(I guess one of the questions will be which one should be used by
default, since the Java defaults and the libpq defaults for TLS are
quite different approaches.)

Just a few quick points, if I may:

- I would suggest keeping the use of Subject Alternative Names (as I had
suggested in my patch [1]), to move towards a hostname verification in
line with RFC 6125, keeping the CN check as a fallback solution. A lot
of CAs now issue certificate with a SAN too, and it tends to be a
cleaner way to represent the hostname in general.
This is something that could probably be implemented in libpq too, but I
haven't looked at its code at all unfortunately.

- To avoid the unlikely event of a problem with using "JKS" on line 131
of LibPQFactory.java, this could be used:
KeyStore.getInstance(KeyStore.getDefaultType())
(Just in case this runs in a JRE with a security provider that doesn't
support JKS...)
Perhaps it might be worth considering using 'getDefaultAlgorithm()' when
creating the TrustManager as well.

- I've just noticed that the FileInputStream opened at line 130 of the
LazyKeyManager is never closed.

- I wonder if it might not be simpler to load the user cert and private
key into an in-memory KeyStore and create a KeyManager using it (as it's
done for the TrustManager), via a KeyManagerFactory. This would probably
remove the need to handle the client alias choice manually.

Best wishes,

Bruno.

[1] http://archives.postgresql.org/pgsql-jdbc/2011-08/msg00023.php

On 10/11/2011 14:30, Bodor András wrote:
> Dear Dave,
>
> The installation of sslinfo is only necessary for the unit tests, it is
> not used at all in the driver itself. Obviously I wanted to test weather
> we were actually using ssl, but it is not essential. It can be removed,
> or an additional option can be introduced to ssltest.properties.
> The relevant lines are in
> org.postgresql.test.ssl.SslTest.driver(String connstr, Object[]
> expected)
>
> There are a few things still to be done with this patch.
> 1. the jdbc datasource interface was not modified at all,
> so it is unaware of the new options,
> 2. it should be decided, what is the expected behaviour of sslmode=allow
> or prefer (they might be omitted completely),
> 3. I have not tested certificate chains yet,
> 4. when a client certificate is available, the v8 and v9 servers
> behave differently (BUG #5468 is fixed in v9) so different unit test are
> needed to check this,
> 5. there is a list of options somewhere in the code, this should
> be updated as well,
> 6. documentation.
>
> Andras