Avoid downcasing/truncation of RADIUS authentication parameters.

tglsfdc · tglsfdc · commit 4be69e2ea14d · 2019-11-13T13:41:04.000-05:00
Commit 6b76f1b changed all the RADIUS auth parameters to be lists rather than single values. But its use of SplitIdentifierString to parse the list format was not very carefully thought through, because that function thinks it's parsing SQL identifiers, which means it will (a) downcase the strings and (b) truncate them to be shorter than NAMEDATALEN. While downcasing should be harmless for the server names and ports, it's just wrong for the shared secrets, and probably for the NAS Identifier strings as well. The truncation aspect is at least potentially a problem too, though typical values for these parameters would fit in 63 bytes. Fortunately, we now have a function SplitGUCList that is exactly the same except for not doing the two unwanted things, so fixing this is a trivial matter of calling that function instead. While here, improve the documentation to show how to double-quote the parameter values. I failed to resist the temptation to do some copy-editing as well. Report and patch from Marcos David (bug #16106); doc changes by me. Back-patch to v10 where the aforesaid commit came in, since this is arguably a regression from our previous behavior with RADIUS auth. Discussion: https://postgr.es/m/16106-7d319e4295d08e70@postgresql.org
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
@@ -1688,7 +1688,7 @@ host ... ldap ldapurl="ldap://ldap.example.net/dc=example,dc=net?uid?sub"
     <literal>user name</>, <literal>password</> (encrypted) and
     <literal>NAS Identifier</>. The request will be encrypted using
     a secret shared with the server. The RADIUS server will respond to
-    this server with either <literal>Access Accept</> or
+    this request with either <literal>Access Accept</> or
     <literal>Access Reject</>. There is no support for RADIUS accounting.
    </para>
 
@@ -1697,11 +1697,11 @@ host ... ldap ldapurl="ldap://ldap.example.net/dc=example,dc=net?uid?sub"
     be tried sequentially. If a negative response is received from
     a server, the authentication will fail. If no response is received,
     the next server in the list will be tried. To specify multiple
-    servers, put the names within quotes and separate the server names
-    with a comma. If multiple servers are specified, all other RADIUS
-    options can also be given as a comma separate list, to apply
-    individual values to each server. They can also be specified as
-    a single value, in which case this value will apply to all servers.
+    servers, separate the server names with commas and surround the list
+    with double quotes. If multiple servers are specified, the other
+    RADIUS options can also be given as comma-separated lists, to provide
+    individual values for each server. They can also be specified as
+    a single value, in which case that value will apply to all servers.
    </para>
 
    <para>
@@ -1711,7 +1711,7 @@ host ... ldap ldapurl="ldap://ldap.example.net/dc=example,dc=net?uid?sub"
        <term><literal>radiusservers</literal></term>
        <listitem>
         <para>
-         The name or IP addresses of the RADIUS servers to connect to.
+         The DNS names or IP addresses of the RADIUS servers to connect to.
          This parameter is required.
         </para>
        </listitem>
@@ -1722,7 +1722,7 @@ host ... ldap ldapurl="ldap://ldap.example.net/dc=example,dc=net?uid?sub"
        <listitem>
         <para>
          The shared secrets used when talking securely to the RADIUS
-         server. This must have exactly the same value on the PostgreSQL
+         servers. This must have exactly the same value on the PostgreSQL
          and RADIUS servers. It is recommended that this be a string of
          at least 16 characters. This parameter is required.
          <note>
@@ -1742,8 +1742,9 @@ host ... ldap ldapurl="ldap://ldap.example.net/dc=example,dc=net?uid?sub"
        <term><literal>radiusports</literal></term>
        <listitem>
         <para>
-         The port number on the RADIUS servers to connect to. If no port
-         is specified, the default port <literal>1812</> will be used.
+         The port numbers to connect to on the RADIUS servers. If no port
+         is specified, the default RADIUS port (<literal>1812</>)
+         will be used.
         </para>
        </listitem>
       </varlistentry>
@@ -1752,10 +1753,10 @@ host ... ldap ldapurl="ldap://ldap.example.net/dc=example,dc=net?uid?sub"
        <term><literal>radiusidentifiers</literal></term>
        <listitem>
         <para>
-         The string used as <literal>NAS Identifier</> in the RADIUS
-         requests. This parameter can be used as a second parameter
-         identifying for example which database user the user is attempting
-         to authenticate as, which can be used for policy matching on
+         The strings to be used as <literal>NAS Identifier</> in the
+         RADIUS requests. This parameter can be used, for example, to
+         identify which database cluster the user is attempting to connect
+         to, which can be useful for policy matching on
          the RADIUS server. If no identifier is specified, the default
          <literal>postgresql</> will be used.
         </para>
@@ -1764,6 +1765,16 @@ host ... ldap ldapurl="ldap://ldap.example.net/dc=example,dc=net?uid?sub"
 
      </variablelist>
    </para>
+
+   <para>
+    If it is necessary to have a comma or whitespace in a RADIUS parameter
+    value, that can be done by putting double quotes around the value, but
+    it is tedious because two layers of double-quoting are now required.
+    An example of putting whitespace into RADIUS secret strings is:
+<programlisting>
+host ... radius radiusservers="server1,server2" radiussecrets="""secret one"",""secret two"""
+</programlisting>
+   </para>
   </sect2>
 
   <sect2 id="auth-cert">
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
@@ -1851,7 +1851,7 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
 
 		REQUIRE_AUTH_OPTION(uaRADIUS, "radiusservers", "radius");
 
-		if (!SplitIdentifierString(dupval, ',', &parsed_servers))
+		if (!SplitGUCList(dupval, ',', &parsed_servers))
 		{
 			/* syntax error in list */
 			ereport(elevel,
@@ -1900,7 +1900,7 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
 
 		REQUIRE_AUTH_OPTION(uaRADIUS, "radiusports", "radius");
 
-		if (!SplitIdentifierString(dupval, ',', &parsed_ports))
+		if (!SplitGUCList(dupval, ',', &parsed_ports))
 		{
 			ereport(elevel,
 					(errcode(ERRCODE_CONFIG_FILE_ERROR),
@@ -1935,7 +1935,7 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
 
 		REQUIRE_AUTH_OPTION(uaRADIUS, "radiussecrets", "radius");
 
-		if (!SplitIdentifierString(dupval, ',', &parsed_secrets))
+		if (!SplitGUCList(dupval, ',', &parsed_secrets))
 		{
 			/* syntax error in list */
 			ereport(elevel,
@@ -1957,7 +1957,7 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
 
 		REQUIRE_AUTH_OPTION(uaRADIUS, "radiusidentifiers", "radius");
 
-		if (!SplitIdentifierString(dupval, ',', &parsed_identifiers))
+		if (!SplitGUCList(dupval, ',', &parsed_identifiers))
 		{
 			/* syntax error in list */
 			ereport(elevel,
