diff mbox series

[2/2] qga: add ssh-{add,remove}-authorized-keys

Message ID 20201013202502.335336-3-marcandre.lureau@redhat.com
State New
Headers show
Series qemu-ga: add ssh-{add,remove}-authorized-keys | expand

Commit Message

Marc-André Lureau Oct. 13, 2020, 8:25 p.m. UTC
From: Marc-André Lureau <marcandre.lureau@redhat.com>

Add new commands to add and remove SSH public keys from
~/.ssh/authorized_keys.

I took a different approach for testing, including the unit tests right
with the code. I wanted to overwrite the function to get the user
details, I couldn't easily do that over QMP. Furthermore, I prefer
having unit tests very close to the code, and unit files that are domain
specific (commands-posix is too crowded already). Fwiw, that
coding/testing style is Rust-style (where tests can or should even be
part of the documentation!).

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1885332

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qga/commands-posix-ssh.c | 394 +++++++++++++++++++++++++++++++++++++++
 qga/commands-win32.c     |  10 +
 qga/meson.build          |  17 +-
 qga/qapi-schema.json     |  32 ++++
 4 files changed, 452 insertions(+), 1 deletion(-)
 create mode 100644 qga/commands-posix-ssh.c

Comments

Philippe Mathieu-Daudé Oct. 13, 2020, 9:14 p.m. UTC | #1
Hi Marc-André,

On 10/13/20 10:25 PM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Add new commands to add and remove SSH public keys from
> ~/.ssh/authorized_keys.
> 
> I took a different approach for testing, including the unit tests right
> with the code. I wanted to overwrite the function to get the user
> details, I couldn't easily do that over QMP. Furthermore, I prefer
> having unit tests very close to the code, and unit files that are domain
> specific (commands-posix is too crowded already). Fwiw, that

FWIW

> coding/testing style is Rust-style (where tests can or should even be
> part of the documentation!).
> 
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1885332
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
...

> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index cec98c7e06..50e2854b45 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -1306,3 +1306,35 @@
>   ##
>   { 'command': 'guest-get-devices',
>     'returns': ['GuestDeviceInfo'] }
> +
> +##
> +# @guest-ssh-add-authorized-keys:
> +#
> +# @username: the user account to add the authorized key
> +# @keys: the public keys to add (in OpenSSH format)

You use plural but the code only seems to add (remove) one key
at a time.

