[PATCH 0/2] qemu-ga: add ssh-{add,remove}-authorized-keys

[PATCH 0/2] qemu-ga: add ssh-{add,remove}-authorized-keys

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

Add two new commands to help modify ~/.ssh/authorized_keys.

Although it's possible already to modify the authorized_keys files via
file-{read,write} or exec, the commands are often denied by default, and the
logic is left to the client. Let's add specific commands for this job.

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

Marc-André Lureau (2):
  glib-compat: add g_unix_get_passwd_entry_qemu()
  qga: add ssh-{add,remove}-authorized-keys

 include/glib-compat.h    |  24 +++
 qga/commands-posix-ssh.c | 394 +++++++++++++++++++++++++++++++++++++++
 qga/commands-win32.c     |  10 +
 qga/meson.build          |  17 +-
 qga/qapi-schema.json     |  32 ++++
 5 files changed, 476 insertions(+), 1 deletion(-)
 create mode 100644 qga/commands-posix-ssh.c

-- 
2.28.0

[PATCH 1/2] glib-compat: add g_unix_get_passwd_entry_qemu()

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The glib function was introduced in 2.64. It's a safer version of
getpwnam, and also simpler to use than getpwnam_r.

Currently, it's only use by the next patch in qemu-ga, which doesn't
(well well...) need the thread safety guarantees. Since the fallback
version is still unsafe, I would rather keep the _qemu postfix, to make
sure it's not being misused by mistake. When/if necessary, we can
implement a safer fallback and drop the _qemu suffix.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/glib-compat.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/glib-compat.h b/include/glib-compat.h
index 0b0ec76299..2a56b3462b 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -30,6 +30,9 @@
 #pragma GCC diagnostic ignored "-Wdeprecated-declarations"
 
 #include <glib.h>
+#if defined(G_OS_UNIX)
+#include <glib-unix.h>
+#endif
 
 /*
  * Note that because of the GLIB_VERSION_MAX_ALLOWED constant above, allowing
@@ -72,6 +75,27 @@
 gint g_poll_fixed(GPollFD *fds, guint nfds, gint timeout);
 #endif
 
+#if defined(G_OS_UNIX)
+/* Note: The fallback implementation is not MT-safe, and it returns a copy of
+ * the libc passwd (must be g_free() after use) but not the content. Because of
+ * these important differences the caller must be aware of, it's not #define for
+ * GLib API substitution. */
+static inline struct passwd *
+g_unix_get_passwd_entry_qemu(const gchar *user_name, GError **error)
+{
+#if GLIB_CHECK_VERSION(2, 64, 0)
+    return g_unix_get_passwd_entry(user_name, error);
+#else
+    struct passwd *p = getpwnam(user_name);
+    if (!p) {
+        g_set_error(error, G_UNIX_ERROR, 0, g_strerror(errno));
+        return NULL;
+    }
+    return g_memdup(p, sizeof(*p));
+#endif
+}
+#endif /* G_OS_UNIX */
+
 #pragma GCC diagnostic pop
 
 #endif
-- 
2.28.0

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

[marcandre.lureau]

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

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'] } }
-- 
2.28.0

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

[Philippe Mathieu-Daudé]

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'] } }
> 

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

[Marc-André Lureau]

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'] } }
> >
>

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

[Philippe Mathieu-Daudé]

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.

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

[Daniel P. Berrangé]

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 :|

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

[Eric Blake]

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

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

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2276 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 3916 bytes --]

Re: [PATCH 0/2] qemu-ga: add ssh-{add,remove}-authorized-keys

[no-reply]

Patchew URL: https://patchew.org/QEMU/20201013202502.335336-1-marcandre.lureau@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20201013202502.335336-1-marcandre.lureau@redhat.com
Subject: [PATCH 0/2] qemu-ga: add ssh-{add,remove}-authorized-keys

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20201013103532.13391-1-peter.maydell@linaro.org -> patchew/20201013103532.13391-1-peter.maydell@linaro.org
 - [tag update]      patchew/20201013172223.443645-1-georg.kotheimer@kernkonzept.com -> patchew/20201013172223.443645-1-georg.kotheimer@kernkonzept.com
 * [new tag]         patchew/20201013202502.335336-1-marcandre.lureau@redhat.com -> patchew/20201013202502.335336-1-marcandre.lureau@redhat.com
Switched to a new branch 'test'
66c8929 qga: add ssh-{add,remove}-authorized-keys
6aa612c glib-compat: add g_unix_get_passwd_entry_qemu()

=== OUTPUT BEGIN ===
1/2 Checking commit 6aa612cc67ad (glib-compat: add g_unix_get_passwd_entry_qemu())
WARNING: Block comments use a leading /* on a separate line
#38: FILE: include/glib-compat.h:79:
+/* Note: The fallback implementation is not MT-safe, and it returns a copy of

WARNING: Block comments use a trailing */ on a separate line
#41: FILE: include/glib-compat.h:82:
+ * GLib API substitution. */

