[libvirt PATCH v5 32/32] qemu: implement keyfile auth for ssh disk with nbdkit

Jonathon Jongsma posted 32 patches 2 years, 12 months ago
There is a newer version of this series
[libvirt PATCH v5 32/32] qemu: implement keyfile auth for ssh disk with nbdkit
Posted by Jonathon Jongsma 2 years, 12 months ago
For ssh disks that are served by nbdkit, we can support logging in with
an ssh key file. Pass the path to the configured key file and the
username to the nbdkit process.

The key file may be password protected, and libvirt cannot prompt the
user for a password to unlock it. But if the adminstrator adds this key
to an ssh agent, they can configure the disk with the path to the unix
socket for the ssh agent so libvirt can pass this socket path to nbdkit
and we can make use of these keys.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
---
 src/conf/domain_conf.c                        | 36 +++++++++++++++----
 src/conf/storage_source_conf.c                |  2 ++
 src/conf/storage_source_conf.h                |  6 ++--
 src/qemu/qemu_nbdkit.c                        | 11 ++++--
 .../disk-network-ssh-key.args.disk0           | 10 ++++++
 .../disk-network-ssh.args.disk2               |  9 +++++
 tests/qemunbdkittest.c                        |  1 +
 .../qemuxml2argvdata/disk-network-ssh-key.xml | 33 +++++++++++++++++
 8 files changed, 97 insertions(+), 11 deletions(-)
 create mode 100644 tests/qemunbdkitdata/disk-network-ssh-key.args.disk0
 create mode 100644 tests/qemunbdkitdata/disk-network-ssh.args.disk2
 create mode 100644 tests/qemuxml2argvdata/disk-network-ssh-key.xml

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index cb9d01dc6d..d5aa92e81b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7214,10 +7214,21 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
             return -1;
         }
     }
