[Libvirt-cim] [PATCH V5 15/15] migration: allow ssh based migration with non root's key file
Wenchao Xia
xiawenc at linux.vnet.ibm.com
Sun Mar 24 08:57:55 UTC 2013
于 2013-3-22 21:22, John Ferlan 写道:
> 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 at linux.vnet.ibm.com>
>> ---
>> 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().
>
A bit different, this function use const CMPIArgs *args, that one
use *inst.
>
>> 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.
>
OK.
>> + 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?
>
can't use cu_get_bool_prop(), parameter is different. it have a
default value, so no need to check whether fail.
>
>>
>> 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
OK, I'll make it fail here as a strict check.
> 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.
User can guarentee that no one is using it since all non-root-migrate
is started by him and he knows if they have complete or fail.
>
> Is it possible for two threads to be using it at the same time? Just
Yes, possible, and it is normal they share the key.
> 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?
>
The user just forgot delete it last time, I think it can be deleted
and recopy, since there is config file and user knows it is only used
by libvirt-cim.
>> + }
>> +
>> + 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
No, this parameter is not needed, it just delete the key, will fail
if config is not set.
> for one thread to request migration without root key while another wants
> root key? Does the caller really need to know/care?
Yes, it is allowed. This patch just added an option.
It looks to be an
> 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?
It is possible user called the method twice, so report the 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvirt-cim
>
--
Best Regards
Wenchao Xia
More information about the Libvirt-cim
mailing list