Re: [Libvirt-cim] [PATCH V5 15/15] migration: allow ssh based migration with non root's key file

On 03/20/2013 11:39 PM, Wenchao Xia wrote: > This patch allow libvirt-cim to use non-root's ssh key in migration > to avoid exposing root's ssh login on server. In some case server are > forbidden to expose or provide any root ssh login, and still use ssh > encryption between two migration nodes with key of special account > created for virtual machine management. > > When it is enabled in config file: > 1 MigrateSSHKeyCopy, use string property [SSH_Key_Src] to tell which key > to be copied. It will be copied to [migrate_ssh_temp_key]. > 2 MigrateVirtualSystemToHost and CheckVirtualSystemIsMigratableToHost, > use bool property [MigrationWithoutRootKey], to tell whether to use the key > as [migrate_ssh_temp_key]. > 3 MigrateSSHKeyDelete, when it is called [migrate_ssh_temp_key] will be > deleted. > > Details: > libvirt-cim would run shell command "cp -f [SSH_Key_Src] > [migrate_ssh_temp_key]", then use [migrate_ssh_temp_key] to generate uri > suffix for remote connection to migration destination. > > Signed-off-by: Wenchao Xia <xiawenc(a)linux.vnet.ibm.com&gt; > --- > libvirt-cim.conf | 19 +++ > libxkutil/misc_util.c | 9 ++ > libxkutil/misc_util.h | 3 + > src/Virt_VSMigrationService.c | 263 ++++++++++++++++++++++++++++++++++++++++- > 4 files changed, 289 insertions(+), 5 deletions(-) > > diff --git a/libvirt-cim.conf b/libvirt-cim.conf > index d3cb2c3..37d7b0f 100644 > --- a/libvirt-cim.conf > +++ b/libvirt-cim.conf > @@ -11,3 +11,22 @@ > # Default value: false > # > # readonly = false; > + > +# migrate_ssh_temp_key (string) > +# Defines a temp key file which would be used as ssh key in ssh migration, > +# Only when it is set, following methods in VirtualSystemMigrationService > +# could be used: > +# 1 MigrateSSHKeyCopy, use string property [SSH_Key_Src] to tell which key > +# to be copied. It will be copied to [migrate_ssh_temp_key]. > +# 2 MigrateVirtualSystemToHost and CheckVirtualSystemIsMigratableToHost, > +# use bool property [MigrationWithoutRootKey], to tell whether to use the key > +# as [migrate_ssh_temp_key]. > +# 3 MigrateSSHKeyDelete, when it is called [migrate_ssh_temp_key] will be > +# deleted. > +# Note: migrate_ssh_temp_key must be set in a directory completely owned by > +# root from bottom to top, such as /root/A, or /tmp/A. > +# > +# Possible values: {any path plus filename on host} > +# Default value: NULL, that is not set. > +# > +# migrate_ssh_temp_key = "/root/vm_migrate_tmp_id_rsa"; > diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c > index 820b42d..00eb4b1 100644 > --- a/libxkutil/misc_util.c > +++ b/libxkutil/misc_util.c > @@ -227,6 +227,15 @@ static int is_read_only(void) > return prop.value_bool; > } > > +const char *get_mig_ssh_tmp_key(void) > +{ > + static LibvirtcimConfigProperty prop = { > + "migrate_ssh_temp_key", CONFIG_STRING, {0}, 0}; > + > + libvirt_cim_config_get(&prop); > + return prop.value_string; > +} > + > virConnectPtr connect_by_classname(const CMPIBroker *broker, > const char *classname, > CMPIStatus *s) > diff --git a/libxkutil/misc_util.h b/libxkutil/misc_util.h > index 90fb2da..0f52290 100644 > --- a/libxkutil/misc_util.h > +++ b/libxkutil/misc_util.h > @@ -152,6 +152,9 @@ int virt_set_status(const CMPIBroker *broker, > > #define REF2STR(r) CMGetCharPtr(CMObjectPathToString(r, NULL)) > > +/* get libvirt-cim config */ > +const char *get_mig_ssh_tmp_key(void); > + > /* > * Local Variables: > * mode: C > diff --git a/src/Virt_VSMigrationService.c b/src/Virt_VSMigrationService.c > index 1f6659d..d03e1a0 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,54 @@ 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 { > + /* ? is already added */ > + rc = asprintf(&uri, "%s%s", uri, dest_params); > + } > + if (rc == -1) { > + uri = NULL; > + goto out; > + } > + } > out: > return uri; > } > > +/* Todo: move it to libcmpiutil */ > +static CMPIrc cu_get_bool_arg_my(const CMPIArgs *args, > + const char *name, > + bool *target) > +{ > + CMPIData argdata; > + CMPIStatus s; > + > + argdata = CMGetArg(args, name, &s); > + if ((s.rc != CMPI_RC_OK) || CMIsNullValue(argdata)) { > + return CMPI_RC_ERR_INVALID_PARAMETER; > + } > + > + if (argdata.type != CMPI_boolean) { > + return CMPI_RC_ERR_TYPE_MISMATCH; > + } > + > + *target = (bool)argdata.value.boolean; > + > + return CMPI_RC_OK; > +} > + I don't believe the above function is necessary - there is already a cu_get_bool_prop().

