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.