Re: [libvirt] [PATCH RFC] update cacrl without restarting libvirtd via virt-admin

2020-01-02 Thread Daniel P . Berrangé
On Sat, Dec 28, 2019 at 02:18:20AM +, Zhangbo (Oscar) wrote:
> This is an RFC request for supporting virt-admin to update cacrl without
> restarting libvirtd.
> 
> When a client wants to establish a TLS connection with libvirtd, a CRL
> file is used by libvirtd to verify the client's certificate. Right now,
> if the CRL file is changed, you must restart libvirtd to make it take
> effect. The restart behavior of libvirtd will cause clients connecting
> with libvirtd to fail.
> 
> In a server cluster, the CRL file may be updated quite frequently due to
> the large amount of certificates.  If the new CRL does not take effect
> in time, there are security risks. So you may need to restart libvirtd
> frequently to make the CRL take effect in time. However, frequent restarts
> will affect the reliability of cluster virtual machine management(such as
> openstack) services.
> 
> This RFC patch adds a virt-admin command to update the server's CRL *online*.
> 
> This patch is not elegant enough, if this feature makes sense, I'd do more
> improvements.

I agree that not being able to update the CRL without restarts is a
significant problem that needs a fix. I'd suggest it is just part of
an even bigger problem - we can't update the CA cert, server cert / key
either. This is increasingly important as the popularity of short-expiry
serve certs increases.

So I think we should make the command be able to update all these TLS
related PEM files. eg have a more general command

 "virt-admin daemon-reload-tls"

to update CA cert, CA crl, server cert+key.  The impl could check the
timestamps on the individual PEM files, so it avoids reloading the
files which haven't changed since last time.

> 
> ---
> include/libvirt/libvirt-admin.h  |  4 ++
> src/admin/admin_protocol.x   | 13 +-
> src/admin/admin_server.c | 13 ++
> src/admin/admin_server.h |  4 ++
> src/admin/libvirt-admin.c  | 33 
> src/admin/libvirt_admin_private.syms   |  1 +
> src/admin/libvirt_admin_public.syms|  1 +
> src/rpc/virnetserver.c | 58 +++
> src/rpc/virnetserver.h |  3 ++
> src/rpc/virnettlscontext.c  | 33 
> src/rpc/virnettlscontext.h  |  3 ++
> tools/virt-admin.c| 59 

docs/manpages/virt-admin.rst will need an update too.



Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list



[libvirt] [PATCH RFC] update cacrl without restarting libvirtd via virt-admin

2019-12-27 Thread Zhangbo (Oscar)
This is an RFC request for supporting virt-admin to update cacrl without 
restarting libvirtd.

When a client wants to establish a TLS connection with libvirtd, a CRL file is 
used by libvirtd to verify the client's certificate. Right now, if the CRL file 
is changed, you must restart libvirtd to make it take effect. The restart 
behavior of libvirtd will cause clients connecting with libvirtd to fail.

In a server cluster, the CRL file may be updated quite frequently due to the 
large amount of certificates.  If the new CRL does not take effect in time, 
there are security risks. So you may need to restart libvirtd frequently to 
make the CRL take effect in time. However, frequent restarts will affect the 
reliability of cluster virtual machine management(such as openstack) services.

This RFC patch adds a virt-admin command to update the server's CRL *online*.

This patch is not elegant enough, if this feature makes sense, I'd do more 
improvements.

---
include/libvirt/libvirt-admin.h  |  4 ++
src/admin/admin_protocol.x   | 13 +-
src/admin/admin_server.c | 13 ++
src/admin/admin_server.h |  4 ++
src/admin/libvirt-admin.c  | 33 
src/admin/libvirt_admin_private.syms   |  1 +
src/admin/libvirt_admin_public.syms|  1 +
src/rpc/virnetserver.c | 58 +++
src/rpc/virnetserver.h |  3 ++
src/rpc/virnettlscontext.c  | 33 
src/rpc/virnettlscontext.h  |  3 ++
tools/virt-admin.c| 59 
12 files changed, 224 insertions(+), 1 deletion(-)

diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h
index abf2792926..2df43db567 100644
--- a/include/libvirt/libvirt-admin.h
+++ b/include/libvirt/libvirt-admin.h
@@ -402,6 +402,10 @@ int virAdmServerSetClientLimits(virAdmServerPtr srv,
 int nparams,
 unsigned int flags);

+int virAdmServerUpdateTlsFile(virAdmServerPtr srv,
+  unsigned int filetype,
+  unsigned int flags);
+
int virAdmConnectGetLoggingOutputs(virAdmConnectPtr conn,
char **outputs,
unsigned int flags);
diff --git a/src/admin/admin_protocol.x b/src/admin/admin_protocol.x
index 42e215d23a..c74c0d777b 100644
--- a/src/admin/admin_protocol.x
+++ b/src/admin/admin_protocol.x
@@ -181,6 +181,12 @@ struct admin_server_set_client_limits_args {
 unsigned int flags;
};

+struct admin_server_update_tls_file_args {
+admin_nonnull_server srv;
+unsigned int filetype;
+unsigned int flags;
+};
+
struct admin_connect_get_logging_outputs_args {
 unsigned int flags;
};
@@ -314,5 +320,10 @@ enum admin_procedure {
 /**
  * @generate: both
  */
-ADMIN_PROC_CONNECT_SET_LOGGING_FILTERS = 17
+ADMIN_PROC_CONNECT_SET_LOGGING_FILTERS = 17,
+
+/**
+ * @generate: both
+ */
+ADMIN_PROC_SERVER_UPDATE_TLS_FILE = 18
};
diff --git a/src/admin/admin_server.c b/src/admin/admin_server.c
index ba87f701c3..2f695eea4f 100644
--- a/src/admin/admin_server.c
+++ b/src/admin/admin_server.c
@@ -367,3 +367,16 @@ adminServerSetClientLimits(virNetServerPtr srv,

 return 0;
}
+
+int
+adminServerUpdateTlsFile(virNetServerPtr srv,
+ unsigned int filetype,
+ unsigned int flags)
+{
+virCheckFlags(0, -1);
+
+if (virNetServerUpdateTlsFile(srv, filetype) < 0)
+return -1;
+
+return 0;
+}
diff --git a/src/admin/admin_server.h b/src/admin/admin_server.h
index 1d5cbec55f..748235811a 100644
--- a/src/admin/admin_server.h
+++ b/src/admin/admin_server.h
@@ -67,3 +67,7 @@ int adminServerSetClientLimits(virNetServerPtr srv,
virTypedParameterPtr params,
int nparams,
unsigned int flags);
+
+int adminServerUpdateTlsFile(virNetServerPtr srv,
+ unsigned int filetype,
+ unsigned int flags);
diff --git a/src/admin/libvirt-admin.c b/src/admin/libvirt-admin.c
index 4099a54854..13c8db016d 100644
--- a/src/admin/libvirt-admin.c
+++ b/src/admin/libvirt-admin.c
@@ -1082,6 +1082,39 @@ virAdmServerSetClientLimits(virAdmServerPtr srv,
 return ret;
}

+/**
+ * virAdmServerUpdateTlsFile:
+ * @srv: a valid server object reference
+ * @filetype: TLS file type, such as crl, cert, key
+ * @flags: extra flags; not used yet, so callers should always pass 0
+ *
+ * update TLS Context in TLS service.
+ *
+ * Returns 0 if the TLS files have been updated successfully or -1 in case of 
an
+ * error.
+ */
+int
+virAdmServerUpdateTlsFile(virAdmServerPtr srv,
+  unsigned int filetype,
+  unsigned int flags)
+{
+int ret = -1;
+
+VIR_DEBUG("srv=%p,