Re: [PATCH 2/6] virnetserver: Introduce virNetServerUpdateTlsFiles
On Sun, Feb 09, 2020 at 10:03:12PM +0800, Zhang Bo wrote:
> Add an API to update server's tls context before admin method can be
> introduced.
> ---
> include/libvirt/libvirt-admin.h | 8
> src/libvirt_remote.syms | 1 +
> src/rpc/virnetserver.c | 72 +
> src/rpc/virnetserver.h | 3 ++
> src/rpc/virnetserverclient.c| 4 ++
> src/rpc/virnettlscontext.c | 41 +++
> src/rpc/virnettlscontext.h | 2 +
> 7 files changed, 131 insertions(+)
>
> diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h
> index abf2792926..3edc044490 100644
> --- a/include/libvirt/libvirt-admin.h
> +++ b/include/libvirt/libvirt-admin.h
> @@ -392,6 +392,14 @@ int virAdmClientClose(virAdmClientPtr client, unsigned
> int flags);
>
> # define VIR_SERVER_CLIENTS_UNAUTH_CURRENT "nclients_unauth"
>
> +/* tls related filetype flags. */
> +typedef enum {
> +VIR_TLS_FILE_TYPE_CA_CERT = (1U << 0),
> +VIR_TLS_FILE_TYPE_CA_CRL = (1U << 1),
> +VIR_TLS_FILE_TYPE_SERVER_CERT = (1U << 2),
> +VIR_TLS_FILE_TYPE_SERVER_KEY = (1U << 3),
> +} virServerTlsFiletype;
[snip]
> diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
> index 12811bed78..8baa6a15b2 100644
> --- a/src/rpc/virnettlscontext.c
> +++ b/src/rpc/virnettlscontext.c
> @@ -1139,6 +1139,47 @@ void virNetTLSContextDispose(void *obj)
> gnutls_certificate_free_credentials(ctxt->x509cred);
> }
>
> +int virNetTLSContextReload(virNetTLSContextPtr ctxt,
> + unsigned int filetypes)
> +{
> +int ret = -1;
> +char *cacert = NULL;
> +char *cacrl = NULL;
> +char *cert = NULL;
> +char *key = NULL;
> +
> +virObjectLock(ctxt);
> +
> +if (virNetTLSContextLocateCredentials(NULL, false, true,
> + , , , ) < 0)
> +goto cleanup;
> +
> +if (filetypes & VIR_TLS_FILE_TYPE_CA_CERT) {
> +if (virNetTLSContextSetCACert(ctxt, cacert, false))
> +goto cleanup;
> +}
> +
> +if (filetypes & VIR_TLS_FILE_TYPE_CA_CRL) {
> +if (virNetTLSContextSetCACRL(ctxt, cacrl, false))
> +goto cleanup;
> +}
> +
> +if (filetypes & VIR_TLS_FILE_TYPE_SERVER_CERT) {
> +gnutls_certificate_free_keys(ctxt->x509cred);
> +if (virNetTLSContextSetCertAndKey(ctxt, cert, key, false))
> +goto cleanup;
> +}
I'm not convinced we should be doing a selective update of only
a subset of files here.
I feel like an oneline update should always have the exact same
result as you would get by doing a restart of libvirtd.
Consider if you update the CA cert, CA CRL and Server Cert
on disk, but then tell libvirtd to only reload Server Cert.
The state of libvirtd is now out of sync with state on disk,
and when libvirtd gets restarted due to a RPM software upgrade
later, its will load different content again.
This can lead to hard to diagnose problems, or delayed discovery
of problems.
The second is is that we're modifying the existing "x509cred"
object in virNetTLSContext.
gnutls_certificate_free_keys(ctxt->x509cred);
if (virNetTLSContextSetCertAndKey(ctxt, cert, key, false))
goto cleanup;
Consider if virNetTLSContextSetCertAndKey() here fails - now libvirtd
is missing the original TLS cert/key, and also missing the new TLS
cert/key.
When we're reloading certs, I think we need to create an entirely
new gnutls_certificate_credentials_t object, and populate it from
all the files on disk.
Only once that is succesful, should we then replace the "x509cred"
object in the virNetTLSContext. This gives us an atomic update
path, so we're guaranteed to always have valid credentials
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 2/6] virnetserver: Introduce virNetServerUpdateTlsFiles
Zhangbo (Oscar) 将撤回邮件“[PATCH 2/6] virnetserver: Introduce virNetServerUpdateTlsFiles”。
[PATCH 2/6] virnetserver: Introduce virNetServerUpdateTlsFiles
Add an API to update server's tls context before admin method can be
introduced.
---
include/libvirt/libvirt-admin.h | 8
src/libvirt_remote.syms | 1 +
src/rpc/virnetserver.c | 72 +
src/rpc/virnetserver.h | 3 ++
src/rpc/virnetserverclient.c| 4 ++
src/rpc/virnettlscontext.c | 41 +++
src/rpc/virnettlscontext.h | 2 +
7 files changed, 131 insertions(+)
diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h
index abf2792926..3edc044490 100644
--- a/include/libvirt/libvirt-admin.h
+++ b/include/libvirt/libvirt-admin.h
@@ -392,6 +392,14 @@ int virAdmClientClose(virAdmClientPtr client, unsigned int
flags);
# define VIR_SERVER_CLIENTS_UNAUTH_CURRENT "nclients_unauth"
+/* tls related filetype flags. */
+typedef enum {
+VIR_TLS_FILE_TYPE_CA_CERT = (1U << 0),
+VIR_TLS_FILE_TYPE_CA_CRL = (1U << 1),
+VIR_TLS_FILE_TYPE_SERVER_CERT = (1U << 2),
+VIR_TLS_FILE_TYPE_SERVER_KEY = (1U << 3),
+} virServerTlsFiletype;
+
int virAdmServerGetClientLimits(virAdmServerPtr srv,
virTypedParameterPtr *params,
int *nparams,
diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 0493467f46..0018a0c41d 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -137,6 +137,7 @@ virNetServerSetClientLimits;
virNetServerSetThreadPoolParameters;
virNetServerSetTLSContext;
virNetServerUpdateServices;
+virNetServerUpdateTlsFiles;
# rpc/virnetserverclient.h
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index c87dade1a8..65ec677d0a 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -1207,3 +1207,75 @@ virNetServerSetClientLimits(virNetServerPtr srv,
virObjectUnlock(srv);
return ret;
}
+
+static virNetTLSContextPtr
+virNetServerGetTLSContext(virNetServerPtr srv)
+{
+size_t i;
+virNetTLSContextPtr ctxt = NULL;
+virNetServerServicePtr svc = NULL;
+
+/* find svcTLS from srv, get svcTLS->tls */
+for (i = 0; i < srv->nservices; i++) {
+svc = srv->services[i];
+ctxt = virNetServerServiceGetTLSContext(svc);
+if (ctxt != NULL)
+break;
+}
+
+return ctxt;
+}
+
+static int virNetServerUpdateTlsFilesCheckParams(unsigned int filetypes)
+{
+bool haveSrvCert = filetypes & VIR_TLS_FILE_TYPE_SERVER_CERT;
+bool haveSrvKey = filetypes & VIR_TLS_FILE_TYPE_SERVER_KEY;
+
+if ((haveSrvCert && !haveSrvKey) ||
+(!haveSrvCert && haveSrvKey)) {
+virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("server cert/key must be updated together. "
+ "filetypes: %d"), filetypes);
+return -1;
+}
+
+return 0;
+}
+
+int
+virNetServerUpdateTlsFiles(virNetServerPtr srv,
+ unsigned int filetypes)
+{
+int ret = -1;
+#ifndef WITH_GNUTLS
+virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("Don't support GNUTLS, can't to update filetypes: %d"),
+ filetypes);
+#else
+virNetTLSContextPtr ctxt = NULL;
+
+if (virNetServerUpdateTlsFilesCheckParams(filetypes))
+return -1;
+
+virObjectLock(srv);
+
+ctxt = virNetServerGetTLSContext(srv);
+if (!ctxt) {
+VIR_ERROR(_("no tls svc found, can't to update filetypes: %d"),
+ filetypes);
+goto cleanup;
+}
+
+if (virNetTLSContextReload(ctxt, filetypes)) {
+VIR_ERROR(_("reload server's tls context fail"));
+goto cleanup;
+}
+
+VIR_INFO("update all tls files complete, filetypes: %d", filetypes);
+ret = 0;
+
+ cleanup:
+virObjectUnlock(srv);
+#endif
+return ret;
+}
diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h
index 260c99b22d..99466dd041 100644
--- a/src/rpc/virnetserver.h
+++ b/src/rpc/virnetserver.h
@@ -133,3 +133,6 @@ size_t virNetServerGetCurrentUnauthClients(virNetServerPtr
srv);
int virNetServerSetClientLimits(virNetServerPtr srv,
long long int maxClients,
long long int maxClientsUnauth);
+
+int virNetServerUpdateTlsFiles(virNetServerPtr srv,
+ unsigned int filetypes);
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 67b3bf9531..f0952cadde 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -1117,7 +1117,9 @@ int virNetServerClientInit(virNetServerClientPtr client)
client->tls);
/* Begin the TLS handshake. */
+virObjectLock(client->tlsCtxt);
ret = virNetTLSSessionHandshake(client->tls);
+virObjectUnlock(client->tlsCtxt);
if (ret == 0) {
/* Unlikely, but ... Next step is to check the certificate. */
if (virNetServerClientCheckAccess(client) < 0)
@@
[PATCH 2/6] virnetserver: Introduce virNetServerUpdateTlsFiles
Add an API to update server's tls context before admin method can be
introduced.
---
include/libvirt/libvirt-admin.h | 8
src/libvirt_remote.syms | 1 +
src/rpc/virnetserver.c | 72 +
src/rpc/virnetserver.h | 3 ++
src/rpc/virnetserverclient.c| 4 ++
src/rpc/virnettlscontext.c | 41 +++
src/rpc/virnettlscontext.h | 2 +
7 files changed, 131 insertions(+)
diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h
index abf2792926..3edc044490 100644
--- a/include/libvirt/libvirt-admin.h
+++ b/include/libvirt/libvirt-admin.h
@@ -392,6 +392,14 @@ int virAdmClientClose(virAdmClientPtr client, unsigned int
flags);
# define VIR_SERVER_CLIENTS_UNAUTH_CURRENT "nclients_unauth"
+/* tls related filetype flags. */
+typedef enum {
+VIR_TLS_FILE_TYPE_CA_CERT = (1U << 0),
+VIR_TLS_FILE_TYPE_CA_CRL = (1U << 1),
+VIR_TLS_FILE_TYPE_SERVER_CERT = (1U << 2),
+VIR_TLS_FILE_TYPE_SERVER_KEY = (1U << 3),
+} virServerTlsFiletype;
+
int virAdmServerGetClientLimits(virAdmServerPtr srv,
virTypedParameterPtr *params,
int *nparams,
diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 0493467f46..0018a0c41d 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -137,6 +137,7 @@ virNetServerSetClientLimits;
virNetServerSetThreadPoolParameters;
virNetServerSetTLSContext;
virNetServerUpdateServices;
+virNetServerUpdateTlsFiles;
# rpc/virnetserverclient.h
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index c87dade1a8..65ec677d0a 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -1207,3 +1207,75 @@ virNetServerSetClientLimits(virNetServerPtr srv,
virObjectUnlock(srv);
return ret;
}
+
+static virNetTLSContextPtr
+virNetServerGetTLSContext(virNetServerPtr srv)
+{
+size_t i;
+virNetTLSContextPtr ctxt = NULL;
+virNetServerServicePtr svc = NULL;
+
+/* find svcTLS from srv, get svcTLS->tls */
+for (i = 0; i < srv->nservices; i++) {
+svc = srv->services[i];
+ctxt = virNetServerServiceGetTLSContext(svc);
+if (ctxt != NULL)
+break;
+}
+
+return ctxt;
+}
+
+static int virNetServerUpdateTlsFilesCheckParams(unsigned int filetypes)
+{
+bool haveSrvCert = filetypes & VIR_TLS_FILE_TYPE_SERVER_CERT;
+bool haveSrvKey = filetypes & VIR_TLS_FILE_TYPE_SERVER_KEY;
+
+if ((haveSrvCert && !haveSrvKey) ||
+(!haveSrvCert && haveSrvKey)) {
+virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("server cert/key must be updated together. "
+ "filetypes: %d"), filetypes);
+return -1;
+}
+
+return 0;
+}
+
+int
+virNetServerUpdateTlsFiles(virNetServerPtr srv,
+ unsigned int filetypes)
+{
+int ret = -1;
+#ifndef WITH_GNUTLS
+virReportError(VIR_ERR_SYSTEM_ERROR,
+ _("Don't support GNUTLS, can't to update filetypes: %d"),
+ filetypes);
+#else
+virNetTLSContextPtr ctxt = NULL;
+
+if (virNetServerUpdateTlsFilesCheckParams(filetypes))
+return -1;
+
+virObjectLock(srv);
+
+ctxt = virNetServerGetTLSContext(srv);
+if (!ctxt) {
+VIR_ERROR(_("no tls svc found, can't to update filetypes: %d"),
+ filetypes);
+goto cleanup;
+}
+
+if (virNetTLSContextReload(ctxt, filetypes)) {
+VIR_ERROR(_("reload server's tls context fail"));
+goto cleanup;
+}
+
+VIR_INFO("update all tls files complete, filetypes: %d", filetypes);
+ret = 0;
+
+ cleanup:
+virObjectUnlock(srv);
+#endif
+return ret;
+}
diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h
index 260c99b22d..99466dd041 100644
--- a/src/rpc/virnetserver.h
+++ b/src/rpc/virnetserver.h
@@ -133,3 +133,6 @@ size_t virNetServerGetCurrentUnauthClients(virNetServerPtr
srv);
int virNetServerSetClientLimits(virNetServerPtr srv,
long long int maxClients,
long long int maxClientsUnauth);
+
+int virNetServerUpdateTlsFiles(virNetServerPtr srv,
+ unsigned int filetypes);
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 67b3bf9531..f0952cadde 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -1117,7 +1117,9 @@ int virNetServerClientInit(virNetServerClientPtr client)
client->tls);
/* Begin the TLS handshake. */
+virObjectLock(client->tlsCtxt);
ret = virNetTLSSessionHandshake(client->tls);
+virObjectUnlock(client->tlsCtxt);
if (ret == 0) {
/* Unlikely, but ... Next step is to check the certificate. */
if (virNetServerClientCheckAccess(client) < 0)
@@ -1438,7
