Thread: Support for cert auth in JDBC
[I posted this by mistake to the main dev mailing list... apologies if you got this multiple times] Hello developers, My project had a requirement to use certificate authentication to the PG server. Our application uses Hibernate. We did just that and my boss has OKed a source release. Now, the current version of the code has dependencies on our internal libraries, so I'll need to spend a bit of time making this 'standard' Java code. Would you please tell me how you'd prefer for me to proceed to do that? Do I need write access to your CVS repo, or should I just send the code and test case by email? Is there a specific version of the JDBC code you want me to work from, should I just pick whatever is HEAD? Any package you'd like me to choose? Any specific crypto/ssl requirements to consider? Any specific dependencies to use instead of others? (e.g. I like SLF4J, but that's not everyone's choice...) -- Marc-André Laverdière Software Security Scientist Innovation Labs, Tata Consultancy Services Hyderabad, India
On 18/05/2011 12:49 PM, Marc-André Laverdière wrote: > [I posted this by mistake to the main dev mailing list... apologies if > you got this multiple times] > > Hello developers, > > My project had a requirement to use certificate authentication to the PG > server. Our application uses Hibernate. > > We did just that and my boss has OKed a source release. Eh? Did what? PgJDBC supports X.509 client certificates just fine. What did you need to do? To which PgJDBC version did you do it? To resolve what perceived issue(s) in the current codebase? -- Craig Ringer Tech-related writing at http://soapyfrogs.blogspot.com/
Hello, This implementations allows to specify which keystore and which truststore to load. This allows certificate authentication in the application easily. In the official documentation, it says: <quote> Custom SSLSocketFactory PostgreSQL™ provides a way for developers to customize how a SSL connection is established. This may be used to provide a custom certificate source or other extensions by allowing the developer to create their own SSLContext instance. The connection URL parameters sslfactory and sslfactoryarg allow the user to specify which custom class to use for creating the SSLSocketFactory. The class name specified by sslfactory must extend javax.net.ssl.SSLSocketFactory and be available to the driver's classloader. This class must have a zero argument constructor or a single argument constructor taking a String argument. This argument may optionally be supplied by sslfactoryarg. Information on how to actually implement such a class is beyond the scope of this documentation. Places to look for help are the JSSE Reference Guide and the source to the NonValidatingFactory provided by the JDBC driver. The Java SSL API is not very well known to the JDBC driver developers and we would be interested in any interesting and generally useful extensions that you have implemented using this mechanism. Specifically it would be nice to be able to provide client certificates to be validated by the server. </quote> What I'm talking about is this factory referred to in the last paragraph. Marc-André Laverdière Software Security Scientist Innovation Labs, Tata Consultancy Services Hyderabad, India On Wednesday 18 May 2011 11:10 AM, Craig Ringer wrote: > On 18/05/2011 12:49 PM, Marc-André Laverdière wrote: >> [I posted this by mistake to the main dev mailing list... apologies if >> you got this multiple times] >> >> Hello developers, >> >> My project had a requirement to use certificate authentication to the PG >> server. Our application uses Hibernate. >> >> We did just that and my boss has OKed a source release. > > Eh? Did what? > > PgJDBC supports X.509 client certificates just fine. > > What did you need to do? To which PgJDBC version did you do it? To > resolve what perceived issue(s) in the current codebase? >
On 18/05/2011 2:06 PM, Marc-André Laverdière wrote:
> Hello,
>
> This implementations allows to specify which keystore and which
> truststore to load. This allows certificate authentication in the
> application easily.
It's already pretty easy. You can:
- Load your keys in the standard keystore;
- Populate a new JECKSf-format keystore with your cert and key and use
the javax.net.ssl.trustStore etc system properties on the cmdline or via
System.setProperty() to use that store instead of the default store;
- Provide your own javax.net.ssl.SSLSocketFactory that wraps the default
implementation with any additional behavior you need.
PgJDBC could use some documentation/examples for the latter option, but
it shouldn't need any changes to the PgJDBC code.
> The Java SSL API is not very well known to the JDBC driver developers
> and we would be interested in any interesting and generally useful
> extensions that you have implemented using this mechanism. Specifically
> it would be nice to be able to provide client certificates to be
> validated by the server.
> </quote>
>
> What I'm talking about is this factory referred to in the last paragraph.
OK, so you've just written an easy-to-use canned SSLSocketFactory that
loads client certificates from an application-configurable
keystore/truststore?
I did this in my app to allow it to use a PKCS#12 format X.509
certificate directly and it was a pretty low-pain procedure. It'd be
nice to have more general purpose examples in the PgJDBC sources,
though. Maybe I should dig out the examples I posted to this mailing
list a while ago and submit them, too?
I'm not one of the PgJDBC developers, but I'd be happy to have a look at
what you've done and see if I can be of any help, provide useful
feedback, etc.
It'd probably be best if you published your work as a patch ("diff")
against PgJDBC CVS HEAD, posting the patch to this mailing list so
people can look at it and try it. Make sure the email you send the patch
in clearly explains the reason for any change to the existing PgJDBC
code, and if you do change the core PgJDBC code make sure it passes any
tests and that it compiles under the oldest supported JDK.
--
Craig Ringer
Tech-related writing at http://soapyfrogs.blogspot.com/
It is true that anyone who knows the right Java APIs can put that
factory together with little pain. I just think that the average user
shouldn't have to bother about it.
Also, some use cases do not allow to use the default keystore and trust
store for everything.
Just for example, this is the setup in my application:
* 1 Cert for some server component that speaks TLS
* 1 Cert for the pgsql client component
* 1 Cert for the JNDI client component (connects to LDAP)
So we end up with 3 different keystores (and maybe trust stores too),
all of which may have different passwords. It is actually not that much
of a big deal to use, but the 'defaults' won't help us here too much I
think.
Using this kind of configurable setup allows much simpler deployments
than having to change stuff in a central keystore (which may have other
keys to NOT mess up with), and allows to test in different environments
and very easily.
Maybe our use case is overkill. Anyways, I'm attaching the code as I
have it now. I tested the code from which this one comes from, but this
incarnation isn't tested. I'll put more time on it once I hear back from
the maintainers. :)
I'd love to have some comments on this rough version also (CVS diff
didn't do anything with this file, so there is no point to include that).
P.S. Undocumented detail: we need to put the user's name in the
connection settings anyway, as it won't work without it.
Regards,
Marc-André Laverdière
Software Security Scientist
Innovation Labs, Tata Consultancy Services
Hyderabad, India
On Wednesday 18 May 2011 12:32 PM, Craig Ringer wrote:
> On 18/05/2011 2:06 PM, Marc-André Laverdière wrote:
>> Hello,
>>
>> This implementations allows to specify which keystore and which
>> truststore to load. This allows certificate authentication in the
>> application easily.
>
> It's already pretty easy. You can:
>
> - Load your keys in the standard keystore;
> - Populate a new JECKSf-format keystore with your cert and key and use
> the javax.net.ssl.trustStore etc system properties on the cmdline or via
> System.setProperty() to use that store instead of the default store;
> - Provide your own javax.net.ssl.SSLSocketFactory that wraps the default
> implementation with any additional behavior you need.
>
> PgJDBC could use some documentation/examples for the latter option, but
> it shouldn't need any changes to the PgJDBC code.
>
>> The Java SSL API is not very well known to the JDBC driver developers
>> and we would be interested in any interesting and generally useful
>> extensions that you have implemented using this mechanism. Specifically
>> it would be nice to be able to provide client certificates to be
>> validated by the server.
>> </quote>
>>
>> What I'm talking about is this factory referred to in the last paragraph.
>
> OK, so you've just written an easy-to-use canned SSLSocketFactory that
> loads client certificates from an application-configurable
> keystore/truststore?
>
> I did this in my app to allow it to use a PKCS#12 format X.509
> certificate directly and it was a pretty low-pain procedure. It'd be
> nice to have more general purpose examples in the PgJDBC sources,
> though. Maybe I should dig out the examples I posted to this mailing
> list a while ago and submit them, too?
>
> I'm not one of the PgJDBC developers, but I'd be happy to have a look at
> what you've done and see if I can be of any help, provide useful
> feedback, etc.
>
> It'd probably be best if you published your work as a patch ("diff")
> against PgJDBC CVS HEAD, posting the patch to this mailing list so
> people can look at it and try it. Make sure the email you send the patch
> in clearly explains the reason for any change to the existing PgJDBC
> code, and if you do change the core PgJDBC code make sure it passes any
> tests and that it compiles under the oldest supported JDK.
>
Attachment
On 05/18/2011 03:48 PM, Marc-André Laverdière wrote: > It is true that anyone who knows the right Java APIs can put that > factory together with little pain. I just think that the average user > shouldn't have to bother about it. In that, I agree. Canned factories for simple cases like loading a PKCS#12 format cert file, loading an app-specified keystore with a callback for a password prompt, etc would make a lot of sense. > So we end up with 3 different keystores (and maybe trust stores too), > all of which may have different passwords. It is actually not that much > of a big deal to use, but the 'defaults' won't help us here too much I > think. Quite right, if your different keystores must have different passwords then you'd need to implement your own factory. > P.S. Undocumented detail: we need to put the user's name in the > connection settings anyway, as it won't work without it. Interesting. I've only ever used setups where X.509 certs are supplementary to traditional password authentication. I'm surprised that the username isn't just deduced from the X.509 DN. A few comments on your code: Overall, it looks pretty sensible at first reading. There are a few things I noticed: - The code doesn't accept arguments for keystore and truststore type, so it won't be possible to load (eg) a PKCS#12 certificate file directly. Only stores of the default types will work. - It's not very subclassing-friendly, which could be an issue if it's to be shipped in PgJDBC. Specific subclassing use cases I see are allowing different configuration mechanisms (Properties instances in thread-local storage, JNDI, BeanManager contexts, blah blah) and accepting pre-configured KeyStore & TrustStore instances. I needed to use the latter in my app, and the former could become an issue whenever you need to create CertAuthFactory instances with different configurations in the same app - for example, for different users. Personally, I dislike the use of system properties for configuration of the class. It's totally thread-unsafe and is a real pain in any situation where you don't necessarily want exactly the same configuration for every instance. There's nothing wrong with having it as a convenient default, but it'd be nice to be able to override it with another configuration mechanism if this code is to ship with PgJDBC. Alas the SSLSocketFactory interface doesn't offer any really nice options like a way to pass a Properties instance to the ctor, and accepting URL-encoded query-string like params as a String isn't attractive given their length and the potential password security issues. What I'd want to do is allow subclasses to override the configuration behaviour by: - Lazily initializing _internalFactory on first access rather than in the ctor, so that: - a subclass's ctor can do something useful and application-defined with the String parameter passed to the one-arg form, like reading a config file, looking up a Properties instance from a config service or JNDI, etc. This would require that: - Configuration values are acessed via protected methods rather than direct System.getProperty() calls so those can be overridden by a subclass to get the information from elsewhere. Ideally (IMO) also: - the KeyStore instantiation and loading for ks and trustKs is done in protected methods too, so these can be completely overridden to obtain instances another way. -- Craig Ringer
Oh, also, your code has a simple thinko or copy-n-paste bug. On line 79: fInTrustStore = new FileInputStream(keyPath); should read: fInTrustStore = new FileInputStream(trustPath); Currently, trustPath never escapes the method and is effectively unused; it's only tested for null/empty, then ignored. -- Craig Ringer
On Wed, 18 May 2011, Craig Ringer wrote: > Alas the SSLSocketFactory interface doesn't offer any really nice options > like a way to pass a Properties instance to the ctor, That'd be an easy thing to fix, just add another case to org.postgresql.ssl.MakeSSL#convert to pass through the provided Properties object. I've thought about that in the past, but with no real use case, I wasn't in a big rush to do so. Kris Jurka
On 05/18/2011 11:32 PM, Kris Jurka wrote: > > > On Wed, 18 May 2011, Craig Ringer wrote: > >> Alas the SSLSocketFactory interface doesn't offer any really nice >> options like a way to pass a Properties instance to the ctor, > > That'd be an easy thing to fix, just add another case to > org.postgresql.ssl.MakeSSL#convert to pass through the provided > Properties object. I've thought about that in the past, but with no real > use case, I wasn't in a big rush to do so. OK, good to know. It wasn't a big issue for me in the end, as I landed up fetching my configuration using the String argument form as a key to an application-specific configuration provider. It's much easier to pass a String argument through various layers one often has over the JDBC driver, like an ORM or the like. -- Craig Ringer
On 05/18/2011 03:48 PM, Marc-André Laverdière wrote: > It is true that anyone who knows the right Java APIs can put that > factory together with little pain. I just think that the average user > shouldn't have to bother about it. > > Also, some use cases do not allow to use the default keystore and trust > store for everything. I agree, and I think you're right that the PgJDBC code should have a re-usable class to provide support for these use cases without everyone having to code their own. I've taken your code and re-worked it a bit to: - Use the PgJDBC WrappedFactory rather than re-implementing an effectively identical one; - Separate the basic construction process of the SSLSocketFactory from its configuration, providing an abstract superclass that does the construction via a few simple method calls, so someone who needs different configuration sources/methods can re-use the code; - Provided a concrete subclass of the above that gets its configuration from system properties and uses the superclass methods to construct the factory. This class has the same functionality as what you posted originally plus a few additional configuration options for additional use cases like non-default KeyStore types (pkcs12 etc); and - Added more extensive JavaDoc It's also possible to pass your own KeyStore and/or TrustStore instances directly if your app has to do something funky like manage an in-memory KeyStore. What I haven't done yet is tested it! This is a preview so you can comment on it and tell me if you think I've gone completely off the rails or not. The simplest uses of the code remain as simple as they were with your original version. You just set some system properties and specify SysPropCertAuthFactory as the sslsocketfactory class. More complex use cases become possible without having to re-implement the whole lot - in particular, if you can't fetch your configuration from system properties you can provide alternative mechanisms to look it up without having to re-write all the JSSE crap. So: thanks VERY much for posting this code. My original demos weren't flexible enough to be included in PgJDBC, and by posting this you really helped motivate me to try to turn your code and my original demoes into something that _was_. What do you think? Note that you'll need PgJDBC on your classpath to complile this now, because it uses org.postgresql.ssl.WrappedFactory . -- Craig Ringer
Attachment
Hi guys, Here are some improvements based on the comments I saw so far. I'm also adding a small cheapo tester. Definitely not JUnit-ready :) Marc-André Laverdière Software Security Scientist Innovation Labs, Tata Consultancy Services Hyderabad, India On Thursday 19 May 2011 11:16 AM, Craig Ringer wrote: > On 05/18/2011 11:32 PM, Kris Jurka wrote: >> >> >> On Wed, 18 May 2011, Craig Ringer wrote: >> >>> Alas the SSLSocketFactory interface doesn't offer any really nice >>> options like a way to pass a Properties instance to the ctor, >> >> That'd be an easy thing to fix, just add another case to >> org.postgresql.ssl.MakeSSL#convert to pass through the provided >> Properties object. I've thought about that in the past, but with no real >> use case, I wasn't in a big rush to do so. > > OK, good to know. > > It wasn't a big issue for me in the end, as I landed up fetching my > configuration using the String argument form as a key to an > application-specific configuration provider. It's much easier to pass a > String argument through various layers one often has over the JDBC > driver, like an ORM or the like. > > -- > Craig Ringer
Attachment
Hello, 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. There was a tiny bug in SysPropCertAuthFactory whereas a path was sent instead of a password. I'm attaching the fix. 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. I'm also adding the slightly modified tester which uses the right class. Regards, Marc-André Laverdière Software Security Scientist Innovation Labs, Tata Consultancy Services Hyderabad, India On Thursday 19 May 2011 11:28 AM, Craig Ringer wrote: > On 05/18/2011 03:48 PM, Marc-André Laverdière wrote: >> It is true that anyone who knows the right Java APIs can put that >> factory together with little pain. I just think that the average user >> shouldn't have to bother about it. >> >> Also, some use cases do not allow to use the default keystore and trust >> store for everything. > > I agree, and I think you're right that the PgJDBC code should have a > re-usable class to provide support for these use cases without everyone > having to code their own. > > I've taken your code and re-worked it a bit to: > > - Use the PgJDBC WrappedFactory rather than re-implementing an > effectively identical one; > > - Separate the basic construction process of the SSLSocketFactory > from its configuration, providing an abstract superclass that does > the construction via a few simple method calls, so someone who needs > different configuration sources/methods can re-use the code; > > - Provided a concrete subclass of the above that gets its configuration > from system properties and uses the superclass methods to construct > the factory. This class has the same functionality as what you posted > originally plus a few additional configuration options for additional > use cases like non-default KeyStore types (pkcs12 etc); and > > - Added more extensive JavaDoc > > It's also possible to pass your own KeyStore and/or TrustStore instances > directly if your app has to do something funky like manage an in-memory > KeyStore. > > What I haven't done yet is tested it! This is a preview so you can > comment on it and tell me if you think I've gone completely off the > rails or not. > > The simplest uses of the code remain as simple as they were with your > original version. You just set some system properties and specify > SysPropCertAuthFactory as the sslsocketfactory class. More complex use > cases become possible without having to re-implement the whole lot - in > particular, if you can't fetch your configuration from system properties > you can provide alternative mechanisms to look it up without having to > re-write all the JSSE crap. > > So: thanks VERY much for posting this code. My original demos weren't > flexible enough to be included in PgJDBC, and by posting this you really > helped motivate me to try to turn your code and my original demoes into > something that _was_. > > What do you think? Note that you'll need PgJDBC on your classpath to > complile this now, because it uses org.postgresql.ssl.WrappedFactory . > > -- > Craig Ringer
Attachment
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
Attachment
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 > > > >
On 05/19/2011 04:11 PM, Marc-André Laverdière wrote: > 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 :) So long as the option exists, I'm happy. I think it's a reasonable idea to try to auto-detect it by default. > Now, would you please elaborate on those todos? Whoops, I never meant to send the code to you with those in it. The latter one no longer applies, it's resolved. The first one isn't important for now. The main use case is if you want to add additional trusted certs without "hiding" the system trust database. It's a separate task and now that it's possible to pass your own TrustManager can be done by apps that need it without modifying AbstractCertAuthFactory at all. So both may be removed. Thanks for pointing that out. I'll see if I can put together an example X509TrustManager that tries to verify trust against an app-supplied KeyStore first and failing that against the system store. I have one around that I can adapt, but won't be able to do that immediately as I have to get on with other work. -- Craig Ringer
Hi folks, It is not over... It is not in the CVS repository yet :D What would be the next step? My guess is that we need to update the testware for it. That sounds like a tricky proposition that would mess up a bunch of things here and there... Any ideas on how to do it best? Marc-André Laverdière Software Security Scientist Innovation Labs, Tata Consultancy Services Hyderabad, India On Thursday 19 May 2011 11:44 AM, Marc-André Laverdière wrote: > Hi guys, > > Here are some improvements based on the comments I saw so far. > I'm also adding a small cheapo tester. Definitely not JUnit-ready :) > > Marc-André Laverdière > Software Security Scientist > Innovation Labs, Tata Consultancy Services > Hyderabad, India > > On Thursday 19 May 2011 11:16 AM, Craig Ringer wrote: >> On 05/18/2011 11:32 PM, Kris Jurka wrote: >>> >>> >>> On Wed, 18 May 2011, Craig Ringer wrote: >>> >>>> Alas the SSLSocketFactory interface doesn't offer any really nice >>>> options like a way to pass a Properties instance to the ctor, >>> >>> That'd be an easy thing to fix, just add another case to >>> org.postgresql.ssl.MakeSSL#convert to pass through the provided >>> Properties object. I've thought about that in the past, but with no real >>> use case, I wasn't in a big rush to do so. >> >> OK, good to know. >> >> It wasn't a big issue for me in the end, as I landed up fetching my >> configuration using the String argument form as a key to an >> application-specific configuration provider. It's much easier to pass a >> String argument through various layers one often has over the JDBC >> driver, like an ORM or the like. >> >> -- >> Craig Ringer >> >> >>
On Tue, 24 May 2011, Marc-Andr? Laverdi?re wrote: > It is not over... It is not in the CVS repository yet :D > > What would be the next step? It was not clear to me that the discussion between you and Craig had resulted in a final code version. Apparently you think so. Craig do you concur? If so I'll take a look at it and see about committing it. > My guess is that we need to update the testware for it. That sounds like > a tricky proposition that would mess up a bunch of things here and > there... Any ideas on how to do it best? I don't think there's any good way to setup automated tests for things like client certificates which need significant server configuration. So I wouldn't bother with that. Perhaps some documentation updates would be in order, but I haven't looked at the code yet to know what might be appropriate. Kris Jurka
On 25/05/11 00:27, Kris Jurka wrote: > > > On Tue, 24 May 2011, Marc-Andr? Laverdi?re wrote: > >> It is not over... It is not in the CVS repository yet :D >> >> What would be the next step? > > It was not clear to me that the discussion between you and Craig had > resulted in a final code version. Apparently you think so. Craig do > you concur? I'm happy with the state of the code, but should really test it properly before signing off on that. In particular, I need to test PKCS#12 cert files and test a JECKS keystore containing multiple keys only one of which is valid to access Pg. On the other hand, I'm swamped at the moment and unsure if I'll get to that in a reasonable amount of time. The tests Marc-André wrote demonstrate the core functionality pretty well, and the code would be good to get into the official codebase to save others from duplicating the same work over and over as both Marc-André and I have each done already. Argh. I'm going to have to come back to that, as I have a backup server to fix. Maybe it's best if you have a look and see what you think of it, while I try to find some time to do some more testing. > Perhaps some documentation updates > would be in order, but I haven't looked at the code yet to know what > might be appropriate. Some documentation updates are definitely in order, to sit alongside the existing documentation for the non-validating ssl factory. By the way, I _do_ think it'd be useful to add support for constructing the socket factory with: FactoryClass(String arg, Properties jdbcProperties) ... where the properties argument contains all the Pg JDBC properties like the user name and password. It'd make it easier for apps to pass custom args into a socket factory, especially things like the password to the user's private key that they don't want to have to put in the sslocketfactoryarg string. I could also then produce a second version of the cert factory for people to use that got all its settings from the jdbc connection properties instead of the sytem properties. I wouldn't suggest adding that now, though, but maybe as a revision once the working code is already committed. -- Craig Ringer
Hello everybody, I haven't heard back about this testing... did anyone get time to do it? Marc-André Laverdière Software Security Scientist Innovation Labs, Tata Consultancy Services Hyderabad, India On 05/25/2011 07:09 AM, Craig Ringer wrote: > On 25/05/11 00:27, Kris Jurka wrote: >> >> >> On Tue, 24 May 2011, Marc-Andr? Laverdi?re wrote: >> >>> It is not over... It is not in the CVS repository yet :D >>> >>> What would be the next step? >> >> It was not clear to me that the discussion between you and Craig had >> resulted in a final code version. Apparently you think so. Craig do >> you concur? > > I'm happy with the state of the code, but should really test it properly > before signing off on that. In particular, I need to test PKCS#12 cert > files and test a JECKS keystore containing multiple keys only one of > which is valid to access Pg. > > On the other hand, I'm swamped at the moment and unsure if I'll get to > that in a reasonable amount of time. The tests Marc-André wrote > demonstrate the core functionality pretty well, and the code would be > good to get into the official codebase to save others from duplicating > the same work over and over as both Marc-André and I have each done already. > > Argh. I'm going to have to come back to that, as I have a backup server > to fix. Maybe it's best if you have a look and see what you think of it, > while I try to find some time to do some more testing. > >> Perhaps some documentation updates >> would be in order, but I haven't looked at the code yet to know what >> might be appropriate. > > Some documentation updates are definitely in order, to sit alongside the > existing documentation for the non-validating ssl factory. > > > By the way, I _do_ think it'd be useful to add support for constructing > the socket factory with: > > FactoryClass(String arg, Properties jdbcProperties) > > ... where the properties argument contains all the Pg JDBC properties > like the user name and password. It'd make it easier for apps to pass > custom args into a socket factory, especially things like the password > to the user's private key that they don't want to have to put in the > sslocketfactoryarg string. > > I could also then produce a second version of the cert factory for > people to use that got all its settings from the jdbc connection > properties instead of the sytem properties. > > I wouldn't suggest adding that now, though, but maybe as a revision once > the working code is already committed. > > -- > Craig Ringer
On 27/06/11 14:18, Marc-André Laverdière wrote: > Hello everybody, > > I haven't heard back about this testing... did anyone get time to do it? Argh. I've been head-down in work and completely forgot about it. -- Craig Ringer
Hi all,
Following the recent discussions about SSL support, here is a bit of
feedback on this version.
To fix the "TODO: Delegate trust checks to system trustmanager if
no match in our own trustmanager?" is fairly easy: just pass null to
SSLContext.init(...).
I would do this in createTrustManager:
KeyStore trustKs = loadKeyStore(trustStorePath, trustStorePass,
trustStoreType);
if (trustKs != null) {
TrustManagerFactory trustFactory =
TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
trustFactory.init(trustKs);
return trustFactory.getTrustManagers();
} else {
return null;
}
and this in loadKeyStore (instead of throwing an exception):
if (path == null || "".equals(path))
return null;
The null default also works for the SecureRandom if anyone isn't
specifying it.
(I would also do it do avoid potential NPEs perhaps in createKeyManager,
although there is no actual default.)
Since using PKCS#11 was discussed a few days ago on the mailing list
[1], I would also suggest a couple of changes, similar to what I had
done for Apache Tomcat [2].
Essentially, not all keystores are file-based (e.g. PKCS#11, NSS, OSX
KeychainStore, Windows store). The "NONE" file-path should be used for
those:
http://download.oracle.com/javase/6/docs/technotes/guides/security/jsse/JSSERefGuide.html#Customization
In this case, use a null InputStream (not necessarily a FileInputStream).
For PKCS#11, it's also useful to specify the provider (if the PKCS#11
configuration is in the JRE security configuration. This could be done
as follows (with an extra provider name parameter, which would be null
by default):
private static KeyStore loadKeyStoreOfType(String path, char[]
password, String type, String provider) throws IOException,
GeneralSecurityException{
// Load the keystore with the specified keystore type.
if (path == null || "".equals(path))
return null;
if (password == null)
throw new IllegalArgumentException("Password is null");
if (type == null)
type = KeyStore.getDefaultType();
InputStream fIn = null;
try{
KeyStore ks;
if (provider != null) {
ks = KeyStore.getInstance(type, provider);
} else {
ks = KeyStore.getInstance(type);
}
// Using "NONE" for keystore that are not file-based.
//
http://download.oracle.com/javase/6/docs/technotes/guides/security/jsse/JSSERefGuide.html#Customization
if (!"NONE".equals(path)) {
fIn = new FileInputStream(path);
}
ks.load(fIn, password);
return ks;
} finally{
if (fIn != null) {
fIn.close();
}
}
}
On 19/05/2011 08:27, 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?
>
>> 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() .
I would personally prefer not to try to have the driver guess the
keystore type blindly. SSL errors can be obscure enough for users will
little SSL experience as it is (like "Could not find matching cipher
suites" sometimes when it's a problem with the key/cert).
Having the user specify the type or fall back on
KeyStore.getDefaultType() seems more in line with the rest of the
documentation in the JSSR ref. guide.
If this is of interest, I have done a bit of work on trying to make it
easier to configure SSLContexts while keeping default values in
jSSLutils [3].
I found that creating an SSLContextFactory interface/abstract class
tends to make this sort of configuration easier, as you don't need to
extend the SSLSocketFactory as much.
A similar approach could be used in the JDBC driver, but it would
require the creation of such an interface and modifications to
"MakeSSL". It might be too much.
Best wishes,
Bruno.
[1] http://archives.postgresql.org/pgsql-jdbc/2011-11/msg00015.php
[2] https://issues.apache.org/bugzilla/show_bug.cgi?id=43094
[3] http://www.jsslutils.org/