Re: [PATCH 4/6] admin: support server cert update mode
On Sun, Feb 09, 2020 at 10:03:14PM +0800, Zhang Bo wrote:
> virAdmServerUpdateTlsFiles:
> @flags specifies how to update server cert/key in tls service.
> Two modes are currently supported: append mode and clear mode, means
> whether to clear the original cert then add the new one, or just append
> to the original one.
> ---
> include/libvirt/libvirt-admin.h | 14 ++
> src/admin/admin_server.c| 7 +--
> src/admin/libvirt-admin.c | 7 ++-
> src/rpc/virnetserver.c | 17 +
> src/rpc/virnetserver.h | 3 ++-
> src/rpc/virnettlscontext.c | 7 +--
> src/rpc/virnettlscontext.h | 3 ++-
> 7 files changed, 43 insertions(+), 15 deletions(-)
>
> diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h
> index 6e38261129..dfdd81ae83 100644
> --- a/include/libvirt/libvirt-admin.h
> +++ b/include/libvirt/libvirt-admin.h
> @@ -392,6 +392,20 @@ int virAdmClientClose(virAdmClientPtr client, unsigned
> int flags);
>
> # define VIR_SERVER_CLIENTS_UNAUTH_CURRENT "nclients_unauth"
>
> +typedef enum {
> +/* free old credentials and then set new tls context.
> + */
> +VIR_TLS_UPDATE_CLEAR = 0,
> +
> +/* do not clear original certificates and keys.
> + */
> +VIR_TLS_UPDATE_APPEND = 1,
I don't think we should we supporting this operation. In order to achieve
reliability of the TLS reload, we need to re-create the credentials object
and swap out the original credentails on success. This precludes updating
the original credentials
> @@ -1165,7 +1166,9 @@ int virNetTLSContextReload(virNetTLSContextPtr ctxt,
> }
>
> if (filetypes & VIR_TLS_FILE_TYPE_SERVER_CERT) {
> -gnutls_certificate_free_keys(ctxt->x509cred);
> +if (flags == VIR_TLS_UPDATE_CLEAR)
> +gnutls_certificate_free_keys(ctxt->x509cred);
> +
> if (virNetTLSContextSetCertAndKey(ctxt, cert, key, false))
> goto cleanup;
...because this code still has the dangerous behaviour that it leaves the
server without any valid cert/key on failure.
The second bad thing is that the state in-memory does not match the
state on disk. This puts the old & new cert+key in memory, but only
the new cert+key is on disk. So if libvirtd is restarted for any
reason you'll get a change in certs again. The reload operation
should just be updating libvirtd's in-memory state to exactly match
the on-disk state, in the same way that a restart does.
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 :|
撤回: [PATCH 4/6] admin: support server cert update mode
Zhangbo (Oscar) 将撤回邮件“[PATCH 4/6] admin: support server cert update mode”。
[PATCH 4/6] admin: support server cert update mode
virAdmServerUpdateTlsFiles:
@flags specifies how to update server cert/key in tls service.
Two modes are currently supported: append mode and clear mode, means
whether to clear the original cert then add the new one, or just append
to the original one.
---
include/libvirt/libvirt-admin.h | 14 ++
src/admin/admin_server.c| 7 +--
src/admin/libvirt-admin.c | 7 ++-
src/rpc/virnetserver.c | 17 +
src/rpc/virnetserver.h | 3 ++-
src/rpc/virnettlscontext.c | 7 +--
src/rpc/virnettlscontext.h | 3 ++-
7 files changed, 43 insertions(+), 15 deletions(-)
diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h
index 6e38261129..dfdd81ae83 100644
--- a/include/libvirt/libvirt-admin.h
+++ b/include/libvirt/libvirt-admin.h
@@ -392,6 +392,20 @@ int virAdmClientClose(virAdmClientPtr client, unsigned int
flags);
# define VIR_SERVER_CLIENTS_UNAUTH_CURRENT "nclients_unauth"
+typedef enum {
+/* free old credentials and then set new tls context.
+ */
+VIR_TLS_UPDATE_CLEAR = 0,
+
+/* do not clear original certificates and keys.
+ */
+VIR_TLS_UPDATE_APPEND = 1,
+
+/* boundary value for flag check (unreachable).
+ */
+VIR_TLS_UPDATE_FLAG_MAX = 2,
+} virServerTlsUpdateFlag;
+
/* tls related filetype flags. */
typedef enum {
VIR_TLS_FILE_TYPE_CA_CERT = (1U << 0),
diff --git a/src/admin/admin_server.c b/src/admin/admin_server.c
index 558913367b..43c7e00d90 100644
--- a/src/admin/admin_server.c
+++ b/src/admin/admin_server.c
@@ -373,10 +373,5 @@ adminServerUpdateTlsFiles(virNetServerPtr srv,
unsigned int filetypes,
unsigned int flags)
{
-virCheckFlags(0, -1);
-
-if (virNetServerUpdateTlsFiles(srv, filetypes) < 0)
-return -1;
-
-return 0;
+return virNetServerUpdateTlsFiles(srv, filetypes, flags);
}
diff --git a/src/admin/libvirt-admin.c b/src/admin/libvirt-admin.c
index f3f92ed91c..b6ba72b577 100644
--- a/src/admin/libvirt-admin.c
+++ b/src/admin/libvirt-admin.c
@@ -1086,12 +1086,17 @@ virAdmServerSetClientLimits(virAdmServerPtr srv,
* virAdmServerUpdateTlsFiles:
* @srv: a valid server object reference
* @filetypes: bitwise-OR of virServerTlsFiletype
- * @flags: extra flags; not used yet, so callers should always pass 0
+ * @flags: mode that specifies the update method
*
* Notify server to update tls file, such as cacert, cacrl, server cert / key.
* Mark the files that need to be updated by the @filetypes parameter.
* See virServerTlsFiletype for detailed description of accepted filetypes.
*
+ * @flags specifies how to update server cert/key in tls service,
+ * and is either the value VIR_TLS_UPDATE_APPEND, or VIR_TLS_UPDATE_CLEAR.
+ * The default value is VIR_TLS_UPDATE_CLEAR. See virServerTlsUpdateFlag for
+ * detailed description.
+ *
* Returns 0 if the TLS files have been updated successfully or -1 in case of
an
* error.
*/
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 65ec677d0a..72c4d37bc6 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -1226,7 +1226,8 @@ virNetServerGetTLSContext(virNetServerPtr srv)
return ctxt;
}
-static int virNetServerUpdateTlsFilesCheckParams(unsigned int filetypes)
+static int virNetServerUpdateTlsFilesCheckParams(unsigned int filetypes,
+ unsigned int flags)
{
bool haveSrvCert = filetypes & VIR_TLS_FILE_TYPE_SERVER_CERT;
bool haveSrvKey = filetypes & VIR_TLS_FILE_TYPE_SERVER_KEY;
@@ -1239,12 +1240,20 @@ static int
virNetServerUpdateTlsFilesCheckParams(unsigned int filetypes)
return -1;
}
+if (flags >= VIR_TLS_UPDATE_FLAG_MAX) {
+virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("don not support flags: %d"),
+ flags);
+return -1;
+}
+
return 0;
}
int
virNetServerUpdateTlsFiles(virNetServerPtr srv,
- unsigned int filetypes)
+ unsigned int filetypes,
+ unsigned int flags)
{
int ret = -1;
#ifndef WITH_GNUTLS
@@ -1254,7 +1263,7 @@ virNetServerUpdateTlsFiles(virNetServerPtr srv,
#else
virNetTLSContextPtr ctxt = NULL;
-if (virNetServerUpdateTlsFilesCheckParams(filetypes))
+if (virNetServerUpdateTlsFilesCheckParams(filetypes, flags))
return -1;
virObjectLock(srv);
@@ -1266,7 +1275,7 @@ virNetServerUpdateTlsFiles(virNetServerPtr srv,
goto cleanup;
}
-if (virNetTLSContextReload(ctxt, filetypes)) {
+if (virNetTLSContextReload(ctxt, filetypes, flags)) {
VIR_ERROR(_("reload server's tls context fail"));
goto cleanup;
}
diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h
index 99466dd041..1a905aa483 100644
--- a/src/rpc/virnetserver.h
+++ b
[PATCH 4/6] admin: support server cert update mode
virAdmServerUpdateTlsFiles:
@flags specifies how to update server cert/key in tls service.
Two modes are currently supported: append mode and clear mode, means
whether to clear the original cert then add the new one, or just append
to the original one.
---
include/libvirt/libvirt-admin.h | 14 ++
src/admin/admin_server.c| 7 +--
src/admin/libvirt-admin.c | 7 ++-
src/rpc/virnetserver.c | 17 +
src/rpc/virnetserver.h | 3 ++-
src/rpc/virnettlscontext.c | 7 +--
src/rpc/virnettlscontext.h | 3 ++-
7 files changed, 43 insertions(+), 15 deletions(-)
diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h
index 6e38261129..dfdd81ae83 100644
--- a/include/libvirt/libvirt-admin.h
+++ b/include/libvirt/libvirt-admin.h
@@ -392,6 +392,20 @@ int virAdmClientClose(virAdmClientPtr client, unsigned int
flags);
# define VIR_SERVER_CLIENTS_UNAUTH_CURRENT "nclients_unauth"
+typedef enum {
+/* free old credentials and then set new tls context.
+ */
+VIR_TLS_UPDATE_CLEAR = 0,
+
+/* do not clear original certificates and keys.
+ */
+VIR_TLS_UPDATE_APPEND = 1,
+
+/* boundary value for flag check (unreachable).
+ */
+VIR_TLS_UPDATE_FLAG_MAX = 2,
+} virServerTlsUpdateFlag;
+
/* tls related filetype flags. */
typedef enum {
VIR_TLS_FILE_TYPE_CA_CERT = (1U << 0),
diff --git a/src/admin/admin_server.c b/src/admin/admin_server.c
index 558913367b..43c7e00d90 100644
--- a/src/admin/admin_server.c
+++ b/src/admin/admin_server.c
@@ -373,10 +373,5 @@ adminServerUpdateTlsFiles(virNetServerPtr srv,
unsigned int filetypes,
unsigned int flags)
{
-virCheckFlags(0, -1);
-
-if (virNetServerUpdateTlsFiles(srv, filetypes) < 0)
-return -1;
-
-return 0;
+return virNetServerUpdateTlsFiles(srv, filetypes, flags);
}
diff --git a/src/admin/libvirt-admin.c b/src/admin/libvirt-admin.c
index f3f92ed91c..b6ba72b577 100644
--- a/src/admin/libvirt-admin.c
+++ b/src/admin/libvirt-admin.c
@@ -1086,12 +1086,17 @@ virAdmServerSetClientLimits(virAdmServerPtr srv,
* virAdmServerUpdateTlsFiles:
* @srv: a valid server object reference
* @filetypes: bitwise-OR of virServerTlsFiletype
- * @flags: extra flags; not used yet, so callers should always pass 0
+ * @flags: mode that specifies the update method
*
* Notify server to update tls file, such as cacert, cacrl, server cert / key.
* Mark the files that need to be updated by the @filetypes parameter.
* See virServerTlsFiletype for detailed description of accepted filetypes.
*
+ * @flags specifies how to update server cert/key in tls service,
+ * and is either the value VIR_TLS_UPDATE_APPEND, or VIR_TLS_UPDATE_CLEAR.
+ * The default value is VIR_TLS_UPDATE_CLEAR. See virServerTlsUpdateFlag for
+ * detailed description.
+ *
* Returns 0 if the TLS files have been updated successfully or -1 in case of
an
* error.
*/
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 65ec677d0a..72c4d37bc6 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -1226,7 +1226,8 @@ virNetServerGetTLSContext(virNetServerPtr srv)
return ctxt;
}
-static int virNetServerUpdateTlsFilesCheckParams(unsigned int filetypes)
+static int virNetServerUpdateTlsFilesCheckParams(unsigned int filetypes,
+ unsigned int flags)
{
bool haveSrvCert = filetypes & VIR_TLS_FILE_TYPE_SERVER_CERT;
bool haveSrvKey = filetypes & VIR_TLS_FILE_TYPE_SERVER_KEY;
@@ -1239,12 +1240,20 @@ static int
virNetServerUpdateTlsFilesCheckParams(unsigned int filetypes)
return -1;
}
+if (flags >= VIR_TLS_UPDATE_FLAG_MAX) {
+virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("don not support flags: %d"),
+ flags);
+return -1;
+}
+
return 0;
}
int
virNetServerUpdateTlsFiles(virNetServerPtr srv,
- unsigned int filetypes)
+ unsigned int filetypes,
+ unsigned int flags)
{
int ret = -1;
#ifndef WITH_GNUTLS
@@ -1254,7 +1263,7 @@ virNetServerUpdateTlsFiles(virNetServerPtr srv,
#else
virNetTLSContextPtr ctxt = NULL;
-if (virNetServerUpdateTlsFilesCheckParams(filetypes))
+if (virNetServerUpdateTlsFilesCheckParams(filetypes, flags))
return -1;
virObjectLock(srv);
@@ -1266,7 +1275,7 @@ virNetServerUpdateTlsFiles(virNetServerPtr srv,
goto cleanup;
}
-if (virNetTLSContextReload(ctxt, filetypes)) {
+if (virNetTLSContextReload(ctxt, filetypes, flags)) {
VIR_ERROR(_("reload server's tls context fail"));
goto cleanup;
}
diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h
index 99466dd041..1a905aa483 100644
--- a/src/rpc/virnetserver.h
+++ b/src/rpc
