Re: Support for NSS as a libpq TLS backend
> On 4 Feb 2022, at 21:03, Stephen Frost wrote: > I wonder about the 'not in tree' bit since it is in the header files, > certainly for NSPR which I've been poking at due to this discussion. What I meant was that the documentation on the website isn't published from documentation source code (in whichever format) residing in the tree. That being said, I take that back since I just now in a git pull found that they had done just that 6 days ago. It's just as messy and incomplete as what is currently on the web, important API's like NSS_InitContext are still not even mentioned more than in a release note, but I think it stands a better chance of success than before. > I had hoped that they were generating the documentation on the webpage from > what's in the header files, is that not the case then? Not from what I can tell no. -- Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
Greetings, * Daniel Gustafsson (dan...@yesql.se) wrote: > I am writing done above in quotes, since the documentation also needs to be > updated, completed, rewritten, organized etc etc. The above is an import of > what was found, and is in a fairly poor state. Unfortunately, it's still not > in the tree where I personally believe documentation stands the best chance of > being kept up to date. The NSPR documentation is probably the best of the > two, > but it's also much less of a moving target. I wonder about the 'not in tree' bit since it is in the header files, certainly for NSPR which I've been poking at due to this discussion. I had hoped that they were generating the documentation on the webpage from what's in the header files, is that not the case then? Which is more accurate? If it's a simple matter of spending time going through what's in the tree and making sure what's online matches that, I suspect we could find some folks with time to work on helping them there. If the in-tree stuff isn't accurate then that's a bigger problem, of course. > It is true that the documentation is poor and currently in bad shape with lots > of broken links and heavily disorganized etc. It's also true that I managed > to > implement full libpq support without any crystal ball or help from the NSS > folks. The latter doesn't mean we can brush documentation concerns aside, but > let's be fair in our criticism. Agreed. Thanks, Stephen signature.asc Description: PGP signature
Re: Support for NSS as a libpq TLS backend
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Thu, Feb 3, 2022 at 02:33:37PM -0500, Robert Haas wrote: > > As a philosophical matter, I don't think it's great for us - or the > > Internet in general - to be too dependent on OpenSSL. Software > > monocultures are not great, and OpenSSL has near-constant security > > updates and mediocre documentation. Now, maybe anything else we > > I don't think it is fair to be criticizing OpenSSL for its mediocre > documentation when the alternative being considered, NSS, has no public > documentation. Can the source-code-defined NSS documentation be > considered better than the mediocre OpenSSL public documentation? This simply isn't the case and wasn't even the case at the start of this thread. The NSPR documentation was only available through the header files due to it being taken down from MDN. The NSS documentation was actually still there. Looks like they've now (mostly) fixed the lack of NSPR documentation, as noted in the recent email that I sent. > For the record, I do like the idea of adding NSS, but I am concerned > about its long-term maintenance, we you explained. They've come out and explicitly said that the project is active and maintained, and they've been doing regular releases. I don't think there's really any reason to think that it's not being maintained at this point. Thanks, Stephen signature.asc Description: PGP signature
Re: Support for NSS as a libpq TLS backend
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Thu, Feb 3, 2022 at 01:42:53PM -0500, Stephen Frost wrote: > > * Bruce Momjian (br...@momjian.us) wrote: > > > On Tue, Feb 1, 2022 at 01:52:09PM -0800, Andres Freund wrote: > > > > There's https://hg.mozilla.org/projects/nspr/file/tip/pr/src - which is > > > > I > > > > think the upstream source. > > > > > > > > A project without even a bare-minimal README at the root does have a > > > > "internal > > > > only" feel to it... > > > > > > I agree --- it is a library --- if they don't feel the need to publish > > > the API, it seems to mean they want to maintain the ability to change it > > > at any time, and therefore it is inappropriate for other software to > > > rely on that API. > > > > This is really not a reasonable representation of how this library has > > been maintained historically nor is there any reason to think that their > > policy regarding the API has changed recently. They do have a > > documented API and that hasn't changed- it's just that it's not easily > > available in web-page form any longer and that's due to something > > independent of the library maintenance. They've also done a good job > > So they have always been bad at providing an API, not just now, or that > their web content disappeared and they haven't fixed it, for how long? > I guess that is better than the v8 case, but not much. Is posting web > content really that hard for them? To be clear, *part* of the web-based documentation disappeared and hasn't been replaced yet. The NSS-specific pieces are actually still available, it's the NSPR (which is a lower level library used by NSS) part that was removed from MDN and hasn't been brought back yet, but which does still exist as comments in the source of the library. > > with maintaining the API as one would expect from a library and so this > > really isn't a reason to avoid using it. If there's actual specific > > examples of the API not being well maintained and causing issues then > > please point to them and we can discuss if that is a reason to consider > > not depending on NSS/NSPR. > > I have no specifics. Then I don't understand where the claim you made that "it seems to mean they want to maintain the ability to change it at any time" has any merit. > > > This is not the same as Postgres extensions needing to read the Postgres > > > source code --- they are an important but edge use case and we never saw > > > the need to standardize or publish the internal functions that must be > > > studied and adjusted possibly for major releases. > > > > I agree that extensions and public libraries aren't entirely the same > > but I don't think it's all that unreasonable for developers that are > > using a library to look at the source code for that library when > > developing against it, that's certainly something I've done for a > > number of different libraries. > > Wow, you have a much higher tolerance than I do. How do you even know > which functions are the public API if you have to look at the source > code? Because... it's documented? They have public (and private) .h files in the source tree and the function declarations have large comment blocks above them which provide a documented API. I'm not talking about having to decipher from the actual C code what's going on but just reading the function header comment that provides the documentation of the API for each of the functions, and there's larger blocks of comments at the top of those .h files which provide more insight into how the functions in that particular part of the system work and interact with each other. Maybe those things would be better as separate README files like what we do, but maybe not, and I don't see it as a huge failing that they chose to use a big comment block at the top of their .h files to explain things rather than separate README files. Reading comments in code that I'm calling out to, even if it's in another library (or another part of PG where the README isn't helping me enough, or due to there not being a README for that particular thing) almost seems typical, to me anyway. Perhaps the exception being when there are good man pages. > I frankly think we need some public statement from the NSS developers > before moving forward --- there are just too many red flags here, and > once we support it, it will be hard to remove support for it. They have made public statements regarding this and it's been linked to already in this thread: https://github.com/mdn/content/issues/12471 where they explicitly state that the project is alive and maintained, further, it now now also links to this: https://bugzilla.mozilla.org/show_bug.cgi?id=1753127 Which certainly seems to have had a fair bit of action taken on it. Indeed, it looks like they've got a lot of the docs up and online now, including the documentation for the function that started much of this:
Re: Support for NSS as a libpq TLS backend
> On 4 Feb 2022, at 19:22, Bruce Momjian wrote: > > On Thu, Feb 3, 2022 at 02:33:37PM -0500, Robert Haas wrote: >> As a philosophical matter, I don't think it's great for us - or the >> Internet in general - to be too dependent on OpenSSL. Software >> monocultures are not great, and OpenSSL has near-constant security >> updates and mediocre documentation. Now, maybe anything else we > > I don't think it is fair to be criticizing OpenSSL for its mediocre > documentation when the alternative being considered, NSS, has no public > documentation. Can the source-code-defined NSS documentation.. Not that it will shift the needle either way, but to give credit where credit is due: Both NSS and NSPR are documented, and have been since they were published by Netscape in 1998. The documentation does lack things, and some parts are quite out of date. That's true and undisputed even by the projects themselves who state this: "It currently is very deprecated and likely incorrect or broken in many places". The recent issue was that Mozilla decided to remove all 3rd party projects (why they consider their own code 3rd party is a mystery to me) from their MDN site, and so NSS and NSPR were deleted with no replacement. This was said to be worked on but didn't happen and no docs were imported into the tree. When Daniel from curl (the other one, not I) complained, this caused enough momentum to get this work going and it's now been "done". NSS: https://firefox-source-docs.mozilla.org/security/nss/ NSPR: https://firefox-source-docs.mozilla.org/nspr/ I am writing done above in quotes, since the documentation also needs to be updated, completed, rewritten, organized etc etc. The above is an import of what was found, and is in a fairly poor state. Unfortunately, it's still not in the tree where I personally believe documentation stands the best chance of being kept up to date. The NSPR documentation is probably the best of the two, but it's also much less of a moving target. It is true that the documentation is poor and currently in bad shape with lots of broken links and heavily disorganized etc. It's also true that I managed to implement full libpq support without any crystal ball or help from the NSS folks. The latter doesn't mean we can brush documentation concerns aside, but let's be fair in our criticism. > ..be considered better than the mediocre OpenSSL public documentation? OpenSSL has gotten a lot better in recent years, it's still not great or where I would like it to be, but a lot better. -- Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
On Fri, Feb 4, 2022 at 01:33:00PM -0500, Robert Haas wrote: > > I don't think it is fair to be criticizing OpenSSL for its mediocre > > documentation when the alternative being considered, NSS, has no public > > documentation. Can the source-code-defined NSS documentation be > > considered better than the mediocre OpenSSL public documentation? > > I mean, I think it's fair to say that my experiences with trying to > use the OpenSSL documentation have been poor. Admittedly it's been a > few years now so maybe it's gotten better, but my experience was what > it was. In one case, the function I needed wasn't documented at all, > and I had to read the C code, which was weirdly-formatted and had no > comments. That wasn't fun, and knowing that NSS could be an even worse > experience doesn't retroactively turn that into a good one. Oh, yeah, the OpenSSL documentation is verifiably mediocre. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: Support for NSS as a libpq TLS backend
On Fri, Feb 4, 2022 at 1:22 PM Bruce Momjian wrote: > On Thu, Feb 3, 2022 at 02:33:37PM -0500, Robert Haas wrote: > > As a philosophical matter, I don't think it's great for us - or the > > Internet in general - to be too dependent on OpenSSL. Software > > monocultures are not great, and OpenSSL has near-constant security > > updates and mediocre documentation. Now, maybe anything else we > > I don't think it is fair to be criticizing OpenSSL for its mediocre > documentation when the alternative being considered, NSS, has no public > documentation. Can the source-code-defined NSS documentation be > considered better than the mediocre OpenSSL public documentation? I mean, I think it's fair to say that my experiences with trying to use the OpenSSL documentation have been poor. Admittedly it's been a few years now so maybe it's gotten better, but my experience was what it was. In one case, the function I needed wasn't documented at all, and I had to read the C code, which was weirdly-formatted and had no comments. That wasn't fun, and knowing that NSS could be an even worse experience doesn't retroactively turn that into a good one. > For the record, I do like the idea of adding NSS, but I am concerned > about its long-term maintenance, we you explained. It sounds like we come down in about the same place here, in the end. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Support for NSS as a libpq TLS backend
On Thu, Feb 3, 2022 at 02:33:37PM -0500, Robert Haas wrote: > As a philosophical matter, I don't think it's great for us - or the > Internet in general - to be too dependent on OpenSSL. Software > monocultures are not great, and OpenSSL has near-constant security > updates and mediocre documentation. Now, maybe anything else we I don't think it is fair to be criticizing OpenSSL for its mediocre documentation when the alternative being considered, NSS, has no public documentation. Can the source-code-defined NSS documentation be considered better than the mediocre OpenSSL public documentation? For the record, I do like the idea of adding NSS, but I am concerned about its long-term maintenance, we you explained. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: Support for NSS as a libpq TLS backend
On Thu, Feb 3, 2022 at 01:42:53PM -0500, Stephen Frost wrote: > Greetings, > > * Bruce Momjian (br...@momjian.us) wrote: > > On Tue, Feb 1, 2022 at 01:52:09PM -0800, Andres Freund wrote: > > > There's https://hg.mozilla.org/projects/nspr/file/tip/pr/src - which is I > > > think the upstream source. > > > > > > A project without even a bare-minimal README at the root does have a > > > "internal > > > only" feel to it... > > > > I agree --- it is a library --- if they don't feel the need to publish > > the API, it seems to mean they want to maintain the ability to change it > > at any time, and therefore it is inappropriate for other software to > > rely on that API. > > This is really not a reasonable representation of how this library has > been maintained historically nor is there any reason to think that their > policy regarding the API has changed recently. They do have a > documented API and that hasn't changed- it's just that it's not easily > available in web-page form any longer and that's due to something > independent of the library maintenance. They've also done a good job So they have always been bad at providing an API, not just now, or that their web content disappeared and they haven't fixed it, for how long? I guess that is better than the v8 case, but not much. Is posting web content really that hard for them? > with maintaining the API as one would expect from a library and so this > really isn't a reason to avoid using it. If there's actual specific > examples of the API not being well maintained and causing issues then > please point to them and we can discuss if that is a reason to consider > not depending on NSS/NSPR. I have no specifics. > > This is not the same as Postgres extensions needing to read the Postgres > > source code --- they are an important but edge use case and we never saw > > the need to standardize or publish the internal functions that must be > > studied and adjusted possibly for major releases. > > I agree that extensions and public libraries aren't entirely the same > but I don't think it's all that unreasonable for developers that are > using a library to look at the source code for that library when > developing against it, that's certainly something I've done for a > number of different libraries. Wow, you have a much higher tolerance than I do. How do you even know which functions are the public API if you have to look at the source code? > > This kind of feels like the Chrome JavaScript code that used to be able > > to be build separately for PL/v8, but has gotten much harder to do in > > the past few years. > > This isn't at all like that case, where the maintainers made a very > clear and intentional choice to make it quite difficult for packagers to > pull v8 out to package it. Nothing like that has happened with NSS and > there isn't any reason to think that it will based on what the > maintainers have said and what they've done across the many years that > NSS has been around. As far as I know, the v8 developers didn't say anything, they just started moving things around to make it easier for them and harder for packagers --- and they didn't care. I frankly think we need some public statement from the NSS developers before moving forward --- there are just too many red flags here, and once we support it, it will be hard to remove support for it. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: Support for NSS as a libpq TLS backend
On Thu, Feb 3, 2022 at 2:16 PM Peter Eisentraut wrote: > If we want simply an alternative, we had a GnuTLS variant almost done a > few years ago, but in the end people didn't want it enough. It seems to > be similar now. Yeah. I think it's pretty clear that the only real downside of committing support for GnuTLS or NSS or anything else is that we then need to maintain that support (or eventually remove it). I don't really see a problem if Daniel wants to commit this, set up a few buildfarm animals, and fix stuff when it breaks. If he does that, I don't see that we're losing anything. But, if he commits it in the hope that other people are going to step up to do the maintenance work, maybe that's not going to happen, or at least not without grumbling. I'm not objecting to this being committed in the sense that I don't ever want to see it in the tree, but I'm also not volunteering to maintain it. As a philosophical matter, I don't think it's great for us - or the Internet in general - to be too dependent on OpenSSL. Software monocultures are not great, and OpenSSL has near-constant security updates and mediocre documentation. Now, maybe anything else we support will end up having similar issues, or worse. But if we and other projects are never willing to support anything but OpenSSL, then there will never be viable alternatives to OpenSSL, because a library that isn't actually used by the software you care about is of no use. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Support for NSS as a libpq TLS backend
On 03.02.22 15:53, Daniel Gustafsson wrote: I see quite a few valid reasons to want an alternative, a few off the top of my head include: - Using trust stores like Keychain on macOS with Secure Transport. There is AFAIK something similar on Windows and NSS has it's certificate databases. Especially on client side libpq it would be quite nice to integrate with where certificates already are rather than rely on files on disks. - Not having to install OpenSSL, Schannel and Secure Transport would make life easier for packagers. Those are good reasons for Schannel and Secure Transport, less so for NSS. - Simply having an alternative. The OpenSSL projects recent venture into writing transport protocols have made a lot of people worried over their bandwidth for fixing and supporting core features. If we want simply an alternative, we had a GnuTLS variant almost done a few years ago, but in the end people didn't want it enough. It seems to be similar now.
Re: Support for NSS as a libpq TLS backend
Greetings, * Daniel Gustafsson (dan...@yesql.se) wrote: > > On 3 Feb 2022, at 15:07, Peter Eisentraut > > wrote: > > > > On 28.01.22 15:30, Robert Haas wrote: > >> I would really, really like to have an alternative to OpenSSL for PG. > > > > What are the reasons people want that? With OpenSSL 3, the main reasons -- > > license and FIPS support -- have gone away. > > At least it will go away when OpenSSL 3 is FIPS certified, which is yet to > happen (submitted, not processed). > > I see quite a few valid reasons to want an alternative, a few off the top of > my > head include: > > - Using trust stores like Keychain on macOS with Secure Transport. There is > AFAIK something similar on Windows and NSS has it's certificate databases. > Especially on client side libpq it would be quite nice to integrate with where > certificates already are rather than rely on files on disks. > > - Not having to install OpenSSL, Schannel and Secure Transport would make life > easier for packagers. > > - Simply having an alternative. The OpenSSL projects recent venture into > writing transport protocols have made a lot of people worried over their > bandwidth for fixing and supporting core features. > > Just my $0.02, everyones mileage varies on these. Yeah, agreed on all of these. Thanks, Stephen signature.asc Description: PGP signature
Re: Support for NSS as a libpq TLS backend
Greetings, * Bruce Momjian (br...@momjian.us) wrote: > On Tue, Feb 1, 2022 at 01:52:09PM -0800, Andres Freund wrote: > > There's https://hg.mozilla.org/projects/nspr/file/tip/pr/src - which is I > > think the upstream source. > > > > A project without even a bare-minimal README at the root does have a > > "internal > > only" feel to it... > > I agree --- it is a library --- if they don't feel the need to publish > the API, it seems to mean they want to maintain the ability to change it > at any time, and therefore it is inappropriate for other software to > rely on that API. This is really not a reasonable representation of how this library has been maintained historically nor is there any reason to think that their policy regarding the API has changed recently. They do have a documented API and that hasn't changed- it's just that it's not easily available in web-page form any longer and that's due to something independent of the library maintenance. They've also done a good job with maintaining the API as one would expect from a library and so this really isn't a reason to avoid using it. If there's actual specific examples of the API not being well maintained and causing issues then please point to them and we can discuss if that is a reason to consider not depending on NSS/NSPR. > This is not the same as Postgres extensions needing to read the Postgres > source code --- they are an important but edge use case and we never saw > the need to standardize or publish the internal functions that must be > studied and adjusted possibly for major releases. I agree that extensions and public libraries aren't entirely the same but I don't think it's all that unreasonable for developers that are using a library to look at the source code for that library when developing against it, that's certainly something I've done for a number of different libraries. > This kind of feels like the Chrome JavaScript code that used to be able > to be build separately for PL/v8, but has gotten much harder to do in > the past few years. This isn't at all like that case, where the maintainers made a very clear and intentional choice to make it quite difficult for packagers to pull v8 out to package it. Nothing like that has happened with NSS and there isn't any reason to think that it will based on what the maintainers have said and what they've done across the many years that NSS has been around. Thanks, Stephen signature.asc Description: PGP signature
Re: Support for NSS as a libpq TLS backend
> On 3 Feb 2022, at 15:07, Peter Eisentraut > wrote: > > On 28.01.22 15:30, Robert Haas wrote: >> I would really, really like to have an alternative to OpenSSL for PG. > > What are the reasons people want that? With OpenSSL 3, the main reasons -- > license and FIPS support -- have gone away. At least it will go away when OpenSSL 3 is FIPS certified, which is yet to happen (submitted, not processed). I see quite a few valid reasons to want an alternative, a few off the top of my head include: - Using trust stores like Keychain on macOS with Secure Transport. There is AFAIK something similar on Windows and NSS has it's certificate databases. Especially on client side libpq it would be quite nice to integrate with where certificates already are rather than rely on files on disks. - Not having to install OpenSSL, Schannel and Secure Transport would make life easier for packagers. - Simply having an alternative. The OpenSSL projects recent venture into writing transport protocols have made a lot of people worried over their bandwidth for fixing and supporting core features. Just my $0.02, everyones mileage varies on these. -- Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
On 28.01.22 15:30, Robert Haas wrote: I would really, really like to have an alternative to OpenSSL for PG. What are the reasons people want that? With OpenSSL 3, the main reasons -- license and FIPS support -- have gone away.
Re: Support for NSS as a libpq TLS backend
On Tue, Feb 1, 2022 at 01:52:09PM -0800, Andres Freund wrote: > There's https://hg.mozilla.org/projects/nspr/file/tip/pr/src - which is I > think the upstream source. > > A project without even a bare-minimal README at the root does have a "internal > only" feel to it... I agree --- it is a library --- if they don't feel the need to publish the API, it seems to mean they want to maintain the ability to change it at any time, and therefore it is inappropriate for other software to rely on that API. This is not the same as Postgres extensions needing to read the Postgres source code --- they are an important but edge use case and we never saw the need to standardize or publish the internal functions that must be studied and adjusted possibly for major releases. This kind of feels like the Chrome JavaScript code that used to be able to be build separately for PL/v8, but has gotten much harder to do in the past few years. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: Support for NSS as a libpq TLS backend
Hi, On 2022-02-01 15:12:28 -0500, Stephen Frost wrote: > The concern about the documentation not being easily available is > certainly something to consider. I remember in prior reviews not having > that much difficulty looking up documentation for functions I've definitely several times in the course of this thread asked for documentation about specific bits and there was none. And not just recently. > All that said, while have documentation on the web is nice and all, it > seems to still be in the source, at least when I grabbed NSPR locally > with apt-get source and looked at PR_Recv, I found: What I'm most concerned about is less the way individual functions work, and more a bit higher level things. Like e.g. about not being allowed to fork. Which has significant design implications given postgres' process model... I think some documentation has been re-uploaded in the last few days. I recall the content around https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS being gone too, last time I checked. > So, it's not the case that the documentation is completely gone and > utterly unavailable to those who are interested in it, it's just in the > source rather than being on a nicely formatted webpage. One can find it > on the web too, naturally: > https://github.com/thespooler/nspr/blob/29ba433ebceda269d2b0885176b7f8cd4c5c2c52/pr/include/prio.h#L1424 > (no idea what version that is, just found a random github repo with it, > but wouldn't be hard to import the latest version). It's last been updated 2015... There's https://hg.mozilla.org/projects/nspr/file/tip/pr/src - which is I think the upstream source. A project without even a bare-minimal README at the root does have a "internal only" feel to it... Greetings, Andres Freund
Re: Support for NSS as a libpq TLS backend
Greetings, * Daniel Gustafsson (dan...@yesql.se) wrote: > > On 31 Jan 2022, at 22:48, Daniel Gustafsson wrote: > >> On 31 Jan 2022, at 17:24, Stephen Frost wrote: > > >> I agree that it's concerning to hear that OpenLDAP dropped support for > >> NSS... though I don't seem to be able to find any information as to why > >> they decided to do so. > > > > I was also unable to do that. There is no information that I could see in > > either the commit message, Bugzilla entry (#9207) or on the mailinglist. > > Searching the web didn't yield anything either. I've reached out to > > hopefully > > get a bit more information. > > Support issues and Red Hat dropping OpenLDAP was cited [0] as the main drivers > for dropping NSS. That's both very vaugue and oddly specific, I have to say. Also, not really sure that it's a good reason for other projects to move away, or for the large amount of work put into this effort to be thrown out when it seems to be quite close to finally being done and giving us an alternative, supported and maintained, TLS/SSL library. The concern about the documentation not being easily available is certainly something to consider. I remember in prior reviews not having that much difficulty looking up documentation for functions, and in doing some quick looking around there's certainly some (most?) of the NSS documentation still up, the issue is that the NSPR documentation was taken off of the MDN website and that's referenced from the NSS pages and is obviously something that folks working with NSS need to be able to find the documentation for too. All that said, while have documentation on the web is nice and all, it seems to still be in the source, at least when I grabbed NSPR locally with apt-get source and looked at PR_Recv, I found: /* * * FUNCTION: PR_Recv * DESCRIPTION: *Receive a specified number of bytes from a connected socket. * The operation will block until some positive number of bytes are * transferred, a time out has occurred, or there is an error. * No more than 'amount' bytes will be transferred. * INPUTS: * PRFileDesc *fd * points to a PRFileDesc object representing a socket. * void *buf * pointer to a buffer to hold the data received. * PRInt32 amount * the size of 'buf' (in bytes) * PRIntn flags * must be zero or PR_MSG_PEEK. * PRIntervalTime timeout * Time limit for completion of the receive operation. * OUTPUTS: * None * RETURN: PRInt32 * a positive number indicates the number of bytes actually received. * 0 means the network connection is closed. * -1 indicates a failure. The reason for the failure is obtained * by calling PR_GetError(). ** */ So, it's not the case that the documentation is completely gone and utterly unavailable to those who are interested in it, it's just in the source rather than being on a nicely formatted webpage. One can find it on the web too, naturally: https://github.com/thespooler/nspr/blob/29ba433ebceda269d2b0885176b7f8cd4c5c2c52/pr/include/prio.h#L1424 (no idea what version that is, just found a random github repo with it, but wouldn't be hard to import the latest version). Considering how much we point people to our source when they're writing extensions and such, this doesn't strike me as quite the dire situation that it first appeared to be based on the initial comments. There is documentation, it's not actually that hard to find if you're working with the library, and the maintainers have stated their intention to work on improving the web-based documentation. Thanks, Stephen signature.asc Description: PGP signature
Re: Support for NSS as a libpq TLS backend
> On 31 Jan 2022, at 22:48, Daniel Gustafsson wrote: >> On 31 Jan 2022, at 17:24, Stephen Frost wrote: >> I agree that it's concerning to hear that OpenLDAP dropped support for >> NSS... though I don't seem to be able to find any information as to why >> they decided to do so. > > I was also unable to do that. There is no information that I could see in > either the commit message, Bugzilla entry (#9207) or on the mailinglist. > Searching the web didn't yield anything either. I've reached out to hopefully > get a bit more information. Support issues and Red Hat dropping OpenLDAP was cited [0] as the main drivers for dropping NSS. -- Daniel Gustafsson https://vmware.com/ [0] https://curl.se/mail/lib-2022-02/.html
Re: Support for NSS as a libpq TLS backend
> On 31 Jan 2022, at 22:32, Andres Freund wrote: > > Hi, > > On 2022-01-31 14:24:03 +0100, Daniel Gustafsson wrote: >>> On 28 Jan 2022, at 15:30, Robert Haas wrote: >>> I would really, really like to have an alternative to OpenSSL for PG. >>> I don't know if this is the right thing, though. If other people are >>> dropping support for it, that's a pretty bad sign IMHO. Later in the >>> thread it says OpenLDAP have dropped support for it already as well. >> >> I'm counting this and Andres' comment as a -1 on the patchset, and given >> where >> we are in the cycle I'm mark it rejected in the CF app shortly unless anyone >> objects. > > I'd make mine more a -0.2 or so. I'm concerned about the lack of non-code > documentation and the state of code documentation. I'd like an openssl > alternative, although not as much as a few years ago - it seems that the state > of openssl has improved compared to most of the other implementations. IMHO I think OpenSSL has improved over OpenSSL of the past - which is great to see - but they have also diverged themselves into writing a full QUIC implementation which *I personally think* is a distraction they don't need. That being said, there aren't too many other options. -- Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
> On 31 Jan 2022, at 17:24, Stephen Frost wrote: > * Daniel Gustafsson (dan...@yesql.se) wrote: >> I'm counting this and Andres' comment as a -1 on the patchset, and given >> where >> we are in the cycle I'm mark it rejected in the CF app shortly unless anyone >> objects. > > I agree that it's concerning to hear that OpenLDAP dropped support for > NSS... though I don't seem to be able to find any information as to why > they decided to do so. I was also unable to do that. There is no information that I could see in either the commit message, Bugzilla entry (#9207) or on the mailinglist. Searching the web didn't yield anything either. I've reached out to hopefully get a bit more information. > I'm also very much a fan of having an alternative to OpenSSL and the > NSS/NSPR license fits well for us, unlike the alternatives to OpenSSL > used by other projects, such as GnuTLS (which is the alternative to > OpenSSL that OpenLDAP now has) or other libraries like wolfSSL. Short of platform specific (proprietary) libraries like Schannel and Secure Transport, the alternatives are indeed slim. > Beyond the documentation issue, which I agree is a concern but also > seems to be actively realized as an issue by the NSS/NSPR folks, It is, but it has also been an issue for years to be honest, getting the docs up to scratch will require a very large effort. > is there some other reason that the curl folks are thinking of dropping > support > for it? It's also not really used anymore in conjunction with curl, with Red Hat no longer shipping builds against it. -- Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
Hi, On 2022-01-31 14:24:03 +0100, Daniel Gustafsson wrote: > > On 28 Jan 2022, at 15:30, Robert Haas wrote: > > I would really, really like to have an alternative to OpenSSL for PG. > > I don't know if this is the right thing, though. If other people are > > dropping support for it, that's a pretty bad sign IMHO. Later in the > > thread it says OpenLDAP have dropped support for it already as well. > > I'm counting this and Andres' comment as a -1 on the patchset, and given where > we are in the cycle I'm mark it rejected in the CF app shortly unless anyone > objects. I'd make mine more a -0.2 or so. I'm concerned about the lack of non-code documentation and the state of code documentation. I'd like an openssl alternative, although not as much as a few years ago - it seems that the state of openssl has improved compared to most of the other implementations. Greetings, Andres Freund
Re: Support for NSS as a libpq TLS backend
Greetings, * Daniel Gustafsson (dan...@yesql.se) wrote: > > On 28 Jan 2022, at 15:30, Robert Haas wrote: > > On Fri, Jan 28, 2022 at 9:08 AM Daniel Gustafsson wrote: > >>> Kinda makes me question the wisdom of starting to depend on NSS. When > >>> openssl > >>> docs are vastly outshining a library's, that library really should start > >>> to > >>> ask itself some hard questions. > > > > Yeah, OpenSSL is very poor, so being worse is not good. > > > >> Sadly, there is that. While this is not a new problem, Mozilla has been > >> making > >> some very weird decisions around NSS governance as of late. Another data > >> point > >> is the below thread from libcurl: > >> > >>https://curl.se/mail/lib-2022-01/0120.html > > > > I would really, really like to have an alternative to OpenSSL for PG. > > I don't know if this is the right thing, though. If other people are > > dropping support for it, that's a pretty bad sign IMHO. Later in the > > thread it says OpenLDAP have dropped support for it already as well. > > I'm counting this and Andres' comment as a -1 on the patchset, and given where > we are in the cycle I'm mark it rejected in the CF app shortly unless anyone > objects. I agree that it's concerning to hear that OpenLDAP dropped support for NSS... though I don't seem to be able to find any information as to why they decided to do so. NSS is clearly still supported and maintained and they do seem to understand that they need to work on the documentation situation and to get that fixed (the current issue seems to be around NSS vs. NSPR and the migration off of MDN to the in-tree documentation as Daniel mentioned, if I followed the discussion correctly in the bug that was filed by the curl folks and was then actively responded to by the NSS/NSPR folks), which seems to be the main issue that's being raised about it by the curl folks and here. I'm also very much a fan of having an alternative to OpenSSL and the NSS/NSPR license fits well for us, unlike the alternatives to OpenSSL used by other projects, such as GnuTLS (which is the alternative to OpenSSL that OpenLDAP now has) or other libraries like wolfSSL. Beyond the documentation issue, which I agree is a concern but also seems to be actively realized as an issue by the NSS/NSPR folks, is there some other reason that the curl folks are thinking of dropping support for it? Or does anyone have insight into why OpenLDAP decided to remove support? Thanks, Stephen signature.asc Description: PGP signature
Re: Support for NSS as a libpq TLS backend
> On 28 Jan 2022, at 15:30, Robert Haas wrote: > > On Fri, Jan 28, 2022 at 9:08 AM Daniel Gustafsson wrote: >>> Kinda makes me question the wisdom of starting to depend on NSS. When >>> openssl >>> docs are vastly outshining a library's, that library really should start to >>> ask itself some hard questions. > > Yeah, OpenSSL is very poor, so being worse is not good. > >> Sadly, there is that. While this is not a new problem, Mozilla has been >> making >> some very weird decisions around NSS governance as of late. Another data >> point >> is the below thread from libcurl: >> >>https://curl.se/mail/lib-2022-01/0120.html > > I would really, really like to have an alternative to OpenSSL for PG. > I don't know if this is the right thing, though. If other people are > dropping support for it, that's a pretty bad sign IMHO. Later in the > thread it says OpenLDAP have dropped support for it already as well. I'm counting this and Andres' comment as a -1 on the patchset, and given where we are in the cycle I'm mark it rejected in the CF app shortly unless anyone objects. -- Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
> On 28 Jan 2022, at 15:30, Robert Haas wrote: > > On Fri, Jan 28, 2022 at 9:08 AM Daniel Gustafsson wrote: >>> Kinda makes me question the wisdom of starting to depend on NSS. When >>> openssl >>> docs are vastly outshining a library's, that library really should start to >>> ask itself some hard questions. > > Yeah, OpenSSL is very poor, so being worse is not good. Some background on this for anyone interested: Mozilla removed the documentation from the MDN website and the attempt at resurrecting it in the tree (where it should've been all along ) isn't making much progress. Some more can be found in this post on the NSS mailinglist: https://groups.google.com/a/mozilla.org/g/dev-tech-crypto/c/p0MO7030K4A/m/Mx5St_2sAwAJ -- Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
On Fri, Jan 28, 2022 at 9:08 AM Daniel Gustafsson wrote: > > Kinda makes me question the wisdom of starting to depend on NSS. When > > openssl > > docs are vastly outshining a library's, that library really should start to > > ask itself some hard questions. Yeah, OpenSSL is very poor, so being worse is not good. > Sadly, there is that. While this is not a new problem, Mozilla has been > making > some very weird decisions around NSS governance as of late. Another data > point > is the below thread from libcurl: > > https://curl.se/mail/lib-2022-01/0120.html I would really, really like to have an alternative to OpenSSL for PG. I don't know if this is the right thing, though. If other people are dropping support for it, that's a pretty bad sign IMHO. Later in the thread it says OpenLDAP have dropped support for it already as well. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Support for NSS as a libpq TLS backend
>>> Can we propose a patch to document them? Don't want to get bitten by this >>> suddenly changing... >> >> I can certainly propose something on their mailinglist, but I unfortunately >> wouldn't get my hopes up too high as NSS and documentation aren't exactly >> best >> friends (the in-tree docs doesn't cover the API and Mozilla recently removed >> most of the online docs in their neverending developer site reorg). > > Kinda makes me question the wisdom of starting to depend on NSS. When openssl > docs are vastly outshining a library's, that library really should start to > ask itself some hard questions. Sadly, there is that. While this is not a new problem, Mozilla has been making some very weird decisions around NSS governance as of late. Another data point is the below thread from libcurl: https://curl.se/mail/lib-2022-01/0120.html -- Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
On Wed, 2022-01-26 at 15:59 -0800, Andres Freund wrote: > > > Do we have a testcase for embedded NULLs in common names? > > > > We don't, neither for OpenSSL or NSS. AFAICR Jacob spent days trying to > > get a > > certificate generation to include an embedded NULL byte but in the end gave > > up. > > We would have to write our own tools for generating certificates to add that > > (which may or may not be a bad idea, but it hasn't been done). > > Hah, that's interesting. Yeah, OpenSSL just refused to do it, with any method I could find at least. My personal test suite is using pyca/cryptography and psycopg2 to cover that case. --Jacob
Re: Support for NSS as a libpq TLS backend
Hi,
On 2022-01-26 21:39:16 +0100, Daniel Gustafsson wrote:
> > What about
> > reconfiguring (note: add --enable-depend) the linux tasks to build against
> > nss, and then run the relevant subset of tests with it? Most tests don't
> > use
> > tcp / SSL anyway, so rerunning a small subset of tests should be feasible?
>
> That's an interesting idea, I think that could work and be reasonably readable
> at the same time (and won't require in-depth knowledge of Cirrus). As it's
> the
> same task it does spend more time towards the max runtime per task, but that's
> not a problem for now. It's worth keeping in mind though if we deem this to
> be
> a way forward with testing multiple settings.
I think it's a way for a limited number of settings, that each only require a
limited amount of tests... Rerunning all tests etc is a different story.
> > Is it a great idea to have common/nss.h when there's a library header nss.h?
> > Perhaps we should have a pg_ssl_{nss,openssl}.h or such?
>
> That's a good point, I modelled it after common/openssl.h but I agree it's
> better to differentiate the filenames. I've renamed it to common/pg_nss.h and
> we should IMO rename common/openssl.h regardless of what happens to this
> patch.
+1
> > Does this make some things notably more expensive? Presumably it does
> > remove a
> > bunch of COW opportunities, but likely that's not a huge factor compared to
> > assymetric crypto negotiation...
>
> Right, the context of setting up crypto across a network connection it's
> highly
> likely to drown out the costs.
If you start to need to run a helper to decrypt an encrypted private key, and
do all the initialization, I'm not sure sure that holds true anymore... Have
you done any connection speed tests? pgbench -C is helpful for that.
> > Maybe soem of this commentary should migrate to the file header or such?
>
> Maybe, or perhaps README.ssl? Not sure where it would be most reasonable to
> keep it such that it's also kept up to date.
Either would work for me.
> >> This introduce
> >> + * differences with the OpenSSL support where some errors are only
> >> reported
> >> + * at runtime with NSS where they are reported at startup with OpenSSL.
> >
> > Found this sentence hard to parse somehow.
> >
> > It seems pretty unfriendly to only have minimal error checking at postmaster
> > startup time. Seems at least the presence and usability of keys should be
> > done
> > *also* at that time?
>
> I'll look at adding some setup, and subsequent teardown, of NSS at startup
> during which we could do checking to be more on par with how the OpenSSL
> backend will report errors.
Cool.
> >> +/*
> >> + * raw_subject_common_name
> >> + *
> >> + * Returns the Subject Common Name for the given certificate as a raw char
> >> + * buffer (that is, without any form of escaping for unprintable
> >> characters or
> >> + * embedded nulls), with the length of the buffer returned in the len
> >> param.
> >> + * The buffer is allocated in the TopMemoryContext and is given a NULL
> >> + * terminator so that callers are safe to call strlen() on it.
> >> + *
> >> + * This is used instead of CERT_GetCommonName(), which always performs
> >> quoting
> >> + * and/or escaping. NSS doesn't appear to give us a way to easily
> >> unescape the
> >> + * result, and we need to store the raw CN into port->peer_cn for
> >> compatibility
> >> + * with the OpenSSL implementation.
> >> + */
> >
> > Do we have a testcase for embedded NULLs in common names?
>
> We don't, neither for OpenSSL or NSS. AFAICR Jacob spent days trying to get a
> certificate generation to include an embedded NULL byte but in the end gave
> up.
> We would have to write our own tools for generating certificates to add that
> (which may or may not be a bad idea, but it hasn't been done).
Hah, that's interesting.
> >> +/*
> >> + * pg_SSLShutdownFunc
> >> + *Callback for NSS shutdown
> >> + *
> >> + * If NSS is terminated from the outside when the connection is still in
> >> use
> >
> > What does "NSS is terminated from the outside when the connection" really
> > mean? Does this mean the client initiating something?
>
> If an extension, or other server-loaded code, interfered with NSS and managed
> to close contexts in order to interfere with connections this would ensure us
> closing it down cleanly.
>
> That being said, I was now unable to get my old testcase working so I've for
> now removed this callback from the patch until I can work out if we can make
> proper use of it. AFAICS other mature NSS implementations aren't using it
> (OpenLDAP did in the past but have since removed it, will look at how/why).
I think that'd be elog(FATAL) time if we want to do anything (after changing
state so that no data is sent to client).
> >> + /*
> >> + * The error cases for PR_Recv are not
> >> documented, but can be
> >> + *
Re: Support for NSS as a libpq TLS backend
On Tue, 2022-01-25 at 22:26 +, Jacob Champion wrote: > On Wed, 2022-01-19 at 10:01 +0100, Daniel Gustafsson wrote: > > > On 18 Jan 2022, at 17:37, Jacob Champion wrote: > > > > > > On Wed, 2021-12-15 at 23:10 +0100, Daniel Gustafsson wrote: > > > > I've attached a v50 which fixes the issues found by Joshua upthread, as > > > > well as > > > > rebases on top of all the recent SSL and pgcrypto changes. > > > > > > I'm currently tracking down a slot leak. When opening and closing large > > > numbers of NSS databases, at some point we appear to run out of slots > > > and then NSS starts misbehaving, even though we've closed all of our > > > context handles. > > > > Interesting, are you able to share a reproducer for this so I can assist in > > debugging it? > > (This was in my spam folder, sorry for the delay...) Let me see if I > can minimize my current reproduction case and get it ported out of > Python. Here's my attempt at a Bash port. It has races but reliably reproduces on my machine after 98 connections (there's a hardcoded slot limit of 100, so that makes sense when factoring in the internal NSS slots). --Jacob nss-repro.tar.gz Description: nss-repro.tar.gz
Re: Support for NSS as a libpq TLS backend
On Wed, 2022-01-19 at 10:01 +0100, Daniel Gustafsson wrote: > > On 18 Jan 2022, at 17:37, Jacob Champion wrote: > > > > On Wed, 2021-12-15 at 23:10 +0100, Daniel Gustafsson wrote: > > > I've attached a v50 which fixes the issues found by Joshua upthread, as > > > well as > > > rebases on top of all the recent SSL and pgcrypto changes. > > > > I'm currently tracking down a slot leak. When opening and closing large > > numbers of NSS databases, at some point we appear to run out of slots > > and then NSS starts misbehaving, even though we've closed all of our > > context handles. > > Interesting, are you able to share a reproducer for this so I can assist in > debugging it? (This was in my spam folder, sorry for the delay...) Let me see if I can minimize my current reproduction case and get it ported out of Python. --Jacob
Re: Support for NSS as a libpq TLS backend
Hi,
On 2022-01-18 13:42:54 +0100, Daniel Gustafsson wrote:
> Fixed, I had made a mistake in the OpenSSL.pm testcode and failed to catch it
> in testing.
> +task:
> + name: Linux - Debian Bullseye (nss)
> [ copy of a bunch of code ]
I also needed similar-but-not-quite-equivalent tasks for the meson patch as
well. I just moved to having a splitting the tasks into a template and a use
of it. It's probably not quite right as I did there, but it might be worth
looking into:
https://github.com/anarazel/postgres/blob/meson/.cirrus.yml#L181
But maybe this case actually has a better solution, see two paragraphs down:
> + install_script: |
> +DEBIAN_FRONTEND=noninteractive apt-get --yes install libnss3 libnss3-dev
> libnss3-tools libnspr4 libnspr4-dev
This needs an apt-get update beforehand to succeed. That's what caused the last
few runs
to fail, see e.g.
https://cirrus-ci.com/task/6293612580306944
Just duplicating the task doesn't really scale once in tree. What about
reconfiguring (note: add --enable-depend) the linux tasks to build against
nss, and then run the relevant subset of tests with it? Most tests don't use
tcp / SSL anyway, so rerunning a small subset of tests should be feasible?
> From 297ee9ab31aa579e002edc335cce83dae19711b1 Mon Sep 17 00:00:00 2001
> From: Daniel Gustafsson
> Date: Mon, 8 Feb 2021 23:52:22 +0100
> Subject: [PATCH v52 01/11] nss: Support libnss as TLS library in libpq
> 16 files changed, 3192 insertions(+), 7 deletions(-)
Phew. This is a huge patch.
Damn, I only opened this thread to report the CI failure. But now I ended up
doing a small review...
> +#include "common/nss.h"
> +
> +/*
> + * The nspr/obsolete/protypes.h NSPR header typedefs uint64 and int64 with
> + * colliding definitions from ours, causing a much expected compiler error.
> + * Remove backwards compatibility with ancient NSPR versions to avoid this.
> + */
> +#define NO_NSPR_10_SUPPORT
> +#include
> +#include
> +#include
> +#include
> +#include
Duplicated with nss.h. Which brings me to:
> +#include
Is it a great idea to have common/nss.h when there's a library header nss.h?
Perhaps we should have a pg_ssl_{nss,openssl}.h or such?
> +/* */
> +/*Public interface
> */
> +/* */
Nitpicks:
I don't think we typically do multiple /* */ comments in a row for this type
of thing. I also don't particularly like centering things like this, tends to
get inconsistent across comments.
> +/*
> + * be_tls_open_server
> + *
> + * Since NSPR initialization must happen after forking, most of the actual
> + * setup of NSPR/NSS is done here rather than in be_tls_init.
The "Since ... must happen after forking" sounds like it's referencing a
previously remarked upon fact. But I don't see anything but a copy of this
comment.
Does this make some things notably more expensive? Presumably it does remove a
bunch of COW opportunities, but likely that's not a huge factor compared to
assymetric crypto negotiation...
Maybe soem of this commentary should migrate to the file header or such?
> This introduce
> + * differences with the OpenSSL support where some errors are only reported
> + * at runtime with NSS where they are reported at startup with OpenSSL.
Found this sentence hard to parse somehow.
It seems pretty unfriendly to only have minimal error checking at postmaster
startup time. Seems at least the presence and usability of keys should be done
*also* at that time?
> + /*
> + * If no ciphers are specified, enable them all.
> + */
> + if (!SSLCipherSuites || strlen(SSLCipherSuites) == 0)
> + {
> + status = NSS_SetDomesticPolicy();
> + if (status != SECSuccess)
> + {
> + ereport(COMMERROR,
> + (errmsg("unable to set cipher policy:
> %s",
> +
> pg_SSLerrmessage(PR_GetError();
> + return -1;
> + }
> + }
> + else
> + {
> + char *ciphers,
> +*c;
> +
> + char *sep = ":;, ";
> + PRUint16ciphercode;
> + const PRUint16 *nss_ciphers;
> + boolfound = false;
> +
> + /*
> + * If the user has specified a set of preferred cipher suites
> we start
> + * by turning off all the existing suites to avoid the risk of
> down-
> + * grades to a weaker cipher than expected.
> + */
> + nss_ciphers = SSL_GetImplementedCiphers();
> + for (int i = 0; i < SSL_GetNumImplementedCiphers(); i++)
> + SSL_CipherPrefSet(model, nss_ciphers[i], PR_FALSE);
> +
> +
Re: Support for NSS as a libpq TLS backend
> On 18 Jan 2022, at 17:37, Jacob Champion wrote: > > On Wed, 2021-12-15 at 23:10 +0100, Daniel Gustafsson wrote: >> I've attached a v50 which fixes the issues found by Joshua upthread, as well >> as >> rebases on top of all the recent SSL and pgcrypto changes. > > I'm currently tracking down a slot leak. When opening and closing large > numbers of NSS databases, at some point we appear to run out of slots > and then NSS starts misbehaving, even though we've closed all of our > context handles. Interesting, are you able to share a reproducer for this so I can assist in debugging it? -- Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
On Wed, 2021-12-15 at 23:10 +0100, Daniel Gustafsson wrote: > I've attached a v50 which fixes the issues found by Joshua upthread, as well > as > rebases on top of all the recent SSL and pgcrypto changes. I'm currently tracking down a slot leak. When opening and closing large numbers of NSS databases, at some point we appear to run out of slots and then NSS starts misbehaving, even though we've closed all of our context handles. I don't have anything more helpful to share yet, but I wanted to make a note of it here in case anyone else had seen it or has ideas on what may be causing it. My next move will be to update the version of NSS I'm running. --Jacob
Re: Support for NSS as a libpq TLS backend
On Tue, Jan 18, 2022 at 7:43 AM Daniel Gustafsson wrote: > > > On 18 Jan 2022, at 07:36, Julien Rouhaud wrote: > > > On Mon, Jan 17, 2022 at 03:09:11PM +0100, Daniel Gustafsson wrote: > >> > >> I must've fat-fingered the "git add -p" for v50 as the fix was in > >> configure.ac > >> but not configure. Fixed now. > > > > Thanks! Apparently this version now fails on all OS, e.g.: > > Fixed, I had made a mistake in the OpenSSL.pm testcode and failed to catch it > in testing. LGTM +1
Re: Support for NSS as a libpq TLS backend
Hi, On Mon, Jan 17, 2022 at 03:09:11PM +0100, Daniel Gustafsson wrote: > > I must've fat-fingered the "git add -p" for v50 as the fix was in configure.ac > but not configure. Fixed now. Thanks! Apparently this version now fails on all OS, e.g.: https://cirrus-ci.com/task/4643868095283200 [22:17:39.965] # Failed test 'certificate authorization succeeds with correct client cert in PEM format' [22:17:39.965] # at t/001_ssltests.pl line 456. [22:17:39.965] # got: '2' [22:17:39.965] # expected: '0' [22:17:39.965] [22:17:39.965] # Failed test 'certificate authorization succeeds with correct client cert in PEM format: no stderr' [22:17:39.965] # at t/001_ssltests.pl line 456. [22:17:39.965] # got: 'psql: error: connection to server at "127.0.0.1", port 50023 failed: certificate present, but not private key file "/home/postgres/.postgresql/postgresql.key"' [22:17:39.965] # expected: '' [22:17:39.965] [22:17:39.965] # Failed test 'certificate authorization succeeds with correct client cert in DER format' [22:17:39.965] # at t/001_ssltests.pl line 475. [22:17:39.965] # got: '2' [22:17:39.965] # expected: '0' [...]
Re: Support for NSS as a libpq TLS backend
Hi, On Wed, Dec 15, 2021 at 11:10:14PM +0100, Daniel Gustafsson wrote: > > I've attached a v50 which fixes the issues found by Joshua upthread, as well > as > rebases on top of all the recent SSL and pgcrypto changes. The cfbot reports that the patchset doesn't apply anymore: http://cfbot.cputube.org/patch_36_3138.log === Applying patches on top of PostgreSQL commit ID 74527c3e022d3ace648340b79a6ddec3419f6732 === [...] === applying patch ./v50-0010-nss-Build-infrastructure.patch patching file configure patching file configure.ac Hunk #3 succeeded at 1566 (offset 1 line). Hunk #4 succeeded at 2366 (offset 1 line). Hunk #5 succeeded at 2379 (offset 1 line). patching file src/backend/libpq/Makefile patching file src/common/Makefile patching file src/include/pg_config.h.in Hunk #3 succeeded at 926 (offset 3 lines). patching file src/interfaces/libpq/Makefile patching file src/tools/msvc/Install.pm Hunk #1 FAILED at 440. 1 out of 1 hunk FAILED -- saving rejects to file src/tools/msvc/Install.pm.rej Could you send a rebased version, possibly with an updated configure as reported by Joshua? In the meantime I will switch the entry to Waitinng on Author.
Re: Support for NSS as a libpq TLS backend
On Wed, Dec 15, 2021 at 5:05 PM Daniel Gustafsson wrote: > > > On 25 Nov 2021, at 14:39, Joshua Brindle > > wrote: > > On Wed, Nov 24, 2021 at 8:49 AM Joshua Brindle > > wrote: > >> > >> On Wed, Nov 24, 2021 at 8:46 AM Joshua Brindle > >> wrote: > > >> I don't know enough about NSS to know if this is problematic or not > >> but if I try verify-full without having the root CA in the certificate > >> store I get: > >> > >> $ /usr/pgsql-15/bin/psql "host=localhost sslmode=verify-full user=postgres" > >> psql: error: SSL error: Issuer certificate is invalid. > >> unable to shut down NSS context: NSS could not shutdown. Objects are > >> still in use. > > Fixed. > > > Something is strange with ssl downgrading and a bad ssldatabase > > [postgres@11cdfa30f763 ~]$ /usr/pgsql-15/bin/psql "ssldatabase=oops > > sslcert=client_cert host=localhost" > > Password for user postgres: > > > > > > Also fixed. > > > On the server side: > > 2021-11-25 01:52:01.984 UTC [269] LOG: unable to handshake: > > Encountered end of file (PR_END_OF_FILE_ERROR) > > This is normal and expected, but to make it easier on users I've changed this > error message to be aligned with the OpenSSL implementation. > > > Other than that and I still haven't tested --with-llvm I've gotten > > everything working, including with an openssl client. Attached is a > > dockerfile that gets to the point where a client can connect with > > clientcert=verify-full. I've removed some of the old cruft and > > debugging from the previous versions. > > Very cool, thanks! I've been unable to reproduce any issues with llvm but > I'll > keep poking at that. A new version will be posted shortly with the above and > a > few more fixes. For v50 this change was required for an llvm build to succeed on my Fedora system: diff --git a/configure b/configure index 25388a75a2..62d554806a 100755 --- a/configure +++ b/configure @@ -13276,6 +13276,7 @@ fi LDFLAGS="$LDFLAGS $NSS_LIBS $NSPR_LIBS" CFLAGS="$CFLAGS $NSS_CFLAGS $NSPR_CFLAGS" + CPPFLAGS="$CPPFLAGS $NSS_CFLAGS $NSPR_CFLAGS" $as_echo "#define USE_NSS 1" >>confdefs.h I'm not certain why configure didn't already have that, configure.ac appears to, but nonetheless it builds, all tests succeed, and a quick tire kicking looks good. Thank you.
Re: Support for NSS as a libpq TLS backend
On Wed, 2021-12-15 at 23:10 +0100, Daniel Gustafsson wrote: > > On 30 Nov 2021, at 20:03, Jacob Champion wrote: > > > > On Mon, 2021-09-27 at 15:44 +0200, Daniel Gustafsson wrote: > > > > Speaking of IP addresses in SANs, it doesn't look like our OpenSSL > > > > backend can handle those. That's a separate conversation, but I might > > > > take a look at a patch for next commitfest. > > > > > > Please do. > > > > Didn't get around to it for November, but I'm putting the finishing > > touches on that now. > > Cool, thanks! Done and registered in Commitfest. > Yeah, that's clearly bogus. I followed the bouncing ball reading NSS code and > from what I can tell the comment is correct. I removed the dead code, only > realizing after the fact that I might cause conflict with your tree doing so, > in that case sorry. No worries, there weren't any issues with the rebase. > I've attached a v50 which fixes the issues found by Joshua upthread, as well > as > rebases on top of all the recent SSL and pgcrypto changes. Thanks! --Jacob
Re: Support for NSS as a libpq TLS backend
> On 25 Nov 2021, at 14:39, Joshua Brindle > wrote: > On Wed, Nov 24, 2021 at 8:49 AM Joshua Brindle > wrote: >> >> On Wed, Nov 24, 2021 at 8:46 AM Joshua Brindle >> wrote: >> I don't know enough about NSS to know if this is problematic or not >> but if I try verify-full without having the root CA in the certificate >> store I get: >> >> $ /usr/pgsql-15/bin/psql "host=localhost sslmode=verify-full user=postgres" >> psql: error: SSL error: Issuer certificate is invalid. >> unable to shut down NSS context: NSS could not shutdown. Objects are >> still in use. Fixed. > Something is strange with ssl downgrading and a bad ssldatabase > [postgres@11cdfa30f763 ~]$ /usr/pgsql-15/bin/psql "ssldatabase=oops > sslcert=client_cert host=localhost" > Password for user postgres: > > Also fixed. > On the server side: > 2021-11-25 01:52:01.984 UTC [269] LOG: unable to handshake: > Encountered end of file (PR_END_OF_FILE_ERROR) This is normal and expected, but to make it easier on users I've changed this error message to be aligned with the OpenSSL implementation. > Other than that and I still haven't tested --with-llvm I've gotten > everything working, including with an openssl client. Attached is a > dockerfile that gets to the point where a client can connect with > clientcert=verify-full. I've removed some of the old cruft and > debugging from the previous versions. Very cool, thanks! I've been unable to reproduce any issues with llvm but I'll keep poking at that. A new version will be posted shortly with the above and a few more fixes. -- Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
On Mon, 2021-09-27 at 15:44 +0200, Daniel Gustafsson wrote:
> > Speaking of IP addresses in SANs, it doesn't look like our OpenSSL
> > backend can handle those. That's a separate conversation, but I might
> > take a look at a patch for next commitfest.
>
> Please do.
Didn't get around to it for November, but I'm putting the finishing
touches on that now.
While I was looking at the new SAN code (in fe-secure-nss.c,
pgtls_verify_peer_name_matches_certificate_guts()), I noticed that code
coverage never seemed to touch a good chunk of it:
> +for (cn = san_list; cn != san_list; cn = CERT_GetNextGeneralName(cn))
> +{
> +char *alt_name;
> +int rv;
> +chartmp[512];
That loop can never execute. But I wonder if all of that extra SAN code
should be removed anyway? There's this comment above it:
> + /*
> + * CERT_VerifyCertName will internally perform RFC 2818 SubjectAltName
> + * verification.
> + */
and it seems like SAN verification is working in my testing, despite
the dead loop.
--Jacob
Re: Support for NSS as a libpq TLS backend
On Wed, Nov 24, 2021 at 8:49 AM Joshua Brindle wrote: > > On Wed, Nov 24, 2021 at 8:46 AM Joshua Brindle > wrote: > > > > On Wed, Nov 24, 2021 at 6:59 AM Daniel Gustafsson wrote: > > > > > > > On 23 Nov 2021, at 23:39, Joshua Brindle > > > > wrote: > > > > > > > It no longer happens with v49, since it was a null deref of the pr_fd > > > > which no longer happens. > > > > > > > > I'll continue testing now, so far it's looking better. > > > > > > Great, thanks for confirming. I'm still keen on knowing how you > > > triggered the > > > segfault so I can ensure there are no further bugs around there. > > > > > > > It happened when I ran psql with hostssl on the server but before I'd > > initialized my client certificate store. > > I don't know enough about NSS to know if this is problematic or not > but if I try verify-full without having the root CA in the certificate > store I get: > > $ /usr/pgsql-15/bin/psql "host=localhost sslmode=verify-full user=postgres" > psql: error: SSL error: Issuer certificate is invalid. > unable to shut down NSS context: NSS could not shutdown. Objects are > still in use. Something is strange with ssl downgrading and a bad ssldatabase [postgres@11cdfa30f763 ~]$ /usr/pgsql-15/bin/psql "ssldatabase=oops sslcert=client_cert host=localhost" Password for user postgres: On the server side: 2021-11-25 01:52:01.984 UTC [269] LOG: unable to handshake: Encountered end of file (PR_END_OF_FILE_ERROR) Other than that and I still haven't tested --with-llvm I've gotten everything working, including with an openssl client. Attached is a dockerfile that gets to the point where a client can connect with clientcert=verify-full. I've removed some of the old cruft and debugging from the previous versions. Thank you. Dockerfile Description: Binary data
Re: Support for NSS as a libpq TLS backend
On Wed, Nov 24, 2021 at 8:46 AM Joshua Brindle wrote: > > On Wed, Nov 24, 2021 at 6:59 AM Daniel Gustafsson wrote: > > > > > On 23 Nov 2021, at 23:39, Joshua Brindle > > > wrote: > > > > > It no longer happens with v49, since it was a null deref of the pr_fd > > > which no longer happens. > > > > > > I'll continue testing now, so far it's looking better. > > > > Great, thanks for confirming. I'm still keen on knowing how you triggered > > the > > segfault so I can ensure there are no further bugs around there. > > > > It happened when I ran psql with hostssl on the server but before I'd > initialized my client certificate store. I don't know enough about NSS to know if this is problematic or not but if I try verify-full without having the root CA in the certificate store I get: $ /usr/pgsql-15/bin/psql "host=localhost sslmode=verify-full user=postgres" psql: error: SSL error: Issuer certificate is invalid. unable to shut down NSS context: NSS could not shutdown. Objects are still in use.
Re: Support for NSS as a libpq TLS backend
On Wed, Nov 24, 2021 at 6:59 AM Daniel Gustafsson wrote: > > > On 23 Nov 2021, at 23:39, Joshua Brindle > > wrote: > > > It no longer happens with v49, since it was a null deref of the pr_fd > > which no longer happens. > > > > I'll continue testing now, so far it's looking better. > > Great, thanks for confirming. I'm still keen on knowing how you triggered the > segfault so I can ensure there are no further bugs around there. > It happened when I ran psql with hostssl on the server but before I'd initialized my client certificate store.
Re: Support for NSS as a libpq TLS backend
> On 23 Nov 2021, at 23:39, Joshua Brindle > wrote: > It no longer happens with v49, since it was a null deref of the pr_fd > which no longer happens. > > I'll continue testing now, so far it's looking better. Great, thanks for confirming. I'm still keen on knowing how you triggered the segfault so I can ensure there are no further bugs around there. -- Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
On Tue, Nov 23, 2021 at 9:12 AM Daniel Gustafsson wrote: > > > On 17 Nov 2021, at 19:42, Joshua Brindle > > wrote: > > On Tue, Nov 16, 2021 at 1:26 PM Joshua Brindle > > wrote: > > >> I think there it a typo in the docs here that prevents them from > >> building (this diff seems to fix it): > > Ah yes, thanks, I had noticed that one but forgot to send out a new version to > make the CFBot green. > > > After a bit more testing, the server is up and running with an nss > > database but before configuring the client database I tried connecting > > and got a segfault: > > Interesting. I'm unable to reproduce this crash, can you show the sequence of > commands which led to this? It no longer happens with v49, since it was a null deref of the pr_fd which no longer happens. I'll continue testing now, so far it's looking better. Did the build issue with --with-llvm get fixed in this update also? I haven't tried building with it yet. > > It looks like the ssl connection falls through to attempt a non-ssl > > connection but at some point conn->ssl_in_use gets set to true, > > despite pr_fd and nss_context being null. > > pgtls_close missed setting ssl_in_use to false, fixed in the attached. I've > also added some assertions to the connection setup for debugging this. > > > This patch fixes the segfault but I suspect is not the correct fix, > > due to the error when connecting saying "Success": > > Right, without an SSL enabled FD we should never get here. > Thank you.
Re: Support for NSS as a libpq TLS backend
On Tue, Nov 16, 2021 at 1:26 PM Joshua Brindle
wrote:
>
> On Tue, Nov 16, 2021 at 9:45 AM Joshua Brindle
> wrote:
> >
> > On Mon, Nov 15, 2021 at 5:37 PM Joshua Brindle
> > wrote:
> > >
> > > On Mon, Nov 15, 2021 at 4:44 PM Daniel Gustafsson wrote:
> > > >
> > > > > On 15 Nov 2021, at 20:51, Joshua Brindle
> > > > > wrote:
> > > >
> > > > > Apologies for the delay, this didn't go to my inbox and I missed it
> > > > > on list.
> > > > >
> > > > > The bitcode generation is still broken, this time for nspr.h:
> > > >
> > > > Interesting, I am unable to replicate that in my tree but I'll
> > > > investigate
> > > > further tomorrow using your Dockerfile. For the sake of testing, does
> > > > compilation pass for you in the same place without using --with-llvm?
> > > >
> > >
> > > Yes, it builds and check-world passes. I'll continue testing with this
> > > build. Thank you.
> >
> > The previous Dockerfile had some issues due to a hasty port from RHEL
> > to Fedora, attached is one that works with your patchset, llvm
> > currently disabled, and the llvm deps removed.
> >
> > The service file is also attached since it's referenced in the
> > Dockerfile and you'd have had to reproduce it.
> >
> > After building, run with:
> > docker run --name pg-test -p 5432:5432 --cap-add=SYS_ADMIN -v
> > /sys/fs/cgroup:/sys/fs/cgroup:ro -d
>
> I think there it a typo in the docs here that prevents them from
> building (this diff seems to fix it):
>
> diff --git a/doc/src/sgml/pgcrypto.sgml b/doc/src/sgml/pgcrypto.sgml
> index 56b73e033c..844aa31e86 100644
> --- a/doc/src/sgml/pgcrypto.sgml
> +++ b/doc/src/sgml/pgcrypto.sgml
> @@ -767,7 +767,7 @@ pgp_sym_encrypt(data, psw, 'compress-algo=1,
> cipher-algo=aes256')
>
> Which cipher algorithm to use. cast5 is only
> available
> if PostgreSQL was built with
> -OpenSSL.
> +OpenSSL.
>
>
> Values: bf, aes128, aes192, aes256, 3des, cast5
After a bit more testing, the server is up and running with an nss
database but before configuring the client database I tried connecting
and got a segfault:
#0 PR_Write (fd=0x0, buf=0x141ba60, amount=84) at
io/../../.././nspr/pr/src/io/priometh.c:114
#1 0x7ff33dfdc62f in pgtls_write (conn=0x13cecb0, ptr=0x141ba60,
len=84) at fe-secure-nss.c:583
#2 0x7ff33dfd6e18 in pqsecure_write (conn=0x13cecb0,
ptr=0x141ba60, len=84) at fe-secure.c:295
#3 0x7ff33dfd04dc in pqSendSome (conn=0x13cecb0, len=84) at fe-misc.c:834
#4 0x7ff33dfd06c8 in pqFlush (conn=0x13cecb0) at fe-misc.c:972
#5 0x7ff33dfc257c in pqPacketSend (conn=0x13cecb0, pack_type=0
'\000', buf=0x1414c60, buf_len=80) at fe-connect.c:4619
#6 0x7ff33dfbfadd in PQconnectPoll (conn=0x13cecb0) at fe-connect.c:2986
#7 0x7ff33dfbe55c in connectDBComplete (conn=0x13cecb0) at
fe-connect.c:2218
#8 0x7ff33dfbbaef in PQconnectdbParams (keywords=0x1427d10,
values=0x1427e60, expand_dbname=1) at fe-connect.c:668
#9 0x0043ebc7 in main (argc=2, argv=0x7ffdccd0e2f8) at startup.c:273
It looks like the ssl connection falls through to attempt a non-ssl
connection but at some point conn->ssl_in_use gets set to true,
despite pr_fd and nss_context being null.
This patch fixes the segfault but I suspect is not the correct fix,
due to the error when connecting saying "Success":
--- a/src/interfaces/libpq/fe-secure-nss.c
+++ b/src/interfaces/libpq/fe-secure-nss.c
@@ -498,6 +498,11 @@ pgtls_read(PGconn *conn, void *ptr, size_t len)
* for closed connections, while -1 indicates an error within
the ongoing
* connection.
*/
+ if (!conn->pr_fd) {
+ SOCK_ERRNO_SET(read_errno);
+ return -1;
+ }
+
nread = PR_Recv(conn->pr_fd, ptr, len, 0, PR_INTERVAL_NO_WAIT);
if (nread == 0)
@@ -580,6 +585,11 @@ pgtls_write(PGconn *conn, const void *ptr, size_t len)
PRErrorCode status;
int write_errno = 0;
+ if (!conn->pr_fd) {
+ SOCK_ERRNO_SET(write_errno);
+ return -1;
+ }
+
n = PR_Write(conn->pr_fd, ptr, len);
if (n < 0)
Re: Support for NSS as a libpq TLS backend
On Tue, Nov 16, 2021 at 9:45 AM Joshua Brindle wrote: > > On Mon, Nov 15, 2021 at 5:37 PM Joshua Brindle > wrote: > > > > On Mon, Nov 15, 2021 at 4:44 PM Daniel Gustafsson wrote: > > > > > > > On 15 Nov 2021, at 20:51, Joshua Brindle > > > > wrote: > > > > > > > Apologies for the delay, this didn't go to my inbox and I missed it on > > > > list. > > > > > > > > The bitcode generation is still broken, this time for nspr.h: > > > > > > Interesting, I am unable to replicate that in my tree but I'll investigate > > > further tomorrow using your Dockerfile. For the sake of testing, does > > > compilation pass for you in the same place without using --with-llvm? > > > > > > > Yes, it builds and check-world passes. I'll continue testing with this > > build. Thank you. > > The previous Dockerfile had some issues due to a hasty port from RHEL > to Fedora, attached is one that works with your patchset, llvm > currently disabled, and the llvm deps removed. > > The service file is also attached since it's referenced in the > Dockerfile and you'd have had to reproduce it. > > After building, run with: > docker run --name pg-test -p 5432:5432 --cap-add=SYS_ADMIN -v > /sys/fs/cgroup:/sys/fs/cgroup:ro -d I think there it a typo in the docs here that prevents them from building (this diff seems to fix it): diff --git a/doc/src/sgml/pgcrypto.sgml b/doc/src/sgml/pgcrypto.sgml index 56b73e033c..844aa31e86 100644 --- a/doc/src/sgml/pgcrypto.sgml +++ b/doc/src/sgml/pgcrypto.sgml @@ -767,7 +767,7 @@ pgp_sym_encrypt(data, psw, 'compress-algo=1, cipher-algo=aes256') Which cipher algorithm to use. cast5 is only available if PostgreSQL was built with -OpenSSL. +OpenSSL. Values: bf, aes128, aes192, aes256, 3des, cast5
Re: Support for NSS as a libpq TLS backend
On Mon, Nov 15, 2021 at 5:37 PM Joshua Brindle wrote: > > On Mon, Nov 15, 2021 at 4:44 PM Daniel Gustafsson wrote: > > > > > On 15 Nov 2021, at 20:51, Joshua Brindle > > > wrote: > > > > > Apologies for the delay, this didn't go to my inbox and I missed it on > > > list. > > > > > > The bitcode generation is still broken, this time for nspr.h: > > > > Interesting, I am unable to replicate that in my tree but I'll investigate > > further tomorrow using your Dockerfile. For the sake of testing, does > > compilation pass for you in the same place without using --with-llvm? > > > > Yes, it builds and check-world passes. I'll continue testing with this > build. Thank you. The previous Dockerfile had some issues due to a hasty port from RHEL to Fedora, attached is one that works with your patchset, llvm currently disabled, and the llvm deps removed. The service file is also attached since it's referenced in the Dockerfile and you'd have had to reproduce it. After building, run with: docker run --name pg-test -p 5432:5432 --cap-add=SYS_ADMIN -v /sys/fs/cgroup:/sys/fs/cgroup:ro -d Dockerfile Description: Binary data postgresql-15.service Description: Binary data
Re: Support for NSS as a libpq TLS backend
On Mon, Nov 15, 2021 at 4:44 PM Daniel Gustafsson wrote: > > > On 15 Nov 2021, at 20:51, Joshua Brindle > > wrote: > > > Apologies for the delay, this didn't go to my inbox and I missed it on list. > > > > The bitcode generation is still broken, this time for nspr.h: > > Interesting, I am unable to replicate that in my tree but I'll investigate > further tomorrow using your Dockerfile. For the sake of testing, does > compilation pass for you in the same place without using --with-llvm? > Yes, it builds and check-world passes. I'll continue testing with this build. Thank you.
Re: Support for NSS as a libpq TLS backend
> On 15 Nov 2021, at 20:51, Joshua Brindle > wrote: > Apologies for the delay, this didn't go to my inbox and I missed it on list. > > The bitcode generation is still broken, this time for nspr.h: Interesting, I am unable to replicate that in my tree but I'll investigate further tomorrow using your Dockerfile. For the sake of testing, does compilation pass for you in the same place without using --with-llvm? -- Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
On Wed, Nov 10, 2021 at 8:49 AM Daniel Gustafsson wrote: > > > On 9 Nov 2021, at 22:22, Joshua Brindle > > wrote: > > On Tue, Nov 9, 2021 at 2:02 PM Joshua Brindle > > wrote: > >> > >> On Tue, Nov 9, 2021 at 1:59 PM Joshua Brindle > >> wrote: > > >>> Hello, I'm looking to help out with reviews for this CF and I'm > >>> currently looking at this patchset. > > Thanks, much appreciated! > > >>> currently I'm stuck trying to configure: > >>> > >>> checking for nss-config... /usr/bin/nss-config > >>> checking for nspr-config... /usr/bin/nspr-config > >>> ... > >>> checking nss/ssl.h usability... no > >>> checking nss/ssl.h presence... no > >>> checking for nss/ssl.h... no > >>> configure: error: header file is required for NSS > >>> > >>> This is on fedora 33 and nss-devel is installed, nss-config is > >>> available (and configure finds it) but the directory is different from > >>> Ubuntu: > >>> (base) [vagrant@fedora ~]$ nss-config --includedir > >>> /usr/include/nss3 > >>> (base) [vagrant@fedora ~]$ ls -al /usr/include/nss3/ssl.h > >>> -rw-r--r--. 1 root root 70450 Sep 30 05:41 /usr/include/nss3/ssl.h > >>> > >>> So if nss-config --includedir is used then #include should be > >>> used, or if not then #include but on this system #include > >>> is not going to work. > > Interesting rename, I doubt any version but NSS 3 and NSPR 4 is alive anywhere > and an incremented major version seems highly unlikely. Going back to plain > #include and have the includeflags sort out the correct directories > seems like the best option then. Fixed in the attached. > > >> FYI, if I make a symlink to get past this, configure completes but > >> compilation fails because nspr/nspr.h cannot be found (I'm not sure > >> why configure doesn't discover this) > >> ../../src/include/common/nss.h:31:10: fatal error: 'nspr/nspr.h' file not > >> found > >> #include In file included from protocol_nss.c:24: > >> ../../src/include/common/nss.h:31:10: fatal error: 'nspr/nspr.h' file not > >> found > >> #include > >> ^ > >> > >> It's a similar issue: > >> $ nspr-config --includedir > >> /usr/include/nspr4 > > Fixed. > > > If these get resolved the next issue is llvm bitcode doesn't compile > > because the nss includedir is missing from CPPFLAGS: > > > > /usr/bin/clang -Wno-ignored-attributes -fno-strict-aliasing -fwrapv > > -O2 -I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 > > -I/usr/include -flto=thin -emit-llvm -c -o be-secure-nss.bc > > be-secure-nss.c > > In file included from be-secure-nss.c:20: > > In file included from ../../../src/include/common/nss.h:38: > > In file included from /usr/include/nss/nss.h:34: > > /usr/include/nss/seccomon.h:17:10: fatal error: 'prtypes.h' file not found > > #include "prtypes.h" > > ^~~ > > 1 error generated. > > Fixed. Apologies for the delay, this didn't go to my inbox and I missed it on list. The bitcode generation is still broken, this time for nspr.h: /usr/bin/clang -Wno-ignored-attributes -fno-strict-aliasing -fwrapv -O2 -I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -I/usr/include -flto=thin -emit-llvm -c -o be-secure-nss.bc be-secure-nss.c In file included from be-secure-nss.c:20: ../../../src/include/common/nss.h:31:10: fatal error: 'nspr.h' file not found #include ^~~~ 1 error generated. FWIW I attached the Dockerfile I've been using to test this, primarily to ensure that there were no openssl devel files lurking around during compilation. It expects a ./postgres directory with whatever patches already applied to it. > > The attached also resolves the conflicts in pgcrypto following db7d1a7b05. > PGP > elgamel and RSA pubkey functions aren't supported for now as there is no > bignum > functions similar to the BN_* in OpenSSL. I will look into more how hard it > would be to support, for now this gets us ahead. > > -- > Daniel Gustafsson https://vmware.com/ > Dockerfile Description: Binary data
Re: Support for NSS as a libpq TLS backend
On Tue, Nov 9, 2021 at 2:02 PM Joshua Brindle wrote: > > On Tue, Nov 9, 2021 at 1:59 PM Joshua Brindle > wrote: > > > > On Fri, Nov 5, 2021 at 6:01 AM Daniel Gustafsson wrote: > > > > > > Attached is a rebase fixing a tiny bug in the documentation which > > > prevented it > > > from being able to compile. > > > > > > > Hello, I'm looking to help out with reviews for this CF and I'm > > currently looking at this patchset. > > > > currently I'm stuck trying to configure: > > > > checking for nss-config... /usr/bin/nss-config > > checking for nspr-config... /usr/bin/nspr-config > > ... > > checking nss/ssl.h usability... no > > checking nss/ssl.h presence... no > > checking for nss/ssl.h... no > > configure: error: header file is required for NSS > > > > This is on fedora 33 and nss-devel is installed, nss-config is > > available (and configure finds it) but the directory is different from > > Ubuntu: > > (base) [vagrant@fedora ~]$ nss-config --includedir > > /usr/include/nss3 > > (base) [vagrant@fedora ~]$ ls -al /usr/include/nss3/ssl.h > > -rw-r--r--. 1 root root 70450 Sep 30 05:41 /usr/include/nss3/ssl.h > > > > So if nss-config --includedir is used then #include should be > > used, or if not then #include but on this system #include > > is not going to work. > > FYI, if I make a symlink to get past this, configure completes but > compilation fails because nspr/nspr.h cannot be found (I'm not sure > why configure doesn't discover this) > ../../src/include/common/nss.h:31:10: fatal error: 'nspr/nspr.h' file not > found > #include In file included from protocol_nss.c:24: > ../../src/include/common/nss.h:31:10: fatal error: 'nspr/nspr.h' file not > found > #include > ^ > > It's a similar issue: > $ nspr-config --includedir > /usr/include/nspr4 If these get resolved the next issue is llvm bitcode doesn't compile because the nss includedir is missing from CPPFLAGS: /usr/bin/clang -Wno-ignored-attributes -fno-strict-aliasing -fwrapv -O2 -I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -I/usr/include -flto=thin -emit-llvm -c -o be-secure-nss.bc be-secure-nss.c In file included from be-secure-nss.c:20: In file included from ../../../src/include/common/nss.h:38: In file included from /usr/include/nss/nss.h:34: /usr/include/nss/seccomon.h:17:10: fatal error: 'prtypes.h' file not found #include "prtypes.h" ^~~ 1 error generated.
Re: Support for NSS as a libpq TLS backend
On Tue, Nov 9, 2021 at 1:59 PM Joshua Brindle wrote: > > On Fri, Nov 5, 2021 at 6:01 AM Daniel Gustafsson wrote: > > > > Attached is a rebase fixing a tiny bug in the documentation which prevented > > it > > from being able to compile. > > > > Hello, I'm looking to help out with reviews for this CF and I'm > currently looking at this patchset. > > currently I'm stuck trying to configure: > > checking for nss-config... /usr/bin/nss-config > checking for nspr-config... /usr/bin/nspr-config > ... > checking nss/ssl.h usability... no > checking nss/ssl.h presence... no > checking for nss/ssl.h... no > configure: error: header file is required for NSS > > This is on fedora 33 and nss-devel is installed, nss-config is > available (and configure finds it) but the directory is different from > Ubuntu: > (base) [vagrant@fedora ~]$ nss-config --includedir > /usr/include/nss3 > (base) [vagrant@fedora ~]$ ls -al /usr/include/nss3/ssl.h > -rw-r--r--. 1 root root 70450 Sep 30 05:41 /usr/include/nss3/ssl.h > > So if nss-config --includedir is used then #include should be > used, or if not then #include but on this system #include > is not going to work. FYI, if I make a symlink to get past this, configure completes but compilation fails because nspr/nspr.h cannot be found (I'm not sure why configure doesn't discover this) ../../src/include/common/nss.h:31:10: fatal error: 'nspr/nspr.h' file not found #include In file included from protocol_nss.c:24: ../../src/include/common/nss.h:31:10: fatal error: 'nspr/nspr.h' file not found #include ^ It's a similar issue: $ nspr-config --includedir /usr/include/nspr4
Re: Support for NSS as a libpq TLS backend
On Fri, Nov 5, 2021 at 6:01 AM Daniel Gustafsson wrote: > > Attached is a rebase fixing a tiny bug in the documentation which prevented it > from being able to compile. > Hello, I'm looking to help out with reviews for this CF and I'm currently looking at this patchset. currently I'm stuck trying to configure: checking for nss-config... /usr/bin/nss-config checking for nspr-config... /usr/bin/nspr-config ... checking nss/ssl.h usability... no checking nss/ssl.h presence... no checking for nss/ssl.h... no configure: error: header file is required for NSS This is on fedora 33 and nss-devel is installed, nss-config is available (and configure finds it) but the directory is different from Ubuntu: (base) [vagrant@fedora ~]$ nss-config --includedir /usr/include/nss3 (base) [vagrant@fedora ~]$ ls -al /usr/include/nss3/ssl.h -rw-r--r--. 1 root root 70450 Sep 30 05:41 /usr/include/nss3/ssl.h So if nss-config --includedir is used then #include should be used, or if not then #include but on this system #include is not going to work. Thanks
Re: Support for NSS as a libpq TLS backend
For anyone else trying to test out this branch I'm able to get the patches to apply cleanly if I check out e.g. commit 92e6a98c3636948e7ece9a3260f9d89dd60da278. Kevin -- Kevin Burke phone: 925-271-7005 | kevin.burke.dev On Thu, Oct 28, 2021 at 9:31 PM Kevin Burke wrote: > Hi all, apologies but I'm having trouble applying the latest patch (v45) > to the latest commit on master (6b0f6f79eef2168ce38a8ee99c3ed76e3df5d7ad) > > I downloaded all of the patches to my local filesystem, and then ran: > > for patch in > ../../kevinburke/rustls-postgres/patchsets/2021-10-05-gustafsson-mailing-list/*.patch; > do git am $patch; done; > > I get the following error on the second patch file: > > Applying: Refactor SSL testharness for multiple library > error: patch failed: src/test/ssl/t/001_ssltests.pl:7 > error: src/test/ssl/t/001_ssltests.pl: patch does not apply > error: patch failed: src/test/ssl/t/SSLServer.pm:26 > error: src/test/ssl/t/SSLServer.pm: patch does not apply > Patch failed at 0001 Refactor SSL testharness for multiple library > hint: Use 'git am --show-current-patch=diff' to see the failed patch > > I believe that these patches need to integrate the refactoring in > commit b3b4d8e68ae83f432f43f035c7eb481ef93e1583 - git is searching for the > wrong text in the existing file, but I'm not sure how to submit a patch > against a patch. > > Thanks, > Kevin > > On Tue, Oct 5, 2021 at 8:05 AM Jacob Champion > wrote: > >> On Tue, 2021-10-05 at 15:08 +0200, Daniel Gustafsson wrote: >> > Thanks! These changes looks good. Since you accidentally based this >> on v43 >> > and not the v44 I posted with the cryptohash fix in, the attached is a >> v45 with >> > both your v44 and the previous one, all rebased over HEAD. >> >> Thanks, and sorry about that. >> >> --Jacob >> >
Re: Support for NSS as a libpq TLS backend
Hi all, apologies but I'm having trouble applying the latest patch (v45) to the latest commit on master (6b0f6f79eef2168ce38a8ee99c3ed76e3df5d7ad) I downloaded all of the patches to my local filesystem, and then ran: for patch in ../../kevinburke/rustls-postgres/patchsets/2021-10-05-gustafsson-mailing-list/*.patch; do git am $patch; done; I get the following error on the second patch file: Applying: Refactor SSL testharness for multiple library error: patch failed: src/test/ssl/t/001_ssltests.pl:7 error: src/test/ssl/t/001_ssltests.pl: patch does not apply error: patch failed: src/test/ssl/t/SSLServer.pm:26 error: src/test/ssl/t/SSLServer.pm: patch does not apply Patch failed at 0001 Refactor SSL testharness for multiple library hint: Use 'git am --show-current-patch=diff' to see the failed patch I believe that these patches need to integrate the refactoring in commit b3b4d8e68ae83f432f43f035c7eb481ef93e1583 - git is searching for the wrong text in the existing file, but I'm not sure how to submit a patch against a patch. Thanks, Kevin On Tue, Oct 5, 2021 at 8:05 AM Jacob Champion wrote: > On Tue, 2021-10-05 at 15:08 +0200, Daniel Gustafsson wrote: > > Thanks! These changes looks good. Since you accidentally based this on > v43 > > and not the v44 I posted with the cryptohash fix in, the attached is a > v45 with > > both your v44 and the previous one, all rebased over HEAD. > > Thanks, and sorry about that. > > --Jacob >
Re: Support for NSS as a libpq TLS backend
On Tue, 2021-10-05 at 15:08 +0200, Daniel Gustafsson wrote: > Thanks! These changes looks good. Since you accidentally based this on v43 > and not the v44 I posted with the cryptohash fix in, the attached is a v45 > with > both your v44 and the previous one, all rebased over HEAD. Thanks, and sorry about that. --Jacob
Re: Support for NSS as a libpq TLS backend
> On 1 Oct 2021, at 02:02, Jacob Champion wrote: > On my machine, at least, exit() is coming in due to a few calls to > psprintf(), pstrdup(), and pg_malloc() in the new NSS code. > (Disassembly via `objdump -S libpq.so` helped me track those down.) I'm > working on a patch. Ah, that makes perfect sense. I was too focused on hunting in what new was linked against that I overlooked the obvious. Thanks for finding these. -- Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
On Thu, 2021-09-30 at 16:04 +, Jacob Champion wrote: > On Thu, 2021-09-30 at 14:17 +0200, Daniel Gustafsson wrote: > > The libpq libnss implementation doesn't call either of these, and neither > > does > > libnss. > > I thought the refs check only searched for direct symbol dependencies; > is that piece of NSPR being statically included somehow? On my machine, at least, exit() is coming in due to a few calls to psprintf(), pstrdup(), and pg_malloc() in the new NSS code. (Disassembly via `objdump -S libpq.so` helped me track those down.) I'm working on a patch. --Jacob
Re: Support for NSS as a libpq TLS backend
On Thu, 2021-09-30 at 14:17 +0200, Daniel Gustafsson wrote: > The libpq libnss implementation doesn't call either of these, and neither does > libnss. I thought the refs check only searched for direct symbol dependencies; is that piece of NSPR being statically included somehow? > I'm not entirely sure what to do here, it clearly requires an exception in the > Makefile check of sorts if we deem we can live with this. > > @Jacob: how did you configure your copy of NSPR? I use the Ubuntu 20.04 builtin (NSPR 4.25.0), but it looks like the reason I haven't been seeing this is because I've always used --enable- coverage. If I take that out, I see the same exit check failure. --Jacob
Re: Support for NSS as a libpq TLS backend
On Mon, Sep 20, 2021 at 2:38 AM Daniel Gustafsson wrote: > > Rebased on top of HEAD with off-list comment fixes by Kevin Burke. > Hello Daniel, I've been playing with your patch on Mac (OS 11.6 Big Sur) and have run into a couple of issues so far. 1. I get 7 warnings while running make (truncated): cryptohash_nss.c:101:21: warning: implicit conversion from enumeration type 'SECOidTag' to different enumeration type 'HASH_HashType' [-Wenum-conversion] ctx->hash_type = SEC_OID_SHA1; ~ ^~~~ ... cryptohash_nss.c:134:34: warning: implicit conversion from enumeration type 'HASH_HashType' to different enumeration type 'SECOidTag' [-Wenum-conversion] hash = SECOID_FindOIDByTag(ctx->hash_type); ~~~ ~^ 7 warnings generated. 2. libpq-refs-stamp fails -- it appears an exit is being injected into libpq on Mac Notes about my environment: I've installed nss via homebrew (at version 3.70) and linked it. Cheers, Rachel
Re: Support for NSS as a libpq TLS backend
On Mon, 2021-09-27 at 15:44 +0200, Daniel Gustafsson wrote: > > On 21 Sep 2021, at 02:06, Jacob Champion wrote: > > but I'm not sure that would be > > correct either. If the user has the default sslsni="1" and supplies an > > IP address for the host parameter, I don't think we should fail the > > connection. > > Maybe not, but doing so is at least in line with how the OpenSSL support will > handle the same config AFAICT. Or am I missing something? With OpenSSL, I don't see a connection failure when using sslsni=1 with IP addresses. (verify-full can't work, but that's a separate problem.) > > > + if (host && host[0] && > > > + !(strspn(host, "0123456789.") == strlen(host) || > > > + strchr(host, ':'))) > > > + SSL_SetURL(conn->pr_fd, host); > > > > It looks like NSS may already have some code that prevents SNI from > > being sent for IP addresses, so that part of the guard might not be > > necessary. (And potentially counterproductive, because it looks like > > NSS can perform verification against the certificate's SANs if you pass > > an IP address to SSL_SetURL().) > > Skimming the NSS code I wasn't able find the countermeasures, can you provide > a > reference to where I should look? I see the check in ssl_ShouldSendSNIExtension(), in ssl3exthandle.c. > Feel free to post a new version of the NSS patch with these changes if you > want. Will do! Thanks, --Jacob
Re: Support for NSS as a libpq TLS backend
> On 21 Sep 2021, at 02:06, Jacob Champion wrote:
>
> On Mon, 2021-07-26 at 15:26 +0200, Daniel Gustafsson wrote:
>>> On 19 Jul 2021, at 21:33, Jacob Champion wrote:
>>> ..client connections will crash if
>>> hostaddr is provided rather than host, because SSL_SetURL can't handle
>>> a NULL argument. I'm running with 0002 to fix it for the moment, but
>>> I'm not sure yet if it does the right thing for IP addresses, which the
>>> OpenSSL side has a special case for.
>>
>> AFAICT the idea is to handle it in the cert auth callback, so I've added some
>> PoC code to check for sslsni there and updated the TODO comment to reflect
>> that.
>
> I dug a bit deeper into the SNI stuff:
>
>> +server_hostname = SSL_RevealURL(conn->pr_fd);
>> +if (!server_hostname || server_hostname[0] == '\0')
>> +{
>> +/* If SNI is enabled we must have a hostname set */
>> +if (conn->sslsni && conn->sslsni[0])
>> +status = SECFailure;
>
> conn->sslsni can be explicitly set to "0" to disable it, so this should
> probably be changed to a check for "1",
Agreed.
> but I'm not sure that would be
> correct either. If the user has the default sslsni="1" and supplies an
> IP address for the host parameter, I don't think we should fail the
> connection.
Maybe not, but doing so is at least in line with how the OpenSSL support will
handle the same config AFAICT. Or am I missing something?
>> +if (host && host[0] &&
>> +!(strspn(host, "0123456789.") == strlen(host) ||
>> + strchr(host, ':')))
>> +SSL_SetURL(conn->pr_fd, host);
>
> It looks like NSS may already have some code that prevents SNI from
> being sent for IP addresses, so that part of the guard might not be
> necessary. (And potentially counterproductive, because it looks like
> NSS can perform verification against the certificate's SANs if you pass
> an IP address to SSL_SetURL().)
Skimming the NSS code I wasn't able find the countermeasures, can you provide a
reference to where I should look?
Feel free to post a new version of the NSS patch with these changes if you want.
> Speaking of IP addresses in SANs, it doesn't look like our OpenSSL
> backend can handle those. That's a separate conversation, but I might
> take a look at a patch for next commitfest.
Please do.
--
Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
On Mon, 2021-07-26 at 15:26 +0200, Daniel Gustafsson wrote:
> > On 19 Jul 2021, at 21:33, Jacob Champion wrote:
> > ..client connections will crash if
> > hostaddr is provided rather than host, because SSL_SetURL can't handle
> > a NULL argument. I'm running with 0002 to fix it for the moment, but
> > I'm not sure yet if it does the right thing for IP addresses, which the
> > OpenSSL side has a special case for.
>
> AFAICT the idea is to handle it in the cert auth callback, so I've added some
> PoC code to check for sslsni there and updated the TODO comment to reflect
> that.
I dug a bit deeper into the SNI stuff:
> + server_hostname = SSL_RevealURL(conn->pr_fd);
> + if (!server_hostname || server_hostname[0] == '\0')
> + {
> + /* If SNI is enabled we must have a hostname set */
> + if (conn->sslsni && conn->sslsni[0])
> + status = SECFailure;
conn->sslsni can be explicitly set to "0" to disable it, so this should
probably be changed to a check for "1", but I'm not sure that would be
correct either. If the user has the default sslsni="1" and supplies an
IP address for the host parameter, I don't think we should fail the
connection.
> + if (host && host[0] &&
> + !(strspn(host, "0123456789.") == strlen(host) ||
> + strchr(host, ':')))
> + SSL_SetURL(conn->pr_fd, host);
It looks like NSS may already have some code that prevents SNI from
being sent for IP addresses, so that part of the guard might not be
necessary. (And potentially counterproductive, because it looks like
NSS can perform verification against the certificate's SANs if you pass
an IP address to SSL_SetURL().)
Speaking of IP addresses in SANs, it doesn't look like our OpenSSL
backend can handle those. That's a separate conversation, but I might
take a look at a patch for next commitfest.
--Jacob
Re: Support for NSS as a libpq TLS backend
> On 18 Aug 2021, at 02:32, Michael Paquier wrote: > > On Wed, Aug 18, 2021 at 12:06:59AM +, Jacob Champion wrote: >> I have a local test suite that I've been writing against libpq. With >> the new ssldatabase connection option, one tricky aspect is figuring >> out whether it's supported or not. It doesn't look like there's any way >> to tell, from a client application, whether NSS or OpenSSL (or neither) >> is in use. > > That's about guessing which library libpq is compiled with, so yes > that's a problem. > >> so that you don't have to have an actual connection first in order to >> figure out what connection options you need to supply. Clients that >> support multiple libpq versions would need to know whether that call is >> reliable (older versions of libpq will always return NULL, whether SSL >> is compiled in or not), so maybe we could add a feature macro at the >> same time? > > Still, the problem is wider than that, no? One cannot know either if > a version of libpq is able to work with GSSAPI until they attempt a > connection with gssencmode. It seems to me that we should work on the > larger picture here. I think we should do both. PQsslAttribute() already exists, and being able to get the library attribute for NULL conn object when there are multiple libraries makes a lot of sense to me. That doesn’t exclude working on a better way for apps to interrogate the libpq they have at hand for which capabilities it has. Personally I’m not sure what that API could look like, but we should discuss that in a separate thread I guess. -- Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
On Wed, Aug 18, 2021 at 12:06:59AM +, Jacob Champion wrote: > I have a local test suite that I've been writing against libpq. With > the new ssldatabase connection option, one tricky aspect is figuring > out whether it's supported or not. It doesn't look like there's any way > to tell, from a client application, whether NSS or OpenSSL (or neither) > is in use. That's about guessing which library libpq is compiled with, so yes that's a problem. > so that you don't have to have an actual connection first in order to > figure out what connection options you need to supply. Clients that > support multiple libpq versions would need to know whether that call is > reliable (older versions of libpq will always return NULL, whether SSL > is compiled in or not), so maybe we could add a feature macro at the > same time? Still, the problem is wider than that, no? One cannot know either if a version of libpq is able to work with GSSAPI until they attempt a connection with gssencmode. It seems to me that we should work on the larger picture here. > We could also add a new API (say, PQsslLibrary()) but I don't know if > that gives us anything in practice. Thoughts? Knowing that the GSSAPI stuff is part of fe-secure.c, we may want instead a call that returns a list of supported secure libraries. -- Michael signature.asc Description: PGP signature
Re: Support for NSS as a libpq TLS backend
On Tue, 2021-08-10 at 19:22 +0200, Daniel Gustafsson wrote: > Another rebase to work around the recent changes in the ssl Makefile. I have a local test suite that I've been writing against libpq. With the new ssldatabase connection option, one tricky aspect is figuring out whether it's supported or not. It doesn't look like there's any way to tell, from a client application, whether NSS or OpenSSL (or neither) is in use. You'd mentioned that perhaps we should support a call like PQsslAttribute(NULL, "library"); /* returns "NSS", "OpenSSL", or NULL */ so that you don't have to have an actual connection first in order to figure out what connection options you need to supply. Clients that support multiple libpq versions would need to know whether that call is reliable (older versions of libpq will always return NULL, whether SSL is compiled in or not), so maybe we could add a feature macro at the same time? We could also add a new API (say, PQsslLibrary()) but I don't know if that gives us anything in practice. Thoughts? --Jacob
Re: Support for NSS as a libpq TLS backend
On Wed, 2021-06-23 at 15:48 +0200, Daniel Gustafsson wrote:
> Attached is a rebased version which incorporates your recent patchset for
> resource handling, as well as the connect_ok test patch.
With v38 I do see the "one-time function was previously called and
failed" message you mentioned before, as well as some PR_Assert()
crashes. Looks like it's just due to the placement of
SSL_ClearSessionCache(); gating it behind the conn->nss_context check
ensures that we don't call it if no NSS context actually exists. Patch
attached (0001).
--
Continuing my jog around the patch... client connections will crash if
hostaddr is provided rather than host, because SSL_SetURL can't handle
a NULL argument. I'm running with 0002 to fix it for the moment, but
I'm not sure yet if it does the right thing for IP addresses, which the
OpenSSL side has a special case for.
Early EOFs coming from the server don't currently have their own error
message, which leads to a confusingly empty
connection to server at "127.0.0.1", port 47447 failed:
0003 adds one, to roughly match the corresponding OpenSSL message.
While I was fixing that I noticed that I was getting a "unable to
verify certificate" error message for the early EOF case, even with
sslmode=require. That error message is being printed to conn-
>errorMessage during pg_cert_auth_handler(), even if we're not
verifying certificates, and then that message is included in later
unrelated failures. 0004 patches that.
--Jacob
From a675f4b6808c695d3691ad8a1a5e004ed71312c3 Mon Sep 17 00:00:00 2001
From: Jacob Champion
Date: Tue, 15 Jun 2021 15:59:39 -0700
Subject: [PATCH 1/4] nss: move SSL_ClearSessionCache
Don't clear the cache if no context exists.
---
src/interfaces/libpq/fe-secure-nss.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/interfaces/libpq/fe-secure-nss.c b/src/interfaces/libpq/fe-secure-nss.c
index 613e7d6a1d..87e18fdde2 100644
--- a/src/interfaces/libpq/fe-secure-nss.c
+++ b/src/interfaces/libpq/fe-secure-nss.c
@@ -141,8 +141,6 @@ pgtls_close(PGconn *conn)
* All NSS references must be cleaned up before we close out the
* context.
*/
- SSL_ClearSessionCache();
-
if (conn->pr_fd)
{
PRStatus status;
@@ -161,6 +159,10 @@ pgtls_close(PGconn *conn)
if (conn->nss_context)
{
SECStatus status;
+
+ /* The session cache must be cleared, or we'll leak references. */
+ SSL_ClearSessionCache();
+
status = NSS_ShutdownContext(conn->nss_context);
if (status != SECSuccess)
--
2.25.1
From 2a23b00084538d9d9849a1ca46d054c1c84c53c3 Mon Sep 17 00:00:00 2001
From: Jacob Champion
Date: Mon, 19 Jul 2021 10:29:45 -0700
Subject: [PATCH 2/4] nss: handle NULL host
...in the case that we're using a hostaddr instead.
---
src/interfaces/libpq/fe-secure-nss.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/interfaces/libpq/fe-secure-nss.c b/src/interfaces/libpq/fe-secure-nss.c
index 87e18fdde2..cc66f5bb8c 100644
--- a/src/interfaces/libpq/fe-secure-nss.c
+++ b/src/interfaces/libpq/fe-secure-nss.c
@@ -183,6 +183,7 @@ pgtls_open_client(PGconn *conn)
PRFileDesc *model;
NSSInitParameters params;
SSLVersionRange desired_range;
+ const char *host;
#ifdef ENABLE_THREAD_SAFETY
#ifdef WIN32
@@ -449,9 +450,11 @@ pgtls_open_client(PGconn *conn)
/*
* Specify which hostname we are expecting to talk to for the ClientHello
- * SNI extension.
+ * SNI extension. TODO: what if the host is an IP address?
*/
- SSL_SetURL(conn->pr_fd, (conn->connhost[conn->whichhost]).host);
+ host = conn->connhost[conn->whichhost].host;
+ if (host && host[0])
+ SSL_SetURL(conn->pr_fd, host);
status = SSL_ForceHandshake(conn->pr_fd);
--
2.25.1
From 9caa50893fc3b8d3447ce14d4b93876aa7ffe91a Mon Sep 17 00:00:00 2001
From: Jacob Champion
Date: Mon, 19 Jul 2021 12:14:00 -0700
Subject: [PATCH 3/4] nss: fix spurious "unable to verify certificate" message
That message gets written to conn->errorMessage even if sslmode isn't
verifying the certificate contents, which can lead to a confusing UX if
the connection later fails for some other reason.
---
src/interfaces/libpq/fe-secure-nss.c | 25 -
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/src/interfaces/libpq/fe-secure-nss.c b/src/interfaces/libpq/fe-secure-nss.c
index cc66f5bb8c..4490fb03e4 100644
--- a/src/interfaces/libpq/fe-secure-nss.c
+++ b/src/interfaces/libpq/fe-secure-nss.c
@@ -858,12 +858,6 @@ pg_cert_auth_handler(void *arg, PRFileDesc *fd, PRBool checksig, PRBool isServer
if (!pq_verify_peer_name_matches_certificate(conn))
status = SECFailure;
}
- else
- {
- printfPQExpBuffer(>errorMessage,
- libpq_gettext("unable to verify certificate: %s"),
- pg_SSLerrmessage(PR_GetError()));
- }
CERT_DestroyCertificate(server_cert);
return status;
@@ -923,6 +917,8 @@ pg_bad_cert_handler(void *arg, PRFileDesc *fd)
if (!arg)
return SECFailure;
+ err = PORT_GetError();
+
/*
Re: Support for NSS as a libpq TLS backend
> On 16 Jun 2021, at 18:15, Jacob Champion wrote: > > On Wed, 2021-06-16 at 15:31 +0200, Daniel Gustafsson wrote: >>> On 16 Jun 2021, at 01:50, Jacob Champion wrote: >>> I've been tracking down reference leaks in the client. These open >>> references prevent NSS from shutting down cleanly, which then makes it >>> impossible to open a new context in the future. This probably affects >>> other libpq clients more than it affects psql. >> >> Ah, nice catch, that's indeed a bug in the frontend implementation. The >> problem is that the NSS trustdomain cache *must* be empty before shutting >> down >> the context, else this very issue happens. Note this in be_tls_destroy(): >> >>/* >> * It reads a bit odd to clear a session cache when we are destroying the >> * context altogether, but if the session cache isn't cleared before >> * shutting down the context it will fail with SEC_ERROR_BUSY. >> */ >>SSL_ClearSessionCache(); >> >> Calling SSL_ClearSessionCache() in pgtls_close() fixes the error. > > That's unfortunate. The session cache is global, right? So I'm guessing > we'll need to refcount and lock that call, to avoid cleaning up out > from under a thread that's actively using the the cache? I'm not sure, the documentation doesn't give any answers and implementations of libnss tend to just clear the cache without consideration. In libcurl we do just that, and haven't had any complaints - which doesn't mean it's correct but it's a datapoint. >> There is another resource leak left (visible in one test after the above is >> added), the SECMOD module needs to be unloaded in case it's been loaded. >> Implementing that with SECMOD_UnloadUserModule trips a segfault in NSS which >> I >> have yet to figure out (when acquiring a lock with NSSRWLock_LockRead). >> >> [...] >> >> With your patches I'm seeing a couple of these: >> >> SSL error: The one-time function was previously called and failed. Its >> error code is no longer available > > Hmm. Adding SSL_ClearSessionCache() (without thread-safety at the > moment) fixes all of the SSL tests for me, and I don't see either the > SECMOD leak or the "one-time function" error that you've mentioned. Reading the code I don't think a loaded user module is considered a resource that must've been released prior to closing the context. I will dig for what showed up in my tests, but I don't think it was caused by this. > What version of NSS are you running? I'm on 3.63. Right now I'm using what Debian 10 is packaging which is 3.42. Admittedly not hot off the press but I've been trying to develop off a packaged version which we might see users wanting to deploy against should this get shipped. -- Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
On Wed, 2021-06-16 at 15:31 +0200, Daniel Gustafsson wrote:
> > On 16 Jun 2021, at 01:50, Jacob Champion wrote:
> > I've been tracking down reference leaks in the client. These open
> > references prevent NSS from shutting down cleanly, which then makes it
> > impossible to open a new context in the future. This probably affects
> > other libpq clients more than it affects psql.
>
> Ah, nice catch, that's indeed a bug in the frontend implementation. The
> problem is that the NSS trustdomain cache *must* be empty before shutting down
> the context, else this very issue happens. Note this in be_tls_destroy():
>
> /*
> * It reads a bit odd to clear a session cache when we are destroying the
> * context altogether, but if the session cache isn't cleared before
> * shutting down the context it will fail with SEC_ERROR_BUSY.
> */
> SSL_ClearSessionCache();
>
> Calling SSL_ClearSessionCache() in pgtls_close() fixes the error.
That's unfortunate. The session cache is global, right? So I'm guessing
we'll need to refcount and lock that call, to avoid cleaning up out
from under a thread that's actively using the the cache?
> There is another resource leak left (visible in one test after the above is
> added), the SECMOD module needs to be unloaded in case it's been loaded.
> Implementing that with SECMOD_UnloadUserModule trips a segfault in NSS which I
> have yet to figure out (when acquiring a lock with NSSRWLock_LockRead).
>
> [...]
>
> With your patches I'm seeing a couple of these:
>
> SSL error: The one-time function was previously called and failed. Its
> error code is no longer available
Hmm. Adding SSL_ClearSessionCache() (without thread-safety at the
moment) fixes all of the SSL tests for me, and I don't see either the
SECMOD leak or the "one-time function" error that you've mentioned.
What version of NSS are you running? I'm on 3.63.
I've attached my current patchset (based on v37) for comparison.
> > I am currently stuck on one last failing test. This leak seems to only
> > show up when using TLSv1.2 or below.
>
> AFAICT the session cache is avoided for TLSv1.3 due to 1.3 not supporting
> renegotiation.
Nice, at least that mystery is solved. :D
Thanks,
--Jacob
From 6976d15a4be50276ea2f77fd5c37d238493f9447 Mon Sep 17 00:00:00 2001
From: Jacob Champion
Date: Tue, 15 Jun 2021 10:01:14 -0700
Subject: [PATCH 1/3] nss: don't ignore failures during context shutdown
The biggest culprit of a shutdown failure so far seems to be object
leaks. A failure here may prevent future contexts from being created
(and they'll fail in confusing ways), so don't ignore it.
There's not a great way to signal errors from this layer of the stack,
but a notice will at least give us a chance of seeing the problem.
---
src/interfaces/libpq/fe-secure-nss.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/interfaces/libpq/fe-secure-nss.c
b/src/interfaces/libpq/fe-secure-nss.c
index 1b9bd4f1a5..90f57e0d99 100644
--- a/src/interfaces/libpq/fe-secure-nss.c
+++ b/src/interfaces/libpq/fe-secure-nss.c
@@ -139,7 +139,16 @@ pgtls_close(PGconn *conn)
{
if (conn->nss_context)
{
- NSS_ShutdownContext(conn->nss_context);
+ SECStatus status;
+ status = NSS_ShutdownContext(conn->nss_context);
+
+ if (status != SECSuccess)
+ {
+ pqInternalNotice(>noticeHooks,
+"unable to shut down
NSS context: %s",
+
pg_SSLerrmessage(PR_GetError()));
+ }
+
conn->nss_context = NULL;
}
}
--
2.25.1
From acabc4d51d38124db748a6be9dca0ff83693e039 Mon Sep 17 00:00:00 2001
From: Jacob Champion
Date: Tue, 15 Jun 2021 14:37:28 -0700
Subject: [PATCH 2/3] test: check for empty stderr during connect_ok()
...in a similar manner to command_like(), to catch notices-as-errors
coming from NSS.
---
src/test/authentication/t/001_password.pl | 2 +-
src/test/authentication/t/002_saslprep.pl | 2 +-
src/test/kerberos/t/001_auth.pl | 2 +-
src/test/ldap/t/001_auth.pl | 2 +-
src/test/perl/PostgresNode.pm | 5 -
src/test/ssl/t/001_ssltests.pl| 4 ++--
src/test/ssl/t/002_scram.pl | 4 ++--
7 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/src/test/authentication/t/001_password.pl
b/src/test/authentication/t/001_password.pl
index 427a360198..327dfb4ed9 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -20,7 +20,7 @@ if (!$use_unix_sockets)
}
else
{
- plan tests => 23;
+ plan tests => 32;
}
diff --git a/src/test/authentication/t/002_saslprep.pl
b/src/test/authentication/t/002_saslprep.pl
index f080a0ccba..d6508df8bd 100644
--- a/src/test/authentication/t/002_saslprep.pl
+++
Re: Support for NSS as a libpq TLS backend
> On 16 Jun 2021, at 01:50, Jacob Champion wrote: > I've been tracking down reference leaks in the client. These open > references prevent NSS from shutting down cleanly, which then makes it > impossible to open a new context in the future. This probably affects > other libpq clients more than it affects psql. Ah, nice catch, that's indeed a bug in the frontend implementation. The problem is that the NSS trustdomain cache *must* be empty before shutting down the context, else this very issue happens. Note this in be_tls_destroy(): /* * It reads a bit odd to clear a session cache when we are destroying the * context altogether, but if the session cache isn't cleared before * shutting down the context it will fail with SEC_ERROR_BUSY. */ SSL_ClearSessionCache(); Calling SSL_ClearSessionCache() in pgtls_close() fixes the error. There is another resource leak left (visible in one test after the above is added), the SECMOD module needs to be unloaded in case it's been loaded. Implementing that with SECMOD_UnloadUserModule trips a segfault in NSS which I have yet to figure out (when acquiring a lock with NSSRWLock_LockRead). > The first step to fixing that is not ignoring failures during NSS > shutdown, so I've tried a patch to pgtls_close() that pushes any > failures through the pqInternalNotice(). That seems to be working well. I'm keeping these in during hacking, with a comment that they need to be revisited during review since they are mainly useful for debugging. > The tests were still mostly green, so I taught connect_ok() to fail if > any stderr showed up, and that exposed quite a few failures. With your patches I'm seeing a couple of these: SSL error: The one-time function was previously called and failed. Its error code is no longer available This is an error from NSPR, but it's not clear to me which PR_CallOnce call it's coming from. It seems to be hitting in the SAN and CRL tests, so it smells of some form of caching implemented with NSPR API's to me but thats a mere hunch. > I am currently stuck on one last failing test. This leak seems to only > show up when using TLSv1.2 or below. AFAICT the session cache is avoided for TLSv1.3 due to 1.3 not supporting renegotiation. -- Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
On Wed, 2021-06-16 at 00:08 +0200, Daniel Gustafsson wrote:
> > On 15 Jun 2021, at 00:15, Jacob Champion wrote:
> > Attached is a quick patch; does it work on your machine?
>
> It does, thanks! I've included it in the attached v37 along with a few tiny
> non-functional improvements in comment spelling etc.
Great, thanks!
I've been tracking down reference leaks in the client. These open
references prevent NSS from shutting down cleanly, which then makes it
impossible to open a new context in the future. This probably affects
other libpq clients more than it affects psql.
The first step to fixing that is not ignoring failures during NSS
shutdown, so I've tried a patch to pgtls_close() that pushes any
failures through the pqInternalNotice(). That seems to be working well.
The tests were still mostly green, so I taught connect_ok() to fail if
any stderr showed up, and that exposed quite a few failures.
I am currently stuck on one last failing test. This leak seems to only
show up when using TLSv1.2 or below. There doesn't seem to be a
substantial difference in libpq code coverage between 1.2 and 1.3, so
I'm worried that either 1) there's some API we use that "requires"
cleanup, but only on 1.2 and below, or 2) there's some bug in my
version of NSS.
Attached are a few work-in-progress patches. I think the reference
cleanups themselves are probably solid, but the rest of it could use
some feedback. Are there better ways to test for this? and can anyone
reproduce the TLSv1.2 leak?
--Jacob
From 6976d15a4be50276ea2f77fd5c37d238493f9447 Mon Sep 17 00:00:00 2001
From: Jacob Champion
Date: Tue, 15 Jun 2021 10:01:14 -0700
Subject: [PATCH 1/3] nss: don't ignore failures during context shutdown
The biggest culprit of a shutdown failure so far seems to be object
leaks. A failure here may prevent future contexts from being created
(and they'll fail in confusing ways), so don't ignore it.
There's not a great way to signal errors from this layer of the stack,
but a notice will at least give us a chance of seeing the problem.
---
src/interfaces/libpq/fe-secure-nss.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/interfaces/libpq/fe-secure-nss.c
b/src/interfaces/libpq/fe-secure-nss.c
index 1b9bd4f1a5..90f57e0d99 100644
--- a/src/interfaces/libpq/fe-secure-nss.c
+++ b/src/interfaces/libpq/fe-secure-nss.c
@@ -139,7 +139,16 @@ pgtls_close(PGconn *conn)
{
if (conn->nss_context)
{
- NSS_ShutdownContext(conn->nss_context);
+ SECStatus status;
+ status = NSS_ShutdownContext(conn->nss_context);
+
+ if (status != SECSuccess)
+ {
+ pqInternalNotice(>noticeHooks,
+"unable to shut down
NSS context: %s",
+
pg_SSLerrmessage(PR_GetError()));
+ }
+
conn->nss_context = NULL;
}
}
--
2.25.1
From acabc4d51d38124db748a6be9dca0ff83693e039 Mon Sep 17 00:00:00 2001
From: Jacob Champion
Date: Tue, 15 Jun 2021 14:37:28 -0700
Subject: [PATCH 2/3] test: check for empty stderr during connect_ok()
...in a similar manner to command_like(), to catch notices-as-errors
coming from NSS.
---
src/test/authentication/t/001_password.pl | 2 +-
src/test/authentication/t/002_saslprep.pl | 2 +-
src/test/kerberos/t/001_auth.pl | 2 +-
src/test/ldap/t/001_auth.pl | 2 +-
src/test/perl/PostgresNode.pm | 5 -
src/test/ssl/t/001_ssltests.pl| 4 ++--
src/test/ssl/t/002_scram.pl | 4 ++--
7 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/src/test/authentication/t/001_password.pl
b/src/test/authentication/t/001_password.pl
index 427a360198..327dfb4ed9 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -20,7 +20,7 @@ if (!$use_unix_sockets)
}
else
{
- plan tests => 23;
+ plan tests => 32;
}
diff --git a/src/test/authentication/t/002_saslprep.pl
b/src/test/authentication/t/002_saslprep.pl
index f080a0ccba..d6508df8bd 100644
--- a/src/test/authentication/t/002_saslprep.pl
+++ b/src/test/authentication/t/002_saslprep.pl
@@ -17,7 +17,7 @@ if (!$use_unix_sockets)
}
else
{
- plan tests => 12;
+ plan tests => 20;
}
# Delete pg_hba.conf from the given node, add a new entry to it
diff --git a/src/test/kerberos/t/001_auth.pl b/src/test/kerberos/t/001_auth.pl
index b5594924ca..62015339a0 100644
--- a/src/test/kerberos/t/001_auth.pl
+++ b/src/test/kerberos/t/001_auth.pl
@@ -23,7 +23,7 @@ use Time::HiRes qw(usleep);
if ($ENV{with_gssapi} eq 'yes')
{
- plan tests => 44;
+ plan tests => 54;
}
else
{
diff --git a/src/test/ldap/t/001_auth.pl b/src/test/ldap/t/001_auth.pl
index 0ae14e4c85..e857c26258 100644
--- a/src/test/ldap/t/001_auth.pl
+++ b/src/test/ldap/t/001_auth.pl
@@ -9,7
Re: Support for NSS as a libpq TLS backend
On Fri, 2021-05-28 at 11:04 +0200, Daniel Gustafsson wrote:
> Attached is a rebase to keep bitrot at bay.
I get a failure during one of the CRL directory tests due to a missing
database -- it looks like the Makefile is missing an entry. (I'm
dusting off my build after a few months away, so I don't know if this
latest rebase introduced it or not.)
Attached is a quick patch; does it work on your machine?
--Jacob
From 7a7b8904ef22212190bb988fab1ef696fe1a59de Mon Sep 17 00:00:00 2001
From: Jacob Champion
Date: Mon, 14 Jun 2021 15:04:26 -0700
Subject: [PATCH] test/ssl: fix NSS server-side CRL test
Make sure the database is created during `make nssfiles`, and expect a
revocation failure message.
---
src/test/ssl/Makefile | 2 ++
src/test/ssl/t/001_ssltests.pl | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/test/ssl/Makefile b/src/test/ssl/Makefile
index 14ca1f8bf3..557cbe223f 100644
--- a/src/test/ssl/Makefile
+++ b/src/test/ssl/Makefile
@@ -45,6 +45,7 @@ NSSFILES := ssl/nss/client_ca.crt.db \
ssl/nss/client-revoked.crt__client-revoked.key.db \
ssl/nss/server-cn-only.crt__server-password.key.db \
ssl/nss/server-cn-only.crt__server-cn-only.key.db \
+ ssl/nss/server-cn-only.crt__server-cn-only.key.crldir.db \
ssl/nss/root.crl \
ssl/nss/server.crl \
ssl/nss/client.crl \
@@ -167,6 +168,7 @@ ssl/nss/server-cn-only.crt__server-cn-only.key.db:
ssl/server-cn-only.crt ssl/se
pk12util -i ssl/nss/server-cn-only.pfx -d "sql:$@" -W ''
ssl/nss/server-cn-only.crt__server-cn-only.key.crldir.db:
ssl/nss/server-cn-only.crt__server-cn-only.key.db
+ cp -R $< $@
for c in $(shell ls ssl/root+client-crldir) ; do \
echo $${c} ; \
openssl crl -in ssl/root+client-crldir/$${c} -outform der -out
ssl/nss/$${c} ; \
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 4105a67b94..aec99e7bf6 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -664,7 +664,7 @@ $node->connect_fails(
"$common_connstr user=ssltestuser sslcert=ssl/client-revoked.crt
sslkey=ssl/client-revoked_tmp.key
ssldatabase=ssl/nss/client-revoked.crt__client-revoked.key.db",
"certificate authorization fails with revoked client cert with
server-side CRL directory",
expected_stderr =>
- qr/SSL error: sslv3 alert certificate revoked|SSL error: Encountered
end of file/);
+ qr/SSL error: sslv3 alert certificate revoked|SSL peer rejected your
certificate as revoked/);
# clean up
--
2.25.1
Re: Support for NSS as a libpq TLS backend
On Thu, Jun 3, 2021 at 11:14 PM Daniel Gustafsson wrote: > > > On 3 Jun 2021, at 23:11, Tom Lane wrote: > > > > Bruce Momjian writes: > >> I wonder if we should use SSL/TLS in more places in our documentation. > > > > No objection to doing that in the docs; I'm just questioning > > switching the code-visible names. +1. I also don't think it's worth changing the actual names, I think that'll cause more problems than it solves. But we can, and probably should, change the messaging around it, particularly the docs (but probably also comments in the config file). > As long as it's still searchable by "SSL", "TLS" and "SSL/TLS" and not just > the > latter. Agreed, making it searchable and easily cross-linkable.. And maybe both terms should be in the glossary. -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
Re: Support for NSS as a libpq TLS backend
> On 3 Jun 2021, at 23:11, Tom Lane wrote: > > Bruce Momjian writes: >> I wonder if we should use SSL/TLS in more places in our documentation. > > No objection to doing that in the docs; I'm just questioning > switching the code-visible names. As long as it's still searchable by "SSL", "TLS" and "SSL/TLS" and not just the latter. -- Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
Bruce Momjian writes: > I wonder if we should use SSL/TLS in more places in our documentation. No objection to doing that in the docs; I'm just questioning switching the code-visible names. regards, tom lane
Re: Support for NSS as a libpq TLS backend
On Thu, Jun 3, 2021 at 04:55:45PM -0400, Tom Lane wrote: > Daniel Gustafsson writes: > > It might also put us a hard spot if the next TLS spec ends up being called > > something other than TLS? It's clearly happened before =) > > Good point. I'm inclined to just stick with the SSL terminology. I wonder if we should use SSL/TLS in more places in our documentation. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
Re: Support for NSS as a libpq TLS backend
> On 3 Jun 2021, at 22:55, Tom Lane wrote: >>> Also, do we have precedent for GUC aliases? That might be a little >>> weird. > >> I don't think we do currently, but I have a feeling the topic has surfaced >> here >> before. > > We do, look for "sort_mem" in guc.c. I knew it seemed familiar but I failed to find it, thanks for the pointer. -- Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
Daniel Gustafsson writes: > It might also put us a hard spot if the next TLS spec ends up being called > something other than TLS? It's clearly happened before =) Good point. I'm inclined to just stick with the SSL terminology. >> Also, do we have precedent for GUC aliases? That might be a little >> weird. > I don't think we do currently, but I have a feeling the topic has surfaced > here > before. We do, look for "sort_mem" in guc.c. So it's not like it'd be inconvenient to implement. But I think user confusion and the potential for the new terminology to fail to be any more future-proof are good reasons to just leave the names alone. regards, tom lane
Re: Support for NSS as a libpq TLS backend
> On 3 Jun 2021, at 22:14, Jeff Davis wrote: > > On Thu, 2021-06-03 at 15:53 -0400, Andrew Dunstan wrote: >> Yeah, but it's annoying to have to start every talk I give touching >> this >> subject with the slide that says "When we say SSL we really means >> TLS". >> Maybe release 15 would be a good time to rename user-visible option >> names etc, with support for legacy names. Perhaps. Having spent some time in this space, SSL has IMHO become the de facto term for an encrypted connection at the socket layer, with TLS being the current protocol suite (additionally, often referred to SSL/TLS). Offering tls* counterparts to our ssl GUCs etc will offer a level of correctness but I doubt we'll ever get rid of ssl* so we might not help too many users by the added complexity. It might also put us a hard spot if the next TLS spec ends up being called something other than TLS? It's clearly happened before =) > Sounds good to me, though I haven't looked into how big of a diff that > will be. > > Also, do we have precedent for GUC aliases? That might be a little > weird. I don't think we do currently, but I have a feeling the topic has surfaced here before. If we end up settling on this being something we want I can volunteer to do the legwork, but it seems a discussion best had before a patch is drafted. -- Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
On Thu, 2021-06-03 at 15:53 -0400, Andrew Dunstan wrote: > Yeah, but it's annoying to have to start every talk I give touching > this > subject with the slide that says "When we say SSL we really means > TLS". > Maybe release 15 would be a good time to rename user-visible option > names etc, with support for legacy names. Sounds good to me, though I haven't looked into how big of a diff that will be. Also, do we have precedent for GUC aliases? That might be a little weird. Regards, Jeff Davis
Re: Support for NSS as a libpq TLS backend
On 6/3/21 1:47 PM, Daniel Gustafsson wrote:
>> On 3 Jun 2021, at 19:37, Jeff Davis wrote:
>>
>> On Tue, 2020-10-27 at 23:39 -0700, Andres Freund wrote:
>>> Maybe we should just have --with-ssl={openssl,nss}? That'd avoid
>>> needing
>>> to check for errors.
>> [ apologies for the late reply ]
>>
>> Would it be more proper to call it --with-tls={openssl,nss} ?
> Well, we use SSL for everything else (GUCs, connection params and env vars
> etc)
> so I think --with-ssl is sensible.
>
> However, SSL and TLS are used quite interchangeably these days so I think it
> makes sense to provide --with-tls as an alias.
>
Yeah, but it's annoying to have to start every talk I give touching this
subject with the slide that says "When we say SSL we really means TLS".
Maybe release 15 would be a good time to rename user-visible option
names etc, with support for legacy names.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: Support for NSS as a libpq TLS backend
> On 3 Jun 2021, at 19:37, Jeff Davis wrote:
>
> On Tue, 2020-10-27 at 23:39 -0700, Andres Freund wrote:
>> Maybe we should just have --with-ssl={openssl,nss}? That'd avoid
>> needing
>> to check for errors.
>
> [ apologies for the late reply ]
>
> Would it be more proper to call it --with-tls={openssl,nss} ?
Well, we use SSL for everything else (GUCs, connection params and env vars etc)
so I think --with-ssl is sensible.
However, SSL and TLS are used quite interchangeably these days so I think it
makes sense to provide --with-tls as an alias.
--
Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
On Tue, 2020-10-27 at 23:39 -0700, Andres Freund wrote:
> Maybe we should just have --with-ssl={openssl,nss}? That'd avoid
> needing
> to check for errors.
[ apologies for the late reply ]
Would it be more proper to call it --with-tls={openssl,nss} ?
Regards,
Jeff Davis
Re: Support for NSS as a libpq TLS backend
> On 25 Mar 2021, at 00:56, Jacob Champion wrote: > Databases that are opened *after* the first one are given their own separate > slots. Any certificates that are part of those databases seemingly can't be > referenced directly by nickname. They have to be prefixed by their token name > -- a name which you don't have if you used NSS_InitContext() to create the > database. You have to use SECMOD_OpenUserDB() instead. This explains some > strange failures I was seeing in local testing, where the order of > InitContext determined whether our client certificate selection succeeded or > failed. Sorry for the latency is responding, but I'm now back from parental leave. AFAICT the tokenname for the database can be set with the dbTokenDescription member in the NSSInitParameters struct passed to NSS_InitContext() (documented in nss.h). Using this we can avoid the messier SECMOD machinery and use the token in the auth callback to refer to the database we loaded. I hacked this up in my local tree (rebased patchset coming soon) and it seems to work as intended. -- Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
On Mon, Apr 05, 2021 at 11:12:22AM +0900, Michael Paquier wrote: > Please find an updated set, v35, attached, and my apologies for > breaking again your patch set. While testing this patch set and > adjusting the SSL tests with HEAD, I have noticed what looks like a > bug with the DN mapping that NSS does not run. The connection strings > are the same in v35 and in v34, with dbname only changing in-between. > > Just to be sure, because I could have done something wrong with the > rebase of v35, I have done the same test with v34 applied on top of > dfc843d and things are failing. So it seems to me that there is an > issue with the DN mapping part. For now, I have marked this patch set as returned with feedback as it is still premature for integration, and there are still bugs in it. FWIW, I think that there is a future for providing an alternative to OpenSSL, so, even if it could not make it for this release, I'd like to push forward with this area more seriously as of 15. The recent libcrypto-related refactorings were one step in this direction, as well. -- Michael signature.asc Description: PGP signature
Re: Support for NSS as a libpq TLS backend
Greetings, * Michael Paquier (mich...@paquier.xyz) wrote: > On Wed, Mar 31, 2021 at 10:15:15PM +, Jacob Champion wrote: > > I think we're going to need some analogue to PQinitOpenSSL() to help > > client applications cut through the mess, but I'm not sure what it > > should look like, or how we would maintain any sort of API > > compatibility between the two flavors. And does libpq already have some > > notion of a "main thread" that I'm missing? > > Nope as far as I recall. With OpenSSL, the initialization of the SSL > mutex lock and the crypto callback initialization is done by the first > thread in. Yeah, we haven't got any such concept in libpq. I do think that some of this can simply be documented as "if you do this, then you need to make sure to do this". Thanks, Stephen signature.asc Description: PGP signature
Re: Support for NSS as a libpq TLS backend
On Wed, Mar 31, 2021 at 10:15:15PM +, Jacob Champion wrote: > I think we're going to need some analogue to PQinitOpenSSL() to help > client applications cut through the mess, but I'm not sure what it > should look like, or how we would maintain any sort of API > compatibility between the two flavors. And does libpq already have some > notion of a "main thread" that I'm missing? Nope as far as I recall. With OpenSSL, the initialization of the SSL mutex lock and the crypto callback initialization is done by the first thread in. -- Michael signature.asc Description: PGP signature
Re: Support for NSS as a libpq TLS backend
On Fri, 2021-03-26 at 18:05 -0400, Stephen Frost wrote: > * Jacob Champion (pchamp...@vmware.com) wrote: > > Yeah. I was hoping to avoid implementing our own locks and refcounts, > > but it seems like it's going to be required. > > Yeah, afraid so. I think it gets worse, after having debugged some confusing crashes. There's already been a discussion on PR_Init upthread a bit: > Once we settle on a version we can confirm if PR_Init is/isn't needed and > remove all traces of it if not. What the NSPR documentation omits is that implicit initialization is not threadsafe. So NSS_InitContext() is technically "threadsafe" because it's built on PR_CallOnce(), but if you haven't called PR_Init() yet, multiple simultaneous PR_CallOnce() calls can crash into each other. So, fine. We just add our own locks around NSS_InitContext() (or around a single call to PR_Init()). Well, the first thread to win and successfully initialize NSPR gets marked as the "primordial" thread using thread-local state. And it gets a pthread destructor that does... something. So lazy initialization seems a bit dangerous regardless of whether or not we add locks, but I can't really prove whether it's dangerous or not in practice. I do know that only the primordial thread is allowed to call PR_Cleanup(), and of course we wouldn't be able to control which thread does what for libpq clients. I don't know what other assumptions are made about the primordial thread, or if there are any platform-specific behaviors with older versions of NSPR that we'd need to worry about. It used to be that the primordial thread was not allowed to exit before any other threads, but that restriction was lifted at some point [1]. I think we're going to need some analogue to PQinitOpenSSL() to help client applications cut through the mess, but I'm not sure what it should look like, or how we would maintain any sort of API compatibility between the two flavors. And does libpq already have some notion of a "main thread" that I'm missing? --Jacob [1] https://bugzilla.mozilla.org/show_bug.cgi?id=294955
Re: Support for NSS as a libpq TLS backend
Greetings, * Jacob Champion (pchamp...@vmware.com) wrote: > On Fri, 2021-03-26 at 15:33 -0400, Stephen Frost wrote: > > * Jacob Champion (pchamp...@vmware.com) wrote: > > > Databases that are opened *after* the first one are given their own > > > separate slots. [...] > > > > This is more-or-less what we would want though, right..? If a user asks > > for a connection with ssl_database=blah and sslcert=whatever, we'd want > > to open database 'blah' and search (just) that database for cert > > 'whatever'. We could possibly offer other options in the future but > > certainly this would work and be the most straight-forward and expected > > behavior. > > Yes, but see below. > > > > If you SECMOD_OpenUserDB() a database that is identical to the first > > > (internal) database, NSS deduplicates for you and just returns the > > > internal slot. Which seems like it's helpful, except you're not > > > allowed to close that database, and you have to know not to close it > > > by checking to see whether that slot is the "internal key slot". It > > > appears to remain open until NSS is shut down entirely. > > > > Seems like we shouldn't do that and should just use SECMOD_OpenUserDB() > > for opening databases. > > We don't have control over whether or not this happens. If the > application embedding libpq has already loaded the database into the > internal slot via its own NSS initialization, then when we call > SECMOD_OpenUserDB() for that same database, the internal slot will be > returned and we have to handle it accordingly. > > It's not a huge amount of work, but it is magic knowledge that has to > be maintained, especially in the absence of specialized clientside > tests. Ah.. yeah, fair enough. We could document that we discourage applications from doing so, but I agree that we'll need to deal with it since it could happen. > > > But if you open a database that is *not* the magic internal database, > > > and then open a duplicate of that one, NSS creates yet another new slot > > > for the duplicate. So SECMOD_OpenUserDB() may or may not be a resource > > > hog, depending on the global state of the process at the time libpq > > > opens its first connection. We won't be able to control what the parent > > > application will do before loading us up. > > > > I would think we'd want to avoid re-opening the same database multiple > > times, to avoid the duplicate slots and such. If the application code > > does it themselves, well, there's not much we can do about that, but we > > could at least avoid doing so in *our* code. I wouldn't expect us to be > > opening hundreds of databases either and so keeping a simple list around > > of what we've opened and scanning it seems like it'd be workable. Of > > course, this could likely be improved in the future but I would think > > that'd be good for an initial implementation. > > > > [...] > > > > > It also doesn't look like any of the SECMOD_* machinery that we're > > > looking at is thread-safe, but I'd really like to be wrong... > > > > That's unfortuante but solvable by using our own locks, similar > > to what's done in fe-secure-openssl.c. > > Yeah. I was hoping to avoid implementing our own locks and refcounts, > but it seems like it's going to be required. Yeah, afraid so. Thanks! Stephen signature.asc Description: PGP signature
Re: Support for NSS as a libpq TLS backend
On Fri, 2021-03-26 at 15:33 -0400, Stephen Frost wrote: > * Jacob Champion (pchamp...@vmware.com) wrote: > > Databases that are opened *after* the first one are given their own > > separate slots. [...] > > This is more-or-less what we would want though, right..? If a user asks > for a connection with ssl_database=blah and sslcert=whatever, we'd want > to open database 'blah' and search (just) that database for cert > 'whatever'. We could possibly offer other options in the future but > certainly this would work and be the most straight-forward and expected > behavior. Yes, but see below. > > If you SECMOD_OpenUserDB() a database that is identical to the first > > (internal) database, NSS deduplicates for you and just returns the > > internal slot. Which seems like it's helpful, except you're not > > allowed to close that database, and you have to know not to close it > > by checking to see whether that slot is the "internal key slot". It > > appears to remain open until NSS is shut down entirely. > > Seems like we shouldn't do that and should just use SECMOD_OpenUserDB() > for opening databases. We don't have control over whether or not this happens. If the application embedding libpq has already loaded the database into the internal slot via its own NSS initialization, then when we call SECMOD_OpenUserDB() for that same database, the internal slot will be returned and we have to handle it accordingly. It's not a huge amount of work, but it is magic knowledge that has to be maintained, especially in the absence of specialized clientside tests. > > But if you open a database that is *not* the magic internal database, > > and then open a duplicate of that one, NSS creates yet another new slot > > for the duplicate. So SECMOD_OpenUserDB() may or may not be a resource > > hog, depending on the global state of the process at the time libpq > > opens its first connection. We won't be able to control what the parent > > application will do before loading us up. > > I would think we'd want to avoid re-opening the same database multiple > times, to avoid the duplicate slots and such. If the application code > does it themselves, well, there's not much we can do about that, but we > could at least avoid doing so in *our* code. I wouldn't expect us to be > opening hundreds of databases either and so keeping a simple list around > of what we've opened and scanning it seems like it'd be workable. Of > course, this could likely be improved in the future but I would think > that'd be good for an initial implementation. > > [...] > > > It also doesn't look like any of the SECMOD_* machinery that we're > > looking at is thread-safe, but I'd really like to be wrong... > > That's unfortuante but solvable by using our own locks, similar > to what's done in fe-secure-openssl.c. Yeah. I was hoping to avoid implementing our own locks and refcounts, but it seems like it's going to be required. --Jacob
Re: Support for NSS as a libpq TLS backend
Greetings, * Jacob Champion (pchamp...@vmware.com) wrote: > On Wed, 2021-03-24 at 14:10 -0400, Stephen Frost wrote: > > * Jacob Champion (pchamp...@vmware.com) wrote: > > > I could see this being a problem if two client certificate nicknames > > > collide across multiple in-use databases, maybe? > > > > Right, in such a case either cert might get returned and it's possible > > that the "wrong" one is returned and therefore the connection would end > > up failing, assuming that they aren't actually the same and just happen > > to be in both. > > > > Seems like we could use SECMOD_OpenUserDB() and then pass the result > > from that into PK11_ListCertsInSlot() and scan through the certs in just > > the specified database to find the one we're looking for if we really > > feel compelled to try and address this risk. I've reached out to the > > NSS folks to see if they have any thoughts about the best way to address > > this. > > Some additional findings (NSS 3.63), please correct me if I've made any > mistakes: > > The very first NSSInitContext created is special. If it contains a database, > that database will be considered part of the "internal" slot and its > certificates can be referenced directly by nickname. If it doesn't have a > database, the internal slot has no certificates, and it will continue to have > zero certificates until NSS is completely shut down and reinitialized with a > new "first" context. > > Databases that are opened *after* the first one are given their own separate > slots. Any certificates that are part of those databases seemingly can't be > referenced directly by nickname. They have to be prefixed by their token name > -- a name which you don't have if you used NSS_InitContext() to create the > database. You have to use SECMOD_OpenUserDB() instead. This explains some > strange failures I was seeing in local testing, where the order of > InitContext determined whether our client certificate selection succeeded or > failed. This is more-or-less what we would want though, right..? If a user asks for a connection with ssl_database=blah and sslcert=whatever, we'd want to open database 'blah' and search (just) that database for cert 'whatever'. We could possibly offer other options in the future but certainly this would work and be the most straight-forward and expected behavior. > If you SECMOD_OpenUserDB() a database that is identical to the first > (internal) database, NSS deduplicates for you and just returns the internal > slot. Which seems like it's helpful, except you're not allowed to close that > database, and you have to know not to close it by checking to see whether > that slot is the "internal key slot". It appears to remain open until NSS is > shut down entirely. Seems like we shouldn't do that and should just use SECMOD_OpenUserDB() for opening databases. > But if you open a database that is *not* the magic internal database, > and then open a duplicate of that one, NSS creates yet another new slot > for the duplicate. So SECMOD_OpenUserDB() may or may not be a resource > hog, depending on the global state of the process at the time libpq > opens its first connection. We won't be able to control what the parent > application will do before loading us up. I would think we'd want to avoid re-opening the same database multiple times, to avoid the duplicate slots and such. If the application code does it themselves, well, there's not much we can do about that, but we could at least avoid doing so in *our* code. I wouldn't expect us to be opening hundreds of databases either and so keeping a simple list around of what we've opened and scanning it seems like it'd be workable. Of course, this could likely be improved in the future but I would think that'd be good for an initial implementation. We could also just generally caution users in our documentation against using multiple databases. The NSS folks discourage doing so and it doesn't strike me as being a terribly useful thing to do anyway, at least from within one invocation of an application. Still, if we could make it work reasonably well, then I'd say we should go ahead and do so. > It also doesn't look like any of the SECMOD_* machinery that we're > looking at is thread-safe, but I'd really like to be wrong... That's unfortuante but solvable by using our own locks, similar to what's done in fe-secure-openssl.c. Thanks! Stephen signature.asc Description: PGP signature
Re: Support for NSS as a libpq TLS backend
On Fri, 2021-03-26 at 00:22 +0100, Daniel Gustafsson wrote: > > On 23 Mar 2021, at 20:04, Stephen Frost wrote: > > > > Eh, poor wording on my part. You're right, the question, reworded > > again, was "Would someone want to get the context returned by > > NSS_InitContext?". If we think there's a reason that someone might want > > that context then perhaps we should allow getting it, in addition to the > > pr_fd. If there's really no reason to ever want the context from > > NSS_InitContext then what you have here where we're returning pr_fd is > > probably fine. > > I can't think of any reason, maybe Jacob who has been knee-deep in NSS > contexts > have insights which tell a different story? The only thing you can do with a context pointer is shut it down, and I don't think that's something that should be exposed. --Jacob
Re: Support for NSS as a libpq TLS backend
On Wed, 2021-03-24 at 14:10 -0400, Stephen Frost wrote: > * Jacob Champion (pchamp...@vmware.com) wrote: > > I could see this being a problem if two client certificate nicknames > > collide across multiple in-use databases, maybe? > > Right, in such a case either cert might get returned and it's possible > that the "wrong" one is returned and therefore the connection would end > up failing, assuming that they aren't actually the same and just happen > to be in both. > > Seems like we could use SECMOD_OpenUserDB() and then pass the result > from that into PK11_ListCertsInSlot() and scan through the certs in just > the specified database to find the one we're looking for if we really > feel compelled to try and address this risk. I've reached out to the > NSS folks to see if they have any thoughts about the best way to address > this. Some additional findings (NSS 3.63), please correct me if I've made any mistakes: The very first NSSInitContext created is special. If it contains a database, that database will be considered part of the "internal" slot and its certificates can be referenced directly by nickname. If it doesn't have a database, the internal slot has no certificates, and it will continue to have zero certificates until NSS is completely shut down and reinitialized with a new "first" context. Databases that are opened *after* the first one are given their own separate slots. Any certificates that are part of those databases seemingly can't be referenced directly by nickname. They have to be prefixed by their token name -- a name which you don't have if you used NSS_InitContext() to create the database. You have to use SECMOD_OpenUserDB() instead. This explains some strange failures I was seeing in local testing, where the order of InitContext determined whether our client certificate selection succeeded or failed. If you SECMOD_OpenUserDB() a database that is identical to the first (internal) database, NSS deduplicates for you and just returns the internal slot. Which seems like it's helpful, except you're not allowed to close that database, and you have to know not to close it by checking to see whether that slot is the "internal key slot". It appears to remain open until NSS is shut down entirely. But if you open a database that is *not* the magic internal database, and then open a duplicate of that one, NSS creates yet another new slot for the duplicate. So SECMOD_OpenUserDB() may or may not be a resource hog, depending on the global state of the process at the time libpq opens its first connection. We won't be able to control what the parent application will do before loading us up. It also doesn't look like any of the SECMOD_* machinery that we're looking at is thread-safe, but I'd really like to be wrong... --Jacob
Re: Support for NSS as a libpq TLS backend
> On 24 Mar 2021, at 04:54, Michael Paquier wrote:
>
> On Tue, Mar 23, 2021 at 12:38:50AM +0100, Daniel Gustafsson wrote:
>> Thanks again for reviewing, another version which addresses the remaining
>> issues will be posted soon but I wanted to get this out to give further
>> reviews
>> something that properly works.
>
> I have been looking at the infrastructure of the tests, patches 0002
> (some refactoring) and 0003 (more refactoring with tests for NSS), and
> I am a bit confused by its state.
>
> First, I think that the split is not completely clear. For example,
> patch 0003 has changes for OpenSSL.pm and Server.pm, but wouldn't it
> be better to have all the refactoring infrastructure only in 0002,
> with 0003 introducing only the NSS pieces for its internal data and
> NSS.pm?
Yes. Juggling a patchset of this size is errorprone. This is why I opened the
separate thread for this where the patch can be held apart cleaner, so let's
take this discussion over there. I will post an updated patch there shortly.
> + keyfile => 'server-password',
> + nssdatabase => 'server-cn-only.crt__server-password.key.db',
> + passphrase_cmd => 'echo secret1',
> 001_ssltests.pl and 002_scram.pl have NSS-related parameters, which
> does not look like a clean separation to me as there are OpenSSL tests
> that use some NSS parts, and the main scripts should remain neutral in
> terms setting contents, including only variables and callbacks that
> should be filled specifically for each SSL implementation, no? Aren't
> we missing a second piece here with a set of callbacks for the
> per-library test paths then?
Well, then again, keyfile is an OpenSSL specific parameter, it just happens to
be named quite neutrally. I'm not sure how to best express the certificate and
key requirements of a test since the testcase is the source of truth in terms
of what it requires. If we introduce a standard set of cert/keys which all
backends are required to supply, we could refer to those. Tests that need
something more specific can then go into 00X_.pl. There is a balance
to strike though, there is a single backend now with at most one on the horizon
which is yet to be decided upon, making it too generic may end up making test
writing overcomplicated. Do you have any concretee ideas?
> + if (defined($openssl))
> + {
> + copy_files("ssl/server-*.crt", $pgdata);
> + copy_files("ssl/server-*.key", $pgdata);
> + chmod(0600, glob "$pgdata/server-*.key") or die $!;
> + copy_files("ssl/root+client_ca.crt", $pgdata);
> + copy_files("ssl/root_ca.crt",$pgdata);
> + copy_files("ssl/root+client.crl",$pgdata);
> + mkdir("$pgdata/root+client-crldir");
> + copy_files("ssl/root+client-crldir/*",
> "$pgdata/root+client-crldir/");
> + }
> + elsif (defined($nss))
> + {
> + RecursiveCopy::copypath("ssl/nss", $pgdata . "/nss") if -e
> "ssl/nss";
> + }
> This had better be in its own callback, for example.
Yes, this one is a clearer case, fixed in the v2 patch which will be posted on
the separate thread.
--
Daniel Gustafsson https://vmware.com/
Re: Support for NSS as a libpq TLS backend
Greetings, * Jacob Champion (pchamp...@vmware.com) wrote: > On Wed, 2021-03-24 at 13:00 -0400, Stephen Frost wrote: > > * Jacob Champion (pchamp...@vmware.com) wrote: > > > Right, but to clarify -- I was asking if *NSS* supports loading and > > > using separate certificate databases as part of its API. It seems like > > > the internals make it possible, but I don't see the public interfaces > > > to actually use those internals. > > > > Yes, this is done using SECMOD_OpenUserDB, see: > > > > https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/PKCS11_Functions#SECMOD_OpenUserDB > > Ah, I had assumed that the DB-specific InitContext was using this > behind the scenes; apparently not. I will give that a try, thanks! > > > also there's info here: > > > > https://groups.google.com/g/mozilla.dev.tech.crypto/c/Xz6Emfcue0E > > > > We should document that, as mentioned in the link above, the NSS find > > functions will find certs in all the opened databases. As this would > > all be under one application which is linked against libpq and passing > > in different values for ssl_database for different connections, this > > doesn't seem like it's really that much of an issue. > > I could see this being a problem if two client certificate nicknames > collide across multiple in-use databases, maybe? Right, in such a case either cert might get returned and it's possible that the "wrong" one is returned and therefore the connection would end up failing, assuming that they aren't actually the same and just happen to be in both. Seems like we could use SECMOD_OpenUserDB() and then pass the result from that into PK11_ListCertsInSlot() and scan through the certs in just the specified database to find the one we're looking for if we really feel compelled to try and address this risk. I've reached out to the NSS folks to see if they have any thoughts about the best way to address this. Thanks, Stephen signature.asc Description: PGP signature
Re: Support for NSS as a libpq TLS backend
On Wed, 2021-03-24 at 13:00 -0400, Stephen Frost wrote: > * Jacob Champion (pchamp...@vmware.com) wrote: > > Right, but to clarify -- I was asking if *NSS* supports loading and > > using separate certificate databases as part of its API. It seems like > > the internals make it possible, but I don't see the public interfaces > > to actually use those internals. > > Yes, this is done using SECMOD_OpenUserDB, see: > > https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/PKCS11_Functions#SECMOD_OpenUserDB Ah, I had assumed that the DB-specific InitContext was using this behind the scenes; apparently not. I will give that a try, thanks! > also there's info here: > > https://groups.google.com/g/mozilla.dev.tech.crypto/c/Xz6Emfcue0E > > We should document that, as mentioned in the link above, the NSS find > functions will find certs in all the opened databases. As this would > all be under one application which is linked against libpq and passing > in different values for ssl_database for different connections, this > doesn't seem like it's really that much of an issue. I could see this being a problem if two client certificate nicknames collide across multiple in-use databases, maybe? --Jacob
Re: Support for NSS as a libpq TLS backend
Greetings Jacob, * Jacob Champion (pchamp...@vmware.com) wrote: > On Wed, 2021-03-24 at 09:28 +0900, Michael Paquier wrote: > > On Wed, Mar 24, 2021 at 12:05:35AM +, Jacob Champion wrote: > > > I can work around it temporarily for the > > > tests, but this will be a problem if any libpq clients load up multiple > > > independent databases for use with separate connections. Anyone know if > > > this is a supported use case for NSS? > > > > Are you referring to the case of threading here? This should be a > > supported case, as threads created by an application through libpq > > could perfectly use completely different connection strings. > Right, but to clarify -- I was asking if *NSS* supports loading and > using separate certificate databases as part of its API. It seems like > the internals make it possible, but I don't see the public interfaces > to actually use those internals. Yes, this is done using SECMOD_OpenUserDB, see: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/PKCS11_Functions#SECMOD_OpenUserDB also there's info here: https://groups.google.com/g/mozilla.dev.tech.crypto/c/Xz6Emfcue0E We should document that, as mentioned in the link above, the NSS find functions will find certs in all the opened databases. As this would all be under one application which is linked against libpq and passing in different values for ssl_database for different connections, this doesn't seem like it's really that much of an issue. Thanks! Stephen signature.asc Description: PGP signature
Re: Support for NSS as a libpq TLS backend
On Wed, 2021-03-24 at 09:28 +0900, Michael Paquier wrote: > On Wed, Mar 24, 2021 at 12:05:35AM +, Jacob Champion wrote: > > I can work around it temporarily for the > > tests, but this will be a problem if any libpq clients load up multiple > > independent databases for use with separate connections. Anyone know if > > this is a supported use case for NSS? > > Are you referring to the case of threading here? This should be a > supported case, as threads created by an application through libpq > could perfectly use completely different connection strings. Right, but to clarify -- I was asking if *NSS* supports loading and using separate certificate databases as part of its API. It seems like the internals make it possible, but I don't see the public interfaces to actually use those internals. --Jacob
