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.hindex 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list