[PATCH 1/2] tls: Drop support for tls_allowed_dn_list

Tue Nov 9 16:35:46 UTC 2021

On a Tuesday in 2021, Martin Kletzander wrote:
>This setting was unsafe for a number of reasons, so bear with me here.
>
>In the Distinguished Name (or rather the string representation of ASN.1
>RelativeDistinguishedName if you will) the individial fields (or rather each
>AttributeTypeAndValue) can be in any order as defined in RFC4514, section 2.2.
>The function we use to get the DN is gnutls_x509_crt_get_dn(3) which claims to
>return the DN as described in the aforementioned RFC.
>
>The help we are providing to the user when no DN from the list of allowed DNs
>matches an incoming TLS connection says to check the output of certtool,
>particularly `certtool -i --infile clientcert.pem`.  However in the output of
>that command the order of the fields changed in some previous version exposing
>the inconsistency (see bugzilla link below for more details).
>
>This is one reason why we should not depend on the order of the fields being
>stable as the same change can happen (and maybe already happened) with the
>gnutls_x509_crt_get_dn(3) function.
>
>Another issue is that we are matching the patterns with a simple glob match,
>particularly g_pattern_match_simple(3) from glib.  Due to the fact that any
>asterisk in that pattern might match not only the field, but anything in the
>whole DN, it is very prone to errors, sometimes even not caused by the
>administrator or application setting up the allowed list.
>
>This functionality is therefore considered unsafe and should not be used, hence
>this commit makes the daemon fail during startup with a descriptive explanation
>which is the safest option that does not allow unwanted behaviour and makes the
>error message immediately apparent.
>
>Possible solutions were considered, such as ordering of the fields and
>implementing better matching configuration options and algorithm.  However these
>could lead to unsafe behaviour if not implemented exactly based on the RFC and
>even with that taken into the consideration it is not really an efficient way of
>defining filters when done with the configuration in conf (ini) format.  In case
>of using low level functions like gnutls_x509_crt_get_subject(3) and
>gnutls_x509_dn_get_rdn_ava(3) this would add huge amount of complexity to offer
>proper filtering and string representations (including encoding etc.).
>
>https://bugzilla.redhat.com/show_bug.cgi?id=2018488
>
>Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>---
>I am very happy to discuss this in more detail.
>
>I am also working on a better way to provide ACLs for remote connections and I
>would be OK with postponing this patch until that is merged so that there is a
>supported way of limiting remote users if there are any current users of this
>functionality.
>---
> docs/remote.html.in               | 26 --------------
> docs/tlscerts.html.in             |  6 ----
> src/remote/libvirtd.aug.in        |  1 -
> src/remote/libvirtd.conf.in       | 16 ---------
> src/remote/remote_daemon.c        |  2 --
> src/remote/remote_daemon_config.c | 19 +++++-----
> src/remote/remote_daemon_config.h |  1 -
> src/remote/test_libvirtd.aug.in   |  4 ---
> src/rpc/virnettlscontext.c        | 60 ++++++-------------------------
> src/rpc/virnettlscontext.h        |  2 --
> tests/virconfdata/libvirtd.conf   | 17 ---------
> tests/virconfdata/libvirtd.out    | 14 --------
> tests/virnettlscontexttest.c      |  1 -
> tests/virnettlssessiontest.c      |  1 -
> 14 files changed, 21 insertions(+), 149 deletions(-)
>
>diff --git a/docs/remote.html.in b/docs/remote.html.in
>index cc8db80c959c..211557aad66b 100644
>--- a/docs/remote.html.in
>+++ b/docs/remote.html.in
>@@ -236,32 +236,6 @@ Blank lines and comments beginning with <code>#</code> are ignored.
>   If you set this to an empty string, then no CRL is loaded.
> </td>
>       </tr>
>-      <tr>
>-        <td> tls_allowed_dn_list ["DN1", "DN2"] </td>
>-        <td> (none - DNs are not checked) </td>
>-        <td>
>-          <p>
>-  Enable an access control list of client certificate Distinguished
>-  Names (DNs) which can connect to the TLS port on this server.
>-  </p>
>-          <p>
>-  The default is that DNs are not checked.
>-  </p>
>-          <p>
>-  This list may contain wildcards such as <code>"C=GB,ST=London,L=London,O=Libvirt Project,CN=*"</code>
>-  See the POSIX <code>fnmatch</code> function for the format
>-  of the wildcards.
>-  </p>
>-          <p>
>-  Note that if this is an empty list, <i>no client can connect</i>.
>-  </p>
>-          <p>
>-  Note also that GnuTLS returns DNs without spaces
>-  after commas between the fields (and this is what we check against),
>-  but the <code>openssl x509</code> tool shows spaces.
>-</p>
>-        </td>
>-      </tr>
>     </table>
>     <h2>
>       <a id="Remote_IPv6">IPv6 support</a>
>diff --git a/docs/tlscerts.html.in b/docs/tlscerts.html.in
>index 5b7a5f56e4c2..c5206172f806 100644
>--- a/docs/tlscerts.html.in
>+++ b/docs/tlscerts.html.in
>@@ -71,9 +71,6 @@ next section.
>         <td> Installed on the client </td>
>         <td> Client's certificate signed by the CA
>   (<a href="#Remote_TLS_client_certificates">more info</a>) </td>
>-        <td> Distinguished Name (DN) can be checked against an access
>-  control list (<code>tls_allowed_dn_list</code>).
>-  </td>
>       </tr>
>       <tr>
>         <td>
>@@ -90,9 +87,6 @@ next section.
>         <td> Installed on the client </td>
>         <td> Client's certificate signed by the CA
>   (<a href="#Remote_TLS_client_certificates">more info</a>) </td>
>-        <td> Distinguished Name (DN) can be checked against an access
>-  control list (<code>tls_allowed_dn_list</code>).
>-  </td>
>       </tr>
>     </table>
>     <p>
>diff --git a/src/remote/libvirtd.aug.in b/src/remote/libvirtd.aug.in
>index d744548f4126..5bfc0a501aa5 100644
>--- a/src/remote/libvirtd.aug.in
>+++ b/src/remote/libvirtd.aug.in
>@@ -52,7 +52,6 @@ module @DAEMON_NAME_UC@ =
>
>    let tls_authorization_entry = bool_entry "tls_no_verify_certificate"
>                            | bool_entry "tls_no_sanity_certificate"
>-                           | str_array_entry "tls_allowed_dn_list"
>                            | str_entry "tls_priority"
> @END@
>
>diff --git a/src/remote/libvirtd.conf.in b/src/remote/libvirtd.conf.in
>index 8e709856aacb..5e4a8c34915f 100644
>--- a/src/remote/libvirtd.conf.in
>+++ b/src/remote/libvirtd.conf.in
>@@ -285,22 +285,6 @@
> #tls_no_verify_certificate = 1
>
>
>-# An access control list of allowed x509 Distinguished Names
>-# This list may contain wildcards such as
>-#
>-#    "C=GB,ST=London,L=London,O=Red Hat,CN=*"
>-#
>-# See the g_pattern_match function for the format of the wildcards:
>-#
>-# https://developer.gnome.org/glib/stable/glib-Glob-style-pattern-matching.html
>-#

I like your justification for removing this dead link.

Reviewed-by: Ján Tomko <jtomko at redhat.com>

Jano

>-# NB If this is an empty list, no client can connect, so comment out
>-# entirely rather than using empty list to disable these checks
>-#
>-# By default, no DN's are checked
>-#tls_allowed_dn_list = ["DN1", "DN2"]
>-
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20211109/712256be/attachment-0001.sig>