Moving this to RHEL-AV first and adding CNV as the dependent product.  Once something is completed in RHEL-AV, then we can decide where it needs to be cloned for RHEL.

Is this only about appending keys to /root/.ssh/authorized_keys and/or various $HOME ?

If a qemu-ga user can do that for /root, he can effectively become root easily, and gain all privileges, changing qemu-ga configuration etc. (see also bug 1885341)

If the main reason to implement a new command is to limit what the qemu-ga user can do, it shouldn't be able to set /root keys, I guess.

Oh wait, you can just change root password already.
Eh, why do we have a qemu-ga command denylist in the end?

Or the ability to touch root account should be a configurable setting, either in QGA config, or in a SELinux policy tunable, or both.

> Eh, why do we have a qemu-ga command denylist in the end?

We got to this conclusion as well, once we noticed that root pw can be changed.

However, to be fair, setting the root pw does still require one to gain access to the console.

> Or the ability to touch root account should be a configurable setting, either in QGA config, or in a SELinux policy tunable, or both.

Could you explain this a little more?

Would this command work for you?

##
# @guest-ssh-add-authorized-key
#
# @username: the user account to add the authorized key
# @key: the public key to add (in OpenSSH format)
#
# Append a public key to user $HOME/.ssh/authorized_keys on Unix systems (not
# implemented for other systems).
#
# Returns: Nothing on success.
#
# Since: 5.X
##
{ 'command': 'guest-ssh-add-authorized-key',
  'data': { 'username': 'str', 'key': 'str' } }

> Would this command work for you?
> ##
> # @guest-ssh-add-authorized-key
> #
> # @username: the user account to add the authorized key
> # @key: the public key to add (in OpenSSH format)
> #
> # Append a public key to user $HOME/.ssh/authorized_keys on Unix systems (not
> # implemented for other systems).
> #
> # Returns: Nothing on success.
> #
> # Since: 5.X
> ##
> { 'command': 'guest-ssh-add-authorized-key',
>   'data': { 'username': 'str', 'key': 'str' } }

I don't think my description provided the full context of what we need well enough. From my discription, a single "set" command would only work if the guest agent was capable of keeping track of the entries it sets in the authorized_keys files and re-set all those entries each time the API is called. So if items from the previous "set" API call are not present in the next "set" call, they are removed, and if new items are present in the next "set" call that were not previously in the last "set" call, they are added.

For context, this is how I was thinking about the command from the KubeVirt perspective and how I approached the topic in the description of this BZ, but I don't necessarily think that's realistic for the qemu guest agent.

For KubeVirt's purposes, we can achieve what we need with a simple "add" and "remove" api. It would be helpful if the apis take an array of keys per a user. For this to work, we'll need adds/removes to be idempotent with respect to duplicates. So if we attempt to call add and a key already exists, the agent just reports success. Same with remove, if a key doesn't exist it just reports success.

-------------------------------------------------------
EXAMPLE. Add keys to user "fedora" authorized_keys file
-------------------------------------------------------

$cat fedora/.ssh/authorized_keys
ssh-rsa 1111
ssh-rsa 2222
ssh-rsa 3333

execute ga command, '{"execute": "guest-ssh-add-authorized-keys", "arguments": { "username": "fedora", "keys": [ "ssh-rsa 4444", "ssh-rsa 5555" ] } }'

$cat fedora/.ssh/authorized_keys
ssh-rsa 1111
ssh-rsa 2222
ssh-rsa 3333
ssh-rsa 4444
ssh-rsa 5555

------------------------------------------------------------
EXAMPLE: remove keys from user "fedora" authorized_keys file
------------------------------------------------------------

$cat fedora/.ssh/authorized_keys
ssh-rsa 1111
ssh-rsa 2222
ssh-rsa 3333
ssh-rsa 4444
ssh-rsa 5555

execute ga command, '{"execute": "guest-ssh-remove-authorized-keys", "arguments": { "username": "fedora", "keys": [ "ssh-rsa 1111", "ssh-rsa 3333" ] } }'

$cat fedora/.ssh/authorized_keys
ssh-rsa 2222
ssh-rsa 4444
ssh-rsa 5555

--------------------------------------------------------------------------------------------------
EXAMPLE: Remove keys were some exist and some don't
--------------------------------------------------------------------------------------------------

$cat fedora/.ssh/authorized_keys
ssh-rsa 2222
ssh-rsa 4444
ssh-rsa 5555

execute ga command, '{"execute": "guest-ssh-remove-authorized-keys", "arguments": { "username": "fedora", "keys": [ "ssh-rsa 2222", "ssh-rsa 3333" ] } }'

$cat fedora/.ssh/authorized_keys
ssh-rsa 4444
ssh-rsa 5555

--------------------------------------------------------------------------------------------------
EXAMPLE: Add keys were some already exist and some don't
--------------------------------------------------------------------------------------------------

$cat fedora/.ssh/authorized_keys
ssh-rsa 4444
ssh-rsa 5555

