Subject: Re: Support for NSS as a libpq TLS backend
To: Daniel Gustafsson <daniel@yesql.se>, Andres Freund <andres@anarazel.de>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>,
 Michael Paquier <michael@paquier.xyz>,
 Andrew Dunstan <andrew.dunstan@2ndquadrant.com>,
 Stephen Frost <sfrost@snowman.net>, Thomas Munro <thomas.munro@gmail.com>
References: 
 <CAA8=A7-suZg90M7Fgyo1Rfymkrpdh_Z547RKXrbHirsAmojQSw@mail.gmail.com>
 <20200904012334.GF19499@paquier.xyz> <20200917074134.GX2873@paquier.xyz>
 <BAD11CCA-E712-4906-8A27-94C2AA0E696E@yesql.se>
 <20200929055939.GF7117@paquier.xyz>
 <DE377D9A-BFFC-492C-A2B8-2AE6EBBBE740@yesql.se>
 <411593A7-E037-474D-BFD7-D3D6683C1D46@yesql.se>
 <B1ADEE76-3E33-4E9D-B830-3AA89B1C42D2@yesql.se>
 <20201020191529.5yverw3ybaube3pg@alap3.anarazel.de>
 <907FA7D1-9BD9-464B-A6EC-DEEB53438D36@yesql.se>
 <20201028063957.cvln377jgao33ssu@alap3.anarazel.de>
 <C86C2DFE-CB21-431B-AA25-CF5BD0779717@yesql.se>
From: Andrew Dunstan <andrew@dunslane.net>
Autocrypt: addr=andrew@dunslane.net; prefer-encrypt=mutual; keydata=
 mQENBE7KWFkBCAClridxur2AIc7eW2AR7izbfp3EnNefie2HbLF0izW5Ik5UjX2HBXBx4syI
 gY6b0ugohXrr274+baoAlvSbq6cAoQuEVrk5IZFzt20b1Xkx65FwGSEj526yiKLocqkJceSq
 Xr9xcA5SGY+FZv441chh5SU92v4q6z+6LPpoHOh97ptAVXZYNTtU0LevyvD5lja0TzbvJm6C
 eFXitJfnm1pLEr0DGJCR/iUOl/N62Kh4855zZC7NHIjQHPOvV5Stz/l5ilDhvGVk+xkXFPys
 SjZoUr1rXhYLpiyi5sR0X9FHXT0KnGuz1F5ERO7ZTLSSQ6fJwPj6gOk9K+vvoKvoeql5ABEB
 AAG0JEFuZHJldyBEdW5zdGFuIDxhbmRyZXdAZHVuc2xhbmUubmV0PokBPgQTAQIAKAIbAwYL
 CQgHAwIGFQgCCQoLBBYCAwECHgECF4AFAl3WlJ0FCRLOoywACgkQmfp/y1n8O4GxaQgAmtHC
 VYmtz4XxLLOu0p3aTcYrRNeTiFA5NahXljvOTODLO923Jh2HT4/PNsAOJ3L4Q5SEYoCn9Qh9
 BQUScpUGZmzInrcgE6iAYMqDX+T2CwJciuPt8SkPlYuupwkBmtcKKGH70+dyH4VL3zrK24as
 QUwncv+/632AqNCiIKa0nRCrlq/2bJaeD3ULgxrcuRVNyAwnfHGXqDo+7q865XE8JhQXjkml
 gjB06xaztVrZ8GMCDskSentmgC7a0q5w7MFFWztb1u0rDHfb0jlDaD9yOYGobkAY/E/xuHho
 7sJLotAWWAaN3t6j5Px7j2OW4sCIYM+sxlHAhrE8yiKlyyBmBbkBDQROylhZAQgA0ZzjNBQs
 7GFuo7Z3IPnViGoI1Gab2gpZqp/7CZbi2kN79VVzECc2LN3gtYId/OlvlwcC0aQ4948x/NZV
 /qe90Xt3d6CVu1/n37Mbs7uWVXTFJKgWKJ5AGysa+oMiddhf/iZFFcy3KEzfnf+2LEL1FYuz
 Wih/JiYI89ddjuH1Ht6fcHwGhWtEVx3OUhj80U2EUZb/12QSIaOCdrZGTp7XmSWfhm8UFZSD
 ben3fD+mpdzFjr6T/xMST4LhH4zrDujY8sY6N1n6Q+JRzDkfznN//JknBBfOKxdo127BYHpV
 p2z1gL12PqQ9HVzhUZGoBpgv74sAQ6quW+bZO+pCplG2cQARAQABiQElBBgBAgAPAhsMBQJd
 1pS0BQkSzqNVAAoJEJn6f8tZ/DuBCLkH/2wduZGWhk0JZbBkab472Ib8kZDFLRZZrgZpmrOp
 JNY5+3b+A7QGBqDz2vV2Org8WBJm3Me8pCmpjP/seQ+taHWOlxLzeL+5hjW7IddRS2l0Safh
 u405sS0PRg1omQZ2EBKwQKrpqTD67tYliYAez9KlHqiToeovDskcNVzaHl1WIt9OTSquANru
 yr8tUygmN7kK38q7dhpYBXphKbOg8/kGY9UWo/XU4xCQKq6si729MtcoVZfrbe8KapEMcnp7
 kYIMdsfxRJT7jAkF8/KZmR1wDs0ab+aZZ4mJYEUgPKPNjGuOS7y3L1NklWxpFedL44VDNfP0
 S7bBOkp0eKPjq5g=
