Re: [HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative
On Mon, Aug 21, 2017 at 6:21 AM, Daniel Gustafsson wrote: >> On 19 Aug 2017, at 23:13, Thomas Munro wrote: >>> I guess it should have a fallback definition, though I don't know what >>> it should be. >> >> Or maybe the guc should only exist if SSL_LIBRARY is defined? > > I think the intended use case of the GUC should drive the decision on > fallback. > If the GUC isn’t supposed to be a way to figure out if the server was built > with SSL support, then not existing in non-SSL backends is fine. If, however, > we want to allow using the GUC to see if the server has SSL support, then > there > needs to be a “None” or similar value for that case. Only GUCs related to debugging have their existence defined based on a #define, so it seems to me that if Postgres is compiled without any SSL support, this parameter should still be visible, but set to "none". -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative
> On 19 Aug 2017, at 23:13, Thomas Munro wrote: > > On Sun, Aug 20, 2017 at 8:10 AM, Thomas Munro > mailto:thomas.mu...@enterprisedb.com>> wrote: >> On Fri, Aug 18, 2017 at 2:14 AM, Daniel Gustafsson wrote: >>> Attached is an updated set of patches, rebased on top of master, with bug >>> fixes >>> and additional features missing in the first set. While not complete >>> (yet), in >>> case anyone is testing this I’d rather send a fresh batch rather than >>> sitting >>> on them too long while I keep hacking at the docs. While not every part of >>> this rather large changeset has been touched, this includes all the patches >>> for >>> completeness sake. >> >> Hi, >> >> +#if defined(USE_OPENSSL) || defined(USE_SECURETRANSPORT) >> #define USE_SSL >> +#if defined(USE_OPENSSL) >> +#define SSL_LIBRARY "OpenSSL" >> +#elif defined(USE_SECURETRANSPORT) >> +#define SSL_LIBRARY "Secure Transport" >> +#endif >> #endif >> >> If you configure with neither --with-securetransport nor >> --with-openssl then SSL_LIBRARY finishes up undefined, and then guc.c >> doesn't compile: >> >> ccache gcc -Wall -Wmissing-prototypes -Wpointer-arith >> -Wdeclaration-after-statement -Wendif-labels >> -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing >> -fwrapv -fexcess-precision=standard -g -O2 -I. -I. >> -I../../../../src/include -D_GNU_SOURCE -c -o guc.o guc.c >> guc.c:3309:3: error: ‘SSL_LIBRARY’ undeclared here (not in a function) >> SSL_LIBRARY, >> ^~~ >> >> I guess it should have a fallback definition, though I don't know what >> it should be. > > Or maybe the guc should only exist if SSL_LIBRARY is defined? I think the intended use case of the GUC should drive the decision on fallback. If the GUC isn’t supposed to be a way to figure out if the server was built with SSL support, then not existing in non-SSL backends is fine. If, however, we want to allow using the GUC to see if the server has SSL support, then there needs to be a “None” or similar value for that case. Personally I think there is risk of regrets down the line if this GUC is used for two things, but thats more of a gut feeling than scientifically studied. Clearly there shouldn’t be a compilation error in either case, sorry about missing that in the submission. cheers ./daniel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative
On Sun, Aug 20, 2017 at 8:10 AM, Thomas Munro wrote: > On Fri, Aug 18, 2017 at 2:14 AM, Daniel Gustafsson wrote: >> Attached is an updated set of patches, rebased on top of master, with bug >> fixes >> and additional features missing in the first set. While not complete (yet), >> in >> case anyone is testing this I’d rather send a fresh batch rather than sitting >> on them too long while I keep hacking at the docs. While not every part of >> this rather large changeset has been touched, this includes all the patches >> for >> completeness sake. > > Hi, > > +#if defined(USE_OPENSSL) || defined(USE_SECURETRANSPORT) > #define USE_SSL > +#if defined(USE_OPENSSL) > +#define SSL_LIBRARY "OpenSSL" > +#elif defined(USE_SECURETRANSPORT) > +#define SSL_LIBRARY "Secure Transport" > +#endif > #endif > > If you configure with neither --with-securetransport nor > --with-openssl then SSL_LIBRARY finishes up undefined, and then guc.c > doesn't compile: > > ccache gcc -Wall -Wmissing-prototypes -Wpointer-arith > -Wdeclaration-after-statement -Wendif-labels > -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing > -fwrapv -fexcess-precision=standard -g -O2 -I. -I. > -I../../../../src/include -D_GNU_SOURCE -c -o guc.o guc.c > guc.c:3309:3: error: ‘SSL_LIBRARY’ undeclared here (not in a function) >SSL_LIBRARY, >^~~ > > I guess it should have a fallback definition, though I don't know what > it should be. Or maybe the guc should only exist if SSL_LIBRARY is defined? I mean #if defined(SSL_LIBRARY) around this: + { + /* Can't be set in postgresql.conf */ + {"ssl_library", PGC_INTERNAL, PRESET_OPTIONS, + gettext_noop("Shows the SSL library used."), + NULL, + GUC_REPORT | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE + }, + &ssl_library_string, + SSL_LIBRARY, + NULL, NULL, NULL + }, -- Thomas Munro http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative
On Fri, Aug 18, 2017 at 2:14 AM, Daniel Gustafsson wrote: > Attached is an updated set of patches, rebased on top of master, with bug > fixes > and additional features missing in the first set. While not complete (yet), > in > case anyone is testing this I’d rather send a fresh batch rather than sitting > on them too long while I keep hacking at the docs. While not every part of > this rather large changeset has been touched, this includes all the patches > for > completeness sake. Hi, +#if defined(USE_OPENSSL) || defined(USE_SECURETRANSPORT) #define USE_SSL +#if defined(USE_OPENSSL) +#define SSL_LIBRARY "OpenSSL" +#elif defined(USE_SECURETRANSPORT) +#define SSL_LIBRARY "Secure Transport" +#endif #endif If you configure with neither --with-securetransport nor --with-openssl then SSL_LIBRARY finishes up undefined, and then guc.c doesn't compile: ccache gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -O2 -I. -I. -I../../../../src/include -D_GNU_SOURCE -c -o guc.o guc.c guc.c:3309:3: error: ‘SSL_LIBRARY’ undeclared here (not in a function) SSL_LIBRARY, ^~~ I guess it should have a fallback definition, though I don't know what it should be. -- Thomas Munro http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative
On Thu, Aug 3, 2017 at 11:26 PM, Daniel Gustafsson wrote: >> On 03 Aug 2017, at 19:27, Michael Paquier wrote: >> There were no APIs to get the TLS finish message last time I looked at OSX >> stuff, which mattered for tls-unique. It would be nice if we could get one. > > Yeah, AFAICT there is no API for that. Perhaps my last words were confusing. I meant that it would be nice to get at least one type of channel binding working. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative
> On 03 Aug 2017, at 19:27, Michael Paquier wrote: > > On Thu, Aug 3, 2017 at 12:02 PM, Daniel Gustafsson wrote: >> In https://postgr.es/m/69db7657-3f9d-4d30-8a4b-e06034251...@yesql.se I >> presented a WIP patch for adding support for the Apple Secure Transport SSL >> library on macOS as, an alternative to OpenSSL. That patch got put on the >> backburner for a bit, but I’ve now found the time to make enough progress to >> warrant a new submission for discussions on this (and hopefully help >> hacking). >> >> It is a drop-in replacement for the OpenSSL code, and supports all the same >> features and options, except for two things: compression is not supported and >> the CRL cannot be loaded from a plain PEM file. A Keychain must be used for >> that instead. > > Is there a set of APIs to be able to get server certificate for the > frontend and the backend, and generate a hash of it? That matters for > channel binding support of SCRAM for tls-server-end-point. I believe we can use SSLCopyPeerTrust() for that. Admittedly I haven’t looked at that yet so need to get my head around channel binding, but it seems to fit the bill. > There were no APIs to get the TLS finish message last time I looked at OSX > stuff, which mattered for tls-unique. It would be nice if we could get one. Yeah, AFAICT there is no API for that. cheers ./daniel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative
On Thu, Aug 3, 2017 at 12:02 PM, Daniel Gustafsson wrote: > In https://postgr.es/m/69db7657-3f9d-4d30-8a4b-e06034251...@yesql.se I > presented a WIP patch for adding support for the Apple Secure Transport SSL > library on macOS as, an alternative to OpenSSL. That patch got put on the > backburner for a bit, but I’ve now found the time to make enough progress to > warrant a new submission for discussions on this (and hopefully help hacking). > > It is a drop-in replacement for the OpenSSL code, and supports all the same > features and options, except for two things: compression is not supported and > the CRL cannot be loaded from a plain PEM file. A Keychain must be used for > that instead. Is there a set of APIs to be able to get server certificate for the frontend and the backend, and generate a hash of it? That matters for channel binding support of SCRAM for tls-server-end-point. There were no APIs to get the TLS finish message last time I looked at OSX stuff, which mattered for tls-unique. It would be nice if we could get one. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative
> On 03 Aug 2017, at 13:06, Heikki Linnakangas wrote: > > On 08/03/2017 01:02 PM, Daniel Gustafsson wrote: >> >> The frontend has support for using PEM files for certificates and keys. It >> can >> also look up the key for the certificate in a Keychain, or both certificate >> and >> key in a Keychain. The root certificate is still just read from a PEM file. > > Why can't you have the root certificate in the keychain, too? Just not > implemented yet, or is there some fundamental problem with it? Just not implemented yet, awaiting Keychain discussions. >> The existence of an sslcert config trumps a keychain, but when a keychain is >> used I’m currently using the sslcert parameter (awaiting a discussion on how >> to >> properly do this) for the certificate label required to search the keychain. >> There is a new frontend parameter called “keychain” with which a path to a >> custom Keychain file can be passed. If set, this Keychain will be searched >> as >> well as the default. If not, only the default user Keychain is used. There >> is >> nothing that modifies the Keychain in this patch, it can read identities >> (certificates and its key) but not alter them in any way. > > OpenSSL also has a mechanism somewhat similar to the keychain, called > "engines". You can e.g. keep the private key corresponding a certificate on a > smart card, and speak to it with an OpenSSL "smart card reader" engine. If > you do that, the 'sslkey' syntax is ":". Perhaps we > should adopt that syntax here as well? For example, to read the client > certificate from the key chain, you would use libpq options like > "keychain=/home/heikki/my_keychain sslcert=keychain:my_client_cert”. Thats definitely an option, although it carries a bit redudancy in this case which can confuse users. With “keychain=/foo/bar.keychain sslcert=my_cert”, did the user mean a file called my_cert or is it a reference to a cert in the keychain? Nothing that strict parsing rules, good errormessages and documentation can’t solve but needs careful thinking. >> “keychain” is obviously a very Secure Transport specific name, and I >> personally >> think we should avoid that. Any new configuration added here should take >> future potential implementation into consideration such that avoid the need >> for >> lots of backend specific knobs. “sslcertstore” comes to mind as an >> alternative, but we’d also need parameters to point into the certstore for >> finding what we need. Another option would be to do a URL based scheme >> perhaps. > > I wouldn't actually mind using implementation-specific terms like "keychain" > here. It makes it clear that it's implementation-specific. I think it would > be confusing, to use the same generic option name, like "sslcertstore", for > both a macOS keychain and e.g. the private key store in Windows. Or GNU > Keyring. In the worst case, you might even have multiple such "key stores" on > the same system, so you'd anyways need a way to specify which one you mean. Thats a good point. > Actually, perhaps it should be made even more explicit, and call it > "secure_transport_keychain". That's quite long, though. Yeah, secure_transport_ is a pretty long prefix. > Wrt. keychains, is there a system-global or per-user keychain in macOS? And > does this patch use them? If you load a CRL into a global keychain, does it > take effect? Each user has a default Keychain , and on top of that you can create as many Keychains as you want (a Keychain is really just a database file containing secret things). The frontend use the default one for lookups unless the keychain parameter is set in which case it uses both. When evaluating trust, Secure Transport will use the default Keychain even if not explicitly opened (as in the backend code). It does however only search for intermediate certificates and not root certs/CRLs. Adding a policy object for using CRLs should force it to afaik, but we’d need to support additional keychains (if only to be able to test without polluting the default). >> Testing >> === >> In order to test this we need to provide an alternative to the openssl calls >> in >> src/test/ssl/Makefile for Secure Transport. > > Those openssl commands are only needed to re-generate the test certificates. > The certificates are included in the git repository, so you only need to > re-generate them if you want to modify them or add new ones. I think it's OK > to require the openssl tool for that, it won't be needed just to run the test > suite. Ah, of course. We still need support for re-generating Keychains (or at all generate them in case we don’t want binaries in the repository). >> Documentation >> = >> I have started fiddling with this a little, but to avoid spending time on the >> wrong thing I have done very little awaiting the outcome of discussions here. >> I have tried to add lots of comments to the code however, to explain the >> quirks >> of Secure Transport.
Re: [HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative
On 08/03/2017 01:02 PM, Daniel Gustafsson wrote: In https://postgr.es/m/69db7657-3f9d-4d30-8a4b-e06034251...@yesql.se I presented a WIP patch for adding support for the Apple Secure Transport SSL library on macOS as, an alternative to OpenSSL. That patch got put on the backburner for a bit, but I’ve now found the time to make enough progress to warrant a new submission for discussions on this (and hopefully help hacking). Hooray! Keychains = The frontend has support for using PEM files for certificates and keys. It can also look up the key for the certificate in a Keychain, or both certificate and key in a Keychain. The root certificate is still just read from a PEM file. Why can't you have the root certificate in the keychain, too? Just not implemented yet, or is there some fundamental problem with it? The existence of an sslcert config trumps a keychain, but when a keychain is used I’m currently using the sslcert parameter (awaiting a discussion on how to properly do this) for the certificate label required to search the keychain. There is a new frontend parameter called “keychain” with which a path to a custom Keychain file can be passed. If set, this Keychain will be searched as well as the default. If not, only the default user Keychain is used. There is nothing that modifies the Keychain in this patch, it can read identities (certificates and its key) but not alter them in any way. OpenSSL also has a mechanism somewhat similar to the keychain, called "engines". You can e.g. keep the private key corresponding a certificate on a smart card, and speak to it with an OpenSSL "smart card reader" engine. If you do that, the 'sslkey' syntax is ":name>". Perhaps we should adopt that syntax here as well? For example, to read the client certificate from the key chain, you would use libpq options like "keychain=/home/heikki/my_keychain sslcert=keychain:my_client_cert". “keychain” is obviously a very Secure Transport specific name, and I personally think we should avoid that. Any new configuration added here should take future potential implementation into consideration such that avoid the need for lots of backend specific knobs. “sslcertstore” comes to mind as an alternative, but we’d also need parameters to point into the certstore for finding what we need. Another option would be to do a URL based scheme perhaps. I wouldn't actually mind using implementation-specific terms like "keychain" here. It makes it clear that it's implementation-specific. I think it would be confusing, to use the same generic option name, like "sslcertstore", for both a macOS keychain and e.g. the private key store in Windows. Or GNU Keyring. In the worst case, you might even have multiple such "key stores" on the same system, so you'd anyways need a way to specify which one you mean. Actually, perhaps it should be made even more explicit, and call it "secure_transport_keychain". That's quite long, though. Wrt. keychains, is there a system-global or per-user keychain in macOS? And does this patch use them? If you load a CRL into a global keychain, does it take effect? Testing === In order to test this we need to provide an alternative to the openssl calls in src/test/ssl/Makefile for Secure Transport. Those openssl commands are only needed to re-generate the test certificates. The certificates are included in the git repository, so you only need to re-generate them if you want to modify them or add new ones. I think it's OK to require the openssl tool for that, it won't be needed just to run the test suite. Documentation = I have started fiddling with this a little, but to avoid spending time on the wrong thing I have done very little awaiting the outcome of discussions here. I have tried to add lots of comments to the code however, to explain the quirks of Secure Transport. I think this patch in general is in very good shape, and the next step is to write the documentation. In particular, I'd like to see documentation on how the keychain stuff should work. It'll be easier to discuss the behavior and the interactions, once it's documented. In fact, better to write the documentation for that now, and not bother to change the code, until after we've discussed and are happy with the documented behavior. I went into this thinking I would write a README for how to implement a new SSL library. But in the end, turns out the refactoring that went into our SSL code earlier made that part almost too easy to warrant that. It’s really quite straightforward. That's nice to hear! - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative
In https://postgr.es/m/69db7657-3f9d-4d30-8a4b-e06034251...@yesql.se I presented a WIP patch for adding support for the Apple Secure Transport SSL library on macOS as, an alternative to OpenSSL. That patch got put on the backburner for a bit, but I’ve now found the time to make enough progress to warrant a new submission for discussions on this (and hopefully help hacking). It is a drop-in replacement for the OpenSSL code, and supports all the same features and options, except for two things: compression is not supported and the CRL cannot be loaded from a plain PEM file. A Keychain must be used for that instead. Current state = The frontend and backend are implemented more or less fully, the two main things missing being the CRL support (further details below) and loading DH files (to support the GUC added in c0a15e07cd). All non-CRL tests but one are passing. The failing test is in the frontend and it also fails when running against an OpenSSL backend, but I can’t find where the logic is flawed and could do with some help there. Threads === Just like the CFLocaleCopyCurrent() call referenced in postmaster.c, the Secure Transport APIs makes the process multithreaded. To keep this out of the postmaster, and contained in the backend, I’ve moved all functionality to open_server and left init empty. I could definitely need some clues on how to properly handle this, or if it’s a complete showstopper. Keychains = The frontend has support for using PEM files for certificates and keys. It can also look up the key for the certificate in a Keychain, or both certificate and key in a Keychain. The root certificate is still just read from a PEM file. The existence of an sslcert config trumps a keychain, but when a keychain is used I’m currently using the sslcert parameter (awaiting a discussion on how to properly do this) for the certificate label required to search the keychain. There is a new frontend parameter called “keychain” with which a path to a custom Keychain file can be passed. If set, this Keychain will be searched as well as the default. If not, only the default user Keychain is used. There is nothing that modifies the Keychain in this patch, it can read identities (certificates and its key) but not alter them in any way. The backend is only supporting PEM files at this point. Once we have support for Keychains, we can however use them for additionally supporting other Secure Transport features like OCSP etc. “keychain” is obviously a very Secure Transport specific name, and I personally think we should avoid that. Any new configuration added here should take future potential implementation into consideration such that avoid the need for lots of backend specific knobs. “sslcertstore” comes to mind as an alternative, but we’d also need parameters to point into the certstore for finding what we need. Another option would be to do a URL based scheme perhaps. Certificate Revocation == Secure Transport supports loading CRL lists into Keychain files, the command line certtool can for example do that. When doing the trust evaluation on the connection, a policy can be added which enables revocation checking via CRL. I have however been unable to figure out how to load a CRL list programmatically, and as far as I can tell there is no API for this. The certtool utility is using the low-level CSSM APIs for this it seems, but adding that level of complexity doesn’t seem worth it for us to maintain (I did a test and it turned big, ugly and messy). Unless someone knows how to implement this, we’d be limited to requiring the use of a Keychain file for CRL support. This would break drop-in replacement support, but supporting certificate revocation seems more important. Platform Support I’ve tested this on 10.11 El Capitan and 10.12 Sierra, which are the systems I have access to. Supporting prairiedog and dromedary in the buildfarm will probably be too hard (if at all possible), but down to 10.9 Mavericks should be quite possible (if someone has the required systems to test on). It adds a dependency on Core Foundation on top of Secure Transport, no other macOS APIs are used. Testing === In order to test this we need to provide an alternative to the openssl calls in src/test/ssl/Makefile for Secure Transport. On top of that, code to generate Keychains is required. The certtool application can do all the Keychain generations (I think) but this is still left open. The main thing to figure out here is how to avoid having to type in the Keychain password in a modal GUI that Secure Transport pops up. Since a Keychain can be passwordless it should be doable, but the right certtool incantations for that is still evading me. I’ve included a show-and-tell patch for this which I’ve used locally for testing during hacking. Documentation = I have started fiddling with this a little, but to avoid spending time on the