[Libvirt-cim] [PATCH] provide a hack to borrow non-root ssh keys in migration
xiaxia347work
xiaxia347work at 163.com
Sat Sep 15 00:36:37 UTC 2012
HI
still have 2 need to look at I think, one is the patch with the title of the mail, another is
https://www.redhat.com/archives/libvirt-cim/2012-July/msg00007.html
mentioned before.
About the non-root ssh keys patch:
- how do you allow another key
it is based on adding aditional parameter "?keyfile=[PLACE]" in URL for libvirt remote access,
this patch added an optionin libvirt-cim migration call, so client can pass this option in now.
- how do you tell which one to pick, where and who can set this up
It is decided by the client who have authority to start migration, basically this patch added 3
parameters: DestinationHostParams, SSH_Key_Src, SSH_Key_Dest. This patch would first
cp -f SSH_Key_Src, SSH_Key_Dest, then append DestinationHostParams in URI. Copying
was done because libvirt reject the keyfile if the directory was not owned by the caller, in
libvirt-cim it is root. Finally user must do: first setup a non-root account who have the capability
to manage system VMs in libvirt(call it USER A here), generate USER A's ssh-key pairs and
setup them on two nodes that want to do migration, then specify these three options to borrow
USER A's private key. After migration, caller could delete the key-pairs. If these 3 options were
not sepcified, then they can do migration with root's indentity as before.
- there is a copy done, it's hard to tell from which soure to which
destination based just on code review
Yes, cim client must know this themself, but usually there would be only one account's key file
for migration exist, and cim caller knows where is source and destination.
- does that work with SELinux turned on ?
my test was done with SELinux turned off, when SELinux was on it may succeed unless
"copy" or "virsh connect qemu+ssh:URI?keyfile=XXX" broke for SELinux. But for simple
I think SELinux was not guarenteed to work for this.
2012-09-15
Best Regards
Wenchao Xia
发件人：snmishra
发送时间：2012-09-15 04:13
主题：Re: [Libvirt-cim] [PATCH] provide a hack to borrow non-root ssh keys in migration
收件人："Daniel Veillard"<veillard at redhat.com>
抄送："libvirt-cim"<libvirt-cim at redhat.com>
Thanks DV for looking at the patch.
Wayne, do you have any other solution?
DV, Can you please push this patch -
https://www.redhat.com/archives/libvirt-cim/2012-July/msg00007.html
Wayne, Are there any other patches to be reviewed/pushed?
Thanks
Sharad Mishra
IBM
Quoting Daniel Veillard <veillard at redhat.com>:
> On Fri, Aug 24, 2012 at 06:18:29PM +0800, Wenchao Xia wrote:
>> This patch allow libvirt-cim to use non-root's ssh key, avoid
>> using root's key to avoid exposing root's ssh login. Because libvirt-cim
>> runs in root mode so it is hard to satisfy some server security rules
>> especially about root's ssh key exposing. This is a walk around to improve
>> it.
>
> This has serious security implications, so I think this really must be
> documented in a very clear way:
> - how do you allow another key
> - how do you tell which one to pick, where and who can set this up
> - there is a copy done, it's hard to tell from which soure to which
> destination based just on code review
> - does that work with SELinux turned on ?
>
> As is it's very hard to review the code to give an ACK,
>
>> Signed-off-by: Wenchao Xia <xiawenc at linux.vnet.ibm.com>
>> ---
>> src/Virt_VSMigrationService.c | 96
>> +++++++++++++++++++++++++++++++++++++++--
>> 1 files changed, 92 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/Virt_VSMigrationService.c b/src/Virt_VSMigrationService.c
>> index 76e3d25..55442ee 100644
>> --- a/src/Virt_VSMigrationService.c
>> +++ b/src/Virt_VSMigrationService.c
>> @@ -150,6 +150,7 @@ static CMPIStatus get_migration_uri(CMPIInstance *msd,
>>
>> static char *dest_uri(const char *cn,
>> const char *dest,
>> + const char *dest_params,
>> uint16_t transport)
>> {
>> const char *prefix;
>> @@ -157,6 +158,7 @@ static char *dest_uri(const char *cn,
>> const char *param = "";
>> char *uri = NULL;
>> int rc;
>> + int param_labeled = 0;
>>
>> if (STARTS_WITH(cn, "Xen"))
>> prefix = "xen";
>> @@ -197,16 +199,75 @@ static char *dest_uri(const char *cn,
>> goto out;
>> }
>>
>> - if (!STREQC(param, ""))
>> + if (!STREQC(param, "")) {
>> rc = asprintf(&uri, "%s/%s", uri, param);
>> + param_labeled = 1;
>> + }
>>
>> - if (rc == -1)
>> + if (rc == -1) {
>> uri = NULL;
>> + goto out;
>> + }
>>
>> + if (dest_params) {
>> + if (param_labeled == 0) {
>> + rc = asprintf(&uri, "%s?%s", uri, dest_params);
>> + } else {
>> + rc = asprintf(&uri, "%s%s", uri, dest_params);
>> + }
>> + if (rc == -1) {
>> + uri = NULL;
>> + goto out;
>> + }
>> + }
>> out:
>> return uri;
>> }
>>
>> +/* libvirt need private key specified must be placed in a
>> directory owned by
>> + root, because libvirt-cim now runs as root. So here the key would
>> be copied,
>> + up layer need to delete that key after migration. This method could allow
>> + libvirt-cim borrow a non-root ssh private key, instead of using
>> root's private
>> + key, avoid security risk. */
>> +static int ssh_key_cp(const char *src, const char *dest)
>
> though the function is named ssh_key_cp it can be used to copy
> anything to anywhere. Moreover it runs popen on a built command,
> where are src and dest coming from, they aren't validated for
> sanity. what happen if dest is "/tmp ; rm-rf /" ???
>
>> +{
>> + char *cmd = NULL;
>> + int rc;
>> + int ret = 0;
>> + FILE *stream = NULL;
>> + char buf[256];
>> +
>> + rc = asprintf(&cmd, "cp -f %s %s", src, dest);
>> + if (rc == -1) {
>> + cmd = NULL;
>> + ret = -1;
>> + goto out;
>> + }
>> +
>> + CU_DEBUG("excuting system cmd [%s].", cmd);
>> + stream = popen(cmd, "r");
>
> the "popen" man page says
> The command argument is a pointer to a null-terminated string contain‐
> ing a shell command line. This command is passed to /bin/sh using the
> -c flag; interpretation, if any, is performed by the shell.
>
> Sorry, the reason of that patch is supposedly to increase security
> and to me it's actually opening a huge security hole there. if you need
> to copy a file, do it cleanly, and verify the arguments are sane.
>
> NACK
>
>> + if (stream == NULL) {
>> + CU_DEBUG("Failed to open pipe to run command");
>> + ret = -2;
>> + goto out;
>> + }
>> + usleep(10000);
>> +
>> + buf[255] = 0;
>> + while (fgets(buf, sizeof(buf), stream) != NULL) {
>> + CU_DEBUG("Exception got: %s.", buf);
>> + ret = -3;
>> + goto out;
>> + }
>> +
>> + out:
>> + if (stream != NULL) {
>> + pclose(stream);
>> + }
>> + free(cmd);
>> + return ret;
>> +}
>> +
>> static CMPIStatus get_msd_values(const CMPIObjectPath *ref,
>> const char *destination,
>> const CMPIArgs *argsin,
>> @@ -217,6 +278,14 @@ static CMPIStatus get_msd_values(const
>> CMPIObjectPath *ref,
>> CMPIInstance *msd;
>> uint16_t uri_type;
>> char *uri = NULL;
>> + const char *dest_params = NULL;
>> + const char *ssh_hack_src = NULL;
>> + const char *ssh_hack_dest = NULL;
>> + int ret;
>> +
>> + cu_get_str_arg(argsin, "DestinationHostParams", &dest_params);
>> + cu_get_str_arg(argsin, "SSH_Key_Src", &ssh_hack_src);
>> + cu_get_str_arg(argsin, "SSH_Key_Dest", &ssh_hack_dest);
>>
>> s = get_msd(ref, argsin, &msd);
>> if (s.rc != CMPI_RC_OK)
>> @@ -230,7 +299,7 @@ static CMPIStatus get_msd_values(const
>> CMPIObjectPath *ref,
>> if (s.rc != CMPI_RC_OK)
>> goto out;
>>
>> - uri = dest_uri(CLASSNAME(ref), destination, uri_type);
>> + uri = dest_uri(CLASSNAME(ref), destination, dest_params, uri_type);
>> if (uri == NULL) {
>> cu_statusf(_BROKER, &s,
>> CMPI_RC_ERR_FAILED,
>> @@ -238,6 +307,19 @@ static CMPIStatus get_msd_values(const
>> CMPIObjectPath *ref,
>> goto out;
>> }
>>
>> + if ((ssh_hack_src) && (ssh_hack_dest)) {
>> + CU_DEBUG("hacking ssh keys src %s, dest %s.",
>> + ssh_hack_src, ssh_hack_dest);
>> + ret = ssh_key_cp(ssh_hack_src, ssh_hack_dest);
>> + if (ret < 0) {
>> + cu_statusf(_BROKER, &s,
>> + CMPI_RC_ERR_FAILED,
>> + "Failed to copy ssh key files");
>> + goto out;
>> + }
>> + }
>> +
>> + CU_DEBUG("Migrate tring to connect remote host with uri %s.", uri);
>> *conn = virConnectOpen(uri);
>> if (*conn == NULL) {
>> CU_DEBUG("Failed to connect to remote host (%s)", uri);
>> @@ -1537,7 +1619,7 @@ static CMPIStatus migrate_vs_host(CMPIMethodMI *self,
>> const char *dhost = NULL;
>> CMPIObjectPath *system;
>> const char *name = NULL;
>> -
>> +
>> cu_get_str_arg(argsin, "DestinationHost", &dhost);
>> cu_get_ref_arg(argsin, "ComputerSystem", &system);
>>
>> @@ -1608,6 +1690,9 @@ static struct method_handler vsimth = {
>> .handler = vs_migratable_host,
>> .args = {{"ComputerSystem", CMPI_ref, false},
>> {"DestinationHost", CMPI_string, false},
>> + {"DestinationHostParams", CMPI_string, true},
>> + {"SSH_Key_Src", CMPI_string, true},
>> + {"SSH_Key_Dest", CMPI_string, true},
>> {"MigrationSettingData", CMPI_instance, true},
>> {"NewSystemSettingData", CMPI_instance, true},
>> {"NewResourceSettingData", CMPI_instanceA, true},
>> @@ -1632,6 +1717,9 @@ static struct method_handler mvsth = {
>> .handler = migrate_vs_host,
>> .args = {{"ComputerSystem", CMPI_ref, false},
>> {"DestinationHost", CMPI_string, false},
>> + {"DestinationHostParams", CMPI_string, true},
>> + {"SSH_Key_Src", CMPI_string, true},
>> + {"SSH_Key_Dest", CMPI_string, true},
>> {"MigrationSettingData", CMPI_instance, true},
>> {"NewSystemSettingData", CMPI_instance, true},
>> {"NewResourceSettingData", CMPI_instanceA, true},
>> --
>> 1.7.1
>>
>
> --
> Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
> daniel at veillard.com | Rpmfind RPM search engine http://rpmfind.net/
> http://veillard.com/ | virtualization library http://libvirt.org/
>
> _______________________________________________
> Libvirt-cim mailing list
> Libvirt-cim at redhat.com
> https://www.redhat.com/mailman/listinfo/libvirt-cim
_______________________________________________
Libvirt-cim mailing list
Libvirt-cim at redhat.com
https://www.redhat.com/mailman/listinfo/libvirt-cim
More information about the Libvirt-cim
mailing list