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@xxxxxxxxxx>
> ---
> 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;
> +}
