[PATCH 0/6] Introduce OpenSSH authorized key file mgmt APIs
Marc-André posted a patch that implements agent handling. I've written the rest. Marc-André Lureau (1): qemu_agent: add qemuAgentSSH{Add,Remove,Get}AuthorizedKeys Michal Prívozník (5): Introduce OpenSSH authorized key file mgmt APIs remote: Implement OpenSSH authorized key file mgmt APIs virsh: Expose OpenSSH authorized key file mgmt APIs qemu: Implement OpenSSH authorized key file mgmt APIs news: Document recent OpenSSH authorized key file mgmt APIs NEWS.rst | 6 ++ docs/manpages/virsh.rst | 37 +++++++ include/libvirt/libvirt-domain.h | 17 ++++ src/driver-hypervisor.h | 15 +++ src/libvirt-domain.c | 115 +++++++++++++++++++++ src/libvirt_public.syms | 6 ++ src/qemu/qemu_agent.c | 140 +++++++++++++++++++++++++ src/qemu/qemu_agent.h | 15 +++ src/qemu/qemu_driver.c | 81 +++++++++++++++ src/remote/remote_daemon_dispatch.c | 82 +++++++++++++++ src/remote/remote_driver.c | 87 ++++++++++++++++ src/remote/remote_protocol.x | 34 ++++++- src/remote_protocol-structs | 22 ++++ tests/qemuagenttest.c | 79 +++++++++++++++ tools/virsh-domain.c | 152 ++++++++++++++++++++++++++++ 15 files changed, 887 insertions(+), 1 deletion(-) -- 2.26.2
When setting up a new guest or when a management software wants to allow access to an existing guest the virDomainSetUserPassword() API can be used, but that might be not good enough if user want to ssh into the guest. Not only sshd has to be configured to accept password authentication (which is usually not the case for root), user have to type in their password. Using SSH keys is more convenient. Therefore, two new APIs are introduced: virDomainAuthorizedSSHKeysGet() which lists authorized keys for given user, and virDomainAuthorizedSSHKeysSet() which modifies the authorized keys file for given user (append, set or remove keys from the file). It's worth nothing that while authorized_keys file entries have some structure (as defined by sshd(8)), expressing that structure goes beyond libvirt's focus and thus "keys" are nothing but an opaque string to libvirt. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt-domain.h | 17 +++++ src/driver-hypervisor.h | 15 ++++ src/libvirt-domain.c | 115 +++++++++++++++++++++++++++++++ src/libvirt_public.syms | 6 ++ 4 files changed, 153 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index b3310729bf..60a7ddefa8 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -5096,4 +5096,21 @@ int virDomainBackupBegin(virDomainPtr domain, char *virDomainBackupGetXMLDesc(virDomainPtr domain, unsigned int flags); +int virDomainAuthorizedSSHKeysGet(virDomainPtr domain, + const char *user, + char ***keys, + unsigned int flags); + +typedef enum { + VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND = (1 << 0), /* don't truncate file, just append */ + VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE = (1 << 1), /* remove keys, instead of adding them */ + +} virDomainAuthorizedSSHKeysSetFlags; + +int virDomainAuthorizedSSHKeysSet(virDomainPtr domain, + const char *user, + const char **keys, + int nkeys, + unsigned int flags); + #endif /* LIBVIRT_DOMAIN_H */ diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index bce023017d..5a5ea95c51 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -1387,6 +1387,19 @@ typedef char * (*virDrvDomainBackupGetXMLDesc)(virDomainPtr domain, unsigned int flags); +typedef int +(*virDrvDomainAuthorizedSSHKeysGet)(virDomainPtr domain, + const char *user, + char ***keys, + unsigned int flags); + +typedef int +(*virDrvDomainAuthorizedSSHKeysSet)(virDomainPtr domain, + const char *user, + const char **keys, + int nkeys, + unsigned int flags); + typedef struct _virHypervisorDriver virHypervisorDriver; typedef virHypervisorDriver *virHypervisorDriverPtr; @@ -1650,4 +1663,6 @@ struct _virHypervisorDriver { virDrvDomainAgentSetResponseTimeout domainAgentSetResponseTimeout; virDrvDomainBackupBegin domainBackupBegin; virDrvDomainBackupGetXMLDesc domainBackupGetXMLDesc; + virDrvDomainAuthorizedSSHKeysGet domainAuthorizedSSHKeysGet; + virDrvDomainAuthorizedSSHKeysSet domainAuthorizedSSHKeysSet; }; diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 3c5f55176a..0a55a48952 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -12758,3 +12758,118 @@ virDomainBackupGetXMLDesc(virDomainPtr domain, virDispatchError(conn); return NULL; } + + +/** + * virDomainAuthorizedSSHKeysGet: + * @domain: a domain object + * @user: user to list keys for + * @keys: pointer to a variable to store authorized keys + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * For given @user in @domain fetch list of public SSH authorized + * keys and store them into @keys array which is allocated upon + * successful return. The caller is responsible for freeing @keys + * when no longer needed. + * + * Keys are in OpenSSH format (see sshd(8)) but from libvirt's + * point of view are opaque strings, i.e. not interpreted. + * + * Returns: number of keys stored in @keys, + * -1 otherwise. + */ +int virDomainAuthorizedSSHKeysGet(virDomainPtr domain, + const char *user, + char ***keys, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "user=%s, keys=%p, flags=0x%x", + user, keys, flags); + + virResetLastError(); + + virCheckDomainReturn(domain, -1); + virCheckNonNullArgReturn(user, -1); + virCheckNonNullArgReturn(keys, -1); + conn = domain->conn; + + if (conn->driver->domainAuthorizedSSHKeysGet) { + int ret; + ret = conn->driver->domainAuthorizedSSHKeysGet(domain, user, + keys, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + error: + virDispatchError(conn); + return -1; +} + + +/** + * virDomainAuthorizedSSHKeysSet: + * @domain: a domain object + * @user: user to add keys for + * @keys: authorized keys to set + * @nkeys: number of keys in @keys array + * @flags: bitwise or of virDomainAuthorizedSSHKeysSetFlags + * + * For given @user in @domain set @keys in authorized keys file. + * Any previous content of the file is overwritten with new keys. + * + * Keys are in OpenSSH format (see sshd(8)) but from libvirt's + * point of view are opaque strings, i.e. not interpreted. + * + * If VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND flag is set + * then the file is not overwritten and new @keys are appended + * instead. + * + * If VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE flag is set then + * instead of adding any new keys, provided @keys are removed + * from the file. It's not considered error if the key doesn't + * exist. + * + * Returns: 0 on success, + * -1 otherwise. + */ +int virDomainAuthorizedSSHKeysSet(virDomainPtr domain, + const char *user, + const char **keys, + int nkeys, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "user=%s, keys=%p, nkeys=%d, flags=0x%x", + user, keys, nkeys, flags); + + virResetLastError(); + + virCheckDomainReturn(domain, -1); + virCheckNonNullArgReturn(user, -1); + + VIR_EXCLUSIVE_FLAGS_RET(VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND, + VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE, + -1); + + conn = domain->conn; + + if (conn->driver->domainAuthorizedSSHKeysSet) { + int ret; + ret = conn->driver->domainAuthorizedSSHKeysSet(domain, user, + keys, nkeys, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + error: + virDispatchError(conn); + return -1; +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 539d2e3943..cf31f937d5 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -873,4 +873,10 @@ LIBVIRT_6.0.0 { virDomainBackupGetXMLDesc; } LIBVIRT_5.10.0; +LIBVIRT_6.10.0 { + global: + virDomainAuthorizedSSHKeysGet; + virDomainAuthorizedSSHKeysSet; +} LIBVIRT_6.0.0; + # .... define new API here using predicted next version number .... -- 2.26.2
On Tue, Nov 10, 2020 at 16:11:41 +0100, Michal Privoznik wrote:
When setting up a new guest or when a management software wants to allow access to an existing guest the virDomainSetUserPassword() API can be used, but that might be not good enough if user want to ssh into the guest. Not only sshd has
Doesn't management software already run something inside the VM which does all of this?
to be configured to accept password authentication (which is usually not the case for root), user have to type in their password. Using SSH keys is more convenient. Therefore, two new APIs are introduced:
virDomainAuthorizedSSHKeysGet() which lists authorized keys for given user, and
virDomainAuthorizedSSHKeysSet() which modifies the authorized keys file for given user (append, set or remove keys from the file).
It's worth nothing that while authorized_keys file entries have some structure (as defined by sshd(8)), expressing that structure goes beyond libvirt's focus and thus "keys" are nothing but an opaque string to libvirt.
To be fair, I surely hope that the qemu-guest-agent feature is disabled by default. This seems a bit scary to me.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt-domain.h | 17 +++++ src/driver-hypervisor.h | 15 ++++ src/libvirt-domain.c | 115 +++++++++++++++++++++++++++++++ src/libvirt_public.syms | 6 ++ 4 files changed, 153 insertions(+)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 3c5f55176a..0a55a48952 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -12758,3 +12758,118 @@ virDomainBackupGetXMLDesc(virDomainPtr domain, virDispatchError(conn); return NULL; } + + +/** + * virDomainAuthorizedSSHKeysGet: + * @domain: a domain object + * @user: user to list keys for + * @keys: pointer to a variable to store authorized keys + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * For given @user in @domain fetch list of public SSH authorized + * keys and store them into @keys array which is allocated upon + * successful return. The caller is responsible for freeing @keys + * when no longer needed. + * + * Keys are in OpenSSH format (see sshd(8)) but from libvirt's + * point of view are opaque strings, i.e. not interpreted.
Missing mention that hypervisor may require use of guest-agent.
+ * + * Returns: number of keys stored in @keys, + * -1 otherwise. + */ +int virDomainAuthorizedSSHKeysGet(virDomainPtr domain, + const char *user, + char ***keys, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "user=%s, keys=%p, flags=0x%x", + user, keys, flags); + + virResetLastError(); + + virCheckDomainReturn(domain, -1); + virCheckNonNullArgReturn(user, -1); + virCheckNonNullArgReturn(keys, -1); + conn = domain->conn;
This API IMO _must_ use 'virCheckReadOnlyGoto(conn->flags, error);' read-only users should not be able to access the guest agent for security reasons and also getting the list of authorized keys may actually leak somewhat sensitive data (allowing identification of the user)
+ + if (conn->driver->domainAuthorizedSSHKeysGet) { + int ret; + ret = conn->driver->domainAuthorizedSSHKeysGet(domain, user, + keys, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + error: + virDispatchError(conn); + return -1; +} + + +/** + * virDomainAuthorizedSSHKeysSet: + * @domain: a domain object + * @user: user to add keys for + * @keys: authorized keys to set + * @nkeys: number of keys in @keys array + * @flags: bitwise or of virDomainAuthorizedSSHKeysSetFlags + * + * For given @user in @domain set @keys in authorized keys file. + * Any previous content of the file is overwritten with new keys. + * + * Keys are in OpenSSH format (see sshd(8)) but from libvirt's + * point of view are opaque strings, i.e. not interpreted. + * + * If VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND flag is set + * then the file is not overwritten and new @keys are appended + * instead. + * + * If VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE flag is set then + * instead of adding any new keys, provided @keys are removed + * from the file. It's not considered error if the key doesn't + * exist.
Missing mention that guest agent is needed.
+ * + * Returns: 0 on success, + * -1 otherwise. + */ +int virDomainAuthorizedSSHKeysSet(virDomainPtr domain, + const char *user, + const char **keys, + int nkeys, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "user=%s, keys=%p, nkeys=%d, flags=0x%x", + user, keys, nkeys, flags); + + virResetLastError(); + + virCheckDomainReturn(domain, -1); + virCheckNonNullArgReturn(user, -1);
This API definitely _must_ use 'virCheckReadOnlyGoto(conn->flags, error);' You don't want random read-only users installing keys. Also according to the description you can enforce that keys is non-NULL if VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE is not provided.
+ + VIR_EXCLUSIVE_FLAGS_RET(VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND, + VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE, + -1); + + conn = domain->conn; + + if (conn->driver->domainAuthorizedSSHKeysSet) { + int ret; + ret = conn->driver->domainAuthorizedSSHKeysSet(domain, user, + keys, nkeys, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + error: + virDispatchError(conn); + return -1; +}
On 11/11/20 9:06 AM, Peter Krempa wrote:
On Tue, Nov 10, 2020 at 16:11:41 +0100, Michal Privoznik wrote:
When setting up a new guest or when a management software wants to allow access to an existing guest the virDomainSetUserPassword() API can be used, but that might be not good enough if user want to ssh into the guest. Not only sshd has
Doesn't management software already run something inside the VM which does all of this?
Not really. oVirt does, Kubevirt doesn't.
to be configured to accept password authentication (which is usually not the case for root), user have to type in their password. Using SSH keys is more convenient. Therefore, two new APIs are introduced:
virDomainAuthorizedSSHKeysGet() which lists authorized keys for given user, and
virDomainAuthorizedSSHKeysSet() which modifies the authorized keys file for given user (append, set or remove keys from the file).
It's worth nothing that while authorized_keys file entries have some structure (as defined by sshd(8)), expressing that structure goes beyond libvirt's focus and thus "keys" are nothing but an opaque string to libvirt.
To be fair, I surely hope that the qemu-guest-agent feature is disabled by default. This seems a bit scary to me.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt-domain.h | 17 +++++ src/driver-hypervisor.h | 15 ++++ src/libvirt-domain.c | 115 +++++++++++++++++++++++++++++++ src/libvirt_public.syms | 6 ++ 4 files changed, 153 insertions(+)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 3c5f55176a..0a55a48952 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -12758,3 +12758,118 @@ virDomainBackupGetXMLDesc(virDomainPtr domain, virDispatchError(conn); return NULL; } + + +/** + * virDomainAuthorizedSSHKeysGet: + * @domain: a domain object + * @user: user to list keys for + * @keys: pointer to a variable to store authorized keys + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * For given @user in @domain fetch list of public SSH authorized + * keys and store them into @keys array which is allocated upon + * successful return. The caller is responsible for freeing @keys + * when no longer needed. + * + * Keys are in OpenSSH format (see sshd(8)) but from libvirt's + * point of view are opaque strings, i.e. not interpreted.
Missing mention that hypervisor may require use of guest-agent.
+ * + * Returns: number of keys stored in @keys, + * -1 otherwise. + */ +int virDomainAuthorizedSSHKeysGet(virDomainPtr domain, + const char *user, + char ***keys, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "user=%s, keys=%p, flags=0x%x", + user, keys, flags); + + virResetLastError(); + + virCheckDomainReturn(domain, -1); + virCheckNonNullArgReturn(user, -1); + virCheckNonNullArgReturn(keys, -1); + conn = domain->conn;
This API IMO _must_ use 'virCheckReadOnlyGoto(conn->flags, error);'
read-only users should not be able to access the guest agent for security reasons and also getting the list of authorized keys may actually leak somewhat sensitive data (allowing identification of the user)
Ah, good point. I keep forgetting about RO connection. Michal
Since both APIs accept/return an array of strings we can't have client/server dispatch code generated. But implementation is fairly trivial, although verbose. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/remote/remote_daemon_dispatch.c | 82 +++++++++++++++++++++++++++ src/remote/remote_driver.c | 87 +++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 34 ++++++++++- src/remote_protocol-structs | 22 ++++++++ 4 files changed, 224 insertions(+), 1 deletion(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index eb5f6ebb0c..46683aa4a7 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -7381,3 +7381,85 @@ remoteDispatchDomainGetGuestInfo(virNetServerPtr server G_GNUC_UNUSED, return rv; } + +static int +remoteDispatchDomainAuthorizedSshKeysGet(virNetServerPtr server G_GNUC_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg G_GNUC_UNUSED, + virNetMessageErrorPtr rerr, + remote_domain_authorized_ssh_keys_get_args *args, + remote_domain_authorized_ssh_keys_get_ret *ret) +{ + int rv = -1; + virConnectPtr conn = remoteGetHypervisorConn(client); + int nkeys = 0; + char **keys = NULL; + virDomainPtr dom = NULL; + + if (!conn) + goto cleanup; + + if (!(dom = get_nonnull_domain(conn, args->dom))) + goto cleanup; + + if ((nkeys = virDomainAuthorizedSSHKeysGet(dom, args->user, + &keys, args->flags)) < 0) + goto cleanup; + + if (nkeys > REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Number of keys %d, which exceeds max liit: %d"), + nkeys, REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX); + goto cleanup; + } + + ret->keys.keys_val = g_steal_pointer(&keys); + ret->keys.keys_len = nkeys; + + rv = nkeys; + + cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (nkeys > 0) + virStringListFreeCount(keys, nkeys); + virObjectUnref(dom); + + return rv; +} + +static int +remoteDispatchDomainAuthorizedSshKeysSet(virNetServerPtr server G_GNUC_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg G_GNUC_UNUSED, + virNetMessageErrorPtr rerr, + remote_domain_authorized_ssh_keys_set_args *args) +{ + int rv = -1; + virConnectPtr conn = remoteGetHypervisorConn(client); + virDomainPtr dom = NULL; + + if (!conn) + goto cleanup; + + if (!(dom = get_nonnull_domain(conn, args->dom))) + goto cleanup; + + if (args->keys.keys_len > REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Number of keys %d, which exceeds max liit: %d"), + args->keys.keys_len, REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX); + goto cleanup; + } + + rv = virDomainAuthorizedSSHKeysSet(dom, args->user, + (const char **) args->keys.keys_val, + args->keys.keys_len, args->flags); + + cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + virObjectUnref(dom); + + return rv; +} diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 9cd2fd36ae..0b8d1e753f 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8027,6 +8027,91 @@ remoteDomainGetGuestInfo(virDomainPtr dom, return rv; } +static int +remoteDomainAuthorizedSSHKeysGet(virDomainPtr domain, + const char *user, + char ***keys, + unsigned int flags) +{ + int rv = -1; + size_t i; + struct private_data *priv = domain->conn->privateData; + remote_domain_authorized_ssh_keys_get_args args; + remote_domain_authorized_ssh_keys_get_ret ret; + + remoteDriverLock(priv); + + make_nonnull_domain(&args.dom, domain); + args.user = (char *) user; + args.flags = flags; + memset(&ret, 0, sizeof(ret)); + + if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_GET, + (xdrproc_t) xdr_remote_domain_authorized_ssh_keys_get_args, (char *)&args, + (xdrproc_t) xdr_remote_domain_authorized_ssh_keys_get_ret, (char *)&ret) == -1) { + goto cleanup; + } + + if (ret.keys.keys_len > REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX) { + virReportError(VIR_ERR_RPC, "%s", + _("remoteDomainAuthorizedSSHKeysGet: " + "returned number of keys exceeds limit")); + goto cleanup; + } + + *keys = g_new0(char *, ret.keys.keys_len); + for (i = 0; i < ret.keys.keys_len; i++) + (*keys)[i] = g_strdup(ret.keys.keys_val[i]); + + rv = ret.keys.keys_len; + + cleanup: + remoteDriverUnlock(priv); + xdr_free((xdrproc_t)xdr_remote_domain_authorized_ssh_keys_get_ret, + (char *) &ret); + return rv; +} + +static int +remoteDomainAuthorizedSSHKeysSet(virDomainPtr domain, + const char *user, + const char **keys, + int nkeys, + unsigned int flags) +{ + int rv = -1; + struct private_data *priv = domain->conn->privateData; + remote_domain_authorized_ssh_keys_set_args args; + + remoteDriverLock(priv); + + if (nkeys > REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX) { + virReportError(VIR_ERR_RPC, "%s", + _("remoteDomainAuthorizedSSHKeysSet: " + "returned number of keys exceeds limit")); + goto cleanup; + } + + make_nonnull_domain(&args.dom, domain); + args.user = (char *) user; + args.keys.keys_len = nkeys; + args.keys.keys_val = (char **) keys; + args.flags = flags; + + if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_SET, + (xdrproc_t) xdr_remote_domain_authorized_ssh_keys_set_args, (char *)&args, + (xdrproc_t) xdr_void, (char *) NULL) == -1) { + goto cleanup; + } + + rv = 0; + + cleanup: + remoteDriverUnlock(priv); + return rv; +} + + /* get_nonnull_domain and get_nonnull_network turn an on-wire * (name, uuid) pair into virDomainPtr or virNetworkPtr object. * These can return NULL if underlying memory allocations fail, @@ -8458,6 +8543,8 @@ static virHypervisorDriver hypervisor_driver = { .domainAgentSetResponseTimeout = remoteDomainAgentSetResponseTimeout, /* 5.10.0 */ .domainBackupBegin = remoteDomainBackupBegin, /* 6.0.0 */ .domainBackupGetXMLDesc = remoteDomainBackupGetXMLDesc, /* 6.0.0 */ + .domainAuthorizedSSHKeysGet = remoteDomainAuthorizedSSHKeysGet, /* 6.10.0 */ + .domainAuthorizedSSHKeysSet = remoteDomainAuthorizedSSHKeysSet, /* 6.10.0 */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 5e5e781e76..89ecc832ff 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -280,6 +280,9 @@ const REMOTE_DOMAIN_GUEST_INFO_PARAMS_MAX = 2048; */ const REMOTE_NETWORK_PORT_PARAMETERS_MAX = 16; +/* Upper limit on number of SSH keys */ +const REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX = 2048; + /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ typedef opaque remote_uuid[VIR_UUID_BUFLEN]; @@ -3779,6 +3782,23 @@ struct remote_domain_backup_get_xml_desc_ret { remote_nonnull_string xml; }; +struct remote_domain_authorized_ssh_keys_get_args { + remote_nonnull_domain dom; + remote_nonnull_string user; + unsigned int flags; +}; + +struct remote_domain_authorized_ssh_keys_get_ret { + remote_nonnull_string keys<REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX>; +}; + +struct remote_domain_authorized_ssh_keys_set_args { + remote_nonnull_domain dom; + remote_nonnull_string user; + remote_nonnull_string keys<REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX>; + unsigned int flags; +}; + /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -6682,5 +6702,17 @@ enum remote_procedure { * @generate: both * @acl: none */ - REMOTE_PROC_DOMAIN_EVENT_MEMORY_FAILURE = 423 + REMOTE_PROC_DOMAIN_EVENT_MEMORY_FAILURE = 423, + + /** + * @generate: none + * @acl: domain:read + */ + REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_GET = 424, + + /** + * @generate: none + * @acl: domain:write + */ + REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_SET = 425 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index c2ae411885..9bcd14603d 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -3142,6 +3142,26 @@ struct remote_domain_backup_get_xml_desc_args { struct remote_domain_backup_get_xml_desc_ret { remote_nonnull_string xml; }; +struct remote_domain_authorized_ssh_keys_get_args { + remote_nonnull_domain dom; + remote_nonnull_string user; + u_int flags; +}; +struct remote_domain_authorized_ssh_keys_get_ret { + struct { + u_int keys_len; + remote_nonnull_string * keys_val; + } keys; +}; +struct remote_domain_authorized_ssh_keys_set_args { + remote_nonnull_domain dom; + remote_nonnull_string user; + struct { + u_int keys_len; + remote_nonnull_string * keys_val; + } keys; + u_int flags; +}; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, REMOTE_PROC_CONNECT_CLOSE = 2, @@ -3566,4 +3586,6 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_BACKUP_BEGIN = 421, REMOTE_PROC_DOMAIN_BACKUP_GET_XML_DESC = 422, REMOTE_PROC_DOMAIN_EVENT_MEMORY_FAILURE = 423, + REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_GET = 424, + REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_SET = 425, }; -- 2.26.2
On Tue, Nov 10, 2020 at 16:11:42 +0100, Michal Privoznik wrote:
Since both APIs accept/return an array of strings we can't have client/server dispatch code generated. But implementation is fairly trivial, although verbose.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/remote/remote_daemon_dispatch.c | 82 +++++++++++++++++++++++++++ src/remote/remote_driver.c | 87 +++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 34 ++++++++++- src/remote_protocol-structs | 22 ++++++++ 4 files changed, 224 insertions(+), 1 deletion(-)
[...]
static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 5e5e781e76..89ecc832ff 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x
[...]
@@ -6682,5 +6702,17 @@ enum remote_procedure { * @generate: both * @acl: none */ - REMOTE_PROC_DOMAIN_EVENT_MEMORY_FAILURE = 423 + REMOTE_PROC_DOMAIN_EVENT_MEMORY_FAILURE = 423, + + /** + * @generate: none + * @acl: domain:read
We mandate domain:write for anything touching the guest agent.
+ */ + REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_GET = 424, + + /** + * @generate: none + * @acl: domain:write + */ + REMOTE_PROC_DOMAIN_AUTHORIZED_SSH_KEYS_SET = 425 };
The new virsh commands are: get-user-sshkeys set-user-sshkeys Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/manpages/virsh.rst | 37 ++++++++++ tools/virsh-domain.c | 152 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 189 insertions(+) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index bfd26e3120..30b234aeab 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -2636,6 +2636,21 @@ When *--timestamp* is used, a human-readable timestamp will be printed before the event. +get-user-sshkeys +---------------- + +**Syntax:** + +:: + + get-user-sshkeys domain user + +Print SSH authorized keys for given *user* in the guest *domain*. Please note, +that an entry in the file has internal structure as defined by *sshd(8)* and +virsh/libvirt does handle keys as opaque strings, i.e. does not interpret +them. + + guest-agent-timeout ------------------- @@ -4004,6 +4019,28 @@ For QEMU/KVM, this requires the guest agent to be configured and running. +set-user-sshkeys +---------------- + +**Syntax:** + +:: + + set-user-sshkeys domain user [{--append | --remove}] [--keys keys] + +Replace the contents of *user*'s SSH authorized file in the guest *domain* with +*keys*. Please note, that an entry in the file has internal structure as +defined by *sshd(8)* and virsh/libvirt does handle keys as opaque strings, +i.e. does not interpret them. + +If *--append* is specified, then the file content is not replaced and new keys +are just appended. + +If *--remove* is specified, then instead of adding any new keys these are +removed from the file. It is not considered an error if the key does not exist +in the file. + + setmaxmem --------- diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 1ae936c6b2..f51765cb42 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -14259,6 +14259,146 @@ cmdGuestInfo(vshControl *ctl, const vshCmd *cmd) return ret; } +/* + * "get-user-sshkeys" command + */ +static const vshCmdInfo info_get_user_sshkeys[] = { + {.name = "help", + .data = N_("list authorized SSH keys for given user (via agent)") + }, + {.name = "desc", + .data = N_("Use the guest agent to query authorized SSH keys for given " + "user") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_get_user_sshkeys[] = { + VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), + {.name = "user", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("user to list authorized keys for"), + }, + {.name = NULL} +}; + +static bool +cmdGetUserSSHKeys(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + const char *user; + char **keys = NULL; + int nkeys = 0; + size_t i; + const unsigned int flags = 0; + vshTablePtr table = NULL; + bool ret = false; + + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (vshCommandOptStringReq(ctl, cmd, "user", &user) < 0) + goto cleanup; + + nkeys = virDomainAuthorizedSSHKeysGet(dom, user, &keys, flags); + if (nkeys < 0) + goto cleanup; + + if (!(table = vshTableNew(_("key"), NULL))) + goto cleanup; + + for (i = 0; i < nkeys; i++) { + if (vshTableRowAppend(table, keys[i], NULL) < 0) + goto cleanup; + } + + vshTablePrintToStdout(table, ctl); + + ret = true; + cleanup: + vshTableFree(table); + if (nkeys > 0) + virStringListFreeCount(keys, nkeys); + virshDomainFree(dom); + return ret; +} + + +/* + * "set-user-sshkeys" command + */ +static const vshCmdInfo info_set_user_sshkeys[] = { + {.name = "help", + .data = N_("manipulate authorized SSH keys file for given user (via agent)") + }, + {.name = "desc", + .data = N_("Append, reset or remove specified key from the authorized " + "keys file for given user") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_set_user_sshkeys[] = { + VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), + {.name = "user", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("user to list authorized keys for"), + }, + {.name = "append", + .type = VSH_OT_BOOL, + .help = N_("append keys to the file"), + }, + {.name = "remove", + .type = VSH_OT_BOOL, + .help = N_("remove keys from the file"), + }, + {.name = "keys", + .type = VSH_OT_ARGV, + .help = N_("OpenSSH keys"), + }, + {.name = NULL} +}; + +static bool +cmdSetUserSSHKeys(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + const char *user; + const vshCmdOpt *opt = NULL; + g_autofree const char **keys = NULL; + int nkeys = 0; + unsigned int flags = 0; + bool ret = false; + + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (vshCommandOptStringReq(ctl, cmd, "user", &user) < 0) + goto cleanup; + + if (vshCommandOptBool(cmd, "append")) + flags |= VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND; + if (vshCommandOptBool(cmd, "remove")) + flags |= VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE; + + while ((opt = vshCommandOptArgv(ctl, cmd, opt))) { + keys = g_renew(const char *, keys, nkeys + 1); + keys[nkeys] = opt->data; + nkeys++; + } + + if (virDomainAuthorizedSSHKeysSet(dom, user, keys, nkeys, flags) < 0) + goto cleanup; + + ret = true; + cleanup: + virshDomainFree(dom); + return ret; +} + + const vshCmdDef domManagementCmds[] = { {.name = "attach-device", .handler = cmdAttachDevice, @@ -14526,6 +14666,12 @@ const vshCmdDef domManagementCmds[] = { .info = info_event, .flags = 0 }, + {.name = "get-user-sshkeys", + .handler = cmdGetUserSSHKeys, + .opts = opts_get_user_sshkeys, + .info = info_get_user_sshkeys, + .flags = 0 + }, {.name = "inject-nmi", .handler = cmdInjectNMI, .opts = opts_inject_nmi, @@ -14772,6 +14918,12 @@ const vshCmdDef domManagementCmds[] = { .info = info_setLifecycleAction, .flags = 0 }, + {.name = "set-user-sshkeys", + .handler = cmdSetUserSSHKeys, + .opts = opts_set_user_sshkeys, + .info = info_set_user_sshkeys, + .flags = 0 + }, {.name = "set-user-password", .handler = cmdSetUserPassword, .opts = opts_set_user_password, -- 2.26.2
On Tue, Nov 10, 2020 at 16:11:43 +0100, Michal Privoznik wrote:
The new virsh commands are:
get-user-sshkeys set-user-sshkeys
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/manpages/virsh.rst | 37 ++++++++++ tools/virsh-domain.c | 152 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 189 insertions(+)
[...]
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 1ae936c6b2..f51765cb42 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c
[...]
+static const vshCmdOptDef opts_set_user_sshkeys[] = { + VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), + {.name = "user", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("user to list authorized keys for"), + }, + {.name = "append", + .type = VSH_OT_BOOL, + .help = N_("append keys to the file"), + }, + {.name = "remove", + .type = VSH_OT_BOOL, + .help = N_("remove keys from the file"), + }, + {.name = "keys", + .type = VSH_OT_ARGV, + .help = N_("OpenSSH keys"), + }, + {.name = NULL} +};
The --keys ARGV option is not very userfriendly, given that the ssh key has spaces in it ("ssh-rsa AAA...... user@host") ...
+static bool +cmdSetUserSSHKeys(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + const char *user; + const vshCmdOpt *opt = NULL; + g_autofree const char **keys = NULL; + int nkeys = 0; + unsigned int flags = 0; + bool ret = false; + + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (vshCommandOptStringReq(ctl, cmd, "user", &user) < 0) + goto cleanup; + + if (vshCommandOptBool(cmd, "append")) + flags |= VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND; + if (vshCommandOptBool(cmd, "remove")) + flags |= VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE; + + while ((opt = vshCommandOptArgv(ctl, cmd, opt))) { + keys = g_renew(const char *, keys, nkeys + 1); + keys[nkeys] = opt->data; + nkeys++;
... especially the way it's implemented here, where without using quotes it would treat the key as 3 keys. IMO a way better way is to read the key from a file. If you really want to take key from command line, make using file optional at least.
+ } + + if (virDomainAuthorizedSSHKeysSet(dom, user, keys, nkeys, flags) < 0) + goto cleanup;
From: Marc-André Lureau <marcandre.lureau@redhat.com> In QEMU 5.2, the guest agent learned to manipulate a user ~/.ssh/authorized_keys. Bind the JSON API to libvirt. https://wiki.qemu.org/ChangeLog/5.2#Guest_agent Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_agent.c | 140 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_agent.h | 15 +++++ tests/qemuagenttest.c | 79 ++++++++++++++++++++++++ 3 files changed, 234 insertions(+) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 7fbb4a9431..9fa79338bc 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2496,3 +2496,143 @@ qemuAgentSetResponseTimeout(qemuAgentPtr agent, { agent->timeout = timeout; } + +/** + * qemuAgentSSHGetAuthorizedKeys: + * @agent: agent object + * @user: user to get authorized keys for + * @keys: Array of authorized keys + * + * Fetch the public keys from @user's $HOME/.ssh/authorized_keys. + * + * Returns: number of keys returned on success, + * -1 otherwise (error is reported) + */ +int +qemuAgentSSHGetAuthorizedKeys(qemuAgentPtr agent, + const char *user, + char ***keys) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + virJSONValuePtr data = NULL; + size_t ndata; + size_t i; + char **keys_ret = NULL; + + if (!(cmd = qemuAgentMakeCommand("guest-ssh-get-authorized-keys", + "s:username", user, + NULL))) + return -1; + + if (qemuAgentCommand(agent, cmd, &reply, agent->timeout) < 0) + return -1; + + if (!(data = virJSONValueObjectGetObject(reply, "return")) || + !(data = virJSONValueObjectGetArray(data, "keys"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("qemu agent didn't return an array of keys")); + return -1; + } + + ndata = virJSONValueArraySize(data); + + keys_ret = g_new0(char *, ndata); + + for (i = 0; i < ndata; i++) { + virJSONValuePtr entry = virJSONValueArrayGet(data, i); + + if (!entry) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("array element missing in guest-ssh-get-authorized-keys return " + "value")); + goto error; + } + + keys_ret[i] = g_strdup(virJSONValueGetString(entry)); + } + + *keys = g_steal_pointer(&keys_ret); + return ndata; + + error: + virStringListFreeCount(keys_ret, ndata); + return -1; +} + + +/** + * qemuAgentSSHAddAuthorizedKeys: + * @agent: agent object + * @user: user to add authorized keys for + * @keys: Array of authorized keys + * @nkeys: number of items in @keys array + * @reset: whether to truncate authorized keys file before writing + * + * Append SSH @keys into the @user's authorized keys file. If + * @reset is true then the file is truncated before write and + * thus contains only newly added @keys. + * + * Returns: 0 on success, + * -1 otherwise (error is reported) + */ +int qemuAgentSSHAddAuthorizedKeys(qemuAgentPtr agent, + const char *user, + const char **keys, + size_t nkeys, + bool reset) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + g_autoptr(virJSONValue) jkeys = NULL; + + jkeys = qemuAgentMakeStringsArray(keys, nkeys); + if (jkeys == NULL) + return -1; + + if (!(cmd = qemuAgentMakeCommand("guest-ssh-add-authorized-keys", + "s:username", user, + "a:keys", &jkeys, + "b:reset", reset, + NULL))) + return -1; + + return qemuAgentCommand(agent, cmd, &reply, agent->timeout); +} + + +/** + * qemuAgentSSHRemoveAuthorizedKeys: + * @agent: agent object + * @user: user to remove authorized keys for + * @keys: Array of authorized keys + * @nkeys: number of items in @keys array + * + * Remove SSH @keys from the @user's authorized keys file. It's + * not considered an error when trying to remove a non-existent + * key. + * + * Returns: 0 on success, + * -1 otherwise (error is reported) + */ +int qemuAgentSSHRemoveAuthorizedKeys(qemuAgentPtr agent, + const char *user, + const char **keys, + size_t nkeys) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + g_autoptr(virJSONValue) jkeys = NULL; + + jkeys = qemuAgentMakeStringsArray(keys, nkeys); + if (jkeys == NULL) + return -1; + + if (!(cmd = qemuAgentMakeCommand("guest-ssh-remove-authorized-keys", + "s:username", user, + "a:keys", &jkeys, + NULL))) + return -1; + + return qemuAgentCommand(agent, cmd, &reply, agent->timeout); +} diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h index 2eeb376a68..7cbab489ec 100644 --- a/src/qemu/qemu_agent.h +++ b/src/qemu/qemu_agent.h @@ -170,3 +170,18 @@ int qemuAgentGetTimezone(qemuAgentPtr mon, void qemuAgentSetResponseTimeout(qemuAgentPtr mon, int timeout); + +int qemuAgentSSHGetAuthorizedKeys(qemuAgentPtr agent, + const char *user, + char ***keys); + +int qemuAgentSSHAddAuthorizedKeys(qemuAgentPtr agent, + const char *user, + const char **keys, + size_t nkeys, + bool reset); + +int qemuAgentSSHRemoveAuthorizedKeys(qemuAgentPtr agent, + const char *user, + const char **keys, + size_t nkeys); diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index 607bd97b5c..47ff92733a 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -35,6 +35,84 @@ virQEMUDriver driver; +static int +testQemuAgentSSHKeys(const void *data) +{ + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt); + char **keys = NULL; + int nkeys = 0; + int ret = -1; + + if (!test) + return -1; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "guest-ssh-get-authorized-keys", + "{\"return\": {" + " \"keys\": [" + " \"algo1 key1 comments1\"," + " \"algo2 key2 comments2\"" + " ]" + "}}") < 0) + goto cleanup; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "guest-ssh-add-authorized-keys", + "{ \"return\" : {} }") < 0) + goto cleanup; + + if (qemuMonitorTestAddAgentSyncResponse(test) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "guest-ssh-remove-authorized-keys", + "{ \"return\" : {} }") < 0) + goto cleanup; + + if ((nkeys = qemuAgentSSHGetAuthorizedKeys(qemuMonitorTestGetAgent(test), + "user", + &keys)) < 0) + goto cleanup; + + if (nkeys != 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "expected 2 keys, got %d", nkeys); + ret = -1; + goto cleanup; + } + + if (STRNEQ(keys[1], "algo2 key2 comments2")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "Unexpected key returned: %s", keys[1]); + ret = -1; + goto cleanup; + } + + if ((ret = qemuAgentSSHAddAuthorizedKeys(qemuMonitorTestGetAgent(test), + "user", + (const char **) keys, + nkeys, + true)) < 0) + goto cleanup; + + if ((ret = qemuAgentSSHRemoveAuthorizedKeys(qemuMonitorTestGetAgent(test), + "user", + (const char **) keys, + nkeys)) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virStringListFreeCount(keys, nkeys); + qemuMonitorTestFree(test); + return ret; +} + + static int testQemuAgentFSFreeze(const void *data) { @@ -1315,6 +1393,7 @@ mymain(void) DO_TEST(Users); DO_TEST(OSInfo); DO_TEST(Timezone); + DO_TEST(SSHKeys); DO_TEST(Timeout); /* Timeout should always be called last */ -- 2.26.2
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1888537 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 81 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5f0bb69dd5..7fd29f934f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20121,6 +20121,85 @@ qemuDomainAgentSetResponseTimeout(virDomainPtr dom, } +static int +qemuDomainAuthorizedSSHKeysGet(virDomainPtr dom, + const char *user, + char ***keys, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + qemuAgentPtr agent; + int rv = -1; + + virCheckFlags(0, -1); + + if (!(vm = qemuDomainObjFromDomain(dom))) + return -1; + + if (virDomainAuthorizedSshKeysGetEnsureACL(dom->conn, vm->def) < 0) + return -1; + + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0) + return -1; + + if (!qemuDomainAgentAvailable(vm, true)) + goto endagentjob; + + agent = qemuDomainObjEnterAgent(vm); + rv = qemuAgentSSHGetAuthorizedKeys(agent, user, keys); + qemuDomainObjExitAgent(vm, agent); + + endagentjob: + qemuDomainObjEndAgentJob(vm); + virDomainObjEndAPI(&vm); + return rv; +} + + +static int +qemuDomainAuthorizedSSHKeysSet(virDomainPtr dom, + const char *user, + const char **keys, + int nkeys, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + g_autoptr(virDomainObj) vm = NULL; + qemuAgentPtr agent; + const bool append = flags & VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND; + const bool remove = flags & VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE; + int rv = -1; + + virCheckFlags(VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND | + VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE, -1); + + if (!(vm = qemuDomainObjFromDomain(dom))) + return -1; + + if (virDomainAuthorizedSshKeysSetEnsureACL(dom->conn, vm->def) < 0) + return -1; + + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0) + return -1; + + if (!qemuDomainAgentAvailable(vm, true)) + goto endagentjob; + + agent = qemuDomainObjEnterAgent(vm); + if (remove) + rv = qemuAgentSSHRemoveAuthorizedKeys(agent, user, keys, nkeys); + else + rv = qemuAgentSSHAddAuthorizedKeys(agent, user, keys, nkeys, !append); + qemuDomainObjExitAgent(vm, agent); + + endagentjob: + qemuDomainObjEndAgentJob(vm); + virDomainObjEndAPI(&vm); + return rv; +} + + static virHypervisorDriver qemuHypervisorDriver = { .name = QEMU_DRIVER_NAME, .connectURIProbe = qemuConnectURIProbe, @@ -20360,6 +20439,8 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainAgentSetResponseTimeout = qemuDomainAgentSetResponseTimeout, /* 5.10.0 */ .domainBackupBegin = qemuDomainBackupBegin, /* 6.0.0 */ .domainBackupGetXMLDesc = qemuDomainBackupGetXMLDesc, /* 6.0.0 */ + .domainAuthorizedSSHKeysGet = qemuDomainAuthorizedSSHKeysGet, /* 6.10.0 */ + .domainAuthorizedSSHKeysSet = qemuDomainAuthorizedSSHKeysSet, /* 6.10.0 */ }; -- 2.26.2
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- NEWS.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 0e56f5dbca..3fba272543 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -12,6 +12,12 @@ v6.10.0 (unreleased) ==================== * **New features** + * qemu: Implement OpenSSH authorized key file management APIs + + New APIs (``virDomainAuthorizedSSHKeysGet()`` and + ``virDomainAuthorizedSSHKeysSet()``) and virsh commands + (``get-user-sshkeys`` and ``set-user-sshkeys``) are added to manage + authorized_keys SSH file for user. * **Improvements** -- 2.26.2
On Tue, Nov 10, 2020 at 16:11:40 +0100, Michal Privoznik wrote:
Marc-André posted a patch that implements agent handling. I've written the rest.
Marc-André Lureau (1): qemu_agent: add qemuAgentSSH{Add,Remove,Get}AuthorizedKeys
Michal Prívozník (5): Introduce OpenSSH authorized key file mgmt APIs
One more thing to think about: Since we are getting random requests for setters of various bits which we have to bend the rule "we don't care what's running in the VM" and which don't really scale when adding new APIs. I propose we add a generic guest agent setter which will be extensible using a typed parameters and a type property. It will basically become the counterpart to virDomainGetGuestInfo. The extensions then become enum additions and code additions only and will be more flexible for future use. The same way the getter forthe ssh keys should become part of virDomainGetGuestInfo, obviously auditing whether a read-write connection is used. example: int qemuDomainSetGuestInfo(virDomainPtr dom, virDomainSetGuestInfoType type, virTypedParamPtr params, unsigned int nparams, unsigned int flags); Invocation for setting keys: virTypedParamsAddString(..., "user", "root") virTypedParamsAddString(..., "key", "ssh-rsa AA.... root@localhost") virTypedParamsAddString(..., "key", "ssh-rsa AA.... user@localhost") etc.
On 11/11/20 11:32 AM, Peter Krempa wrote:
On Tue, Nov 10, 2020 at 16:11:40 +0100, Michal Privoznik wrote:
Marc-André posted a patch that implements agent handling. I've written the rest.
Marc-André Lureau (1): qemu_agent: add qemuAgentSSH{Add,Remove,Get}AuthorizedKeys
Michal Prívozník (5): Introduce OpenSSH authorized key file mgmt APIs
One more thing to think about:
Since we are getting random requests for setters of various bits which we have to bend the rule "we don't care what's running in the VM" and which don't really scale when adding new APIs. I propose we add a generic guest agent setter which will be extensible using a typed parameters and a type property.
It will basically become the counterpart to virDomainGetGuestInfo.
The extensions then become enum additions and code additions only and will be more flexible for future use.
The same way the getter forthe ssh keys should become part of virDomainGetGuestInfo, obviously auditing whether a read-write connection is used.
example:
int qemuDomainSetGuestInfo(virDomainPtr dom, virDomainSetGuestInfoType type, virTypedParamPtr params, unsigned int nparams, unsigned int flags);
Invocation for setting keys:
virTypedParamsAddString(..., "user", "root") virTypedParamsAddString(..., "key", "ssh-rsa AA.... root@localhost") virTypedParamsAddString(..., "key", "ssh-rsa AA.... user@localhost")
etc.
Yeah, this is much more extensible. Okay, let me send v2. Michal
On Wed, Nov 11, 2020 at 13:04:19 +0100, Michal Privoznik wrote:
On 11/11/20 11:32 AM, Peter Krempa wrote:
On Tue, Nov 10, 2020 at 16:11:40 +0100, Michal Privoznik wrote:
Marc-André posted a patch that implements agent handling. I've written the rest.
Marc-André Lureau (1): qemu_agent: add qemuAgentSSH{Add,Remove,Get}AuthorizedKeys
Michal Prívozník (5): Introduce OpenSSH authorized key file mgmt APIs
One more thing to think about:
Since we are getting random requests for setters of various bits which we have to bend the rule "we don't care what's running in the VM" and which don't really scale when adding new APIs. I propose we add a generic guest agent setter which will be extensible using a typed parameters and a type property.
It will basically become the counterpart to virDomainGetGuestInfo.
The extensions then become enum additions and code additions only and will be more flexible for future use.
The same way the getter forthe ssh keys should become part of virDomainGetGuestInfo, obviously auditing whether a read-write connection is used.
example:
int qemuDomainSetGuestInfo(virDomainPtr dom, virDomainSetGuestInfoType type, virTypedParamPtr params, unsigned int nparams, unsigned int flags);
Invocation for setting keys:
virTypedParamsAddString(..., "user", "root") virTypedParamsAddString(..., "key", "ssh-rsa AA.... root@localhost") virTypedParamsAddString(..., "key", "ssh-rsa AA.... user@localhost")
etc.
Yeah, this is much more extensible. Okay, let me send v2.
Another possiblity is to also return either a string or again typed parameters, so that we can create an API which will basically be an locked-down and slightly more portable version of virDomainQemuAgentCommand, where we implement only commands which are not used by other libvirt APIs, but still allows to perform the wide variety of stuff that the qemu agent has nowadays (e.g. exec, file read etc.)
On 11/11/20 1:04 PM, Michal Privoznik wrote:
On 11/11/20 11:32 AM, Peter Krempa wrote:
On Tue, Nov 10, 2020 at 16:11:40 +0100, Michal Privoznik wrote:
Marc-André posted a patch that implements agent handling. I've written the rest.
Marc-André Lureau (1): qemu_agent: add qemuAgentSSH{Add,Remove,Get}AuthorizedKeys
Michal Prívozník (5): Introduce OpenSSH authorized key file mgmt APIs
One more thing to think about:
Since we are getting random requests for setters of various bits which we have to bend the rule "we don't care what's running in the VM" and which don't really scale when adding new APIs. I propose we add a generic guest agent setter which will be extensible using a typed parameters and a type property.
It will basically become the counterpart to virDomainGetGuestInfo.
The extensions then become enum additions and code additions only and will be more flexible for future use.
The same way the getter forthe ssh keys should become part of virDomainGetGuestInfo, obviously auditing whether a read-write connection is used.
example:
int qemuDomainSetGuestInfo(virDomainPtr dom, virDomainSetGuestInfoType type, virTypedParamPtr params, unsigned int nparams, unsigned int flags);
Invocation for setting keys:
virTypedParamsAddString(..., "user", "root") virTypedParamsAddString(..., "key", "ssh-rsa AA.... root@localhost") virTypedParamsAddString(..., "key", "ssh-rsa AA.... user@localhost")
etc.
Yeah, this is much more extensible. Okay, let me send v2.
My enthusiasm might had been premature. virDomainGetGuestInfo() does not send anything but virDomainPtr, unsigned int types and flags down the wire. While we can make @params be both in and out type of arguments, it's going to require change of RPC. I mean the way that qemu-ga APIs are implemented is that for listing ssh keys the API expects an user on the input so that it can construct $HOME/.ssh/authorized_keys path. How to pass this through virDomainGetGuestInfo()? Okay, we could work around it by just listing SSH keys for all users and let caller filter our the interesting ones, but this is: a) scary from security POV, b) suboptimal because we might hit message size limit pretty soon. Also, there is no qemu-ga API to list all users, just those logged in currently. And the whole point of these new APIs is to set up SSH keys before user logs in. Michal
On Thu, Nov 12, 2020 at 12:55:23 +0100, Michal Privoznik wrote:
On 11/11/20 1:04 PM, Michal Privoznik wrote:
On 11/11/20 11:32 AM, Peter Krempa wrote:
On Tue, Nov 10, 2020 at 16:11:40 +0100, Michal Privoznik wrote:
Marc-André posted a patch that implements agent handling. I've written the rest.
Marc-André Lureau (1): qemu_agent: add qemuAgentSSH{Add,Remove,Get}AuthorizedKeys
Michal Prívozník (5): Introduce OpenSSH authorized key file mgmt APIs
One more thing to think about:
Since we are getting random requests for setters of various bits which we have to bend the rule "we don't care what's running in the VM" and which don't really scale when adding new APIs. I propose we add a generic guest agent setter which will be extensible using a typed parameters and a type property.
It will basically become the counterpart to virDomainGetGuestInfo.
The extensions then become enum additions and code additions only and will be more flexible for future use.
The same way the getter forthe ssh keys should become part of virDomainGetGuestInfo, obviously auditing whether a read-write connection is used.
example:
int qemuDomainSetGuestInfo(virDomainPtr dom, virDomainSetGuestInfoType type, virTypedParamPtr params, unsigned int nparams, unsigned int flags);
Invocation for setting keys:
virTypedParamsAddString(..., "user", "root") virTypedParamsAddString(..., "key", "ssh-rsa AA.... root@localhost") virTypedParamsAddString(..., "key", "ssh-rsa AA.... user@localhost")
etc.
Yeah, this is much more extensible. Okay, let me send v2.
My enthusiasm might had been premature. virDomainGetGuestInfo() does not send anything but virDomainPtr, unsigned int types and flags down the wire. While we can make @params be both in and out type of arguments, it's going to require change of RPC. I mean the way that qemu-ga APIs are implemented is that for listing ssh keys the API expects an user on the input so that it can construct $HOME/.ssh/authorized_keys path. How to pass this through virDomainGetGuestInfo()? Okay, we could work around it by just listing SSH keys for all users and let caller filter our the interesting ones, but this is: a) scary from security POV, b) suboptimal because we might hit message size limit pretty soon. Also, there is no qemu-ga API to list all users, just those logged in currently. And the whole point of these new APIs is to set up SSH keys before user logs in.
Yeah, using virDomainGetGuestInfo as the getter for the keys will not work. What I ultimately want to suggest is something more generic which will allow driving guest agent APIs libvirt doesn't actually care about without having to add single-use APIs for every single one of them. qemu-ga seems to support following APIs: $ ./qemu-ga --blacklist=? guest-sync-delimited guest-sync guest-ping guest-get-time guest-set-time guest-info guest-shutdown guest-file-open guest-file-close guest-file-read guest-file-write guest-file-seek guest-file-flush guest-fsfreeze-status guest-fsfreeze-freeze guest-fsfreeze-freeze-list guest-fsfreeze-thaw guest-fstrim guest-suspend-disk guest-suspend-ram guest-suspend-hybrid guest-network-get-interfaces guest-get-vcpus guest-set-vcpus guest-get-disks guest-get-fsinfo guest-set-user-password guest-get-memory-blocks guest-set-memory-blocks guest-get-memory-block-info guest-exec-status guest-exec guest-get-host-name guest-get-users guest-get-timezone guest-get-osinfo guest-get-devices guest-ssh-get-authorized-keys guest-ssh-add-authorized-keys guest-ssh-remove-authorized-keys Let's categorize them: Utility functions: guest-sync-delimited guest-sync guest-ping Functions which at least partially map to libvirt APIs: guest-shutdown guest-get-vcpus guest-set-vcpus Functions used internally but also useful externally: guest-fsfreeze-status guest-fsfreeze-freeze guest-fsfreeze-freeze-list guest-fsfreeze-thaw Functions which can be claimed to map to libvirt but don't really: guest-suspend-disk guest-suspend-ram guest-suspend-hybrid Functions which are not really relevant to libvirt but we expose wrappers: guest-get-time guest-set-time guest-fstrim guest-network-get-interfaces guest-get-disks guest-get-fsinfo guest-get-host-name guest-get-users guest-get-timezone guest-get-osinfo Functions which are not really relevant to libvirt, we expose wrappers and they are scary: guest-set-user-password Functions which are not really relevant to libvirt, we don't expose wrappers and they are scary: guest-file-open guest-file-close guest-file-read guest-file-write guest-file-seek guest-file-flush guest-exec-status guest-exec Other stats functions we might be asked for: guest-info guest-get-memory-blocks guest-set-memory-blocks guest-get-memory-block-info guest-get-devices And finally those we have here: guest-ssh-get-authorized-keys guest-ssh-add-authorized-keys guest-ssh-remove-authorized-keys So there's just a very tiny set of QGA APIs we really need. For the rest we are adding more-or-less direct wrappers to satisfy the access to the APIs. With more-and-more APIs added which really deal just with the guest state itself it's a bit weird to add libvirt APIs for them. I'd like us to prevent adding new APIs constantly especially those which deal just with the guest. Since libvirt actually can't ever rely on the state of the guest agent as it can be restarted at any time including the guest OS and thus we we have to internally treat it without any presumptions about state and re-probe everything and we really need just a tiny subset of the commands, we'd really benefint from just multiplexing access between libvirt and any other APP which can use the QGA QMP protocol directly. Thus I think we could arguably make virDomainQemuAgentCommand a supported API. Any guest state change which can be initiated via the GA needs to be supported also without that since the guest itself can trigger it. Saying that virDomainQemuAgentCommand is fully supported to be used would free us from having to add arbitrary unextendable APIs for every single guest agent API, but would still allow libvirt to use APIs we need. We just need to make 100% sure that it can't be abused as argument to to change status of virDomainQemuMonitorCommand.
On 11/12/20 3:46 PM, Peter Krempa wrote:
Saying that virDomainQemuAgentCommand is fully supported to be used would free us from having to add arbitrary unextendable APIs for every single guest agent API, but would still allow libvirt to use APIs we need.
By saying that mgmt apps will need to learn json apart from xml. I'm not saying it's necessarily a bad thing - mgmt application is probably written in a language that already has a JSON library built in (golang, python).
We just need to make 100% sure that it can't be abused as argument to to change status of virDomainQemuMonitorCommand.
There is a difference. You can configure qemu-ga for your guest so that libvirt doesn't know about it (and does not use it), but your app can still talk to it (just don't assign the org.qemu.guest_agent.0 name). With monitor it's not possible; we could not guarantee anything if we'd give up monitor. Michal
On Thu, Nov 12, 2020 at 05:18:06PM +0100, Michal Privoznik wrote:
On 11/12/20 3:46 PM, Peter Krempa wrote:
Saying that virDomainQemuAgentCommand is fully supported to be used would free us from having to add arbitrary unextendable APIs for every single guest agent API, but would still allow libvirt to use APIs we need.
By saying that mgmt apps will need to learn json apart from xml. I'm not saying it's necessarily a bad thing - mgmt application is probably written in a language that already has a JSON library built in (golang, python).
You also loose the benefit of libvirt's API abstraction. If QEMU guest agent is replaced by something different in future, a formal API in libvirt insulates the apps from that difference, both within context of a single hypervisor, and cross hypervisor. 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 :|
On Thu, Nov 12, 2020 at 16:27:02 +0000, Daniel Berrange wrote:
On Thu, Nov 12, 2020 at 05:18:06PM +0100, Michal Privoznik wrote:
On 11/12/20 3:46 PM, Peter Krempa wrote:
Saying that virDomainQemuAgentCommand is fully supported to be used would free us from having to add arbitrary unextendable APIs for every single guest agent API, but would still allow libvirt to use APIs we need.
By saying that mgmt apps will need to learn json apart from xml. I'm not saying it's necessarily a bad thing - mgmt application is probably written in a language that already has a JSON library built in (golang, python).
You also loose the benefit of libvirt's API abstraction. If QEMU guest agent is replaced by something different in future, a formal API in libvirt insulates the apps from that difference, both within context of a single hypervisor, and cross hypervisor.
Yes, basically my question was whether it's worth doing individual APIs for all of those things especially since we usually don't try to deal with what's running inside the VM. I'm not 100% persuaded, but we have plenty prior art and the insulation argument makes sense (as long as the replacement has the capabilities). If we do want to keep adding the APIs I'm fine with the patches as-is, as any other solution would be extremely gross. Obviously v2 is required due to the security problems.
But how about selinux? I'm run qemu-ga in guest and want to modify the authorized_keys file of some user? Do we need to extend the selinux policy to allow modification of such files in all guests? чт, 12 нояб. 2020 г. в 19:41, Peter Krempa <pkrempa@redhat.com>:
On Thu, Nov 12, 2020 at 16:27:02 +0000, Daniel Berrange wrote:
On Thu, Nov 12, 2020 at 05:18:06PM +0100, Michal Privoznik wrote:
On 11/12/20 3:46 PM, Peter Krempa wrote:
Saying that virDomainQemuAgentCommand is fully supported to be used would free us from having to add arbitrary unextendable APIs for every single guest agent API, but would still allow libvirt to use APIs we need.
By saying that mgmt apps will need to learn json apart from xml. I'm not saying it's necessarily a bad thing - mgmt application is probably written in a language that already has a JSON library built in (golang, python).
You also loose the benefit of libvirt's API abstraction. If QEMU guest agent is replaced by something different in future, a formal API in libvirt insulates the apps from that difference, both within context of a single hypervisor, and cross hypervisor.
Yes, basically my question was whether it's worth doing individual APIs for all of those things especially since we usually don't try to deal with what's running inside the VM. I'm not 100% persuaded, but we have plenty prior art and the insulation argument makes sense (as long as the replacement has the capabilities).
If we do want to keep adding the APIs I'm fine with the patches as-is, as any other solution would be extremely gross.
Obviously v2 is required due to the security problems.
-- Vasiliy Tolstov, e-mail: v.tolstov@selfip.ru
On 11/13/20 9:23 AM, Vasiliy Tolstov wrote:
But how about selinux? I'm run qemu-ga in guest and want to modify the authorized_keys file of some user? Do we need to extend the selinux policy to allow modification of such files in all guests?
Yes we do. But since qemu-ga offers this under API it should be fairly easy to argue that it should be allowed. It would be much harder to advocate for selinux policy change using solely file APIs of qemu-ga. Michal
Useful things. As i understand it qemu-ga eventually can replace cloud-init stuff. As for now it already have high level api and low level api (like read/write files) вт, 10 нояб. 2020 г. в 18:49, Michal Privoznik <mprivozn@redhat.com>:
Marc-André posted a patch that implements agent handling. I've written the rest.
Marc-André Lureau (1): qemu_agent: add qemuAgentSSH{Add,Remove,Get}AuthorizedKeys
Michal Prívozník (5): Introduce OpenSSH authorized key file mgmt APIs remote: Implement OpenSSH authorized key file mgmt APIs virsh: Expose OpenSSH authorized key file mgmt APIs qemu: Implement OpenSSH authorized key file mgmt APIs news: Document recent OpenSSH authorized key file mgmt APIs
NEWS.rst | 6 ++ docs/manpages/virsh.rst | 37 +++++++ include/libvirt/libvirt-domain.h | 17 ++++ src/driver-hypervisor.h | 15 +++ src/libvirt-domain.c | 115 +++++++++++++++++++++ src/libvirt_public.syms | 6 ++ src/qemu/qemu_agent.c | 140 +++++++++++++++++++++++++ src/qemu/qemu_agent.h | 15 +++ src/qemu/qemu_driver.c | 81 +++++++++++++++ src/remote/remote_daemon_dispatch.c | 82 +++++++++++++++ src/remote/remote_driver.c | 87 ++++++++++++++++ src/remote/remote_protocol.x | 34 ++++++- src/remote_protocol-structs | 22 ++++ tests/qemuagenttest.c | 79 +++++++++++++++ tools/virsh-domain.c | 152 ++++++++++++++++++++++++++++ 15 files changed, 887 insertions(+), 1 deletion(-)
-- 2.26.2
-- Vasiliy Tolstov, e-mail: v.tolstov@selfip.ru
On 11/12/20 1:16 PM, Vasiliy Tolstov wrote:
Useful things. As i understand it qemu-ga eventually can replace cloud-init stuff. As for now it already have high level api and low level api (like read/write files)
Yeah, the low level file manipulation APIs are terrible because they have to rely on SELinux to prevent qemu-ga from doing something bad. Which in this case would end up in either disabling SELinux (bad) or having to write custom policies so that qemu-ga can modify authorized_keys files. And also, from mgmt application's POV they are not atomic. Michal
participants (4)
-
Daniel P. Berrangé -
Michal Privoznik -
Peter Krempa -
Vasiliy Tolstov