> static CMPIStatus get_msd_values(const CMPIObjectPath *ref, > const char *destination, > const CMPIArgs *argsin, > @@ -217,6 +257,13 @@ static CMPIStatus get_msd_values(const CMPIObjectPath *ref, > CMPIInstance *msd; > uint16_t uri_type; > char *uri = NULL; > + bool no_root_ssh_key = false; s/no_root_ssh_key/use_non_root_ssh_key/ Just a consideration, not a requirement. When I first saw the variable in use I had to think about it. "no_root_ssh_key" just had a different meaning in my mind.

> + char *dest_params = NULL; > + int ret; > + > + cu_get_bool_arg_my(argsin, > + "MigrationWithoutRootKey", > + &no_root_ssh_key); Why not use the existing cu_get_bool_prop(). You don't even check the return value here, so what's the use of your local routine?

> > s = get_msd(ref, argsin, &msd); > if (s.rc != CMPI_RC_OK) > @@ -230,7 +277,27 @@ static CMPIStatus get_msd_values(const CMPIObjectPath *ref, > if (s.rc != CMPI_RC_OK) > goto out; > > - uri = dest_uri(CLASSNAME(ref), destination, uri_type); > + if (no_root_ssh_key) { > + const char *tmp_keyfile = get_mig_ssh_tmp_key(); > + if (!tmp_keyfile) { > + cu_statusf(_BROKER, &s, > + CMPI_RC_ERR_FAILED, > + "Migration with special ssh key " > + "is not enabled in config file."); > + CU_DEBUG("Migration with special ssh key " > + "is not enabled in config file."); > + goto out; > + } > + CU_DEBUG("Trying migrate with specified ssh key file [%s].", > + tmp_keyfile); > + ret = asprintf(&dest_params, "keyfile=%s", tmp_keyfile); > + if (ret < 0) { > + CU_DEBUG("Failed in generating param string."); > + goto out; > + } > + } > + > + uri = dest_uri(CLASSNAME(ref), destination, dest_params, uri_type); > if (uri == NULL) { > cu_statusf(_BROKER, &s, > CMPI_RC_ERR_FAILED, > @@ -238,6 +305,7 @@ static CMPIStatus get_msd_values(const CMPIObjectPath *ref, > 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); > @@ -249,7 +317,7 @@ static CMPIStatus get_msd_values(const CMPIObjectPath *ref, > > out: > free(uri); > - > + free(dest_params); > return s; > } > > @@ -1538,7 +1606,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); > > @@ -1604,11 +1672,178 @@ static CMPIStatus migrate_vs_system(CMPIMethodMI *self, > return migrate_do(ref, ctx, name, dname, argsin, results, argsout); > } > > +/* return 0 on success */ > +static int pipe_exec(const char *cmd) > +{ > + FILE *stream = NULL; > + int ret = 0; > + char buf[256]; > + > + CU_DEBUG("executing system cmd [%s].", cmd); > + /* Todo: We need a better popen, currently stdout have been closed > + and SIGCHILD is handled by tog-pegasus, so fgets always got NULL > + making error detection not possible. */ > + stream = popen(cmd, "r"); > + if (stream == NULL) { > + CU_DEBUG("Failed to open pipe to run the command."); > + ret = -1; > + goto out; > + } > + usleep(10000); > + > + buf[255] = 0; > + while (fgets(buf, sizeof(buf), stream) != NULL) { > + CU_DEBUG("Exception got: [%s].", buf); > + ret = -2; > + goto out; > + } > + > + out: > + if (stream != NULL) { > + pclose(stream); > + } > + return ret; > +} > + > +/* > + * libvirt require private key specified to be placed in a directory owned by > + * root, because libvirt-cim now runs as root. So here the key would be copied. > + * In this way libvirt-cim could borrow a non-root ssh private key, instead of > + * using root's private key, avoid security risk. > + */ > +static int ssh_key_copy(const char *src, const char *dest) > +{ > + char *cmd = NULL; > + int ret = 0; > + struct stat sb; > + > + /* try delete it */ > + unlink(dest); > + ret = stat(dest, &sb); > + if (ret == 0) { > + CU_DEBUG("Can not delete [%s] before copy, " > + "maybe someone is using it.", > + dest); > + /* not a fatal fault */ Maybe not fatal, but what makes you believe a subsequent copy will

succeed? Is there a way to keep track of this being "in use" by some other thread that could delete it? It's almost like there needs to be a reference count.

Is it possible for two threads to be using it at the same time? Just

thinking out loud and trying to consider the negative consequences. In a former job we allowed 1 migration at a time and I'm not aware of the libvirt "restrictions" (yet). IOW, Before we unlink it, if it's already there what does that mean?

> + } > + > + ret = asprintf(&cmd, "cp -f %s %s", src, dest); > + if (ret < 0) { > + CU_DEBUG("Failed in combination for shell command."); > + goto out; > + } > + > + ret = pipe_exec(cmd); > + if (ret < 0) { > + CU_DEBUG("Error in executing command [%s]"); > + goto out; > + } > + > + ret = stat(dest, &sb); > + if (ret < 0) { > + CU_DEBUG("Can not find file [%s] after copy.", dest); > + } > + out: > + free(cmd); > + return ret; > +} > + > +static CMPIStatus migrate_sshkey_copy(CMPIMethodMI *self, > + const CMPIContext *ctx, > + const CMPIResult *results, > + const CMPIObjectPath *ref, > + const CMPIArgs *argsin, > + CMPIArgs *argsout) > +{ > + CMPIStatus s = {CMPI_RC_OK, NULL}; > + const char *ssh_key_src = NULL; > + int ret; > + > + const char *tmp_keyfile = get_mig_ssh_tmp_key(); > + if (!tmp_keyfile) { > + cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, > + "Migration with special ssh key " > + "is not enabled in config file."); > + CU_DEBUG("Migration with special ssh key " > + "is not enabled in config file."); > + goto out; > + } > + > + cu_get_str_arg(argsin, "SSH_Key_Src", &ssh_key_src); > + if (!ssh_key_src) { > + cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, > + "Failed to get property 'SSH_Key_Src'."); > + CU_DEBUG("Failed to get property 'SSH_Key_Src'."); > + goto out; > + } > + > + ret = ssh_key_copy(ssh_key_src, tmp_keyfile); > + if (ret < 0) { > + cu_statusf(_BROKER, &s, CMPI_RC_ERR_FAILED, > + "Got error in copying ssh key from [%s] to [%s].", > + ssh_key_src, tmp_keyfile); > + CU_DEBUG("Got error in copying ssh key from [%s] to [%s].", > + ssh_key_src, tmp_keyfile); > + } > + > + out: > + METHOD_RETURN(results, s.rc); > + return s; > +} > + > +static CMPIStatus migrate_sshkey_delete(CMPIMethodMI *self, > + const CMPIContext *ctx, > + const CMPIResult *results, > + const CMPIObjectPath *ref, > + const CMPIArgs *argsin, > + CMPIArgs *argsout) > +{ > + CMPIStatus s = {CMPI_RC_OK, NULL}; > + int ret; > + struct stat sb; Should we check for "MigrationWithoutRootKey" here too? Is it possible

for one thread to request migration without root key while another wants root key? Does the caller really need to know/care?

object setting, so we probably do care. > + > + const char *tmp_keyfile = get_mig_ssh_tmp_key(); > + if (!tmp_keyfile) { > + cu_statusf(_BROKER, &s, > + CMPI_RC_ERR_FAILED, > + "Migration with special ssh key " > + "is not enabled in config file."); > + CU_DEBUG("Migration with special ssh key " > + "is not enabled in config file."); > + goto out; > + } > + > + ret = stat(tmp_keyfile, &sb); > + if (ret == 0) { > + /* need delete */ > + ret = unlink(tmp_keyfile); > + if (ret < 0) { > + cu_statusf(_BROKER, &s, > + CMPI_RC_ERR_FAILED, > + "Failed to delete [%s].", > + tmp_keyfile); > + CU_DEBUG("Failed to delete [%s].", tmp_keyfile); > + } > + } else { > + /* not exist */ > + cu_statusf(_BROKER, &s, > + CMPI_RC_ERR_FAILED, > + "Can not find file [%s] before delete.", > + tmp_keyfile); > + CU_DEBUG("Can not find file [%s] before delete.", tmp_keyfile); Which perhaps means our other code above did the unlink() for us in another thread? Do you want an error here?

John > + } > + > + out: > + METHOD_RETURN(results, s.rc); > + return s; > +}; > + > static struct method_handler vsimth = { > .name = "CheckVirtualSystemIsMigratableToHost", > .handler = vs_migratable_host, > .args = {{"ComputerSystem", CMPI_ref, false}, > {"DestinationHost", CMPI_string, false}, > + {"MigrationWithoutRootKey", CMPI_boolean, true}, > {"MigrationSettingData", CMPI_instance, true}, > {"NewSystemSettingData", CMPI_instance, true}, > {"NewResourceSettingData", CMPI_instanceA, true}, > @@ -1633,6 +1868,7 @@ static struct method_handler mvsth = { > .handler = migrate_vs_host, > .args = {{"ComputerSystem", CMPI_ref, false}, > {"DestinationHost", CMPI_string, false}, > + {"MigrationWithoutRootKey", CMPI_boolean, true}, > {"MigrationSettingData", CMPI_instance, true}, > {"NewSystemSettingData", CMPI_instance, true}, > {"NewResourceSettingData", CMPI_instanceA, true}, > @@ -1652,11 +1888,28 @@ static struct method_handler mvsts = { > } > }; > > +static struct method_handler msshkc = { > + .name = "MigrateSSHKeyCopy", > + .handler = migrate_sshkey_copy, > + .args = {{"SSH_Key_Src", CMPI_string, true}, > + ARG_END > + } > +}; > + > +static struct method_handler msshkd = { > + .name = "MigrateSSHKeyDelete", > + .handler = migrate_sshkey_delete, > + .args = {ARG_END > + } > +}; > + > static struct method_handler *my_handlers[] = { > &vsimth, > &vsimts, > &mvsth, > &mvsts, > + &msshkc, > + &msshkd, > NULL > }; > > _______________________________________________ Libvirt-cim mailing list Libvirt-cim(a)redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim