[libvirt] [PATCH 04/11] storage: Introduce XMLs to use secret object for pool auth
Osier Yang
jyang at redhat.com
Thu Jun 20 06:38:01 UTC 2013
On 06/06/13 23:34, John Ferlan wrote:
> On 05/28/2013 02:39 AM, Osier Yang wrote:
>> Using plain password is still supported for back-compat reason.
>>
>> Example XML:
>> <auth type='chap' username='foo'>
>> <secret uuid='48dcd4a4-b25f-4fc6-8874-84797c6e3678'/>
>> </auth>
>>
>> * docs/schemas/storagepool.rng (Add sourceinfoauthsecret as a choice)
>> * src/conf/storage_conf.h (union "passwd" and virStoragePoolAuthSecret)
>> * src/conf/storage_conf.c (s/chap\.passwd/chap\.u\.passwd/;
>> Add a helper virStoragePoolAuthDefFormat;
>> Parse the secret XMLs for "chap" auth)
>> * tests/storagepoolxml2xmlin/pool-iscsi-auth-secret.xml: (New tests)
>> * tests/storagepoolxml2xmlout/pool-iscsi-auth-secret.xml: (Likewise)
>> ---
>> docs/schemas/storagepool.rng | 9 ++-
>> src/conf/storage_conf.c | 71 +++++++++++++++-------
>> src/conf/storage_conf.h | 11 +++-
>> .../pool-iscsi-auth-secret.xml | 19 ++++++
>> .../pool-iscsi-auth-secret.xml | 22 +++++++
>> 5 files changed, 107 insertions(+), 25 deletions(-)
>> create mode 100644 tests/storagepoolxml2xmlin/pool-iscsi-auth-secret.xml
>> create mode 100644 tests/storagepoolxml2xmlout/pool-iscsi-auth-secret.xml
>>
>> diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng
>> index ba6c741..386de1b 100644
>> --- a/docs/schemas/storagepool.rng
>> +++ b/docs/schemas/storagepool.rng
>> @@ -295,9 +295,12 @@
>> <text/>
>> </attribute>
>> </choice>
>> - <attribute name='passwd'>
>> - <text/>
>> - </attribute>
>> + <choice>
>> + <attribute name='passwd'>
>> + <text/>
>> + </attribute>
>> + <ref name='sourceinfoauthsecret'/>
>> + </choice>
>> </interleave>
>> </group>
>> <group>
>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index 0047372..8f83bae 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -349,7 +349,10 @@ virStoragePoolSourceClear(virStoragePoolSourcePtr source)
>>
>> if (source->authType == VIR_STORAGE_POOL_AUTH_CHAP) {
>> VIR_FREE(source->auth.chap.login);
>> - VIR_FREE(source->auth.chap.passwd);
>> + if (source->auth.chap.type == VIR_STORAGE_POOL_AUTH_CHAP_PLAIN_PASSWORD)
>> + VIR_FREE(source->auth.chap.u.passwd);
>> + else if (source->auth.chap.type == VIR_STORAGE_POOL_AUTH_CHAP_SECRET)
>> + VIR_FREE(source->auth.chap.u.secret.usage);
> See [1] below in 'def'... It seems type must be one or the other *IF*
> we get it defined. As long as there's a "NONE" type option, then this
> code will be OK; otherwise, you have a bit more refactoring to do.
>
>> }
>>
>> if (source->authType == VIR_STORAGE_POOL_AUTH_CEPHX) {
>> @@ -483,6 +486,8 @@ virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt,
>> {
>> char *login = NULL;
>> char *username = NULL;
>> + char *passwd = NULL;
>> + int rc;
>>
>> login = virXPathString("string(./auth/@login)", ctxt);
>> username = virXPathString("string(./auth/@username)", ctxt);
>> @@ -507,10 +512,20 @@ virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt,
>> else if (username)
>> auth->login = username;
>>
>> - auth->passwd = virXPathString("string(./auth/@passwd)", ctxt);
>> - if (auth->passwd == NULL) {
>> + passwd = virXPathString("string(./auth/@passwd)", ctxt);
>> + rc = virStoragePoolDefParseAuthSecret(ctxt, &auth->u.secret);
>> +
>> + if (passwd) {
>> + auth->type = VIR_STORAGE_POOL_AUTH_CHAP_PLAIN_PASSWORD;
>> + auth->u.passwd = passwd;
>> + } else if (rc == 0) {
>> + auth->type = VIR_STORAGE_POOL_AUTH_CHAP_SECRET;
>> + } else if (rc < 0) {
>> + return -1;
>> + } else {
>> virReportError(VIR_ERR_XML_ERROR, "%s",
>> - _("missing auth passwd attribute"));
>> + _("Either 'passwd' attribute or 'secret' element "
>> + "should be specified"));
>> return -1;
>> }
> [1] According to this code - either we have passwd or secret being used;
> however, there's no check if both were supplied, thus making the default
> be passwd and ignoring the secret - which will be confusing in other
> checks... such as a memory leak on the free side...
Indeed.
> Additionally
> because this code may not be reached the 'type' would need a "NONE"
> possibility.
>
> If neither is supplied, then we have an error, so at least one must be
> used. I would think you'd prefer secret over plaintext password..
>
> Thus, consider:
> if (virStoragePoolDefParseAuthSecret(ctxt, &auth->u.secret) < 0) {
> auth->passwd = virXPathString("string(./auth/@passwd)", ctxt);
> if (!auth->passwd) {
> error condition;
> return -1;
> }
> auth->type = VIR_STORAGE_POOL_AUTH_CHAP_PLAIN_PASSWORD;
> } else {
> auth->type = VIR_STORAGE_POOL_AUTH_CHAP_SECRET;
> }
> return 0;
Agreed.
>
> NOTE: No need for locals passwd and rc.
>
>>
>> @@ -1048,13 +1063,30 @@ virStoragePoolDefParseFile(const char *filename)
>> return virStoragePoolDefParse(NULL, filename);
>> }
>>
>> +static void
>> +virStoragePoolAuthDefFormat(virBufferPtr buf,
>> + virStoragePoolAuthSecret secret)
>> +{
>> + char uuid[VIR_UUID_STRING_BUFLEN];
>> +
>> + virBufferAddLit(buf, " <secret");
>> + if (secret.uuidUsable) {
>> + virUUIDFormat(secret.uuid, uuid);
>> + virBufferAsprintf(buf, " uuid='%s'", uuid);
>> + }
>> +
>> + if (secret.usage != NULL) {
>> + virBufferAsprintf(buf, " usage='%s'", secret.usage);
>> + }
>> + virBufferAddLit(buf, "/>\n");
>> +}
>> +
>> static int
>> virStoragePoolSourceFormat(virBufferPtr buf,
>> virStoragePoolOptionsPtr options,
>> virStoragePoolSourcePtr src)
>> {
>> int i, j;
>> - char uuid[VIR_UUID_STRING_BUFLEN];
>>
>> virBufferAddLit(buf," <source>\n");
>> if ((options->flags & VIR_STORAGE_POOL_SOURCE_HOST) && src->nhost) {
>> @@ -1129,26 +1161,23 @@ virStoragePoolSourceFormat(virBufferPtr buf,
>> virBufferAsprintf(buf," <format type='%s'/>\n", format);
>> }
>>
>> - if (src->authType == VIR_STORAGE_POOL_AUTH_CHAP)
>> - virBufferAsprintf(buf," <auth type='chap' username='%s' passwd='%s'/>\n",
>> - src->auth.chap.login,
>> - src->auth.chap.passwd);
>> + if (src->authType == VIR_STORAGE_POOL_AUTH_CHAP) {
>> + virBufferAsprintf(buf," <auth type='chap' username='%s'",
>> + src->auth.chap.login);
>> + if (src->auth.chap.type == VIR_STORAGE_POOL_AUTH_CHAP_PLAIN_PASSWORD) {
>> + virBufferAsprintf(buf, " passwd='%s'/>\n",
>> + src->auth.chap.u.passwd);
>> + } else if (src->auth.chap.type == VIR_STORAGE_POOL_AUTH_CHAP_SECRET) {
>> + virBufferAddLit(buf, ">\n");
>> + virStoragePoolAuthDefFormat(buf, src->auth.chap.u.secret);
>> + virBufferAddLit(buf," </auth>\n");
>> + }
>> + }
>>
>> if (src->authType == VIR_STORAGE_POOL_AUTH_CEPHX) {
>> virBufferAsprintf(buf," <auth username='%s' type='ceph'>\n",
>> src->auth.cephx.username);
>> -
>> - virBufferAddLit(buf," <secret");
>> - if (src->auth.cephx.secret.uuidUsable) {
>> - virUUIDFormat(src->auth.cephx.secret.uuid, uuid);
>> - virBufferAsprintf(buf," uuid='%s'", uuid);
>> - }
>> -
>> - if (src->auth.cephx.secret.usage != NULL) {
>> - virBufferAsprintf(buf," usage='%s'", src->auth.cephx.secret.usage);
>> - }
>> - virBufferAddLit(buf,"/>\n");
>> -
>> + virStoragePoolAuthDefFormat(buf, src->auth.cephx.secret);
>> virBufferAddLit(buf," </auth>\n");
>> }
>>
>> diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
>> index aff1393..b52cdc4 100644
>> --- a/src/conf/storage_conf.h
>> +++ b/src/conf/storage_conf.h
>> @@ -153,11 +153,20 @@ struct _virStoragePoolAuthSecret {
>> bool uuidUsable;
>> };
>>
>> +enum virStoragePoolAuthChapType {
> I think you need a VIR_STORAGE_POOL_AUTH_CHAP_NONE = 0, here
>
I see no need, since it must be either PASSWORD or SECRET,
otherwise there is error, it's mandatory to have either a 'passwd'
or 'secret'.
Osier
More information about the libvir-list
mailing list