'OpenSSH format' is confusing. From sshd(8):

   Each line of the file contains one key (empty lines and lines
   starting with a ‘#’ are ignored as comments).

   Public keys consist of the following space-separated fields:

     options, keytype, base64-encoded key, comment.

   The options field is optional.

   Note that lines in this file can be several hundred bytes long
   (because of the size of the public key encoding) up to a limit
   of 8 kilobytes, which permits RSA keys up to 16 kilobits.

   The options (if present) consist of comma-separated option
   specifications.  No spaces are permitted, except within double
   quotes.

@openssh_authorized_key_line is ugly, maybe use @authorized_key
to make it clearer?

> +#
> +# Append a public key to user $HOME/.ssh/authorized_keys on Unix systems (not
> +# implemented for other systems).

Here "a key" singular, good.

> +#
> +# Returns: Nothing on success.
> +#
> +# Since: 5.2
> +##
> +{ 'command': 'guest-ssh-add-authorized-keys',

Here "keys" plural :/

> +  'data': { 'username': 'str', 'keys': ['str'] } }
> +
> +##
> +# @guest-ssh-remove-authorized-keys:
> +#
> +# @username: the user account to add the authorized key
> +# @keys: the public keys to remove (in OpenSSH format)
> +#
> +# Remove public keys from the user $HOME/.ssh/authorized_keys on Unix systems
> +# (not implemented for other systems).
> +#
> +# Returns: Nothing on success.
> +#
> +# Since: 5.2
> +##
> +{ 'command': 'guest-ssh-remove-authorized-keys',
> +  'data': { 'username': 'str', 'keys': ['str'] } }
>
Marc-André Lureau Oct. 14, 2020, 7:37 a.m. UTC | #2
Hi

On Wed, Oct 14, 2020 at 1:14 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Hi Marc-André,
>
> On 10/13/20 10:25 PM, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Add new commands to add and remove SSH public keys from
> > ~/.ssh/authorized_keys.
> >
> > I took a different approach for testing, including the unit tests right
> > with the code. I wanted to overwrite the function to get the user
> > details, I couldn't easily do that over QMP. Furthermore, I prefer
> > having unit tests very close to the code, and unit files that are domain
> > specific (commands-posix is too crowded already). Fwiw, that
>
> FWIW

ok

>
> > coding/testing style is Rust-style (where tests can or should even be
> > part of the documentation!).
> >
> > Fixes:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1885332
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> ...
>
> > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> > index cec98c7e06..50e2854b45 100644
> > --- a/qga/qapi-schema.json
> > +++ b/qga/qapi-schema.json
> > @@ -1306,3 +1306,35 @@
> >   ##
> >   { 'command': 'guest-get-devices',
> >     'returns': ['GuestDeviceInfo'] }
> > +
> > +##
> > +# @guest-ssh-add-authorized-keys:
> > +#
> > +# @username: the user account to add the authorized key
> > +# @keys: the public keys to add (in OpenSSH format)
>
> You use plural but the code only seems to add (remove) one key
> at a time.

Uh, what makes you believe that?

>
> 'OpenSSH format' is confusing. From sshd(8):
>
>    Each line of the file contains one key (empty lines and lines
>    starting with a ‘#’ are ignored as comments).
>
>    Public keys consist of the following space-separated fields:
>
>      options, keytype, base64-encoded key, comment.
>
>    The options field is optional.
>
>    Note that lines in this file can be several hundred bytes long
>    (because of the size of the public key encoding) up to a limit
>    of 8 kilobytes, which permits RSA keys up to 16 kilobits.
>
>    The options (if present) consist of comma-separated option
>    specifications.  No spaces are permitted, except within double
>    quotes.
>
> @openssh_authorized_key_line is ugly, maybe use @authorized_key
> to make it clearer?

Why? the name of the function already implies we are talking about
authorized keys. The documentation says it's a public key in openssh
format (the ones you expect in ~/.ssh/authorized_keys files)

Yes the format isn't very well defined, so I did simple sanity checks.
After all, people usually append keys with shell >>. I can't find a
common command to do it with some checking.

> > +#
> > +# Append a public key to user $HOME/.ssh/authorized_keys on Unix systems (not
> > +# implemented for other systems).
>
> Here "a key" singular, good.

bad. it should be plural (everywhere else is plural, afaict)

>
> > +#
> > +# Returns: Nothing on success.
> > +#
> > +# Since: 5.2
> > +##
> > +{ 'command': 'guest-ssh-add-authorized-keys',
>
> Here "keys" plural :/
>
> > +  'data': { 'username': 'str', 'keys': ['str'] } }
> > +
> > +##
> > +# @guest-ssh-remove-authorized-keys:
> > +#
> > +# @username: the user account to add the authorized key
> > +# @keys: the public keys to remove (in OpenSSH format)
> > +#
> > +# Remove public keys from the user $HOME/.ssh/authorized_keys on Unix systems
> > +# (not implemented for other systems).
> > +#
> > +# Returns: Nothing on success.
> > +#
> > +# Since: 5.2
> > +##
> > +{ 'command': 'guest-ssh-remove-authorized-keys',
> > +  'data': { 'username': 'str', 'keys': ['str'] } }
> >
>
Philippe Mathieu-Daudé Oct. 14, 2020, 11:18 a.m. UTC | #3
On 10/14/20 9:37 AM, Marc-André Lureau wrote:
> On Wed, Oct 14, 2020 at 1:14 AM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
>> On 10/13/20 10:25 PM, marcandre.lureau@redhat.com wrote:
>>> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>>> index cec98c7e06..50e2854b45 100644
>>> --- a/qga/qapi-schema.json
>>> +++ b/qga/qapi-schema.json
>>> @@ -1306,3 +1306,35 @@
>>>    ##
>>>    { 'command': 'guest-get-devices',
>>>      'returns': ['GuestDeviceInfo'] }
>>> +
>>> +##
>>> +# @guest-ssh-add-authorized-keys:
>>> +#
>>> +# @username: the user account to add the authorized key
>>> +# @keys: the public keys to add (in OpenSSH format)
>>
>> You use plural but the code only seems to add (remove) one key
>> at a time.
> 
> Uh, what makes you believe that?

The code in your patch:

+static bool
+check_openssh_pub_keys(strList *keys, size_t *nkeys, Error **errp)
+{
+    size_t n = 0;
+    strList *k;
+
+    ERRP_GUARD();
+
+    for (k = keys; k != NULL; k = k->next) {
+        if (!check_openssh_pub_key(k->value, errp)) {
+            return false;
+        }
+        n++;
+    }
+
+    if (nkeys) {
+        *nkeys = n;
+    }
+    return true;
+}

> 
>>
>> 'OpenSSH format' is confusing. From sshd(8):
>>
>>     Each line of the file contains one key (empty lines and lines
>>     starting with a ‘#’ are ignored as comments).
>>
>>     Public keys consist of the following space-separated fields:
>>
>>       options, keytype, base64-encoded key, comment.
>>
>>     The options field is optional.
>>
>>     Note that lines in this file can be several hundred bytes long
>>     (because of the size of the public key encoding) up to a limit
>>     of 8 kilobytes, which permits RSA keys up to 16 kilobits.
>>
>>     The options (if present) consist of comma-separated option
>>     specifications.  No spaces are permitted, except within double
>>     quotes.
>>
>> @openssh_authorized_key_line is ugly, maybe use @authorized_key
>> to make it clearer?
> 
> Why? the name of the function already implies we are talking about
> authorized keys. The documentation says it's a public key in openssh
> format (the ones you expect in ~/.ssh/authorized_keys files)

OK then.

> 
> Yes the format isn't very well defined, so I did simple sanity checks.
> After all, people usually append keys with shell >>. I can't find a
> common command to do it with some checking.
Daniel P. Berrangé Oct. 15, 2020, 12:10 p.m. UTC | #4
On Wed, Oct 14, 2020 at 12:25:02AM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Add new commands to add and remove SSH public keys from
> ~/.ssh/authorized_keys.
> 
> I took a different approach for testing, including the unit tests right
> with the code. I wanted to overwrite the function to get the user
> details, I couldn't easily do that over QMP. Furthermore, I prefer
> having unit tests very close to the code, and unit files that are domain
> specific (commands-posix is too crowded already). Fwiw, that
> coding/testing style is Rust-style (where tests can or should even be
> part of the documentation!).
> 
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1885332
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qga/commands-posix-ssh.c | 394 +++++++++++++++++++++++++++++++++++++++
>  qga/commands-win32.c     |  10 +
>  qga/meson.build          |  17 +-
>  qga/qapi-schema.json     |  32 ++++
>  4 files changed, 452 insertions(+), 1 deletion(-)
>  create mode 100644 qga/commands-posix-ssh.c

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


> +##
> +# @guest-ssh-add-authorized-keys:
> +#
> +# @username: the user account to add the authorized key
> +# @keys: the public keys to add (in OpenSSH format)
> +#
> +# Append a public key to user $HOME/.ssh/authorized_keys on Unix systems (not
> +# implemented for other systems).

s/a public key/public keys/

> +#
> +# Returns: Nothing on success.
> +#
> +# Since: 5.2
> +##
> +{ 'command': 'guest-ssh-add-authorized-keys',
> +  'data': { 'username': 'str', 'keys': ['str'] } }
> +
> +##
> +# @guest-ssh-remove-authorized-keys:
> +#
> +# @username: the user account to add the authorized key
> +# @keys: the public keys to remove (in OpenSSH format)
> +#
> +# Remove public keys from the user $HOME/.ssh/authorized_keys on Unix systems
> +# (not implemented for other systems).

Mention that if the requested key does not exist, it is not an
error.

> +#
> +# Returns: Nothing on success.
> +#
> +# Since: 5.2
> +##
> +{ 'command': 'guest-ssh-remove-authorized-keys',
> +  'data': { 'username': 'str', 'keys': ['str'] } }

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
Eric Blake Oct. 19, 2020, 7:02 p.m. UTC | #5
On 10/13/20 3:25 PM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Add new commands to add and remove SSH public keys from
> ~/.ssh/authorized_keys.
> 

> +++ b/qga/qapi-schema.json
> @@ -1306,3 +1306,35 @@
>   ##
>   { 'command': 'guest-get-devices',
>     'returns': ['GuestDeviceInfo'] }
> +
> +##
> +# @guest-ssh-add-authorized-keys:
> +#
> +# @username: the user account to add the authorized key
> +# @keys: the public keys to add (in OpenSSH format)
> +#
> +# Append a public key to user $HOME/.ssh/authorized_keys on Unix systems (not

How is $HOME related to @username?

> +# implemented for other systems).
> +#
> +# Returns: Nothing on success.

Do we really need this line?

> +#
> +# Since: 5.2
> +##
> +{ 'command': 'guest-ssh-add-authorized-keys',
> +  'data': { 'username': 'str', 'keys': ['str'] } }

Should we use QAPI 'if' to avoid even having to compile a stub on 
Windows, and for better introspection (well, if we ever add a way to do 
qga introspection that parallels QMP's query-qmp-schema)?

> +
> +##
> +# @guest-ssh-remove-authorized-keys:
> +#
> +# @username: the user account to add the authorized key
> +# @keys: the public keys to remove (in OpenSSH format)
> +#
> +# Remove public keys from the user $HOME/.ssh/authorized_keys on Unix systems
> +# (not implemented for other systems).
> +#
> +# Returns: Nothing on success.
> +#
> +# Since: 5.2
> +##
> +{ 'command': 'guest-ssh-remove-authorized-keys',
> +  'data': { 'username': 'str', 'keys': ['str'] } }
>
Marc-André Lureau Oct. 20, 2020, 6:46 a.m. UTC | #6
Hi

On Mon, Oct 19, 2020 at 11:09 PM Eric Blake <eblake@redhat.com> wrote:

> On 10/13/20 3:25 PM, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Add new commands to add and remove SSH public keys from
> > ~/.ssh/authorized_keys.
> >
>
> > +++ b/qga/qapi-schema.json
> > @@ -1306,3 +1306,35 @@
> >   ##
> >   { 'command': 'guest-get-devices',
> >     'returns': ['GuestDeviceInfo'] }
> > +
> > +##
> > +# @guest-ssh-add-authorized-keys:
> > +#
> > +# @username: the user account to add the authorized key
> > +# @keys: the public keys to add (in OpenSSH format)
> > +#
> > +# Append a public key to user $HOME/.ssh/authorized_keys on Unix
> systems (not
>
> How is $HOME related to @username?
>

If it's not obvious, I could use help on how to formulate this. Would you
rather use the ~username/ syntax? Or just ~/ ?


> > +# implemented for other systems).
> > +#
> > +# Returns: Nothing on success.
>
> Do we really need this line?
>

For consistency, at least.


> > +#
> > +# Since: 5.2
> > +##
> > +{ 'command': 'guest-ssh-add-authorized-keys',
> > +  'data': { 'username': 'str', 'keys': ['str'] } }
>
> Should we use QAPI 'if' to avoid even having to compile a stub on
> Windows, and for better introspection (well, if we ever add a way to do
> qga introspection that parallels QMP's query-qmp-schema)?
>

There is no 'if' usage in QGA schema. As you point out, there is no
introspection command atm. But we can start using it here, I guess.



> > +
> > +##
> > +# @guest-ssh-remove-authorized-keys:
> > +#
> > +# @username: the user account to add the authorized key
> > +# @keys: the public keys to remove (in OpenSSH format)
> > +#
> > +# Remove public keys from the user $HOME/.ssh/authorized_keys on Unix
> systems
> > +# (not implemented for other systems).
> > +#
> > +# Returns: Nothing on success.
> > +#
> > +# Since: 5.2
> > +##
> > +{ 'command': 'guest-ssh-remove-authorized-keys',
> > +  'data': { 'username': 'str', 'keys': ['str'] } }
> >
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
>
>
>
thanks
diff mbox series

Patch

diff --git a/qga/commands-posix-ssh.c b/qga/commands-posix-ssh.c
new file mode 100644
index 0000000000..3721ac4975
--- /dev/null
+++ b/qga/commands-posix-ssh.c
@@ -0,0 +1,394 @@ 
+ /*
+  * This work is licensed under the terms of the GNU GPL, version 2 or later.
+  * See the COPYING file in the top-level directory.
+  */
+#include "qemu/osdep.h"
+
+#include <glib-unix.h>
+#include <glib/gstdio.h>
+#include <locale.h>
+#include <pwd.h>
+
+#include "qapi/error.h"
+#include "qga-qapi-commands.h"
+
+#ifdef QGA_BUILD_UNIT_TEST
+static struct passwd *
+test_get_passwd_entry(const gchar *user_name, GError **error)
+{
+    struct passwd *p;
+    int ret;
+
+    if (!user_name || g_strcmp0(user_name, g_get_user_name())) {
+        g_set_error_literal(error, G_FILE_ERROR, G_FILE_ERROR_FAILED, "Invalid user name");
+        return NULL;
+    }
+
+    p = g_new0(struct passwd, 1);
+    p->pw_dir = (char *)g_get_home_dir();
+    p->pw_uid = geteuid();
+    p->pw_gid = getegid();
+
+    ret = g_mkdir_with_parents(p->pw_dir, 0700);
+    g_assert_cmpint(ret, ==, 0);
+
+    return p;
+}
+
+#define g_unix_get_passwd_entry_qemu(username, err) test_get_passwd_entry(username, err)
+#endif
+
+static struct passwd *
+get_passwd_entry(const char *username, Error **errp)
+{
+    g_autoptr(GError) err = NULL;
+    struct passwd *p;
+
+    ERRP_GUARD();
+
+    p = g_unix_get_passwd_entry_qemu(username, &err);
+    if (p == NULL) {
+        error_setg(errp, "failed to lookup user '%s': %s",
+                   username, err->message);
+        return NULL;
+    }
+
+    return p;
+}
+
+static bool
+mkdir_for_user(const char *path, const struct passwd *p, mode_t mode, Error **errp)
+{
+    ERRP_GUARD();
+
+    if (g_mkdir(path, mode) == -1) {
+        error_setg(errp, "failed to create directory '%s': %s",
+                   path, g_strerror(errno));
+        return false;
+    }
+
+    if (chown(path, p->pw_uid, p->pw_gid) == -1) {
+        error_setg(errp, "failed to set ownership of directory '%s': %s",
+                   path, g_strerror(errno));
+        return false;
+    }
+
+    if (chmod(path, mode) == -1) {
+        error_setg(errp, "failed to set permissions of directory '%s': %s",
+                   path, g_strerror(errno));
+        return false;
+    }
+
+    return true;
+}
+
+static bool
+check_openssh_pub_key(const char *key, Error **errp)
+{
+    ERRP_GUARD();
+
+    /* simple sanity-check, we may want more? */
+    if (!key || key[0] == '#' || strchr(key, '\n')) {
+        error_setg(errp, "invalid OpenSSH public key: '%s'", key);
+        return false;
+    }
+
+    return true;
+}
+
+static bool
+check_openssh_pub_keys(strList *keys, size_t *nkeys, Error **errp)
+{
+    size_t n = 0;
+    strList *k;
+
+    ERRP_GUARD();
+
+    for (k = keys; k != NULL; k = k->next) {
+        if (!check_openssh_pub_key(k->value, errp)) {
+            return false;
+        }
+        n++;
+    }
+
+    if (nkeys) {
+        *nkeys = n;
+    }
+    return true;
+}
+
+static bool
+write_authkeys(const char *path, const GStrv keys, Error **errp)
+{
+    g_autofree char *contents = NULL;
+    g_autoptr(GError) err = NULL;
+
+    ERRP_GUARD();
+
+    contents = g_strjoinv("\n", keys);
+    if (!g_file_set_contents(path, contents, -1, &err)) {
+        error_setg(errp, "failed to write to '%s': %s", path, err->message);
+        return false;
+    }
+
+    if (chmod(path, 0600) == -1) {
+        error_setg(errp, "failed to set permissions of '%s': %s",
+                   path, g_strerror(errno));
+        return false;
+    }
+
+    return true;
+}
+
+static GStrv
+read_authkeys(const char *path, Error **errp)
+{
+    g_autoptr(GError) err = NULL;
+    g_autofree char *contents = NULL;
+
+    ERRP_GUARD();
+
+    if (!g_file_get_contents(path, &contents, NULL, &err)) {
+        error_setg(errp, "failed to read '%s': %s", path, err->message);
+        return NULL;
+    }
+
+    return g_strsplit(contents, "\n", -1);
+
+}
+
+void
+qmp_guest_ssh_add_authorized_keys(const char *username, strList *keys,
+                                  Error **errp)
+{
+    g_autofree struct passwd *p = NULL;
+    g_autofree char *ssh_path = NULL;
+    g_autofree char *authkeys_path = NULL;
+    g_auto(GStrv) authkeys = NULL;
+    strList *k;
+    size_t nkeys, nauthkeys;
+
+    ERRP_GUARD();
+
+    if (!check_openssh_pub_keys(keys, &nkeys, errp)) {
+        return;
+    }
+
+    p = get_passwd_entry(username, errp);
+    if (p == NULL) {
+        return;
+    }
+
+    ssh_path = g_build_filename(p->pw_dir, ".ssh", NULL);
+    authkeys_path = g_build_filename(ssh_path, "authorized_keys", NULL);
+
+    authkeys = read_authkeys(authkeys_path, NULL);
+    if (authkeys == NULL) {
+        if (!g_file_test(ssh_path, G_FILE_TEST_IS_DIR) &&
+            !mkdir_for_user(ssh_path, p, 0700, errp)) {
+            return;
+        }
+    }
+
+    nauthkeys = authkeys ? g_strv_length(authkeys) : 0;
+    authkeys = g_realloc_n(authkeys, nauthkeys + nkeys + 1, sizeof(char *));
+    memset(authkeys + nauthkeys, 0, (nkeys + 1) * sizeof(char *));
+
+    for (k = keys; k != NULL; k = k->next) {
+        if (g_strv_contains((const gchar * const *)authkeys, k->value)) {
+            continue;
+        }
+        authkeys[nauthkeys++] = g_strdup(k->value);
+    }
+
+    write_authkeys(authkeys_path, authkeys, errp);
+}
+
+void
+qmp_guest_ssh_remove_authorized_keys(const char *username, strList *keys,
+                                     Error **errp)
+{
+    g_autofree struct passwd *p = NULL;
+    g_autofree char *authkeys_path = NULL;
+    g_autofree GStrv new_keys = NULL; /* do not own the strings */
+    g_auto(GStrv) authkeys = NULL;
+    GStrv a;
+    size_t nkeys = 0;
+
+    ERRP_GUARD();
+
+    if (!check_openssh_pub_keys(keys, NULL, errp)) {
+        return;
+    }
+
+    p = get_passwd_entry(username, errp);
+    if (p == NULL) {
+        return;
+    }
+
+    authkeys_path = g_build_filename(p->pw_dir, ".ssh",
+                                     "authorized_keys", NULL);
+    if (!g_file_test(authkeys_path, G_FILE_TEST_EXISTS)) {
+        return;
+    }
+    authkeys = read_authkeys(authkeys_path, errp);
+    if (authkeys == NULL) {
+        return;
+    }
+
+    new_keys = g_new0(char *, g_strv_length(authkeys) + 1);
+    for (a = authkeys; *a != NULL; a++) {
+        strList *k;
+
+        for (k = keys; k != NULL; k = k->next) {
+            if (g_str_equal(k->value, *a)) {
+                break;
+            }
+        }
+        if (k != NULL) {
+            continue;
+        }
+
+        new_keys[nkeys++] = *a;
+    }
+
+    write_authkeys(authkeys_path, new_keys, errp);
+}
+
+
+#ifdef QGA_BUILD_UNIT_TEST
+static const strList test_key2 = {
+    .value = (char *)"algo key2 comments"
+};
+
+static const strList test_key1_2 = {
+    .value = (char *)"algo key1 comments",
+    .next = (strList *)&test_key2,
+};
+
+static char *
+test_get_authorized_keys_path(void)
+{
+    return g_build_filename(g_get_home_dir(), ".ssh", "authorized_keys", NULL);
+}
+
+static void
+test_authorized_keys_set(const char *contents)
+{
+    g_autoptr(GError) err = NULL;
+    g_autofree char *path = NULL;
+    int ret;
+
+    path = g_build_filename(g_get_home_dir(), ".ssh", NULL);
+    ret = g_mkdir_with_parents(path, 0700);
+    g_assert_cmpint(ret, ==, 0);
+    g_free(path);
+
+    path = test_get_authorized_keys_path();
+    g_file_set_contents(path, contents, -1, &err);
+    g_assert_no_error(err);
+}
+
+static void
+test_authorized_keys_equal(const char *expected)
+{
+    g_autoptr(GError) err = NULL;
+    g_autofree char *path = NULL;
+    g_autofree char *contents = NULL;
+
+    path = test_get_authorized_keys_path();
+    g_file_get_contents(path, &contents, NULL, &err);
+    g_assert_no_error(err);
+
+    g_assert_cmpstr(contents, ==, expected);
+}
+
+static void
+test_invalid_user(void)
+{
+    Error *err = NULL;
+
+    qmp_guest_ssh_add_authorized_keys("", NULL, &err);
+    error_free_or_abort(&err);
+
+    qmp_guest_ssh_remove_authorized_keys("", NULL, &err);
+    error_free_or_abort(&err);
+}
+
+static void
+test_invalid_key(void)
+{
+    strList key = {
+        .value = (char *)"not a valid\nkey"
+    };
+    Error *err = NULL;
+
+    qmp_guest_ssh_add_authorized_keys(g_get_user_name(), &key, &err);
+    error_free_or_abort(&err);
+
+    qmp_guest_ssh_remove_authorized_keys(g_get_user_name(), &key, &err);
+    error_free_or_abort(&err);
+}
+
+static void
+test_add_keys(void)
+{
+    Error *err = NULL;
+
+    qmp_guest_ssh_add_authorized_keys(g_get_user_name(),
+                                      (strList *)&test_key2, &err);
+    g_assert_null(err);
+
+    test_authorized_keys_equal("algo key2 comments");
+
+    qmp_guest_ssh_add_authorized_keys(g_get_user_name(),
+                                      (strList *)&test_key1_2, &err);
+    g_assert_null(err);
+
+    /*  key2 came first, and should'nt be duplicated */
+    test_authorized_keys_equal("algo key2 comments\n"
+                               "algo key1 comments");
+}
+
+static void
+test_remove_keys(void)
+{
+    Error *err = NULL;
+    static const char *authkeys =
+        "algo key1 comments\n"
+        /* originally duplicated */
+        "algo key1 comments\n"
+        "# a commented line\n"
+        "algo some-key another\n";
+
+    test_authorized_keys_set(authkeys);
+    qmp_guest_ssh_remove_authorized_keys(g_get_user_name(),
+                                         (strList *)&test_key2, &err);
+    g_assert_null(err);
+    test_authorized_keys_equal(authkeys);
+
+    qmp_guest_ssh_remove_authorized_keys(g_get_user_name(),
+                                         (strList *)&test_key1_2, &err);
+    g_assert_null(err);
+    test_authorized_keys_equal("# a commented line\n"
+                               "algo some-key another\n");
+}
+
+int main(int argc, char *argv[])
+{
+#if GLIB_CHECK_VERSION(2, 60, 0)
+    setlocale(LC_ALL, "");
+
+    g_test_init(&argc, &argv, G_TEST_OPTION_ISOLATE_DIRS, NULL);
+
+    g_test_add_func("/qga/ssh/invalid_user", test_invalid_user);
+    g_test_add_func("/qga/ssh/invalid_key", test_invalid_key);
+    g_test_add_func("/qga/ssh/add_keys", test_add_keys);
+    g_test_add_func("/qga/ssh/remove_keys", test_remove_keys);
+
+    return g_test_run();
+#else
+    g_test_message("test skipped, needs glib >= 2.60");
+#endif
+}
+#endif /*  BUILD_UNIT_TEST */
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 0c3c05484f..27ac3f24f5 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -2457,3 +2457,13 @@  GuestDeviceInfoList *qmp_guest_get_devices(Error **errp)
     }
     return head;
 }
+
+void qmp_guest_ssh_add_authorized_keys(const char *username, strList *keys, Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+}
+
+void qmp_guest_ssh_remove_authorized_keys(const char *username, strList *keys, Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+}
diff --git a/qga/meson.build b/qga/meson.build
index cd08bd953a..58303c70d8 100644
--- a/qga/meson.build
+++ b/qga/meson.build
@@ -35,7 +35,9 @@  qga_ss.add(files(
 ))
 qga_ss.add(when: 'CONFIG_POSIX', if_true: files(
   'channel-posix.c',
-  'commands-posix.c'))
+  'commands-posix.c',
+  'commands-posix-ssh.c',
+))
 qga_ss.add(when: 'CONFIG_WIN32', if_true: files(
   'channel-win32.c',
   'commands-win32.c',
@@ -87,3 +89,16 @@  else
 endif
 
 alias_target('qemu-ga', all_qga)
+
+qga_ssh_test = executable('qga-ssh-test',
+                          files('commands-posix-ssh.c'),
+                          dependencies: [qemuutil],
+                          c_args: ['-DQGA_BUILD_UNIT_TEST'])
+
+test_env = environment()
+test_env.set('G_TEST_SRCDIR', meson.current_source_dir())
+test_env.set('G_TEST_BUILDDIR', meson.current_build_dir())
+test('qga-ssh-test',
+     qga_ssh_test,
+     env: test_env,
+     suite: ['unit', 'qga'])
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index cec98c7e06..50e2854b45 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1306,3 +1306,35 @@ 
 ##
 { 'command': 'guest-get-devices',
   'returns': ['GuestDeviceInfo'] }
+
+##
+# @guest-ssh-add-authorized-keys:
+#
+# @username: the user account to add the authorized key
+# @keys: the public keys to add (in OpenSSH format)
+#
+# Append a public key to user $HOME/.ssh/authorized_keys on Unix systems (not
+# implemented for other systems).
+#
+# Returns: Nothing on success.
+#
+# Since: 5.2
+##
+{ 'command': 'guest-ssh-add-authorized-keys',
+  'data': { 'username': 'str', 'keys': ['str'] } }
+
+##
+# @guest-ssh-remove-authorized-keys:
+#
+# @username: the user account to add the authorized key
+# @keys: the public keys to remove (in OpenSSH format)
+#
+# Remove public keys from the user $HOME/.ssh/authorized_keys on Unix systems
+# (not implemented for other systems).
+#
+# Returns: Nothing on success.
+#
+# Since: 5.2
+##
+{ 'command': 'guest-ssh-remove-authorized-keys',
+  'data': { 'username': 'str', 'keys': ['str'] } }