Message-ID: <994d5fdb-16ff-bb8b-c29f-8bc9f809208c@dunslane.net>
Date: Sun, 1 Nov 2020 08:13:38 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.9.0
MIME-Version: 1.0
In-Reply-To: <C86C2DFE-CB21-431B-AA25-CF5BD0779717@yesql.se>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: quoted-printable
Content-Language: en-US
Precedence: bulk


On 10/29/20 11:20 AM, Daniel Gustafsson wrote:
>> On 28 Oct 2020, at 07:39, Andres Freund <andres@anarazel.de> wrote:
>> Have you done testing to ensure that NSS PG cooperates correctly with
>> openssl PG? Is there a way we can make that easier to do? E.g. allowin=
g
>> to build frontend with NSS and backend with openssl and vice versa?
> When I wrote the Secure Transport patch I had a patch against PostgresN=
ode
> which allowed for overriding the server binaries like so:
>
>    SSLTEST_SERVER_BIN=3D/path/bin/ make -C src/test/ssl/ check
>
> I've used that coupled with manual testing so far to make sure that an =
openssl
> client can talk to an NSS backend and so on.  Before any other backend =
is added
> we clearly need *a* way of doing this, one which no doubt will need to =
be
> improved upon to suit more workflows.
>
> This is sort of the same situation as pg_upgrade, where two trees is ne=
eded to
> really test it.
>
> I can clean that patch up and post as a starting point for discussions.=

