On Thu, Nov 12, 2020 at 12:55:23 +0100, Michal Privoznik wrote:
> On 11/11/20 1:04 PM, Michal Privoznik wrote:
> > On 11/11/20 11:32 AM, Peter Krempa wrote:
> > > On Tue, Nov 10, 2020 at 16:11:40 +0100, Michal Privoznik wrote:
> > > > Marc-André posted a patch that implements agent handling. I've written
> > > > the rest.
> > > >
> > > > Marc-André Lureau (1):
> > > > Â Â qemu_agent: add qemuAgentSSH{Add,Remove,Get}AuthorizedKeys
> > > >
> > > > Michal PrÃvoznÃk (5):
> > > > Â Â Introduce OpenSSH authorized key file mgmt APIs
> > >
> > > One more thing to think about:
> > >
> > > Since we are getting random requests for setters of various bits which
> > > we have to bend the rule "we don't care what's running in the VM" and
> > > which don't really scale when adding new APIs. I propose we add a
> > > generic guest agent setter which will be extensible using a typed
> > > parameters and a type property.
> > >
> > > It will basically become the counterpart to virDomainGetGuestInfo.
> > >
> > > The extensions then become enum additions and code additions only and
> > > will be more flexible for future use.
> > >
> > > The same way the getter forthe ssh keys should become part of
> > > virDomainGetGuestInfo, obviously auditing whether a read-write
> > > connection is used.
> > >
> > > example:
> > >
> > > int
> > > qemuDomainSetGuestInfo(virDomainPtr dom,
> > > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â virDomainSetGuestInfoType type,
> > > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â virTypedParamPtr params,
> > >                        unsigned int nparams,
> > > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â unsigned int flags);
> > >
> > > Invocation for setting keys:
> > >
> > > virTypedParamsAddString(..., "user", "root")
> > > virTypedParamsAddString(..., "key", "ssh-rsa AA.... root@localhost")
> > > virTypedParamsAddString(..., "key", "ssh-rsa AA.... user@localhost")
> > >
> > > etc.
> > >
> >
> > Yeah, this is much more extensible. Okay, let me send v2.
>
> My enthusiasm might had been premature. virDomainGetGuestInfo() does not
> send anything but virDomainPtr, unsigned int types and flags down the wire.
> While we can make @params be both in and out type of arguments, it's going
> to require change of RPC. I mean the way that qemu-ga APIs are implemented
> is that for listing ssh keys the API expects an user on the input so that it
> can construct $HOME/.ssh/authorized_keys path. How to pass this through
> virDomainGetGuestInfo()? Okay, we could work around it by just listing SSH
> keys for all users and let caller filter our the interesting ones, but this
> is: a) scary from security POV, b) suboptimal because we might hit message
> size limit pretty soon. Also, there is no qemu-ga API to list all users,
> just those logged in currently. And the whole point of these new APIs is to
> set up SSH keys before user logs in.
Yeah, using virDomainGetGuestInfo as the getter for the keys will not
work.
What I ultimately want to suggest is something more generic which will
allow driving guest agent APIs libvirt doesn't actually care about
without having to add single-use APIs for every single one of them.
qemu-ga seems to support following APIs:
$ ./qemu-ga --blacklist=?
guest-sync-delimited
guest-sync
guest-ping
guest-get-time
guest-set-time
guest-info
guest-shutdown
guest-file-open
guest-file-close
guest-file-read
guest-file-write
guest-file-seek
guest-file-flush
guest-fsfreeze-status
guest-fsfreeze-freeze
guest-fsfreeze-freeze-list
guest-fsfreeze-thaw
guest-fstrim
guest-suspend-disk
guest-suspend-ram
guest-suspend-hybrid
guest-network-get-interfaces
guest-get-vcpus
guest-set-vcpus
guest-get-disks
guest-get-fsinfo
guest-set-user-password
guest-get-memory-blocks
guest-set-memory-blocks
guest-get-memory-block-info
guest-exec-status
guest-exec
guest-get-host-name
guest-get-users
guest-get-timezone
guest-get-osinfo
guest-get-devices
guest-ssh-get-authorized-keys
guest-ssh-add-authorized-keys
guest-ssh-remove-authorized-keys
Let's categorize them:
Utility functions:
guest-sync-delimited
guest-sync
guest-ping
Functions which at least partially map to libvirt APIs:
guest-shutdown
guest-get-vcpus
guest-set-vcpus
Functions used internally but also useful externally:
guest-fsfreeze-status
guest-fsfreeze-freeze
guest-fsfreeze-freeze-list
guest-fsfreeze-thaw
Functions which can be claimed to map to libvirt but don't really:
guest-suspend-disk
guest-suspend-ram
guest-suspend-hybrid
Functions which are not really relevant to libvirt but we expose wrappers:
guest-get-time
guest-set-time
guest-fstrim
guest-network-get-interfaces
guest-get-disks
guest-get-fsinfo
guest-get-host-name
guest-get-users
guest-get-timezone
guest-get-osinfo
Functions which are not really relevant to libvirt, we expose wrappers
and they are scary:
guest-set-user-password
Functions which are not really relevant to libvirt, we don't expose wrappers
and they are scary:
guest-file-open
guest-file-close
guest-file-read
guest-file-write
guest-file-seek
guest-file-flush
guest-exec-status
guest-exec
Other stats functions we might be asked for:
guest-info
guest-get-memory-blocks
guest-set-memory-blocks
guest-get-memory-block-info
guest-get-devices
And finally those we have here:
guest-ssh-get-authorized-keys
guest-ssh-add-authorized-keys
guest-ssh-remove-authorized-keys
So there's just a very tiny set of QGA APIs we really need.
For the rest we are adding more-or-less direct wrappers to satisfy the
access to the APIs.
With more-and-more APIs added which really deal just with the guest
state itself it's a bit weird to add libvirt APIs for them.
I'd like us to prevent adding new APIs constantly especially those which
deal just with the guest.
Since libvirt actually can't ever rely on the state of the guest agent
as it can be restarted at any time including the guest OS and thus we
we have to internally treat it without any presumptions about state and
re-probe everything and we really need just a tiny
subset of the commands, we'd really benefint from just multiplexing access
between libvirt and any other APP which can use the QGA QMP protocol
directly.
Thus I think we could arguably make virDomainQemuAgentCommand a
supported API. Any guest state change which can be initiated via the GA
needs to be supported also without that since the guest itself can
trigger it.
Saying that virDomainQemuAgentCommand is fully supported to be used
would free us from having to add arbitrary unextendable APIs for every
single guest agent API, but would still allow libvirt to use APIs we
need.
We just need to make 100% sure that it can't be abused as argument to
to change status of virDomainQemuMonitorCommand.