total: 0 errors, 2 warnings, 36 lines checked

Patch 1/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/2 Checking commit 66c89292c34c (qga: add ssh-{add,remove}-authorized-keys)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#25: 
new file mode 100644

ERROR: line over 90 characters
#52: FILE: qga/commands-posix-ssh.c:23:
+        g_set_error_literal(error, G_FILE_ERROR, G_FILE_ERROR_FAILED, "Invalid user name");

ERROR: Use g_assert or g_assert_not_reached
#62: FILE: qga/commands-posix-ssh.c:33:
+    g_assert_cmpint(ret, ==, 0);

WARNING: line over 80 characters
#67: FILE: qga/commands-posix-ssh.c:38:
+#define g_unix_get_passwd_entry_qemu(username, err) test_get_passwd_entry(username, err)

WARNING: line over 80 characters
#89: FILE: qga/commands-posix-ssh.c:60:
+mkdir_for_user(const char *path, const struct passwd *p, mode_t mode, Error **errp)

ERROR: Use g_assert or g_assert_not_reached
#313: FILE: qga/commands-posix-ssh.c:284:
+    g_assert_cmpint(ret, ==, 0);

ERROR: Use g_assert or g_assert_not_reached
#318: FILE: qga/commands-posix-ssh.c:289:
+    g_assert_no_error(err);

ERROR: Use g_assert or g_assert_not_reached
#330: FILE: qga/commands-posix-ssh.c:301:
+    g_assert_no_error(err);

ERROR: Use g_assert or g_assert_not_reached
#332: FILE: qga/commands-posix-ssh.c:303:
+    g_assert_cmpstr(contents, ==, expected);

ERROR: Use g_assert or g_assert_not_reached
#369: FILE: qga/commands-posix-ssh.c:340:
+    g_assert_null(err);

ERROR: Use g_assert or g_assert_not_reached
#375: FILE: qga/commands-posix-ssh.c:346:
+    g_assert_null(err);

ERROR: Use g_assert or g_assert_not_reached
#396: FILE: qga/commands-posix-ssh.c:367:
+    g_assert_null(err);

ERROR: Use g_assert or g_assert_not_reached
#401: FILE: qga/commands-posix-ssh.c:372:
+    g_assert_null(err);

WARNING: line over 80 characters
#433: FILE: qga/commands-win32.c:2461:
+void qmp_guest_ssh_add_authorized_keys(const char *username, strList *keys, Error **errp)

ERROR: line over 90 characters
#438: FILE: qga/commands-win32.c:2466:
+void qmp_guest_ssh_remove_authorized_keys(const char *username, strList *keys, Error **errp)

total: 11 errors, 4 warnings, 468 lines checked

Patch 2/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20201013202502.335336-1-marcandre.lureau@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/2] qemu-ga: add ssh-{add,remove}-authorized-keys

[Michal Privoznik]

On 10/13/20 10:25 PM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Hi,
> 
> Add two new commands to help modify ~/.ssh/authorized_keys.

Apart from Philippe's comments, this path is configurable in 
sshd.config. But I doubt we should care as ssh-copy-id doesn't care.

> 
> Although it's possible already to modify the authorized_keys files via
> file-{read,write} or exec, the commands are often denied by default, and the
> logic is left to the client. Let's add specific commands for this job.
> 
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1885332
> 
> Marc-André Lureau (2):
>    glib-compat: add g_unix_get_passwd_entry_qemu()
>    qga: add ssh-{add,remove}-authorized-keys
> 
>   include/glib-compat.h    |  24 +++
>   qga/commands-posix-ssh.c | 394 +++++++++++++++++++++++++++++++++++++++
>   qga/commands-win32.c     |  10 +
>   qga/meson.build          |  17 +-
>   qga/qapi-schema.json     |  32 ++++
>   5 files changed, 476 insertions(+), 1 deletion(-)
>   create mode 100644 qga/commands-posix-ssh.c
> 

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

Re: [PATCH 0/2] qemu-ga: add ssh-{add,remove}-authorized-keys

[Daniel P. Berrangé]

On Wed, Oct 14, 2020 at 12:25:00AM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Hi,
> 
> Add two new commands to help modify ~/.ssh/authorized_keys.
> 
> Although it's possible already to modify the authorized_keys files via
> file-{read,write} or exec, the commands are often denied by default, and the
> logic is left to the client. Let's add specific commands for this job.

More importantly the mgmt app has no idea what file location the keys
need to be saved in. Knowing the user isn't sufficient as you cannot
assume that $HOME is /home/$USERNAME - it could be in an arbitrarily
different location. So having dedicated commands for this which can
use getpwent in the guest to find $HOME is mcuh saner.


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 :|