Обсуждение: [PATCH] New SSL Socket Factory With Certificate Validation

Hi-

I tried posting this a couple weeks ago but it kept getting rejected
by the list's filter so I ended up putting all the info in a pull
request on GitHub (https://github.com/pgjdbc/pgjdbc/pull/80). Looks
like I can (finally) post here now so here goes ...

Full details are in the linked pull request but the gist of it is that
this patch adds a new SSL socket factory that performs remote server
certificate validation against a pre shared SSL certificate. What's
different about it (vs the existing socket factories) is that it
easily allows specifying the SSL certificate at runtime as either a
string, file, environment variable, or system property.

We got the idea for it working on our product JackDB (a database
client in your browser ... check it out!) as the primary use case for
our public cloud version is to connect to cloud databases (ex: Heroku
Postgres or Amazon RDS). Although most DBaaS providers support
connecting over SSL, the common advice online is ignore the
authentication piece of the SSL handshake by using the
NonValidatingFactory (which makes you vulnerable to a man in the
middle attack). That obviously wasn't acceptable and we looked at
other options as well but none was straightforward enough,
particularly for our use case of dynamically adding data sources at
runtime, so we came up with this class.

We've been using a variant of this in JackDB for a while now and it's
been working great. Let me know what you guys think.

Thanks,
Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | http://www.jackdb.com/ | @jackdb

On Aug 28, 2013, at 7:43 AM, Sehrope Sarkuni <sehrope@jackdb.com> wrote:

> Hi-
>
> I tried posting this a couple weeks ago but it kept getting rejected
> by the list's filter so I ended up putting all the info in a pull
> request on GitHub (https://github.com/pgjdbc/pgjdbc/pull/80). Looks
> like I can (finally) post here now so here goes …


Hi,

This looks like really cool functionality.  My main concern with the code as written is that testing it requires manual
steps,including booting a virtual machine and running a separate build. 

In my experience, any steps beyond the simple "run all the tests" step will never be done.  Human nature.

Would it be possible to convert the test case to run as a part of the normal test suite?

There are also a couple of scary places where exceptions are swallowed, e.g. SingleCertTrustManager#<init>

Best,
Steven

> This looks like really cool functionality.

Thanks! Yes it's a pretty straightforward idea and was kind of
surprising that it didn't already exist.

I submitted a similar patch to the MariaDB JDBC driver project as well
though the final implementation they merged[1] in is significantly
less extensible than this (basically only the string format is
allowed). Although we only use the string format of entering a
certificate in JackDB I can see folks using all the other ones as well
(ex: the "env:" method jives with modern PaaS coding standards) and
like this version much better.

> My main concern with the code as written is that testing it requires manual steps, including booting a virtual
machineand running a separate build. 
>
> In my experience, any steps beyond the simple "run all the tests" step will never be done.  Human nature.

You're right and it definitely should be included into the main build
and automated. That's one of the reasons I split up the new code into
a patch for the factory itself and one for the testing/VM. The latter
is more of a template for a proper test harness and really should be
integrated with the rest of the driver build/test process.

> Would it be possible to convert the test case to run as a part of the normal test suite?

Of course! I'll be honest, I wasn't quite sure how to integrate it
with the existing ant build, hence the separate VM and Maven project
for testing (e.g. not sure if there are separate build/test server for
the JDBC driver).

Still though I think having a built-in VM in the project for folks to
spin up for testing would be a good idea. It makes it *a lot* easier
to test out code changes. Plus it'd be possible to have multiple VMs
with different versions of PG installed (to properly test against all
the combinations of PG versions/drivers). If there's a CI server in
place the test VM should match up with that.

> There are also a couple of scary places where exceptions are swallowed, e.g. SingleCertTrustManager#<init>

The only swallowed exception is in the constructor for
SingleCertTrustManager and that's only when calling the "load" method
of the KeyStore to initialize it (with a bogus null param). It looks
like a no-op but it's required as otherwise the KeyStore won't be
initialized and you can't add certificates to it afterwards.

Thanks,
Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | http://www.jackdb.com/ | @jackdb

[1]: https://kb.askmonty.org/en/mariadb-java-client-113-changelog