Re: "cert" + clientcert=verify-ca in pg_hba.conf? - Mailing list pgsql-hackers

Hello, Bruce.

At Thu, 27 Aug 2020 15:41:40 -0400, Bruce Momjian <bruce@momjian.us> wrote in 
> > My point here is just "are we OK to remove it?"
> 
> Yes, in PG 14.  Security is confusing enough, so having a mis-named
> option that doesn't do anything more than just not specifying clientcert
> is not useful and should be removed.

Ok, this is that. If we spcify clientcert=no-verify other than for
"cert" authentication, server complains as the following at startup.

> LOG:  no-verify or 0 is the default setting that is discouraged to use explicitly for clientcert option
> HINT:  Consider removing the option instead. This option value is going to be deprecated in later version.
> CONTEXT:  line 90 of configuration file "/home/horiguti/data/data_noverify/pg_hba.conf"

And, cert clientcert=verifry-ca (and no-verify) is correctly rejected.

> LOG:  clientcert accepts only "verify-full" when using "cert" authentication

I once I thought that the deprecation message should e WARNING but
later I changed my mind to change it to LOG unifying to surrounding
setting error messages.

I'm going to register this to the coming CF.

regrds.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 32b2c8fc410c174541141a92897997b6349a9434 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyoga.ntt@gmail.com>
Date: Mon, 31 Aug 2020 17:23:18 +0900
Subject: [PATCH] Prepare for deprecating no-verify for clientcert

The option "no-verify" does something that is not intuitively inferred
from its name. It is the alias for the default behavior that lets
server verify the certificate against CA if the both are
available. It's confusing and doesn't offer any advantage, so we are
going to deprecate it. Warn if clientcert is specified with the value
"no-verify".

This also fixes an inconsistent behavior that the "cert"
authentication silently upgrades clientcert="verify-ca" to
"verify-full".
---
 doc/src/sgml/runtime.sgml |  8 ++++----
 src/backend/libpq/hba.c   | 32 ++++++++++++++++++++++----------
 2 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index c8698898f3..6ba2293b68 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -2279,10 +2279,10 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
   <para>
    The <literal>clientcert</literal> authentication option is available for
    all authentication methods, but only in <filename>pg_hba.conf</filename> lines
-   specified as <literal>hostssl</literal>.  When <literal>clientcert</literal> is
-   not specified or is set to <literal>no-verify</literal>, the server will still
-   verify any presented client certificates against its CA file, if one is
-   configured — but it will not insist that a client certificate be presented.
+   specified as <literal>hostssl</literal>.
+   When <literal>clientcert</literal> is not specified, the server verifies
+   the client certificate against its CA file only if any client certificate
+   is presented and the CA is configured.
   </para>
 
   <para>
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 9d63830553..4d0145782b 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -1708,6 +1708,21 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
             *err_msg = "clientcert can only be configured for \"hostssl\" rows";
             return false;
         }
+
+        /* check for incompatible combinations */
+        if (hbaline->auth_method == uaCert &&
+            (strcmp(val, "0") == 0 || strcmp(val, "no-verify") == 0
+             strcmp(val, "1") == 0 || strcmp(val, "verify-ca") == 0)
+            {
+            ereport(elevel,
+                    (errcode(ERRCODE_CONFIG_FILE_ERROR),
+                     errmsg("clientcert accepts only \"verify-full\" when using \"cert\" authentication"),
+                         errcontext("line %d of configuration file \"%s\"",
+                                    line_num, HbaFileName)));
+            *err_msg = "clientcert can not be set to other than \"verify-full\" when using \"cert\" authentication";
+            return false;
+        }
+
         if (strcmp(val, "1") == 0
             || strcmp(val, "verify-ca") == 0)
         {
@@ -1717,19 +1732,16 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
         {
             hbaline->clientcert = clientCertFull;
         }
+        /* XXXX: no-verify is going to be deprecated */
         else if (strcmp(val, "0") == 0
                  || strcmp(val, "no-verify") == 0)
         {
-            if (hbaline->auth_method == uaCert)
-            {
-                ereport(elevel,
-                        (errcode(ERRCODE_CONFIG_FILE_ERROR),
-                         errmsg("clientcert can not be set to \"no-verify\" when using \"cert\" authentication"),
-                         errcontext("line %d of configuration file \"%s\"",
-                                    line_num, HbaFileName)));
-                *err_msg = "clientcert can not be set to \"no-verify\" when using \"cert\" authentication";
-                return false;
-            }
+            ereport(elevel,
+                    (errcode(ERRCODE_CONFIG_FILE_ERROR),
+                     errmsg("no-verify or 0 is the default setting that is discouraged to use explicitly for
clientcertoption"),
 
+                     errhint("Consider removing the option instead. This option value is going to be deprecated in
laterversion."),
 
+                     errcontext("line %d of configuration file \"%s\"",
+                                line_num, HbaFileName)));
             hbaline->clientcert = clientCertOff;
         }
         else
-- 
2.18.4