Обсуждение: [HACKERS] [Patch] Log SSL certificate verification errors

Hi all,

Currently neither the server side nor the client side SSL certificate verify callback does anything, leading to
potentialhair-tearing-out moments. 

The following patch to master implements logging of all certificate verification failures, as well as (crucially) which
certificatesfailed to verify, and at what depth, so the admin can zoom in straight onto the problem without any
guessing.

The SSL test suite runs without regression:

Little-Net:ssl minfrin$ make check
rm -rf '/Users/minfrin/src/postgres/postgres-trunk'/tmp_install
/bin/sh ../../../config/install-sh -c -d '/Users/minfrin/src/postgres/postgres-trunk'/tmp_install/log
/Applications/Xcode.app/Contents/Developer/usr/bin/make -C '../../..'
DESTDIR='/Users/minfrin/src/postgres/postgres-trunk'/tmp_installinstall
>'/Users/minfrin/src/postgres/postgres-trunk'/tmp_install/log/install.log2>&1 
rm -rf '/Users/minfrin/src/postgres/postgres-trunk/src/test/ssl'/tmp_check
/bin/sh ../../../config/install-sh -c -d '/Users/minfrin/src/postgres/postgres-trunk/src/test/ssl'/tmp_check
cd . && TESTDIR='/Users/minfrin/src/postgres/postgres-trunk/src/test/ssl'
PATH="/Users/minfrin/src/postgres/postgres-trunk/tmp_install/tmp/postgresql/bin:$PATH"
DYLD_LIBRARY_PATH="/Users/minfrin/src/postgres/postgres-trunk/tmp_install/tmp/postgresql/lib"PGPORT='65432'
PG_REGRESS='/Users/minfrin/src/postgres/postgres-trunk/src/test/ssl/../../../src/test/regress/pg_regress'
/opt/local/bin/prove-I ../../../src/test/perl/ -I .  t/*.pl 
t/001_ssltests.pl .. ok
All tests successful.
Files=1, Tests=40,  6 wallclock secs ( 0.04 usr  0.01 sys +  2.01 cusr  1.21 csys =  3.27 CPU)
Result: PASS


Index: src/backend/libpq/be-secure-openssl.c
===================================================================
--- src/backend/libpq/be-secure-openssl.c    (revision 61109)
+++ src/backend/libpq/be-secure-openssl.c    (working copy)
@@ -999,9 +999,9 @@/* *    Certificate verification callback *
- *    This callback allows us to log intermediate problems during
- *    verification, but for now we'll see if the final error message
- *    contains enough information.
+ *    There are 50 ways to leave your lover, and 67 ways to fail
+ *    certificate verification. Log details of all failed certificate
+ *    verification results. * *    This callback also allows us to override the default acceptance *    criteria
(e.g.,accepting self-signed or expired certs), but 
@@ -1010,6 +1010,28 @@static intverify_cb(int ok, X509_STORE_CTX *ctx){
+    char *subject, *issuer;
+    X509 *cert;
+    int err, depth;
+
+    if (!ok)
+    {
+        cert = X509_STORE_CTX_get_current_cert(ctx);
+        err = X509_STORE_CTX_get_error(ctx);
+        depth = X509_STORE_CTX_get_error_depth(ctx);
+
+        subject = X509_NAME_oneline(X509_get_subject_name(cert), NULL, 0);
+        issuer = X509_NAME_oneline(X509_get_issuer_name(cert), NULL, 0);
+
+        ereport(COMMERROR,
+            (errcode(ERRCODE_PROTOCOL_VIOLATION),
+                errmsg("SSL certificate verification result: %s (depth %d, subject '%s', issuer '%s')",
+                    X509_verify_cert_error_string(err), depth, subject, issuer)));
+
+        OPENSSL_free(subject);
+        OPENSSL_free(issuer);
+    }
+    return ok;}
Index: src/interfaces/libpq/fe-secure-openssl.c
===================================================================
--- src/interfaces/libpq/fe-secure-openssl.c    (revision 61109)
+++ src/interfaces/libpq/fe-secure-openssl.c    (working copy)
@@ -82,6 +82,8 @@static bool ssl_lib_initialized = false;
+static int ssl_ex_data_index;
+#ifdef ENABLE_THREAD_SAFETYstatic long ssl_open_connections = 0;
@@ -182,6 +184,7 @@     */    SOCK_ERRNO_SET(0);    ERR_clear_error();
+    resetPQExpBuffer(&conn->errorMessage);    n = SSL_read(conn->ssl, ptr, len);    err = SSL_get_error(conn->ssl, n);
@@ -200,7 +203,7 @@            if (n < 0)            {                /* Not supposed to happen, so we don't translate
themsg */ 
-                printfPQExpBuffer(&conn->errorMessage,
+                appendPQExpBuffer(&conn->errorMessage,                                  "SSL_read failed but did not
provideerror information\n");                /* assume the connection is broken */                result_errno =
ECONNRESET;
@@ -224,13 +227,13 @@                result_errno = SOCK_ERRNO;                if (result_errno == EPIPE ||
      result_errno == ECONNRESET) 
-                    printfPQExpBuffer(&conn->errorMessage,
+                    appendPQExpBuffer(&conn->errorMessage,                                      libpq_gettext(
                                          "server closed the connection unexpectedly\n"
                  "\tThis probably means the server terminated abnormally\n"
       "\tbefore or while processing the request.\n"));                else 
-                    printfPQExpBuffer(&conn->errorMessage,
+                    appendPQExpBuffer(&conn->errorMessage,                                      libpq_gettext("SSL
SYSCALLerror: %s\n"),                                      SOCK_STRERROR(result_errno,
                 sebuf, sizeof(sebuf))); 
@@ -237,7 +240,7 @@            }            else            {
-                printfPQExpBuffer(&conn->errorMessage,
+                appendPQExpBuffer(&conn->errorMessage,                                  libpq_gettext("SSL SYSCALL
error:EOF detected\n"));                /* assume the connection is broken */                result_errno = ECONNRESET; 
@@ -248,7 +251,7 @@            {                char       *errm = SSLerrmessage(ecode);
-                printfPQExpBuffer(&conn->errorMessage,
+                appendPQExpBuffer(&conn->errorMessage,                                  libpq_gettext("SSL error:
%s\n"),errm);                SSLerrfree(errm);                /* assume the connection is broken */ 
@@ -263,13 +266,13 @@             * a clean connection closure, so we should not report it as a             * server
crash.            */ 
-            printfPQExpBuffer(&conn->errorMessage,
+            appendPQExpBuffer(&conn->errorMessage,                              libpq_gettext("SSL connection has been
closedunexpectedly\n"));            result_errno = ECONNRESET;            n = -1;            break;        default: 
-            printfPQExpBuffer(&conn->errorMessage,
+            appendPQExpBuffer(&conn->errorMessage,                              libpq_gettext("unrecognized SSL error
code:%d\n"),                              err);            /* assume the connection is broken */ 
@@ -302,6 +305,7 @@    SOCK_ERRNO_SET(0);    ERR_clear_error();
+    resetPQExpBuffer(&conn->errorMessage);    n = SSL_write(conn->ssl, ptr, len);    err = SSL_get_error(conn->ssl,
n);   ecode = (err != SSL_ERROR_NONE || n < 0) ? ERR_get_error() : 0; 
@@ -311,7 +315,7 @@            if (n < 0)            {                /* Not supposed to happen, so we don't translate
themsg */ 
-                printfPQExpBuffer(&conn->errorMessage,
+                appendPQExpBuffer(&conn->errorMessage,                                  "SSL_write failed but did not
provideerror information\n");                /* assume the connection is broken */                result_errno =
ECONNRESET;
@@ -333,13 +337,13 @@            {                result_errno = SOCK_ERRNO;                if (result_errno == EPIPE
||result_errno == ECONNRESET) 
-                    printfPQExpBuffer(&conn->errorMessage,
+                    appendPQExpBuffer(&conn->errorMessage,                                      libpq_gettext(
                                          "server closed the connection unexpectedly\n"
                  "\tThis probably means the server terminated abnormally\n"
       "\tbefore or while processing the request.\n"));                else 
-                    printfPQExpBuffer(&conn->errorMessage,
+                    appendPQExpBuffer(&conn->errorMessage,                                      libpq_gettext("SSL
SYSCALLerror: %s\n"),                                      SOCK_STRERROR(result_errno,
                 sebuf, sizeof(sebuf))); 
@@ -346,7 +350,7 @@            }            else            {
-                printfPQExpBuffer(&conn->errorMessage,
+                appendPQExpBuffer(&conn->errorMessage,                                  libpq_gettext("SSL SYSCALL
error:EOF detected\n"));                /* assume the connection is broken */                result_errno = ECONNRESET; 
@@ -357,7 +361,7 @@            {                char       *errm = SSLerrmessage(ecode);
-                printfPQExpBuffer(&conn->errorMessage,
+                appendPQExpBuffer(&conn->errorMessage,                                  libpq_gettext("SSL error:
%s\n"),errm);                SSLerrfree(errm);                /* assume the connection is broken */ 
@@ -372,13 +376,13 @@             * a clean connection closure, so we should not report it as a             * server
crash.            */ 
-            printfPQExpBuffer(&conn->errorMessage,
+            appendPQExpBuffer(&conn->errorMessage,                              libpq_gettext("SSL connection has been
closedunexpectedly\n"));            result_errno = ECONNRESET;            n = -1;            break;        default: 
-            printfPQExpBuffer(&conn->errorMessage,
+            appendPQExpBuffer(&conn->errorMessage,                              libpq_gettext("unrecognized SSL error
code:%d\n"),                              err);            /* assume the connection is broken */ 
@@ -400,9 +404,9 @@/* *    Certificate verification callback *
- *    This callback allows us to log intermediate problems during
- *    verification, but there doesn't seem to be a clean way to get
- *    our PGconn * structure.  So we can't log anything!
+ *    There are 50 ways to leave your lover, and 67 ways to fail
+ *    certificate verification. Return details of all failed certificate
+ *    verification results. * *    This callback also allows us to override the default acceptance *    criteria
(e.g.,accepting self-signed or expired certs), but 
@@ -411,6 +415,32 @@static intverify_cb(int ok, X509_STORE_CTX *ctx){
+    char *subject, *issuer;
+    X509 *cert;
+    SSL *ssl;
+    PGconn *conn;
+    int err, depth;
+
+    if (!ok)
+    {
+        cert = X509_STORE_CTX_get_current_cert(ctx);
+        err = X509_STORE_CTX_get_error(ctx);
+        depth = X509_STORE_CTX_get_error_depth(ctx);
+
+        subject = X509_NAME_oneline(X509_get_subject_name(cert), NULL, 0);
+        issuer = X509_NAME_oneline(X509_get_issuer_name(cert), NULL, 0);
+
+        ssl = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx());
+        conn = SSL_get_ex_data(ssl, ssl_ex_data_index);
+
+        appendPQExpBuffer(&conn->errorMessage,
+            libpq_gettext("SSL certificate verification result: %s (depth %d, subject '%s', issuer '%s')\n"),
+                X509_verify_cert_error_string(err), depth, subject, issuer);
+
+        OPENSSL_free(subject);
+        OPENSSL_free(issuer);
+    }
+    return ok;}
@@ -490,7 +520,7 @@    /* Should not happen... */    if (name_entry == NULL)    {
-        printfPQExpBuffer(&conn->errorMessage,
+        appendPQExpBuffer(&conn->errorMessage,                          libpq_gettext("SSL certificate's name entry is
missing\n"));       return -1;    } 
@@ -510,7 +540,7 @@    name = malloc(len + 1);    if (name == NULL)    {
-        printfPQExpBuffer(&conn->errorMessage,
+        appendPQExpBuffer(&conn->errorMessage,                          libpq_gettext("out of memory\n"));
return-1;    } 
@@ -524,7 +554,7 @@    if (len != strlen(name))    {        free(name);
-        printfPQExpBuffer(&conn->errorMessage,
+        appendPQExpBuffer(&conn->errorMessage,                          libpq_gettext("SSL certificate's name contains
embeddednull\n"));        return -1;    } 
@@ -576,7 +606,7 @@    /* Check that we have a hostname to compare with. */    if (!(host && host[0] != '\0'))    {
-        printfPQExpBuffer(&conn->errorMessage,
+        appendPQExpBuffer(&conn->errorMessage,                          libpq_gettext("host name must be specified for
averified SSL connection\n"));        return false;    } 
@@ -668,7 +698,7 @@         */        if (names_examined > 1)        {
-            printfPQExpBuffer(&conn->errorMessage,
+            appendPQExpBuffer(&conn->errorMessage,                              libpq_ngettext("server certificate for
\"%s\"(and %d other name) does not match host name \"%s\"\n",                                             "server
certificatefor \"%s\" (and %d other names) does not match host name \"%s\"\n",
  names_examined - 1), 
@@ -676,13 +706,13 @@        }        else if (names_examined == 1)        {
-            printfPQExpBuffer(&conn->errorMessage,
+            appendPQExpBuffer(&conn->errorMessage,                              libpq_gettext("server certificate for
\"%s\"does not match host name \"%s\"\n"),                              first_name, host);        }        else
{
-            printfPQExpBuffer(&conn->errorMessage,
+            appendPQExpBuffer(&conn->errorMessage,                              libpq_gettext("could not get server's
hostname from server certificate\n"));        }    } 
@@ -823,6 +853,8 @@            SSL_library_init();            SSL_load_error_strings();#endif
+            ssl_ex_data_index = SSL_get_ex_new_index(0,
+                "postgresql index", NULL, NULL, NULL);        }        ssl_lib_initialized = true;    }
@@ -899,6 +931,8 @@    bool        have_rootcert;    EVP_PKEY   *pkey = NULL;
+    resetPQExpBuffer(&conn->errorMessage);
+    /*     * We'll need the home directory if any of the relevant parameters are     * defaulted.  If
pqGetHomeDirectoryfails, act as though none of the 
@@ -924,7 +958,7 @@    {        char       *err = SSLerrmessage(ERR_get_error());
-        printfPQExpBuffer(&conn->errorMessage,
+        appendPQExpBuffer(&conn->errorMessage,                          libpq_gettext("could not create SSL context:
%s\n"),                         err);        SSLerrfree(err); 
@@ -961,7 +995,7 @@        {            char       *err = SSLerrmessage(ERR_get_error());
-            printfPQExpBuffer(&conn->errorMessage,
+            appendPQExpBuffer(&conn->errorMessage,                              libpq_gettext("could not read root
certificatefile \"%s\": %s\n"),                              fnbuf, err);            SSLerrfree(err); 
@@ -989,7 +1023,7 @@#else                char       *err = SSLerrmessage(ERR_get_error());
-                printfPQExpBuffer(&conn->errorMessage,
+                appendPQExpBuffer(&conn->errorMessage,                                  libpq_gettext("SSL library
doesnot support CRL certificates (file \"%s\")\n"),                                  fnbuf);
SSLerrfree(err);
@@ -1017,11 +1051,11 @@             * that it seems worth having a specialized error message for it.             */
      if (fnbuf[0] == '\0') 
-                printfPQExpBuffer(&conn->errorMessage,
+                appendPQExpBuffer(&conn->errorMessage,                                  libpq_gettext("could not get
homedirectory to locate root certificate file\n"                                                "Either provide the
fileor change sslmode to disable server certificate verification.\n"));            else 
-                printfPQExpBuffer(&conn->errorMessage,
+                appendPQExpBuffer(&conn->errorMessage,                                  libpq_gettext("root
certificatefile \"%s\" does not exist\n"                                                "Either provide the file or
changesslmode to disable server certificate verification.\n"), fnbuf);            SSL_CTX_free(SSL_context); 
@@ -1052,7 +1086,7 @@         */        if (errno != ENOENT && errno != ENOTDIR)        {
-            printfPQExpBuffer(&conn->errorMessage,
+            appendPQExpBuffer(&conn->errorMessage,                              libpq_gettext("could not open
certificatefile \"%s\": %s\n"),                              fnbuf, pqStrerror(errno, sebuf, sizeof(sebuf)));
SSL_CTX_free(SSL_context); 
@@ -1071,7 +1105,7 @@        {            char       *err = SSLerrmessage(ERR_get_error());
-            printfPQExpBuffer(&conn->errorMessage,
+            appendPQExpBuffer(&conn->errorMessage,                              libpq_gettext("could not read
certificatefile \"%s\": %s\n"),                              fnbuf, err);            SSLerrfree(err); 
@@ -1096,7 +1130,7 @@    {        char       *err = SSLerrmessage(ERR_get_error());
-        printfPQExpBuffer(&conn->errorMessage,
+        appendPQExpBuffer(&conn->errorMessage,                          libpq_gettext("could not establish SSL
connection:%s\n"),                          err);        SSLerrfree(err); 
@@ -1134,7 +1168,7 @@            if (engine_str == NULL)            {
-                printfPQExpBuffer(&conn->errorMessage,
+                appendPQExpBuffer(&conn->errorMessage,                                  libpq_gettext("out of
memory\n"));               return -1;            } 
@@ -1150,7 +1184,7 @@            {                char       *err = SSLerrmessage(ERR_get_error());
-                printfPQExpBuffer(&conn->errorMessage,
+                appendPQExpBuffer(&conn->errorMessage,                                  libpq_gettext("could not load
SSLengine \"%s\": %s\n"),                                  engine_str, err);                SSLerrfree(err); 
@@ -1162,7 +1196,7 @@            {                char       *err = SSLerrmessage(ERR_get_error());
-                printfPQExpBuffer(&conn->errorMessage,
+                appendPQExpBuffer(&conn->errorMessage,                                  libpq_gettext("could not
initializeSSL engine \"%s\": %s\n"),                                  engine_str, err);                SSLerrfree(err); 
@@ -1178,7 +1212,7 @@            {                char       *err = SSLerrmessage(ERR_get_error());
-                printfPQExpBuffer(&conn->errorMessage,
+                appendPQExpBuffer(&conn->errorMessage,                                  libpq_gettext("could not read
privateSSL key \"%s\" from engine \"%s\": %s\n"),                                  engine_colon, engine_str, err);
         SSLerrfree(err); 
@@ -1192,7 +1226,7 @@            {                char       *err = SSLerrmessage(ERR_get_error());
-                printfPQExpBuffer(&conn->errorMessage,
+                appendPQExpBuffer(&conn->errorMessage,                                  libpq_gettext("could not load
privateSSL key \"%s\" from engine \"%s\": %s\n"),                                  engine_colon, engine_str, err);
         SSLerrfree(err); 
@@ -1229,7 +1263,7 @@        if (stat(fnbuf, &buf) != 0)        {
-            printfPQExpBuffer(&conn->errorMessage,
+            appendPQExpBuffer(&conn->errorMessage,                              libpq_gettext("certificate present,
butnot private key file \"%s\"\n"),                              fnbuf);            return -1; 
@@ -1237,7 +1271,7 @@#ifndef WIN32        if (!S_ISREG(buf.st_mode) || buf.st_mode & (S_IRWXG | S_IRWXO))        {
-            printfPQExpBuffer(&conn->errorMessage,
+            appendPQExpBuffer(&conn->errorMessage,                              libpq_gettext("private key file \"%s\"
hasgroup or world access; permissions should be u=rw (0600) or less\n"),                              fnbuf);
return -1; 
@@ -1248,7 +1282,7 @@        {            char       *err = SSLerrmessage(ERR_get_error());
-            printfPQExpBuffer(&conn->errorMessage,
+            appendPQExpBuffer(&conn->errorMessage,                              libpq_gettext("could not load private
keyfile \"%s\": %s\n"),                              fnbuf, err);            SSLerrfree(err); 
@@ -1262,7 +1296,7 @@    {        char       *err = SSLerrmessage(ERR_get_error());
-        printfPQExpBuffer(&conn->errorMessage,
+        appendPQExpBuffer(&conn->errorMessage,                          libpq_gettext("certificate does not match
privatekey file \"%s\": %s\n"),                          fnbuf, err);        SSLerrfree(err); 
@@ -1274,7 +1308,10 @@     * callback.     */    if (have_rootcert)
+    {        SSL_set_verify(conn->ssl, SSL_VERIFY_PEER, verify_cb);
+        SSL_set_ex_data(conn->ssl, ssl_ex_data_index, conn);
+    }    /*     * If the OpenSSL version used supports it (from 1.0.0 on) and the user
@@ -1299,6 +1336,7 @@    int            r;    ERR_clear_error();
+    resetPQExpBuffer(&conn->errorMessage);    r = SSL_connect(conn->ssl);    if (r <= 0)    {
@@ -1319,11 +1357,11 @@                    char        sebuf[256];                    if (r == -1)
-                        printfPQExpBuffer(&conn->errorMessage,
+                        appendPQExpBuffer(&conn->errorMessage,
libpq_gettext("SSLSYSCALL error: %s\n"),                                          SOCK_STRERROR(SOCK_ERRNO, sebuf,
sizeof(sebuf)));                   else 
-                        printfPQExpBuffer(&conn->errorMessage,
+                        appendPQExpBuffer(&conn->errorMessage,
libpq_gettext("SSLSYSCALL error: EOF detected\n"));                    pgtls_close(conn);                    return
PGRES_POLLING_FAILED;
@@ -1332,7 +1370,7 @@                {                    char       *err = SSLerrmessage(ecode);
-                    printfPQExpBuffer(&conn->errorMessage,
+                    appendPQExpBuffer(&conn->errorMessage,                                      libpq_gettext("SSL
error:%s\n"),                                      err);                    SSLerrfree(err); 
@@ -1341,7 +1379,7 @@                }            default:
-                printfPQExpBuffer(&conn->errorMessage,
+                appendPQExpBuffer(&conn->errorMessage,                                  libpq_gettext("unrecognized
SSLerror code: %d\n"),                                  err);                pgtls_close(conn); 
@@ -1362,7 +1400,7 @@        err = SSLerrmessage(ERR_get_error());
-        printfPQExpBuffer(&conn->errorMessage,
+        appendPQExpBuffer(&conn->errorMessage,                          libpq_gettext("certificate could not be
obtained:%s\n"),                          err);        SSLerrfree(err); 


Regards,
Graham
—

On Sat, Nov 11, 2017 at 3:34 AM, Graham Leggett <minfrin@sharp.fm> wrote:
> Currently neither the server side nor the client side SSL certificate verify callback does anything, leading to
potentialhair-tearing-out moments.
 
>
> The following patch to master implements logging of all certificate verification failures, as well as (crucially)
whichcertificates failed to verify, and at what depth, so the admin can zoom in straight onto the problem without any
guessing.

Could you attach as a file to this thread a patch that can be easily
applied? Using git --format-patch or simply diff is just fine.

Here are also some community guidelines on the matter:
https://wiki.postgresql.org/wiki/Submitting_a_Patch

And if you are looking for feedback, you should register it to the
next commit fest:
https://commitfest.postgresql.org/16/
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 11 Nov 2017, at 6:23 AM, Michael Paquier <michael.paquier@gmail.com> wrote:

>> Currently neither the server side nor the client side SSL certificate verify callback does anything, leading to
potentialhair-tearing-out moments. 
>>
>> The following patch to master implements logging of all certificate verification failures, as well as (crucially)
whichcertificates failed to verify, and at what depth, so the admin can zoom in straight onto the problem without any
guessing.
>
> Could you attach as a file to this thread a patch that can be easily
> applied? Using git --format-patch or simply diff is just fine.

I’ve attached it as a separate attachment.

The default behaviour of patch is to ignore all lines before and after the patch, so you can use my entire email as an
inputto patch and it will work (This is what git format-patch does, create something that looks like an email). 

> Here are also some community guidelines on the matter:
> https://wiki.postgresql.org/wiki/Submitting_a_Patch
>
> And if you are looking for feedback, you should register it to the
> next commit fest:
> https://commitfest.postgresql.org/16/

I shall do!

Regards,
Graham
—

Graham Leggett wrote:
> Currently neither the server side nor the client side SSL certificate verify
> callback does anything, leading to potential hair-tearing-out moments.
>
> The following patch to master implements logging of all certificate
> verification failures, as well as (crucially) which certificates failed to verify,
> and at what depth, so the admin can zoom in straight onto the problem without any guessing.

+1 for the idea.

I have been in this situation before, and any information that helps to
clarify what the problem is would be a great help.

Yours,
Laurenz Albe


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 11/11/17 05:50, Graham Leggett wrote:
> On 11 Nov 2017, at 6:23 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
> 
>>> Currently neither the server side nor the client side SSL certificate verify callback does anything, leading to
potentialhair-tearing-out moments.
 
>>>
>>> The following patch to master implements logging of all certificate verification failures, as well as (crucially)
whichcertificates failed to verify, and at what depth, so the admin can zoom in straight onto the problem without any
guessing.
>>
>> Could you attach as a file to this thread a patch that can be easily
>> applied? Using git --format-patch or simply diff is just fine.
> 
> I’ve attached it as a separate attachment.

The server-side changes look pretty reasonable.

On the client side, I'd like to see some comments explaining the
business around ssl_ex_data_index.

We could probably do with some more tests.  I can see the server-side
message printed once in the logs of the ssl tests, but there ought to be
some more cases.  For the client side, we should think of a way to have
the tests expose this new functionality.

Some of the new code in verify_cb() should perhaps be a bit more
defensive.  I don't know all these APIs in detail, but it seems possible
that some calls will return NULL, which could lead to crashes later on.

I'm also wondering whether it is always safe and sane to print subject
and issuer.  I'd imagine a client could craft a silly certificate setup
on purpose and the server would just print whatever the client said into
the logs.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Graham, will you be able to respond to my questions or provide an
updated patch within the next week or so?


On 1/2/18 09:17, Peter Eisentraut wrote:
> The server-side changes look pretty reasonable.
> 
> On the client side, I'd like to see some comments explaining the
> business around ssl_ex_data_index.
> 
> We could probably do with some more tests.  I can see the server-side
> message printed once in the logs of the ssl tests, but there ought to be
> some more cases.  For the client side, we should think of a way to have
> the tests expose this new functionality.
> 
> Some of the new code in verify_cb() should perhaps be a bit more
> defensive.  I don't know all these APIs in detail, but it seems possible
> that some calls will return NULL, which could lead to crashes later on.
> 
> I'm also wondering whether it is always safe and sane to print subject
> and issuer.  I'd imagine a client could craft a silly certificate setup
> on purpose and the server would just print whatever the client said into
> the logs.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-01-17 09:03:51 -0500, Peter Eisentraut wrote:
> Graham, will you be able to respond to my questions or provide an
> updated patch within the next week or so?

Given that nothing has happend since, I've marked this as returned with
feedback.

Greetings,

Andres Freund

On 3/1/18 20:48, Andres Freund wrote:
> On 2018-01-17 09:03:51 -0500, Peter Eisentraut wrote:
>> Graham, will you be able to respond to my questions or provide an
>> updated patch within the next week or so?
> 
> Given that nothing has happend since, I've marked this as returned with
> feedback.

Agreed.

But this patch was quite useful while debugging some of the other
ongoing SSL work.  So if it were revived at some point in the future, it
would be welcome.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Mar 06, 2018 at 02:56:41PM -0500, Peter Eisentraut wrote:
> But this patch was quite useful while debugging some of the other
> ongoing SSL work.  So if it were revived at some point in the future, it
> would be welcome.

+1!
--
Michael