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

At Mon, 31 Aug 2020 11:34:29 -0400, Bruce Momjian <bruce@momjian.us> wrote in 
> On Mon, Aug 31, 2020 at 05:56:58PM +0900, Kyotaro Horiguchi wrote:
> > Ok, this is that. If we spcify clientcert=no-verify other than for
> > "cert" authentication, server complains as the following at startup.
> 
> Why does clientcert=no-verify have any value, even for a
> cert-authentication line?
> 
> > > 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"
> 
> I think it should just be removed in PG 14.  This is a configuration
> setting, not an SQL-level item that needs a deprecation period.

Ok, it is changed to just error out. I tempted to show a suggestion to
removing the option in that case like the following, but *didn't* in
this version of the patch.

 > LOG:  invalid value for clientcert: "no-verify"
?? HINT:  Instead, consider removing the clinetcert option.
 > CONTEXT:  line 90 of configuration file "/h


> > I'm going to register this to the coming CF.
> 
> I plan to apply this once we are done discussing it.

Roger.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 3ef1d3eee86998be934eb67d55d89d965381f5d2 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 v2] Deprecate 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 only if the both are
available. It's confusing and doesn't offer any advantage, so we
deprecate it. Error out and suggest to remove the option if clientcert
is specified with the value "no-verify". The legacy values "0" and "1"
are also deprecated together.

This also fixes an inconsistent behavior that the "cert"
authentication silently upgrades clientcert="verify-ca" to
"verify-full".
---
 doc/src/sgml/client-auth.sgml | 11 +++++------
 doc/src/sgml/runtime.sgml     |  5 ++---
 src/backend/libpq/hba.c       | 33 ++++++++++++++++-----------------
 3 files changed, 23 insertions(+), 26 deletions(-)

diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 5cd88b462d..03248b3d63 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -2043,12 +2043,11 @@ host ... radius radiusservers="server1,server2" radiussecrets="""secret one"",""
 
    <para>
     In a <filename>pg_hba.conf</filename> record specifying certificate
-    authentication, the authentication option <literal>clientcert</literal> is
-    assumed to be <literal>verify-ca</literal> or <literal>verify-full</literal>,
-    and it cannot be turned off since a client certificate is necessary for this
-    method. What the <literal>cert</literal> method adds to the basic
-    <literal>clientcert</literal> certificate validity test is a check that the
-    <literal>cn</literal> attribute matches the database user name.
+    authentication is equivalent to the <literal>trust</literal>
+    authentication having the authentication
+    option <literal>clientcert</literal> with the
+    value <literal>verify-full</literal> and it cannot be turned off or
+    downgraded to <literal>verify-ca</literal>.
    </para>
   </sect1>
 
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index 6cda39f3ab..0f4190d71c 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -2282,9 +2282,8 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
    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.
+   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..b702c8a419 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -1708,35 +1708,34 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
             *err_msg = "clientcert can only be configured for \"hostssl\" rows";
             return false;
         }
-        if (strcmp(val, "1") == 0
-            || strcmp(val, "verify-ca") == 0)
+
+        if (strcmp(val, "verify-ca") == 0)
         {
+            if (hbaline->auth_method == uaCert)
+            {
+                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;
+            }
+
             hbaline->clientcert = clientCertCA;
         }
         else if (strcmp(val, "verify-full") == 0)
         {
             hbaline->clientcert = clientCertFull;
         }
-        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;
-            }
-            hbaline->clientcert = clientCertOff;
-        }
         else
         {
             ereport(elevel,
                     (errcode(ERRCODE_CONFIG_FILE_ERROR),
                      errmsg("invalid value for clientcert: \"%s\"", val),
+                     /* no-verify is an obsolete value */
+                     (strcmp(val, "no-verify") == 0 ?
+                      errhint("Instead, consider removing the clinetcert option."):0),
                      errcontext("line %d of configuration file \"%s\"",
                                 line_num, HbaFileName)));
             return false;
-- 
2.18.4