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 &mdash; 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