execute ga command, '{"execute": "guest-ssh-add-authorized-keys", "arguments": { "username": "fedora", "keys": [ "ssh-rsa 4444", "ssh-rsa 1111" ] } }'

$cat fedora/.ssh/authorized_keys
ssh-rsa 1111
ssh-rsa 4444
ssh-rsa 5555


Would this work?

(In reply to David Vossel from comment #8)
> Would this work?

works for me.
@berrange, any comment?

Having both add & remove commands makes alot of sense, as revoking a user is just as important as granting a user, if not more so.

Sent to the qemu mailing list "[PATCH 0/2] qemu-ga: add ssh-{add,remove}-authorized-keys"

I believe we will want this to be exposed via libvirt APIs, won't we? Should I clone this bug for libvirt then?

> I believe we will want this to be exposed via libvirt APIs, won't we? Should I clone this bug for libvirt then?

isn't there a generic api libvirt provides for passing in agent commands as a JSON string? For instance the command below. 

virsh qemu-agent-command some-domain '{"execute":"guest-exec", "arguments": {"path":"cat", "arg": [ "/some/file" ], "capture-output":true }}'

Would I be able to naturally execute a similarly looking ssh key api using the same sort of JSON command execution without us necessarily needing to make libvirt changes?

Yes, libvirt has support for arbitrary passthrough of guest agent commands, however, usage of this API is only intended for dev/test environments. It is considered unsupported for production deployments in general. 

So we clearly need to expose a formal API for this in libvirt that CNV can use.  In the short term CNV can use the agent passthrough while waiting for the formal API to arrive.

@Marc I was testing the agent a little further today and realized that I'm getting selinux denials (in a fedora 32 guest) when I attempt to use the agent's file related commands to access ~/.ssh/authorized_keys. I'm guessing the agent daemon will hit the same denials with the new ssh api as well. Will the selinux profile for the agent get updated for this new API?

Yes, we will certainly need a BZ to request an update of the SELinux policy to allow these new commands.

On the CNV side, I began integrating the ssh key add/remove logic mocking how this new qemu agent api will work. I hit an issue that we should discuss.

The ssh public keys are stored in a k8s secret. If someone removes a ssh key from the secret, all we see on our end is that the secret has been modified and the new result. We don't have knowledge of what ssh key was removed from the secret. In my implementation, i kept an in-memory history of the previous secret to account for this and diffed the results to determine what was removed.

However there's a glaring hole in my logic here. If the k8s secret is modified while the VM is offline, we have no component that can accurately keep track of the diff between what keys have been applied to the VM vs what's in the current k8s secret. Basically, someone can remove a ssh key from the secret while the VM is offline, and we'll never revoke that key because it's not feasible to keep a stateful transaction history of what keys have been applied/removed per vm at the cluster level.

So, what can we do at the qemu api level to help here? I have a couple of thoughts.

1. At the agent level on the guest, figure out a way to separate the key entries written by the agent from the ones written external of the agent, and include an option in the "add" api command to replace all agent modified keys during the "add" command.

2. (much simpler) include an option to truncate the entire authorized_keys file during an add, so only the added keys exist. This would mean only the contents from the last "add" command would ever exist in the authorized_keys file.

Item 2 is easy, but it means the agent takes complete control over the authorized_keys files. Item 1 is complex, but it would allow a merging of user content with agent injected content.

I'd like to exhaust our options on item 1 before moving to the easier solution. Any thoughts on how we might be able to approach item 1, or if item 1 is even feasible?

(In reply to David Vossel from comment #18)
> I'd like to exhaust our options on item 1 before moving to the easier
> solution. Any thoughts on how we might be able to approach item 1, or if
> item 1 is even feasible?

I don't think we can have extra metadata on the key line except with comments.

So we could simple append a qemu-ga tag there. Or write a #qemu-ga comment line above the key.

Public keys consist of the following space-separated fields: options, keytype, base64-encoded key, comment.

The "comment" field might be populated by the end users, but there's no reason you can't append some extra metadata of your own in the comment. eg the identifier for the k8s secret the key came from.

If QGA provided a way to query keys then you can fetch current keys and figure out which one mapped from the secret that's modified, and then issue a delete call.

I agree with Daniel, I can propose a @guest-ssh-get-authorized-keys command.

The option 1 looks interesting anyway, we could also add a clear/reset argument to the add-authorized-keys command.

> I agree with Daniel, I can propose a @guest-ssh-get-authorized-keys command.
> The option 1 looks interesting anyway, we could also add a clear/reset argument to the add-authorized-keys command.

A "get" command that returns each authorized_key line (including the optional comment) would satisfy both items [1] and [2]. This would give CNV the flexibility to support either option (1 or 2).

As I've thought about this further, I think I'll start by limiting the CNV functionality to the simpler item [2], but I can see the possibility to needing to expand to support item [1]. Having a QGA approach that can technically be used to support both is great.

I think both the "get" command and "clear/reset" argument to the add command are useful. Would you feel comfortable with introducing both of these enhancements?

Sounds fine to support both "get" and the "clear" flag to "add".

#-line comments style are going to be badly managed by qemu-ga imho, as it is too easy to remove a key without removing the line above.

I think qemu-ga (and above) should not accept or rely on #-line, but rather only the comments on the key line itself.

I am sending a new version with reset and get to qemu ML soon.

The patch series is included in qemu 5.2-rc0. Which version of downstream do we target?

> The patch series is included in qemu 5.2-rc0. Which version of downstream do we target?

It actually looks like we're targeting 4.2 downstream. Is it possible to backport? I don't know the policy on these different qemu versions.

Just my two cents: this will require new APIs on libvirt side, and because of ABI stability we can't backport APIs. But this is a qemu bug, so I'll let Marc-Andre answer.

I suppose this rule out any qemu backport, if we need to wait for a libvirt rebase. Is anyone working on the libvirt changes? If not, I can work on it.

(In reply to Marc-Andre Lureau from comment #28)
> I suppose this rule out any qemu backport, if we need to wait for a libvirt
> rebase. Is anyone working on the libvirt changes? If not, I can work on it.

No, I don't think anybody works on that. Be my guest. I can do the review then.

Verify with version qemu-kvm-5.2.0-1.module+el8.4.0+9091+650b220a

Steps to Reproduce:
1.Start guest with virtio serial and start guest agent inside the guest 
2.Connect to the chardev socket in host side for sending following command to the guest:
# nc -U /tmp/qga.sock 
{"execute":"guest-ping"}
{"return": {}}
3.Off the selinux permission in guest:
#setenforce 0 (note: this step will create a related bz for selinux policy.from #comment16 & 17, so this step via manual for now.)
4.create a user in guest:
#useradd fedora
5.issue cmds in the unix socked,cmds like following items:
{"execute": "guest-ssh-add-authorized-keys", "arguments": { "username": "fedora", "keys": ["ssh-rsa aaa"]}}
{"return": {}}

{"execute":"guest-ssh-get-authorized-keys","arguments":{"username":"fedora" }}
{"return": {"keys": ["ssh-rsa aaa"]}}

{"execute": "guest-ssh-add-authorized-keys", "arguments": { "username": "fedora", "keys": [ "ssh-rsa aaa", "ssh-rsa bbb" ] } }
{"return": {}}

{"execute": "guest-ssh-remove-authorized-keys", "arguments": { "username": "fedora", "keys": [ "ssh-rsa bbb", "ssh-rsa ccc" ] } }
{"return": {}}

{"execute":"guest-ssh-add-authorized-keys","arguments":{"username": "fedora", "keys": ["aaa"], "reset": true }}                  
{"return": {}}

{"execute":"guest-ssh-get-authorized-keys","arguments":{"username":"fedora" }}
{"return": {"keys": ["aaa"]}}

Actual result:
New Commands running well and no errors.
Expected result:
New Commands running well and no errors.

(In reply to dehanmeng from comment #32)
> Verify with version qemu-kvm-5.2.0-1.module+el8.4.0+9091+650b220a
> 
> Steps to Reproduce:
> 1.Start guest with virtio serial and start guest agent inside the guest 
> 2.Connect to the chardev socket in host side for sending following command
> to the guest:
> # nc -U /tmp/qga.sock 
> {"execute":"guest-ping"}
> {"return": {}}
> 3.Off the selinux permission in guest:
> #setenforce 0 (note: this step will create a related bz for selinux
> policy.from #comment16 & 17, so this step via manual for now.)
> 4.create a user in guest:
> #useradd fedora
> 5.issue cmds in the unix socked,cmds like following items:
> {"execute": "guest-ssh-add-authorized-keys", "arguments": { "username":
> "fedora", "keys": ["ssh-rsa aaa"]}}
> {"return": {}}
> 
$cat .ssh/authorized_keys
ssh-rsa aaa

> {"execute":"guest-ssh-get-authorized-keys","arguments":{"username":"fedora"
> }}
> {"return": {"keys": ["ssh-rsa aaa"]}}
> 
> {"execute": "guest-ssh-add-authorized-keys", "arguments": { "username":
> "fedora", "keys": [ "ssh-rsa aaa", "ssh-rsa bbb" ] } }
> {"return": {}}
> 
$cat .ssh/authorized_keys
ssh-rsa aaa
ssh-rsa bbb
> {"execute": "guest-ssh-remove-authorized-keys", "arguments": { "username":
> "fedora", "keys": [ "ssh-rsa bbb", "ssh-rsa ccc" ] } }
> {"return": {}}
> 
$cat .ssh/authorized_keys
ssh-rsa aaa

> {"execute":"guest-ssh-add-authorized-keys","arguments":{"username":
> "fedora", "keys": ["aaa"], "reset": true }}                  
> {"return": {}}
> 
$cat .ssh/authorized_keys
aaa
> {"execute":"guest-ssh-get-authorized-keys","arguments":{"username":"fedora"
> }}
> {"return": {"keys": ["aaa"]}}
> 
> Actual result:
> New Commands running well and no errors.
> Expected result:
> New Commands running well and no errors.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (virt:av bug fix and enhancement update), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2021:2098