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 <pchamp...@vmware.com>
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 <pchamp...@vmware.com>
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 <pchamp...@vmware.com>
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(&conn->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();
+
/*
* For sslmodes other than verify-full and verify-ca we don't perform peer
* validation, so return immediately. sslmode require with a database
@@ -934,13 +930,11 @@ pg_bad_cert_handler(void *arg, PRFileDesc *fd)
if (conn->ssldatabase &&
strlen(conn->ssldatabase) > 0 &&
certificate_database_has_CA(conn))
- return SECFailure;
+ goto failure;
}
else if (strcmp(conn->sslmode, "verify-full") == 0 ||
strcmp(conn->sslmode, "verify-ca") == 0)
- return SECFailure;
-
- err = PORT_GetError();
+ goto failure;
/*
* TODO: these are relevant error codes that can occur in certificate
@@ -963,15 +957,20 @@ pg_bad_cert_handler(void *arg, PRFileDesc *fd)
case SEC_ERROR_CA_CERT_INVALID:
case SEC_ERROR_CERT_USAGES_INVALID:
case SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION:
- return SECSuccess;
break;
default:
- return SECFailure;
+ goto failure;
break;
}
- /* Unreachable */
return SECSuccess;
+
+failure:
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("unable to verify certificate: %s"),
+ pg_SSLerrmessage(err));
+
+ return SECFailure;
}
/* ------------------------------------------------------------ */
--
2.25.1
From 17f0cfc86a9b458d4cc964b6a1736f3b2870cf23 Mon Sep 17 00:00:00 2001
From: Jacob Champion <pchamp...@vmware.com>
Date: Mon, 19 Jul 2021 12:16:17 -0700
Subject: [PATCH 4/4] nss: add client error for unexpected-EOF
...to mirror that of the OpenSSL implementation.
---
src/interfaces/libpq/fe-secure-nss.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/interfaces/libpq/fe-secure-nss.c b/src/interfaces/libpq/fe-secure-nss.c
index 4490fb03e4..67dff8b724 100644
--- a/src/interfaces/libpq/fe-secure-nss.c
+++ b/src/interfaces/libpq/fe-secure-nss.c
@@ -488,6 +488,8 @@ pgtls_read(PGconn *conn, void *ptr, size_t len)
if (nread == 0)
{
+ appendPQExpBufferStr(&conn->errorMessage,
+ libpq_gettext("TLS read error: EOF detected\n"));
read_errno = ECONNRESET;
nread = -1;
}
--
2.25.1
