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