Message ID | 20201013202502.335336-3-marcandre.lureau@redhat.com |
---|---|
State | New |
Headers | show |
Series | qemu-ga: add ssh-{add,remove}-authorized-keys | expand |
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'] } } >
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'] } } > > >
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.
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 -- |: 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 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'] } } > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
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 -- Marc-André Lureau <div dir="ltr"><div><div dir="ltr">Hi<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Oct 19, 2020 at 11:09 PM Eric Blake <<a href="mailto:eblake@redhat.com">eblake@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 10/13/20 3:25 PM, <a href="mailto:marcandre.lureau@redhat.com" target="_blank">marcandre.lureau@redhat.com</a> wrote:<br> > From: Marc-André Lureau <<a href="mailto:marcandre.lureau@redhat.com" target="_blank">marcandre.lureau@redhat.com</a>><br> > <br> > Add new commands to add and remove SSH public keys from<br> > ~/.ssh/authorized_keys.<br> > <br> <br> > +++ b/qga/qapi-schema.json<br> > @@ -1306,3 +1306,35 @@<br> > ##<br> > { 'command': 'guest-get-devices',<br> > 'returns': ['GuestDeviceInfo'] }<br> > +<br> > +##<br> > +# @guest-ssh-add-authorized-keys:<br> > +#<br> > +# @username: the user account to add the authorized key<br> > +# @keys: the public keys to add (in OpenSSH format)<br> > +#<br> > +# Append a public key to user $HOME/.ssh/authorized_keys on Unix systems (not<br> <br> How is $HOME related to @username?<br></blockquote><div><br></div><div>If it's not obvious, I could use help on how to formulate this. Would you rather use the ~username/ syntax? Or just ~/ ?<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> > +# implemented for other systems).<br> > +#<br> > +# Returns: Nothing on success.<br> <br> Do we really need this line?<br></blockquote><div><br></div><div>For consistency, at least.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> > +#<br> > +# Since: 5.2<br> > +##<br> > +{ 'command': 'guest-ssh-add-authorized-keys',<br> > + 'data': { 'username': 'str', 'keys': ['str'] } }<br> <br> Should we use QAPI 'if' to avoid even having to compile a stub on <br> Windows, and for better introspection (well, if we ever add a way to do <br> qga introspection that parallels QMP's query-qmp-schema)?<br></blockquote><div><br></div><div>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.<br></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> > +<br> > +##<br> > +# @guest-ssh-remove-authorized-keys:<br> > +#<br> > +# @username: the user account to add the authorized key<br> > +# @keys: the public keys to remove (in OpenSSH format)<br> > +#<br> > +# Remove public keys from the user $HOME/.ssh/authorized_keys on Unix systems<br> > +# (not implemented for other systems).<br> > +#<br> > +# Returns: Nothing on success.<br> > +#<br> > +# Since: 5.2<br> > +##<br> > +{ 'command': 'guest-ssh-remove-authorized-keys',<br> > + 'data': { 'username': 'str', 'keys': ['str'] } }<br> > <br> <br> -- <br> Eric Blake, Principal Software Engineer<br> Red Hat, Inc. +1-919-301-3226<br> Virtualization: <a href="http://qemu.org" rel="noreferrer" target="_blank">qemu.org</a> | <a href="http://libvirt.org" rel="noreferrer" target="_blank">libvirt.org</a><br> <br> <br> </blockquote></div><br clear="all"></div><div>thanks</div><div><br></div><div>-- <br><div dir="ltr" class="gmail_signature">Marc-André Lureau<br></div></div></div>
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'] } }