>
>>>>> if test "$with_openssl" =3D yes ; then
>>>>> +  if test x"$with_nss" =3D x"yes" ; then
>>>>> +    AC_MSG_ERROR([multiple SSL backends cannot be enabled simultan=
eously"])
>>>>> +  fi
>>>> Based on a quick look there's no similar error check for the msvc
>>>> build. Should there be?
>>> Thats a good question.  When embarking on this is seemed quite natura=
l to me
>>> that it should be, but now I'm not so sure.  Maybe there should be a
>>> --with-openssl-preferred like how we handle readline/libedit or just =
allow
>>> multiple and let the last one win?  Do you have any input on what wou=
ld make
>>> sense?
>>>
>>> The only thing I think makes no sense is to allow multiple ones at th=
e same
>>> time given the current autoconf switches, even if it would just be to=
 pick say
>>> pg_strong_random from one and libpq TLS from another.
>> Maybe we should just have --with-ssl=3D{openssl,nss}? That'd avoid nee=
ding
>> to check for errors.
> Thats another option, with --with-openssl being an alias for --with-ssl=
=3Dopenssl.
>
> After another round of thinking I like this even better as it makes the=
 build
> infra cleaner, so the attached patch has this implemented.
>
>> Even better, of course, would be to allow switching of the SSL backend=

>> based on config options (PGC_POSTMASTER GUC for backend, connection
>> string for frontend). Mainly because that would make testing of
>> interoperability so much easier.  Obviously still a few places like
>> pgcrypto, randomness, etc, where only a compile time decision seems to=

>> make sense.
> It would make testing easier, but the expense seems potentially rather =
high.
> How would a GUC switch be allowed to operate, would we have mixed backe=
nds or
> would be require all openssl connectins to be dropped before serving ns=
s ones?
>
>>>>> +  CLEANLDFLAGS=3D"$LDFLAGS"
>>>>> +  # TODO: document this set of LDFLAGS
>>>>> +  LDFLAGS=3D"-lssl3 -lsmime3 -lnss3 -lplds4 -lplc4 -lnspr4 $LDFLAG=
S"
>>>> Shouldn't this use nss-config or such?
>>> Indeed it should, where available.  I've added rudimentary support fo=
r that
>>> without a fallback as of now.
>> When would we need a fallback?
> One one of my boxes I have NSS/NSPR installed via homebrew and they don=
't ship
> an nss-config AFAICT. I wouldn't be surprised if there are other cases.=

>
>>>> I think it'd also be better if we could include these files as nss/s=
sl.h
>>>> etc - ssl.h is a name way too likely to conflict imo.
>>> I've changed this to be nss/ssl.h and nspr/nspr.h etc, but the includ=
e path
>>> will still need the direct path to the headers (from autoconf) since =
nss.h
>>> includes NSPR headers as #include <nspr.h> and so on.
>> Hm. Then it's probably not worth going there...
> It does however make visual parsing of the source files easer since it'=
s clear
> which ssl.h is being referred to.  I'm in favor of keeping it.
>
>>>>> +static SECStatus
>>>>> +pg_cert_auth_handler(void *arg, PRFileDesc * fd, PRBool checksig, =
PRBool isServer)
>>>>> +{
>>>>> +	SECStatus	status;
>>>>> +	Port	   *port =3D (Port *) arg;
>>>>> +	CERTCertificate *cert;
>>>>> +	char	   *peer_cn;
>>>>> +	int			len;
>>>>> +
>>>>> +	status =3D SSL_AuthCertificate(CERT_GetDefaultCertDB(), port->pr_=
fd, checksig, PR_TRUE);
>>>>> +	if (status =3D=3D SECSuccess)
>>>>> +	{
>>>>> +		cert =3D SSL_PeerCertificate(port->pr_fd);
>>>>> +		len =3D strlen(cert->subjectName);
>>>>> +		peer_cn =3D MemoryContextAllocZero(TopMemoryContext, len + 1);
>>>>> +		if (strncmp(cert->subjectName, "CN=3D", 3) =3D=3D 0)
>>>>> +			strlcpy(peer_cn, cert->subjectName + strlen("CN=3D"), len + 1);=

>>>>> +		else
>>>>> +			strlcpy(peer_cn, cert->subjectName, len + 1);
>>>>> +		CERT_DestroyCertificate(cert);
>>>>> +
>>>>> +		port->peer_cn =3D peer_cn;
>>>>> +		port->peer_cert_valid =3D true;
>>>> Hm. We either should have something similar to
>>>>
>>>> 			/*
>>>> 			 * Reject embedded NULLs in certificate common name to prevent
>>>> 			 * attacks like CVE-2009-4034.
>>>> 			 */
>>>> 			if (len !=3D strlen(peer_cn))
>>>> 			{
>>>> 				ereport(COMMERROR,
>>>> 						(errcode(ERRCODE_PROTOCOL_VIOLATION),
>>>> 						 errmsg("SSL certificate's common name contains embedded null"=
)));
>>>> 				pfree(peer_cn);
>>>> 				return -1;
>>>> 			}
>>>> here, or a comment explaining why not.
>>> We should, but it's proving rather difficult as there is no equivalen=
t API call
>>> to get the string as well as the expected length of it.
>> Hm. Should at least have a test to ensure that's not a problem then. I=

>> hope/assume NSS rejects this somewhere internally...
> Agreed, I'll try to hack up a testcase.
>
>>>> Also, what's up with the CN=3D bit? Why is that needed here, but not=
 for
>>>> openssl?
>>> OpenSSL returns only the value portion, whereas NSS returns key=3Dval=
ue so we
>>> need to skip over the key=3D part.
>> Why is it a conditional path though?
> It was mostly just a belts-and-suspenders thing, I don't have any hard =
evidence
> that it's been a thing in any modern NSS version so it can be removed.
>
>>>>> +/*
>>>>> + * PR_ImportTCPSocket() is a private API, but very widely used, as=
 it's the
>>>>> + * only way to make NSS use an already set up POSIX file descripto=
r rather
>>>>> + * than opening one itself. To quote the NSS documentation:
>>>>> + *
>>>>> + *		"In theory, code that uses PR_ImportTCPSocket may break when N=
SPR's
>>>>> + *		implementation changes. In practice, this is unlikely to happe=
n because
>>>>> + *		NSPR's implementation has been stable for years and because of=
 NSPR's
>>>>> + *		strong commitment to backward compatibility."
>>>>> + *
>>>>> + * https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSPR/=
Reference/PR_ImportTCPSocket
>>>>> + *
>>>>> + * The function is declared in <private/pprio.h>, but as it is a h=
eader marked
>>>>> + * private we declare it here rather than including it.
>>>>> + */
>>>>> +NSPR_API(PRFileDesc *) PR_ImportTCPSocket(int);
>>>> Ugh. This is really the way to do this? How do other applications de=
al
>>>> with this problem?
>>> They either #include <private/pprio.h> or they do it like this (or ve=
ndor NSPR
>>> which makes calling private APIs less problematic).  It sure is ugly,=
 but there
>>> is no alternative to using this function.
>> Hm - in debian unstable's NSS this function appears to be in nss/ssl.h=
,
>> not pprio.h:
>>
>> /*
>> ** Imports fd into SSL, returning a new socket.  Copies SSL configurat=
ion
>> ** from model.
>> */
>> SSL_IMPORT PRFileDesc *SSL_ImportFD(PRFileDesc *model, PRFileDesc *fd)=
;
>>
>> and ssl.h starts with:
>> /*
>> * This file contains prototypes for the public SSL functions.
> Right, but that's Import*FD*, not Import*TCPSocket*.  We use ImportFD a=
s well
> since it's the API for importing an NSPR socket into NSS and enabling S=
SL/TLS
> on it.  Thats been a public API for a long time.  ImportTCPSocket is us=
ed to
> import an already opened socket into NSPR, else NSPR must open the sock=
et
> itself.  That part has been kept private for reasons unknown, as it's
> incredibly useful.
>
>>>>> +	PK11_SetPasswordFunc(PQssl_passwd_cb);
>>>> Is it actually OK to do stuff like this when other users of NSS migh=
t be
>>>> present? That's obviously more likely in the libpq case, compared to=
 the
>>>> backend case (where it's also possible, of course). What prevents us=

>>>> from overriding another user's callback?
>>> The password callback pointer is stored in a static variable in NSS (=
in the
>>> file lib/pk11wrap/pk11auth.c).
>> But, uh, how is that not a problem? What happens if a backend imports
>> libpq? What if plpython imports curl which then also uses nss?
> Sorry, that sentence wasn't really finished.  What I meant to write was=
 that I
> don't really have good answers here.  The available implementation is v=
ia the
> static var, and there are no alternative APIs.  I've tried googling for=

> insights but haven't come across any.
>
> The only datapoint I have is that I can't recall there ever being a com=
plaint
> against libcurl doing this exact thing.  That of course doesn't mean it=
 cannot
> happen or cause problems.
>
>>> +	/*
>>> +	 * Finally we must configure the socket for being a server by setti=
ng the
>>> +	 * certificate and key.
>>> +	 */
>>> +	status =3D SSL_ConfigSecureServer(model, server_cert, private_key, =
kt_rsa);
>>> +	if (status !=3D SECSuccess)
>>> +		ereport(ERROR,
>>> +				(errmsg("unable to configure secure server: %s",
>>> +						pg_SSLerrmessage(PR_GetError()))));
>>> +	status =3D SSL_ConfigServerCert(model, server_cert, private_key, NU=
LL, 0);
>>> +	if (status !=3D SECSuccess)
>>> +		ereport(ERROR,
>>> +				(errmsg("unable to configure server for TLS server connections: =
%s",
>>> +						pg_SSLerrmessage(PR_GetError()))));
>> Why do both of these need to get called? The NSS docs say:
>>
>> /*
>> ** Deprecated variant of SSL_ConfigServerCert.
>> **
>> ...
>> SSL_IMPORT SECStatus SSL_ConfigSecureServer(
>>    PRFileDesc *fd, CERTCertificate *ce	rt,
>>    SECKEYPrivateKey *key, SSLKEAType kea);
> They don't, I had missed the deprecation warning as it's not mentioned =
at all
> in the online documentation:
>
> https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/SSL_funct=
ions/sslfnc.html
>
> (SSL_ConfigServerCert isn't at all mentioned there which dates it to be=
fore
> this went it obsoleting SSL_ConfigSecureServer.)
>
> Fixed by removing the superfluous call.
>


I've been looking through the new patch set, in particular the testing
setup.

The way it seems to proceed is to use the existing openssl generated
certificates and imports them into NSS certificate databases. That seems
fine to bootstrap testing, but it seems to me it would be more sound not
to rely on openssl at all. I'd rather see the Makefile containing
commands to create these from scratch, which mirror the openssl
variants. IOW you should be able to build and test this from scratch,
including certificate generation, without having openssl installed at all=
=2E

I also notice that the invocations to pk12util don't contain the "sql:"
prefix to the -d option, even though the database was created with that
prefix a few lines above. That seems like a mistake from my reading of
the pk12util man page.


cheers


andrew