-    if (src->protocol == VIR_STORAGE_NET_PROTOCOL_SSH &&
-        (tmpnode = virXPathNode("./knownHosts", ctxt))) {
-        if (!(src->ssh_known_hosts_file = virXMLPropStringRequired(tmpnode, "path")))
-            return -1;
+    if (src->protocol == VIR_STORAGE_NET_PROTOCOL_SSH) {
+        if ((tmpnode = virXPathNode("./knownHosts", ctxt))) {
+            if (!(src->ssh_known_hosts_file = virXMLPropStringRequired(tmpnode, "path")))
+                return -1;
+        }
+        if ((tmpnode = virXPathNode("./identity", ctxt))) {
+            if (!(src->ssh_keyfile = virXMLPropStringRequired(tmpnode, "keyfile")))
+                return -1;
+
+            if (!(src->ssh_user = virXMLPropStringRequired(tmpnode, "username")))
+                return -1;
+
+            /* optional ssh-agent socket location */
+            src->ssh_agent = virXMLPropString(tmpnode, "agentsock");
+        }
     }
 
     return 0;
@@ -22097,8 +22108,21 @@ virDomainDiskSourceFormatNetwork(virBuffer *attrBuf,
     if (src->timeout)
         virBufferAsprintf(childBuf, "<timeout seconds='%llu'/>\n", src->timeout);
 
-    if (src->protocol == VIR_STORAGE_NET_PROTOCOL_SSH && src->ssh_known_hosts_file)
-        virBufferAsprintf(childBuf, "<knownHosts path='%s'/>\n", src->ssh_known_hosts_file);
+    if (src->protocol == VIR_STORAGE_NET_PROTOCOL_SSH) {
+        if (src->ssh_known_hosts_file)
+            virBufferAsprintf(childBuf, "<knownHosts path='%s'/>\n", src->ssh_known_hosts_file);
+        if (src->ssh_keyfile) {
+            virBufferAddLit(childBuf, "<identity");
+
+            virBufferEscapeString(childBuf, " keyfile='%s'", src->ssh_keyfile);
+            if (src->ssh_user)
+                virBufferEscapeString(childBuf, " username='%s'", src->ssh_user);
+            if (src->ssh_agent)
+                virBufferEscapeString(childBuf, " agentsock='%s'", src->ssh_agent);
+
+            virBufferAddLit(childBuf, "/>\n");
+        }
+    }
 }
 
 
diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c
index 5d60c46cfc..4b8397420b 100644
--- a/src/conf/storage_source_conf.c
+++ b/src/conf/storage_source_conf.c
@@ -1168,6 +1168,8 @@ virStorageSourceClear(virStorageSource *def)
 
     VIR_FREE(def->ssh_user);
     VIR_FREE(def->ssh_known_hosts_file);
+    VIR_FREE(def->ssh_keyfile);
+    VIR_FREE(def->ssh_agent);
 
     VIR_FREE(def->nfs_user);
     VIR_FREE(def->nfs_group);
diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h
index a2d8b1f8bd..ed4deaf58c 100644
--- a/src/conf/storage_source_conf.h
+++ b/src/conf/storage_source_conf.h
@@ -401,12 +401,12 @@ struct _virStorageSource {
 
     bool hostcdrom; /* backing device is a cdrom */
 
-    /* passthrough variables for the ssh driver which we don't handle properly */
-    /* these must not be used apart from formatting the output JSON in the qemu driver */
+    /* ssh variables */
     char *ssh_user;
     bool ssh_host_key_check_disabled;
-    /* additional ssh variables */
     char *ssh_known_hosts_file;
+    char *ssh_keyfile;
+    char *ssh_agent;
 
     /* nfs_user and nfs_group store the strings passed in by the user for NFS params.
      * nfs_uid and nfs_gid represent the converted/looked up ID numbers which are used
diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
index dbbe71944f..811be7a583 100644
--- a/src/qemu/qemu_nbdkit.c
+++ b/src/qemu/qemu_nbdkit.c
@@ -1071,10 +1071,17 @@ qemuNbdkitProcessBuildCommandSSH(qemuNbdkitProcess *proc,
     if (proc->source->auth) {
         if (qemuNbdkitProcessBuildCommandAuth(proc->source->auth, cmd) < 0)
             return -1;
-    } else if (proc->source->ssh_user) {
-        virCommandAddArgPair(cmd, "user", proc->source->ssh_user);
+    } else {
+        if (proc->source->ssh_keyfile)
+            virCommandAddArgPair(cmd, "identity", proc->source->ssh_keyfile);
+
+        if (proc->source->ssh_user)
+            virCommandAddArgPair(cmd, "user", proc->source->ssh_user);
     }
 
+    if (proc->source->ssh_agent)
+        virCommandAddEnvPair(cmd, "SSH_AUTH_SOCK", proc->source->ssh_agent);
+
     if (proc->source->ssh_host_key_check_disabled)
         virCommandAddArgPair(cmd, "verify-remote-host", "false");
 
diff --git a/tests/qemunbdkitdata/disk-network-ssh-key.args.disk0 b/tests/qemunbdkitdata/disk-network-ssh-key.args.disk0
new file mode 100644
index 0000000000..9842300f70
--- /dev/null
+++ b/tests/qemunbdkitdata/disk-network-ssh-key.args.disk0
@@ -0,0 +1,10 @@
+SSH_AUTH_SOCK=/path/to/agent/socket \
+nbdkit \
+--unix /tmp/statedir-0/nbdkit-test-disk-0.socket \
+--foreground ssh \
+host=example.org \
+port=2222 \
+path=test2.img \
+identity=/path/to/id_rsa \
+user=myuser \
+known-hosts=/path/to/ssh_known_hosts
diff --git a/tests/qemunbdkitdata/disk-network-ssh.args.disk2 b/tests/qemunbdkitdata/disk-network-ssh.args.disk2
new file mode 100644
index 0000000000..e269a34351
--- /dev/null
+++ b/tests/qemunbdkitdata/disk-network-ssh.args.disk2
@@ -0,0 +1,9 @@
+nbdkit \
+--unix /tmp/statedir-2/nbdkit-test-disk-2.socket \
+--foreground ssh \
+host=example.org \
+port=2222 \
+path=test2.img \
+identity=/path/to/id_rsa \
+user=myuser \
+known-hosts=/path/to/ssh_known_hosts
diff --git a/tests/qemunbdkittest.c b/tests/qemunbdkittest.c
index 492077e56e..e507df6e42 100644
--- a/tests/qemunbdkittest.c
+++ b/tests/qemunbdkittest.c
@@ -292,6 +292,7 @@ mymain(void)
     DO_TEST("disk-network-source-curl", QEMU_NBDKIT_CAPS_PLUGIN_CURL);
     DO_TEST("disk-network-ssh", QEMU_NBDKIT_CAPS_PLUGIN_SSH);
     DO_TEST("disk-network-ssh-password", QEMU_NBDKIT_CAPS_PLUGIN_SSH);
+    DO_TEST("disk-network-ssh-key", QEMU_NBDKIT_CAPS_PLUGIN_SSH);
 
     qemuTestDriverFree(&driver);
 
diff --git a/tests/qemuxml2argvdata/disk-network-ssh-key.xml b/tests/qemuxml2argvdata/disk-network-ssh-key.xml
new file mode 100644
index 0000000000..e8e62476d4
--- /dev/null
+++ b/tests/qemuxml2argvdata/disk-network-ssh-key.xml
@@ -0,0 +1,33 @@
+<domain type='kvm'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>219136</memory>
+  <currentMemory unit='KiB'>219136</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <disk type='network' device='disk'>
+      <driver name='qemu' type='raw'/>
+      <source protocol='ssh' name='test2.img'>
+        <host name='example.org' port='2222'/>
+        <timeout seconds='1234'/>
+        <readahead size='1024'/>
+        <identity username='myuser' keyfile='/path/to/id_rsa' agentsock='/path/to/agent/socket'/>
+        <knownHosts path="/path/to/ssh_known_hosts"/>
+      </source>
+      <target dev='vda' bus='virtio'/>
+    </disk>
+    <controller type='usb' index='0'/>
+    <controller type='pci' index='0' model='pci-root'/>
+    <input type='mouse' bus='ps2'/>
+    <input type='keyboard' bus='ps2'/>
+    <memballoon model='none'/>
+  </devices>
+</domain>
-- 
2.39.1
Re: [libvirt PATCH v5 32/32] qemu: implement keyfile auth for ssh disk with nbdkit
Posted by Peter Krempa 2 years, 11 months ago
On Tue, Feb 14, 2023 at 11:08:19 -0600, Jonathon Jongsma wrote:
> For ssh disks that are served by nbdkit, we can support logging in with
> an ssh key file. Pass the path to the configured key file and the
> username to the nbdkit process.
> 
> The key file may be password protected, and libvirt cannot prompt the
> user for a password to unlock it. But if the adminstrator adds this key
> to an ssh agent, they can configure the disk with the path to the unix
> socket for the ssh agent so libvirt can pass this socket path to nbdkit
> and we can make use of these keys.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
>  src/conf/domain_conf.c                        | 36 +++++++++++++++----
>  src/conf/storage_source_conf.c                |  2 ++
>  src/conf/storage_source_conf.h                |  6 ++--
>  src/qemu/qemu_nbdkit.c                        | 11 ++++--
>  .../disk-network-ssh-key.args.disk0           | 10 ++++++
>  .../disk-network-ssh.args.disk2               |  9 +++++
>  tests/qemunbdkittest.c                        |  1 +
>  .../qemuxml2argvdata/disk-network-ssh-key.xml | 33 +++++++++++++++++
>  8 files changed, 97 insertions(+), 11 deletions(-)
>  create mode 100644 tests/qemunbdkitdata/disk-network-ssh-key.args.disk0
>  create mode 100644 tests/qemunbdkitdata/disk-network-ssh.args.disk2
>  create mode 100644 tests/qemuxml2argvdata/disk-network-ssh-key.xml
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index cb9d01dc6d..d5aa92e81b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -7214,10 +7214,21 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
>              return -1;
>          }
>      }
> -    if (src->protocol == VIR_STORAGE_NET_PROTOCOL_SSH &&
> -        (tmpnode = virXPathNode("./knownHosts", ctxt))) {
> -        if (!(src->ssh_known_hosts_file = virXMLPropStringRequired(tmpnode, "path")))
> -            return -1;
> +    if (src->protocol == VIR_STORAGE_NET_PROTOCOL_SSH) {
> +        if ((tmpnode = virXPathNode("./knownHosts", ctxt))) {
> +            if (!(src->ssh_known_hosts_file = virXMLPropStringRequired(tmpnode, "path")))
> +                return -1;
> +        }
> +        if ((tmpnode = virXPathNode("./identity", ctxt))) {
> +            if (!(src->ssh_keyfile = virXMLPropStringRequired(tmpnode, "keyfile")))
> +                return -1;

Why is 'keyfile' mandatory ...

> +
> +            if (!(src->ssh_user = virXMLPropStringRequired(tmpnode, "username")))
> +                return -1;
> +
> +            /* optional ssh-agent socket location */
> +            src->ssh_agent = virXMLPropString(tmpnode, "agentsock");

... if you can use the agent?

In case agent is in use I expect that the key is added to the agent by
the user and then the socket is passed to nbdkit to do the auth. nbdkit
in that case has no need to look at the keyfile.

Similarly selinux labelling may be the problem. But we can theoretically
instruct the user via docs to use one agent per VM. We then can label
the socket instead.
Re: [libvirt PATCH v5 32/32] qemu: implement keyfile auth for ssh disk with nbdkit
Posted by Jonathon Jongsma 2 years, 11 months ago
On 2/16/23 10:56 AM, Peter Krempa wrote:
> On Tue, Feb 14, 2023 at 11:08:19 -0600, Jonathon Jongsma wrote:
>> For ssh disks that are served by nbdkit, we can support logging in with
>> an ssh key file. Pass the path to the configured key file and the
>> username to the nbdkit process.
>>
>> The key file may be password protected, and libvirt cannot prompt the
>> user for a password to unlock it. But if the adminstrator adds this key
>> to an ssh agent, they can configure the disk with the path to the unix
>> socket for the ssh agent so libvirt can pass this socket path to nbdkit
>> and we can make use of these keys.
>>
>> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
>> ---
>>   src/conf/domain_conf.c                        | 36 +++++++++++++++----
>>   src/conf/storage_source_conf.c                |  2 ++
>>   src/conf/storage_source_conf.h                |  6 ++--
>>   src/qemu/qemu_nbdkit.c                        | 11 ++++--
>>   .../disk-network-ssh-key.args.disk0           | 10 ++++++
>>   .../disk-network-ssh.args.disk2               |  9 +++++
>>   tests/qemunbdkittest.c                        |  1 +
>>   .../qemuxml2argvdata/disk-network-ssh-key.xml | 33 +++++++++++++++++
>>   8 files changed, 97 insertions(+), 11 deletions(-)
>>   create mode 100644 tests/qemunbdkitdata/disk-network-ssh-key.args.disk0
>>   create mode 100644 tests/qemunbdkitdata/disk-network-ssh.args.disk2
>>   create mode 100644 tests/qemuxml2argvdata/disk-network-ssh-key.xml
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index cb9d01dc6d..d5aa92e81b 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -7214,10 +7214,21 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node,
>>               return -1;
>>           }
>>       }
>> -    if (src->protocol == VIR_STORAGE_NET_PROTOCOL_SSH &&
>> -        (tmpnode = virXPathNode("./knownHosts", ctxt))) {
>> -        if (!(src->ssh_known_hosts_file = virXMLPropStringRequired(tmpnode, "path")))
>> -            return -1;
>> +    if (src->protocol == VIR_STORAGE_NET_PROTOCOL_SSH) {
>> +        if ((tmpnode = virXPathNode("./knownHosts", ctxt))) {
>> +            if (!(src->ssh_known_hosts_file = virXMLPropStringRequired(tmpnode, "path")))
>> +                return -1;
>> +        }
>> +        if ((tmpnode = virXPathNode("./identity", ctxt))) {
>> +            if (!(src->ssh_keyfile = virXMLPropStringRequired(tmpnode, "keyfile")))
>> +                return -1;
> 
> Why is 'keyfile' mandatory ...
> 
>> +
>> +            if (!(src->ssh_user = virXMLPropStringRequired(tmpnode, "username")))
>> +                return -1;
>> +
>> +            /* optional ssh-agent socket location */
>> +            src->ssh_agent = virXMLPropString(tmpnode, "agentsock");
> 
> ... if you can use the agent?
> 
> In case agent is in use I expect that the key is added to the agent by
> the user and then the socket is passed to nbdkit to do the auth. nbdkit
> in that case has no need to look at the keyfile.
> 
> Similarly selinux labelling may be the problem. But we can theoretically
> instruct the user via docs to use one agent per VM. We then can label
> the socket instead.
> 


Yes, I think you're right, it looks like the agent socket does have some 
selinux labeling issues. I'm afraid that I'm still pretty naive about 
selinux in libvirt (and in general, frankly), and I haven't been able to 
find the proper approach to enable access to this socket from nbdkit. 
Any ideas?

Thanks,
Jonathon