Bug 1885332 - RFE: guest agent public ssh injection api support
Summary: RFE: guest agent public ssh injection api support
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux Advanced Virtualization
Classification: Red Hat
Component: qemu-kvm
Version: ---
Hardware: Unspecified
OS: Unspecified
medium
unspecified
Target Milestone: rc
: ---
Assignee: Marc-Andre Lureau
QA Contact: dehanmeng
URL:
Whiteboard:
Depends On:
Blocks: 1888537
TreeView+ depends on / blocked
 
Reported: 2020-10-05 16:04 UTC by David Vossel
Modified: 2021-05-25 06:44 UTC (History)
8 users (show)

Fixed In Version: qemu-kvm-5.2.0-1.module+el8.4.0+9091+650b220a
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
: 1888537 (view as bug list)
Environment:
Last Closed: 2021-05-25 06:43:38 UTC
Type: Feature Request
Target Upstream Version:


Attachments (Terms of Use)

Description David Vossel 2020-10-05 16:04:06 UTC
Description of problem:


From a high level, this is a request for an API to set values within a guest OS's  authorized_keys file. This is similar in concept to the guest-set-user-password API command that already exists today [1].

The Background here is that OpenShift Virtualization (previously known as CNV) would like to begin leveraging the guest agent to dynamically inject ssh public keys into a running guest OS

We have already proven out the concept using the guest's file manipulation and exec capabilities here [2], however the commands we use in this proof of concept are not generally enabled for RHEL hosts. It's possible these commands are too permissive to ever be considered as being enabled by default. 

A targeted API (like a future guest-set-authorized-keys command) would likely be easier to justify enabling by default in RHEL guests, and would allow us to avoid tunneling a series of complex execution commands using the exec API. 


1. https://www.qemu.org/docs/master/interop/qemu-ga-ref.html#index-guest_002dset_002duser_002dpassword
2. https://github.com/kubevirt/kubevirt/pull/4195

Comment 2 John Ferlan 2020-10-05 17:31:36 UTC
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.

Comment 3 Marc-Andre Lureau 2020-10-08 16:11:48 UTC
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.

Comment 4 Marc-Andre Lureau 2020-10-08 16:16:41 UTC
Oh wait, you can just change root password already.
Eh, why do we have a qemu-ga command denylist in the end?

Comment 5 Daniel Berrangé 2020-10-08 16:18:10 UTC
Or the ability to touch root account should be a configurable setting, either in QGA config, or in a SELinux policy tunable, or both.

Comment 6 Fabian Deutsch 2020-10-08 18:35:06 UTC
> 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?

Comment 7 Marc-Andre Lureau 2020-10-09 15:17:32 UTC
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' } }

Comment 8 David Vossel 2020-10-12 13:31:46 UTC
> 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?

Comment 10 Marc-Andre Lureau 2020-10-12 13:41:44 UTC
(In reply to David Vossel from comment #8)
> Would this work?

works for me.
@berrange, any comment?

Comment 11 Daniel Berrangé 2020-10-12 13:56:15 UTC
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.

Comment 12 Marc-Andre Lureau 2020-10-13 20:25:44 UTC
Sent to the qemu mailing list "[PATCH 0/2] qemu-ga: add ssh-{add,remove}-authorized-keys"

Comment 13 Michal Privoznik 2020-10-14 09:39:39 UTC
I believe we will want this to be exposed via libvirt APIs, won't we? Should I clone this bug for libvirt then?

Comment 14 David Vossel 2020-10-14 17:36:17 UTC
> 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?

Comment 15 Daniel Berrangé 2020-10-14 17:56:03 UTC
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.

Comment 16 David Vossel 2020-10-14 21:08:22 UTC
@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?

Comment 17 Daniel Berrangé 2020-10-15 10:20:30 UTC
Yes, we will certainly need a BZ to request an update of the SELinux policy to allow these new commands.

Comment 18 David Vossel 2020-10-16 13:01:34 UTC
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?

Comment 19 Marc-Andre Lureau 2020-10-19 07:07:52 UTC
(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.

Comment 20 Daniel Berrangé 2020-10-19 07:29:54 UTC
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.

Comment 21 Marc-Andre Lureau 2020-10-19 07:37:07 UTC
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.

Comment 22 David Vossel 2020-10-19 13:35:38 UTC
> 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?

Comment 23 Daniel Berrangé 2020-10-19 13:38:59 UTC
Sounds fine to support both "get" and the "clear" flag to "add".

Comment 24 Marc-Andre Lureau 2020-10-19 14:01:04 UTC
#-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.

Comment 25 Marc-Andre Lureau 2020-11-04 07:50:08 UTC
The patch series is included in qemu 5.2-rc0. Which version of downstream do we target?

Comment 26 David Vossel 2020-11-05 22:12:51 UTC
> 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.

Comment 27 Michal Privoznik 2020-11-06 09:10:42 UTC
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.

Comment 28 Marc-Andre Lureau 2020-11-06 13:25:15 UTC
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.

Comment 29 Michal Privoznik 2020-11-06 13:56:27 UTC
(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.

Comment 32 dehanmeng 2020-12-10 10:01:16 UTC
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.

Comment 33 dehanmeng 2020-12-10 10:42:42 UTC
(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.

Comment 36 errata-xmlrpc 2021-05-25 06:43:38 UTC
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


Note You need to log in before you can comment on or make changes to this bug.