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