The attached patch implements NSS (Network Security Services) [0] with the
required NSPR runtime [1] as a TLS backend for PostgreSQL. While all sslmodes are implemented and work for the most part, the patch is *not* ready yet but I wanted to show progress early so that anyone interested in this can help out with testing and maybe even hacking. Why NSS? Well. It shares no lineage with OpenSSL making it not just an alternative by fork but a 100% alternative. It's also actively maintained, is readily available on many platforms where PostgreSQL is popular and has a FIPS mode which doesn't require an EOL'd library. And finally, I was asked nicely with the promise of a free beverage, an incentive as good as any. Differences with OpenSSL ------------------------ NSS does not use certificates and keys on the filesystem, it instead uses a certificate database in which all certificates, keys and CRL's are loaded. A set of tools are provided to work with the database, like: certutil, crlutil, pk12util etc. We could support plain PEM files as well, and load them into a database ourselves but so far I've opted for just using what is already in the database. This does mean that new GUCs are needed to identify the database. I've mostly repurposed the existing ones for cert/key/crl, but had to invent a new one for the database. Maybe there should be an entirely new set? This needs to be discussed with not only NSS in mind but for additional as-of-yet unknown backends we might get (SChannel comes to mind). NSS also supports partial chain validation per default (as do many other TLS libraries) where OpenSSL does not. I haven't done anything about that just yet, thus there is a failing test as a reminder to address it. The documentation of NSS/NSPR is unfortunately quite poor and often times outdated or simply nonexisting. Cloning the repo and reading the source code is the only option for parts of the API. Featurewise there might be other things we can make use of in NSS which doesn't exist in OpenSSL, but for now I've tried to keep them aligned. Known Bugs and Limitations (in this version of the patch) --------------------------------------------------------- The frontend doesn't attempt to verify whether the specified CRL exists in the database or not. This can be done with pretty much the same code as in the backend, except that we don't have the client side certificate loaded so we either need to read it back from the database, or parse a list of all CRLs (which would save us from having the cert in local memory which generally is a good thing to avoid). pgtls_read is calling PR_Recv which works fine for communicating with an NSS backend cluster, but hangs waiting for IO when communicating with an OpenSSL backend cluster. Using PR_Read reverses the situation. This is probably a simple bug but I haven't had time to track it down yet. The below shifts between the two for debugging. - nread = PR_Recv(conn->pr_fd, ptr, len, 0, PR_INTERVAL_NO_WAIT); + nread = PR_Read(conn->pr_fd, ptr, len); Passphrase handling in the backend is broken, more on that under TODO. There are a few failing tests and a few skipped ones for now, but the majority of the tests pass. Testing ------- In order for the TAP framework to be able to handle backends with different characteristics I've broken up SSLServer.pm into a set of modules: SSL::Server SSL::Backend::NSS SSL::Backend::OpenSSL The SSL tests import SSL::Server which in turn imports the appropriate backend module in order to perform backend specific setup tasks. The backend used should be transparent for the TAP code when it comes to switching server certs etc. So far I've used foo|bar in the matching regex to provide alternative output, and SKIP blocks for tests that don't apply. There might be neater ways to achieve this, but I was trying to avoid having separate test files for the different backends. The certificate databases can be created with a new nssfiles make target in src/test/ssl, which use the existing files (and also depend on OpenSSL which I don't think is a problematic dependency for development environments). To keep it simple I've named the certificates in the NSS database after the filenames, this isn't really NSS best-practices but it makes for an easier time reading the tests IMO. If this direction is of interest, extracting into to a separate patch for just setting up the modules and implementing OpenSSL without a new backend is probably the next step. TODO ---- This patch is a work in progress, and there is work left to do, below is a dump of what is left to fix before this can be considered a full implementation for review. Most of these items have more documentation in the code comments. * The split between init and open needs to be revisited, especially in frontend where we have a bit more freedom. It remains to be seen if we can do better in the backend part. * Documentation, it's currently not even started * Windows support. I've hacked mostly using Debian and have tested versions of the patch on macOS, but not Windows so far. * Figure out how to handle cipher configuration. Getting a set of ciphers that result in a useable socket isn't as easy as with OpenSSL, and policies seem much more preferred. At the very least this needs to be solidly documented. * The rules in src/test/ssl/Makefile for generating certificate databases can probably be generalized into a smaller set of rules based on wildcards. * The password callback on server-side won't be invoked at server start due to init happening in be_tls_open, so something needs to be figured out there. Maybe attempt to open the database with a throw-away context in init just to invoke the password callback? * Identify code duplicated between frontend and backend and try to generalize. * Make sure the handling the error codes correctly in the certificate and auth callbacks to properly handle self-signed certs etc. * Tidy up the tests which are partially hardwired for NSS now to make sure there are no regressions for OpenSSL. * All the code using OpenSSL which isn't the libpq communications parts, like pgcrypto, strong_random, sha2, SCRAM et.al * Review language in code comments and run pgindent et.al * Settle on a minimum required version. I've been using NSS 3.42 and NSPR 4.20 simply since they were the packages Debian wanted to install for me, but I'm quite convinced that we can go a bit lower (featurewise we can go much lower but there are bugfixes in recent versions that we might want to include). Anything lower than a version supporting TLSv1.3 seems like an obvious no-no. I'd be surprised if this is all, but that's at least a start. There isn't really a playbook on how to add a new TLS backend, but I'm hoping to be able to summarize the required bits and pieces in README.SSL once this is a bit closer to completion. My plan is to keep hacking at this to have it reviewable for the 14 cycle, so if anyone has an interest in NSS, then I would love to hear feedback on how it works (and doesn't work). The 0001 patch contains the full NSS support, and 0002 is a fix for the pgstat abstraction which IMO leaks backend implementation details. This needs to go on it's own thread, but since 0001 fails without it I've included it here for simplicity sake for now. cheers ./daniel [0] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS [1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSPR ![]() ![]() |
> On 15 May 2020, at 22:46, Daniel Gustafsson <[hidden email]> wrote:
> The 0001 patch contains the full NSS support, and 0002 is a fix for the pgstat > abstraction which IMO leaks backend implementation details. This needs to go > on it's own thread, but since 0001 fails without it I've included it here for > simplicity sake for now. The attached 0001 and 0002 are the same patchseries as before, but with the OpenSSL test module fixed and a rebase on top of the current master. cheers ./daniel ![]() ![]() |
> On 25 Jun 2020, at 17:39, Daniel Gustafsson <[hidden email]> wrote:
Another rebase to resolve conflicts with the recent fixes in the SSL tests, as
> >> On 15 May 2020, at 22:46, Daniel Gustafsson <[hidden email]> wrote: > >> The 0001 patch contains the full NSS support, and 0002 is a fix for the pgstat >> abstraction which IMO leaks backend implementation details. This needs to go >> on it's own thread, but since 0001 fails without it I've included it here for >> simplicity sake for now. > > The attached 0001 and 0002 are the same patchseries as before, but with the > OpenSSL test module fixed and a rebase on top of the current master. well as some minor cleanup. cheers ./daniel ![]() ![]() |
On Fri, Jul 3, 2020 at 11:51 PM Daniel Gustafsson <[hidden email]> wrote:
> > On 25 Jun 2020, at 17:39, Daniel Gustafsson <[hidden email]> wrote: > >> On 15 May 2020, at 22:46, Daniel Gustafsson <[hidden email]> wrote: > >> The 0001 patch contains the full NSS support, and 0002 is a fix for the pgstat > >> abstraction which IMO leaks backend implementation details. This needs to go > >> on it's own thread, but since 0001 fails without it I've included it here for > >> simplicity sake for now. > > > > The attached 0001 and 0002 are the same patchseries as before, but with the > > OpenSSL test module fixed and a rebase on top of the current master. > > Another rebase to resolve conflicts with the recent fixes in the SSL tests, as > well as some minor cleanup. Hi Daniel, Thanks for blazing the trail for other implementations to coexist in the tree. I see that cURL (another project Daniel works on) supports a lot of TLS implementations[1]. I recognise 4 other library names from that table as having appeared on this mailing list as candidates for PostgreSQL support complete with WIP patches, including another one from you (Apple Secure Transport). I don't have strong views on how many and which libraries we should support, but I was curious how many packages depend on libss1.1, libgnutls30 and libnss3 in the Debian package repos in my sources.list, and I came up with OpenSSL = 820, GnuTLS = 342, and NSS = 87. I guess Solution.pm needs at least USE_NSS => undef for this not to break the build on Windows. Obviously cfbot is useless for testing this code, since its build script does --with-openssl and you need --with-nss, but it still shows us one thing: with your patch, a --with-openssl build is apparently broken: /001_ssltests.pl .. 1/93 Bailout called. Further testing stopped: system pg_ctl failed There are some weird configure-related hunks in the patch: + -runstatedir | --runstatedir | --runstatedi | --runstated \ ...[more stuff like that]... -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62)) +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31)) I see the same when I use Debian's autoconf, but not FreeBSD's or MacPorts', despite all being version 2.69. That seems to be due to non-upstreamed changes added by the Debian maintainers (I see the off_t thing mentioned in /usr/share/doc/autoconf/changelog.Debian.gz). I think you need to build a stock autoconf 2.69 or run autoconf on a non-Debian system. I installed libnss3-dev on my Debian box and then configure had trouble locating and understanding <ssl.h>, until I added --with-includes=/usr/include/nss:/usr/include/nspr. I suspect this is supposed to be done with pkg-config nss --cflags somewhere in configure (or alternatively nss-config --cflags, nspr-config --cflags, I don't know, but we're using pkg-config for other stuff). I installed the Debian package libnss3-tools (for certutil) and then, in src/test/ssl, I ran make nssfiles (I guess that should be automatic?), and make check, and I got this far: Test Summary Report ------------------- t/001_ssltests.pl (Wstat: 3584 Tests: 93 Failed: 14) Failed tests: 14, 16, 18-20, 24, 27-28, 54-55, 78-80 91 Non-zero exit status: 14 You mentioned some were failing in this WIP -- are those results you expect? [1] https://curl.haxx.se/docs/ssl-compared.html |
> On 10 Jul 2020, at 07:10, Thomas Munro <[hidden email]> wrote:
> > On Fri, Jul 3, 2020 at 11:51 PM Daniel Gustafsson <[hidden email]> wrote: >>> On 25 Jun 2020, at 17:39, Daniel Gustafsson <[hidden email]> wrote: >>>> On 15 May 2020, at 22:46, Daniel Gustafsson <[hidden email]> wrote: >>>> The 0001 patch contains the full NSS support, and 0002 is a fix for the pgstat >>>> abstraction which IMO leaks backend implementation details. This needs to go >>>> on it's own thread, but since 0001 fails without it I've included it here for >>>> simplicity sake for now. >>> >>> The attached 0001 and 0002 are the same patchseries as before, but with the >>> OpenSSL test module fixed and a rebase on top of the current master. >> >> Another rebase to resolve conflicts with the recent fixes in the SSL tests, as >> well as some minor cleanup. > > Hi Daniel, > > Thanks for blazing the trail for other implementations to coexist in > the tree. I see that cURL (another project Daniel works on) > supports a lot of TLS implementations[1]. The list on that URL is also just a selection, the total count is 10 (not counting OpenSSL forks) IIRC, after axing support for a few lately. OpenSSL clearly has a large mindshare but the gist of it is that there exist quite a few alternatives each with their own strengths. > I recognise 4 other library > names from that table as having appeared on this mailing list as > candidates for PostgreSQL support complete with WIP patches, including > another one from you (Apple Secure Transport). I don't have strong > views on how many and which libraries we should support, I think it's key to keep in mind *why* it's relevant to provide options in the first place, after all, as they must be 100% interoperable one can easily argue for a single one being enough. We need to to look at what they offer users on top of just a TLS connection, like: managed certificate storage like for example macOS Keychains, FIPS certification, good platform availability and/or OS integration etc. If all a library offers is "not being OpenSSL" then it's not clear that we're adding much value by spending the cycles to support it. My personal opinion is that we should keep it pretty limited, not least to lessen the burden of testing and during feature development. Supporting a new library comes with requirements on both the CFBot as well as the buildfarm, not to mention on developers who dabble in that area of the code. The goal should IMHO be to make it trivial for every postgres installation to use TLS regardless of platform and experience level with the person installing it. The situation is a bit different for curl where we have as a goal to provide enough alternatives such that every platform can have a libcurl/curl more or less regardless of what it contains. As a consequence, we have around 80 CI jobs to test each pull request to provide ample coverage. Being a kitchen- sink is really hard work. > but I was > curious how many packages depend on libss1.1, libgnutls30 and libnss3 > in the Debian package repos in my sources.list, and I came up with > OpenSSL = 820, GnuTLS = 342, and NSS = 87. I don't see a lot of excitement over GnuTLS lately, but Debian shipping it due to (I believe) licensing concerns with OpenSSL does help it along. In my experience, platforms with GnuTLS easily available also have OpenSSL easily available. > I guess Solution.pm needs at least USE_NSS => undef for this not to > break the build on Windows. Thanks, I'll fix (I admittedly haven't tried this at all on Windows yet). > Obviously cfbot is useless for testing this code, since its build > script does --with-openssl and you need --with-nss, Right, this is a CFBot problem with any patch that require specific autoconf flags to be excercised. I wonder if we can make something when we do CF app integration which can inject flags to a Travis pipeline in a safe manner? > but it still shows > us one thing: with your patch, a --with-openssl build is apparently > broken: > > /001_ssltests.pl .. 1/93 Bailout called. Further testing stopped: > system pg_ctl failed Humm .. I hate to say "it worked on my machine" but it did, but my TLS environment is hardly standard. Sorry for posting breakage, most likely this is a bug in the new test module structure that the patch introduce in order to support multiple backends for src/tests/ssl. I'll fix. > There are some weird configure-related hunks in the patch: > > + -runstatedir | --runstatedir | --runstatedi | --runstated \ > ...[more stuff like that]... > > -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62)) > +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31)) > > I see the same when I use Debian's autoconf, but not FreeBSD's or > MacPorts', despite all being version 2.69. That seems to be due to > non-upstreamed changes added by the Debian maintainers (I see the > off_t thing mentioned in /usr/share/doc/autoconf/changelog.Debian.gz). > I think you need to build a stock autoconf 2.69 or run autoconf on a > non-Debian system. Sigh, yes that's a Debianism that slipped through, again sorry about that. > I installed libnss3-dev on my Debian box and then configure had > trouble locating and understanding <ssl.h>, until I added > --with-includes=/usr/include/nss:/usr/include/nspr. I suspect this is > supposed to be done with pkg-config nss --cflags somewhere in > configure (or alternatively nss-config --cflags, nspr-config --cflags, > I don't know, but we're using pkg-config for other stuff). Yeah, that's a good point, I should fix that. Having a metric ton of TLS libraries in various versions around in my environment I've been Stockholm Syndromed to --with-includes to the point where I didn't even think to run without it. It should clearly be as easy to use as OpenSSL wrt autoconf. > I installed the Debian package libnss3-tools (for certutil) and then, > in src/test/ssl, I ran make nssfiles (I guess that should be > automatic?) Yes, it needs to run automatically for NSS builds on make check. > , and make check, and I got this far: > > Test Summary Report > ------------------- > t/001_ssltests.pl (Wstat: 3584 Tests: 93 Failed: 14) > Failed tests: 14, 16, 18-20, 24, 27-28, 54-55, 78-80 > 91 > Non-zero exit status: 14 > > You mentioned some were failing in this WIP -- are those results you expect? I'm not on my dev box at the moment, and I don't remember off the cuff, but that sounds higher than I remember. I wonder if I fat-fingered the regexes in the last version? Thanks for taking a look at the patch, I'll fix up the reported issues Monday at the latest. cheers ./daniel |
In reply to this post by Thomas Munro-5
On Fri, Jul 10, 2020 at 5:10 PM Thomas Munro <[hidden email]> wrote:
> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62)) > +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31)) > > I see the same when I use Debian's autoconf, but not FreeBSD's or > MacPorts', despite all being version 2.69. That seems to be due to > non-upstreamed changes added by the Debian maintainers (I see the > off_t thing mentioned in /usr/share/doc/autoconf/changelog.Debian.gz). By the way, Dagfinn mentioned that these changes were in fact upstreamed, and happened to be beta-released today[1], and are due out in ~3 months as 2.70. That'll be something for us to coordinate a bit further down the road. [1] https://lists.gnu.org/archive/html/autoconf/2020-07/msg00006.html |
In reply to this post by Daniel Gustafsson
On 5/15/20 4:46 PM, Daniel Gustafsson wrote: > > My plan is to keep hacking at this to have it reviewable for the 14 cycle, so > if anyone has an interest in NSS, then I would love to hear feedback on how it > works (and doesn't work). I'll be happy to help, particularly with Windows support and with some of the callback stuff I've had a hand in. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services |
In reply to this post by Daniel Gustafsson
> On 12 Jul 2020, at 00:03, Daniel Gustafsson <[hidden email]> wrote:
> Thanks for taking a look at the patch, I'll fix up the reported issues Monday > at the latest. A bit of life intervened, but attached is a new version of the patch which should work for OpenSSL builds, and have the other issues addressed as well. I took the opportunity to clean up the NSS tests to be more like the OpenSSL ones to lessen the impact on the TAP testcases. On my Debian box, using the standard NSS and NSPR packages, I get 6 failures which are essentially all around CRL handling. I'm going to circle back and look at what is missing there. This version also removes the required patch for statistics reporting as that has been committed in 6a5c750f3f72899f4f982f921d5bf5665f55651e. cheers ./daniel |
In reply to this post by Andrew Dunstan-8
> On 15 Jul 2020, at 20:35, Andrew Dunstan <[hidden email]> wrote:
> > On 5/15/20 4:46 PM, Daniel Gustafsson wrote: >> >> My plan is to keep hacking at this to have it reviewable for the 14 cycle, so >> if anyone has an interest in NSS, then I would love to hear feedback on how it >> works (and doesn't work). > > I'll be happy to help, particularly with Windows support and with some > of the callback stuff I've had a hand in. That would be fantastic, thanks! The password callback handling is still a TODO so feel free to take a stab at that since you have a lot of context on there. For Windows, I've include USE_NSS in Solution.pm as Thomas pointed out in this thread, but that was done blind as I've done no testing on Windows yet. cheers ./daniel |
In reply to this post by Daniel Gustafsson
> On 16 Jul 2020, at 00:16, Daniel Gustafsson <[hidden email]> wrote:
Taking a look at this, the issue was that I had fat-fingered the Makefile rules
> >> On 12 Jul 2020, at 00:03, Daniel Gustafsson <[hidden email]> wrote: > >> Thanks for taking a look at the patch, I'll fix up the reported issues Monday >> at the latest. > > A bit of life intervened, but attached is a new version of the patch which > should work for OpenSSL builds, and have the other issues addressed as well. I > took the opportunity to clean up the NSS tests to be more like the OpenSSL ones > to lessen the impact on the TAP testcases. On my Debian box, using the > standard NSS and NSPR packages, I get 6 failures which are essentially all > around CRL handling. I'm going to circle back and look at what is missing there. for generating the NSS databases. This is admittedly very messy at this point, partly due to trying to mimick OpenSSL filepaths/names to minimize the impact on tests and to keep OpenSSL/NSS tests as "optically" equivalent as I could. With this, I have one failing test ("intermediate client certificate is provided by client") which I've left failing since I believe the case should be supported by NSS. The issue is most likely that I havent figured out the right certinfo incantation to make it so (Mozilla hasn't strained themselves when writing documentation for this toolchain, or any part of NSS for that matter). The failing test when running with OpenSSL also remains, the issue is that the very first test for incorrect key passphrase fails, even though the server is behaving exactly as it should. Something in the test suite hackery breaks for that test but I've been unable to pin down what it is, any help on would be greatly appreciated. This version adds support for sslinfo on NSS for most the functions. In the process I realized that sslinfo never got the memo about SSL support being abstracted behind an API, so I went and did that as well. This part of the patch should perhaps be broken out into a separate patch/thread in case it's deemed interesting regardless of the evetual conclusion on this patch. Doing this removed a bit of duplication with the backend code, and some errorhandling moved to be-secure-openssl.c (originally added in d94c36a45ab45). As the original commit message states, they're mostly code hygiene with belts and suspenders, but if we deemed them valuable enough for a contrib module ISTM they should go into the backend as well. Adding a testcase for sslinfo is a TODO. Support pg_strong_random, sha2 and pgcrypto has been started, but it's less trivial as NSS/NSPR requires a lot more initialization and state than OpenSSL, so it needs a bit more thought. I've also done a rebase over todays HEAD, a pgindent pass and some cleanup here and there. cheers ./daniel |
In reply to this post by Daniel Gustafsson
On 7/15/20 6:18 PM, Daniel Gustafsson wrote: >> On 15 Jul 2020, at 20:35, Andrew Dunstan <[hidden email]> wrote: >> >> On 5/15/20 4:46 PM, Daniel Gustafsson wrote: >>> My plan is to keep hacking at this to have it reviewable for the 14 cycle, so >>> if anyone has an interest in NSS, then I would love to hear feedback on how it >>> works (and doesn't work). >> I'll be happy to help, particularly with Windows support and with some >> of the callback stuff I've had a hand in. > That would be fantastic, thanks! The password callback handling is still a > TODO so feel free to take a stab at that since you have a lot of context on > there. > > For Windows, I've include USE_NSS in Solution.pm as Thomas pointed out in this > thread, but that was done blind as I've done no testing on Windows yet. > OK, here is an update of your patch that compiles and runs against NSS under Windows (VS2019). In addition to some work that was missing in src/tools/msvc, I had to make a few adjustments, including: * strtok_r() isn't available on Windows. We don't use it elsewhere in the postgres code, and it seemed unnecessary to have reentrant calls here, so I just replaced it with equivalent strtok() calls. * We were missing an NSS implementation of pgtls_verify_peer_name_matches_certificate_guts(). I supplied a dummy that's enough to get it building cleanly, but that needs to be filled in properly. There is still plenty of work to go, but this seemed a sufficient milestone to report progress on. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services |
On 7/31/20 4:44 PM, Andrew Dunstan wrote: > On 7/15/20 6:18 PM, Daniel Gustafsson wrote: >>> On 15 Jul 2020, at 20:35, Andrew Dunstan <[hidden email]> wrote: >>> >>> On 5/15/20 4:46 PM, Daniel Gustafsson wrote: >>>> My plan is to keep hacking at this to have it reviewable for the 14 cycle, so >>>> if anyone has an interest in NSS, then I would love to hear feedback on how it >>>> works (and doesn't work). >>> I'll be happy to help, particularly with Windows support and with some >>> of the callback stuff I've had a hand in. >> That would be fantastic, thanks! The password callback handling is still a >> TODO so feel free to take a stab at that since you have a lot of context on >> there. >> >> For Windows, I've include USE_NSS in Solution.pm as Thomas pointed out in this >> thread, but that was done blind as I've done no testing on Windows yet. >> > > OK, here is an update of your patch that compiles and runs against NSS > under Windows (VS2019). > > > In addition to some work that was missing in src/tools/msvc, I had to > make a few adjustments, including: > > > * strtok_r() isn't available on Windows. We don't use it elsewhere in > the postgres code, and it seemed unnecessary to have reentrant calls > here, so I just replaced it with equivalent strtok() calls. > * We were missing an NSS implementation of > pgtls_verify_peer_name_matches_certificate_guts(). I supplied a > dummy that's enough to get it building cleanly, but that needs to be > filled in properly. > > > There is still plenty of work to go, but this seemed a sufficient > milestone to report progress on. > > OK, this version contains pre-generated nss files, and passes a full buildfarm run including the ssl test module, with both openssl and NSS. That should keep the cfbot happy :-) cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services |
On 8/3/20 12:46 PM, Andrew Dunstan wrote: > On 7/31/20 4:44 PM, Andrew Dunstan wrote: >> On 7/15/20 6:18 PM, Daniel Gustafsson wrote: >>>> On 15 Jul 2020, at 20:35, Andrew Dunstan <[hidden email]> wrote: >>>> >>>> On 5/15/20 4:46 PM, Daniel Gustafsson wrote: >>>>> My plan is to keep hacking at this to have it reviewable for the 14 cycle, so >>>>> if anyone has an interest in NSS, then I would love to hear feedback on how it >>>>> works (and doesn't work). >>>> I'll be happy to help, particularly with Windows support and with some >>>> of the callback stuff I've had a hand in. >>> That would be fantastic, thanks! The password callback handling is still a >>> TODO so feel free to take a stab at that since you have a lot of context on >>> there. >>> >>> For Windows, I've include USE_NSS in Solution.pm as Thomas pointed out in this >>> thread, but that was done blind as I've done no testing on Windows yet. >>> >> OK, here is an update of your patch that compiles and runs against NSS >> under Windows (VS2019). >> >> >> In addition to some work that was missing in src/tools/msvc, I had to >> make a few adjustments, including: >> >> >> * strtok_r() isn't available on Windows. We don't use it elsewhere in >> the postgres code, and it seemed unnecessary to have reentrant calls >> here, so I just replaced it with equivalent strtok() calls. >> * We were missing an NSS implementation of >> pgtls_verify_peer_name_matches_certificate_guts(). I supplied a >> dummy that's enough to get it building cleanly, but that needs to be >> filled in properly. >> >> >> There is still plenty of work to go, but this seemed a sufficient >> milestone to report progress on. >> >> > > OK, this version contains pre-generated nss files, and passes a full > buildfarm run including the ssl test module, with both openssl and NSS. > That should keep the cfbot happy :-) > > cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services |
> On 3 Aug 2020, at 21:18, Andrew Dunstan <[hidden email]> wrote:
> On 8/3/20 12:46 PM, Andrew Dunstan wrote: >> On 7/31/20 4:44 PM, Andrew Dunstan wrote: >>> OK, here is an update of your patch that compiles and runs against NSS >>> under Windows (VS2019). Out of curiosity since I'm not familiar with Windows, how hard/easy is it to install NSS for the purpose of a) hacking on postgres+NSS and b) using postgres with NSS as the backend? >>> * strtok_r() isn't available on Windows. We don't use it elsewhere in >>> the postgres code, and it seemed unnecessary to have reentrant calls >>> here, so I just replaced it with equivalent strtok() calls. Fair enough, that makes sense. >>> * We were missing an NSS implementation of >>> pgtls_verify_peer_name_matches_certificate_guts(). I supplied a >>> dummy that's enough to get it building cleanly, but that needs to be >>> filled in properly. Interesting, not sure how I could've missed that one. >> OK, this version contains pre-generated nss files, and passes a full >> buildfarm run including the ssl test module, with both openssl and NSS. >> That should keep the cfbot happy :-) Exciting, thanks a lot for helping out on this! I've started to look at the required documentation changes during vacation, will hopefully be able to post something soon. cheers ./daniel |
On 8/4/20 5:42 PM, Daniel Gustafsson wrote: >> On 3 Aug 2020, at 21:18, Andrew Dunstan <[hidden email]> wrote: >> On 8/3/20 12:46 PM, Andrew Dunstan wrote: >>> On 7/31/20 4:44 PM, Andrew Dunstan wrote: >>>> OK, here is an update of your patch that compiles and runs against NSS >>>> under Windows (VS2019). > Out of curiosity since I'm not familiar with Windows, how hard/easy is it to > install NSS for the purpose of a) hacking on postgres+NSS and b) using postgres > with NSS as the backend? I've laid out the process at https://www.2ndquadrant.com/en/blog/nss-on-windows-for-postgresql-development/ >>> OK, this version contains pre-generated nss files, and passes a full >>> buildfarm run including the ssl test module, with both openssl and NSS. >>> That should keep the cfbot happy :-) > Exciting, thanks a lot for helping out on this! I've started to look at the > required documentation changes during vacation, will hopefully be able to post > something soon. > Good. Having got the tests running cleanly on Linux, I'm now going back to work on that for Windows. After that I'll look at the hook/callback stuff. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services |
> On 5 Aug 2020, at 22:38, Andrew Dunstan <[hidden email]> wrote:
That's fantastic, thanks for putting that together.
> > On 8/4/20 5:42 PM, Daniel Gustafsson wrote: >>> On 3 Aug 2020, at 21:18, Andrew Dunstan <[hidden email]> wrote: >>> On 8/3/20 12:46 PM, Andrew Dunstan wrote: >>>> On 7/31/20 4:44 PM, Andrew Dunstan wrote: >>>>> OK, here is an update of your patch that compiles and runs against NSS >>>>> under Windows (VS2019). >> Out of curiosity since I'm not familiar with Windows, how hard/easy is it to >> install NSS for the purpose of a) hacking on postgres+NSS and b) using postgres >> with NSS as the backend? > > I've laid out the process at > https://www.2ndquadrant.com/en/blog/nss-on-windows-for-postgresql-development/ >>>> OK, this version contains pre-generated nss files, and passes a full >>>> buildfarm run including the ssl test module, with both openssl and NSS. >>>> That should keep the cfbot happy :-) Turns out the CFBot doesn't like the binary diffs. They are included in this version too but we should probably drop them again it seems. >> Exciting, thanks a lot for helping out on this! I've started to look at the >> required documentation changes during vacation, will hopefully be able to post >> something soon. > > Good. Having got the tests running cleanly on Linux, I'm now going back > to work on that for Windows. > > After that I'll look at the hook/callback stuff. The attached v9 contains mostly a first stab at getting some documentation going, it's far from completed but I'd rather share more frequently to not have local trees deviate too much in case you've had time to hack as well. I had a few documentation tweaks in the code too, but no real functionality change for now. The 0001 patch isn't strictly necessary but it seems reasonable to address the various ways OpenSSL was spelled out in the docs while at updating the SSL portions. It essentially ensures that markup around OpenSSL and SSL is used consistently. I didn't address the linelengths being too long in this patch to make review easier instead. cheers ./daniel ![]() ![]() |
>
> >>>> OK, this version contains pre-generated nss files, and passes a full > >>>> buildfarm run including the ssl test module, with both openssl and NSS. > >>>> That should keep the cfbot happy :-) > > Turns out the CFBot doesn't like the binary diffs. They are included in this > version too but we should probably drop them again it seems. > I did ask Thomas about this, he was going to try to fix it. In principle we should want it to accept binary diffs exactly for this sort of thing. > The attached v9 contains mostly a first stab at getting some documentation > going, it's far from completed but I'd rather share more frequently to not have > local trees deviate too much in case you've had time to hack as well. I had a > few documentation tweaks in the code too, but no real functionality change for > now. > > The 0001 patch isn't strictly necessary but it seems reasonable to address the > various ways OpenSSL was spelled out in the docs while at updating the SSL > portions. It essentially ensures that markup around OpenSSL and SSL is used > consistently. I didn't address the linelengths being too long in this patch to > make review easier instead. > I'll take a look. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services |
On Thu, Sep 03, 2020 at 03:26:03PM -0400, Andrew Dunstan wrote:
>> The 0001 patch isn't strictly necessary but it seems reasonable to address the >> various ways OpenSSL was spelled out in the docs while at updating the SSL >> portions. It essentially ensures that markup around OpenSSL and SSL is used >> consistently. I didn't address the linelengths being too long in this patch to >> make review easier instead. > > I'll take a look. Adding a <productname> markup around OpenSSL in the docs makes things consistent. +1. -- Michael |
On Fri, Sep 04, 2020 at 10:23:34AM +0900, Michael Paquier wrote:
> Adding a <productname> markup around OpenSSL in the docs makes things > consistent. +1. I have looked at 0001, and applied it after fixing the line length (thanks for not doing it to ease my lookup), and I found one extra place in need of fix. Patch 0002 is failing to apply. -- Michael |
> On 17 Sep 2020, at 09:41, Michael Paquier <[hidden email]> wrote:
> > On Fri, Sep 04, 2020 at 10:23:34AM +0900, Michael Paquier wrote: >> Adding a <productname> markup around OpenSSL in the docs makes things >> consistent. +1. > > I have looked at 0001, and applied it after fixing the line length > (thanks for not doing it to ease my lookup), and I found one extra > place in need of fix. Thanks! > Patch 0002 is failing to apply. Attached is a v10 rebased to apply on top of HEAD. cheers ./daniel |
Free forum by Nabble | Edit this page |