mbox series

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

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

Message

Marc-André Lureau Oct. 13, 2020, 8:25 p.m. UTC 
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

Comments

no-reply@patchew.org Oct. 13, 2020, 8:33 p.m. UTC | #1 
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
Michal Prívozník Oct. 14, 2020, 10:43 a.m. UTC | #2 
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
Daniel P. Berrangé Oct. 14, 2020, 10:49 a.m. UTC | #3 
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 :|