Re: [HACKERS] [Patch] Log SSL certificate verification errors

[Laurenz Albe]

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

Re: [HACKERS] [Patch] Log SSL certificate verification errors

[Graham Leggett]

On 11 Nov 2017, at 6:23 AM, Michael Paquier  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.
> 
> 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 input to 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
—


postgresql-log-cert-verification.diff
Description: Binary data


smime.p7s
Description: S/MIME cryptographic signature

Re: [HACKERS] [Patch] Log SSL certificate verification errors

[Michael Paquier]

On Sat, Nov 11, 2017 at 3:34 AM, 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.

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

[HACKERS] [Patch] Log SSL certificate verification errors

[Graham Leggett]

Hi all,

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.

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_install install 
>'/Users/minfrin/src/postgres/postgres-trunk'/tmp_install/log/install.log 2>&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 int
 verify_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_SAFETY
 static long ssl_open_connections = 0;
 
@@ -182,6 +184,7 @@
 */
SOCK_ERRNO_SET(0);
ERR_clear_error();
+   resetPQExpBuffer(>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 the msg */
-   printfPQExpBuffer(>errorMessage,
+   appendPQExpBuffer(>errorMessage,
  "SSL_read 
failed but did not provide error 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(>errorMessage,
+   appendPQExpBuffer(>errorMessage,