On 05/28/2013 02:39 AM, Osier Yang wrote:

The attributes/elements for auth type "chap" and "ceph" are complete different, this separates them into groups.

s/complete/completely/ s/this separates/these patches separate/

And add "interleave" for "login" and "passwd" attributes of "chap" type auth.

s/And add/Added or Changed "chap" type "login" and "passwd" attributes to be be interleaved. The only question/comment below is the 'intention' of removing the "optional" attribute from 'passwd' and 'sourceinfoauthsecret'.

--- docs/schemas/storagepool.rng | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index 3c2158a..2595e37 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -280,28 +280,30 @@

<define name='sourceinfoauth'> <element name='auth'> - <attribute name='type'> - <choice> - <value>chap</value> - <value>ceph</value> - </choice> - </attribute> <choice> - <attribute name='login'> - <text/> - </attribute> - <attribute name='username'> - <text/> - </attribute> + <group> + <attribute name='type'> + <value>chap</value> + </attribute> + <interleave> + <attribute name='login'> + <text/> + </attribute> + <attribute name='passwd'> + <text/> + </attribute> + </interleave> + </group> + <group> + <attribute name='type'> + <value>ceph</value> + </attribute> + <attribute name='username'> + <text/> + </attribute> + <ref name='sourceinfoauthsecret'/> + </group> </choice> - <optional> - <attribute name='passwd'> - <text/> - </attribute> - </optional> - <optional> - <ref name='sourceinfoauthsecret'/> - </optional>

Both of these changed to have to be non-optional... Reading the "formatdomain.html" page is "confusing" at best since 'passwd' isn't mentioned. It would seem to me that the formatdomain page should also be updated based on what I see here as part of this change. John

</element> </define>

On 20/06/13 14:18, Osier Yang wrote:

On 06/06/13 22:14, John Ferlan wrote:

...

On 05/28/2013 02:39 AM, Osier Yang wrote:

...

The attributes/elements for auth type "chap" and "ceph" are complete different, this separates them into groups. s/complete/completely/ s/this separates/these patches separate/

...

And add "interleave" for "login" and "passwd" attributes of "chap" type auth. s/And add/Added

or Changed "chap" type "login" and "passwd" attributes to be be interleaved.

The only question/comment below is the 'intention' of removing the "optional" attribute from 'passwd' and 'sourceinfoauthsecret'.

...

--- docs/schemas/storagepool.rng | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index 3c2158a..2595e37 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -280,28 +280,30 @@ <define name='sourceinfoauth'> <element name='auth'> - <attribute name='type'> - <choice> - <value>chap</value> - <value>ceph</value> - </choice> - </attribute> <choice> - <attribute name='login'> - <text/> - </attribute> - <attribute name='username'> - <text/> - </attribute> + <group> + <attribute name='type'> + <value>chap</value> + </attribute> + <interleave> + <attribute name='login'> + <text/> + </attribute> + <attribute name='passwd'> + <text/> + </attribute> + </interleave> + </group> + <group> + <attribute name='type'> + <value>ceph</value> + </attribute> + <attribute name='username'> + <text/> + </attribute> + <ref name='sourceinfoauthsecret'/> + </group> </choice> - <optional> - <attribute name='passwd'> - <text/> - </attribute> - </optional> - <optional> - <ref name='sourceinfoauthsecret'/> - </optional> Both of these changed to have to be non-optional... Reading the "formatdomain.html" page is "confusing" at best since 'passwd' isn't mentioned.

"passwd" is actually mandatory, see virStoragePoolDefParseAuthChap

I misunderstood you a bit, "passwd" is never supported by domain, it's the right thing, plain password is always not good for security. <...> the domain XML intentionally does not expose the password, only the reference to the object that does manage the password </...>

same for "sourceinfoauthsecret", see virStoragePoolDefParseAuthCephx

domain doesn't do the checking for requirement of either "uuid" or "usage", but it's the thing it should do. I don't see any reason why it doesn't require it for a "ceph" type auth.

...

It would seem to me that the formatdomain page should also be updated based on what I see here as part of this change.

You should read formatstorage.html.in, unfortunately, it's a history problem, we lack of documents for most of the storage stuffs, we should do it later, but it will waste lots of time to figure out the right documents, which I don't want to touch at this stage..

Osier

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 05/28/2013 02:39 AM, Osier Yang wrote:

To be consistent with what we use in disk auth, and "ceph" type storage "auth", this supports "username". "login" is still supported for back-compat reason. --- docs/schemas/storagepool.rng | 12 +++++++--- src/conf/storage_conf.c | 27 ++++++++++++++++++---- .../storagepoolxml2xmlin/pool-iscsi-auth-login.xml | 17 ++++++++++++++ .../pool-iscsi-auth-username.xml | 17 ++++++++++++++ tests/storagepoolxml2xmlin/pool-iscsi-auth.xml | 17 -------------- .../pool-iscsi-auth-login.xml | 20 ++++++++++++++++ .../pool-iscsi-auth-username.xml | 20 ++++++++++++++++ tests/storagepoolxml2xmlout/pool-iscsi-auth.xml | 20 ---------------- .../pool-iscsi-vendor-product.xml | 2 +- tests/storagepoolxml2xmltest.c | 3 ++- 10 files changed, 109 insertions(+), 46 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-iscsi-auth-login.xml create mode 100644 tests/storagepoolxml2xmlin/pool-iscsi-auth-username.xml delete mode 100644 tests/storagepoolxml2xmlin/pool-iscsi-auth.xml create mode 100644 tests/storagepoolxml2xmlout/pool-iscsi-auth-login.xml create mode 100644 tests/storagepoolxml2xmlout/pool-iscsi-auth-username.xml delete mode 100644 tests/storagepoolxml2xmlout/pool-iscsi-auth.xml

diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index 2595e37..ba6c741 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -286,9 +286,15 @@ <value>chap</value> </attribute> <interleave> - <attribute name='login'> - <text/> - </attribute> + <choice> + <!-- Legacy, but still supported to keep back-compat --> + <attribute name='login'> + <text/> + </attribute> + <attribute name='username'> + <text/> + </attribute> + </choice> <attribute name='passwd'> <text/> </attribute> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index ed9effd..c0bf084 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -444,13 +444,32 @@ static int virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt, virStoragePoolAuthChapPtr auth) { - auth->login = virXPathString("string(./auth/@login)", ctxt); - if (auth->login == NULL) { + char *login = NULL; + char *username = NULL; + + login = virXPathString("string(./auth/@login)", ctxt); + username = virXPathString("string(./auth/@username)", ctxt); + + if (!login && !username) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing auth login attribute")); + _("missing auth login or username attribute")); return -1; }

+ if (login && username) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'login' is legacy name of 'username', they " + "are exclusive"));

Consider: s/are exclusive/cannot both be supplied/

+ VIR_FREE(login); + VIR_FREE(username); + return -1; + } + + if (login) + auth->login = login; + else if (username) + auth->login = username; + auth->login = username ? username : login;

Both cannot be NULL and both cannot be supplied.

auth->passwd = virXPathString("string(./auth/@passwd)", ctxt); if (auth->passwd == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s",

hrmph... my question in 1/11 now seems negated since it seems passwd was required after all. ACK - just cleanup grammar in commit message and John

@@ -1101,7 +1120,7 @@ virStoragePoolSourceFormat(virBufferPtr buf, }

if (src->authType == VIR_STORAGE_POOL_AUTH_CHAP) - virBufferAsprintf(buf," <auth type='chap' login='%s' passwd='%s'/>\n", + virBufferAsprintf(buf," <auth type='chap' username='%s' passwd='%s'/>\n", src->auth.chap.login, src->auth.chap.passwd);

diff --git a/tests/storagepoolxml2xmlin/pool-iscsi-auth-login.xml b/tests/storagepoolxml2xmlin/pool-iscsi-auth-login.xml new file mode 100644 index 0000000..f7d4d52 --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-iscsi-auth-login.xml @@ -0,0 +1,17 @@ +<pool type='iscsi'> + <name>virtimages</name> + <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid> + <source> + <host name="iscsi.example.com"/> + <device path="demo-target"/> + <auth type='chap' login='foobar' passwd='frobbar'/> + </source> + <target> + <path>/dev/disk/by-path</path> + <permissions> + <mode>0700</mode> + <owner>0</owner> + <group>0</group> + </permissions> + </target> +</pool> diff --git a/tests/storagepoolxml2xmlin/pool-iscsi-auth-username.xml b/tests/storagepoolxml2xmlin/pool-iscsi-auth-username.xml new file mode 100644 index 0000000..b8e4d37 --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-iscsi-auth-username.xml @@ -0,0 +1,17 @@ +<pool type='iscsi'> + <name>virtimages</name> + <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid> + <source> + <host name="iscsi.example.com"/> + <device path="demo-target"/> + <auth type='chap' username='foobar' passwd='frobbar'/> + </source> + <target> + <path>/dev/disk/by-path</path> + <permissions> + <mode>0700</mode> + <owner>0</owner> + <group>0</group> + </permissions> + </target> +</pool> diff --git a/tests/storagepoolxml2xmlin/pool-iscsi-auth.xml b/tests/storagepoolxml2xmlin/pool-iscsi-auth.xml deleted file mode 100644 index f7d4d52..0000000 --- a/tests/storagepoolxml2xmlin/pool-iscsi-auth.xml +++ /dev/null @@ -1,17 +0,0 @@ -<pool type='iscsi'> - <name>virtimages</name> - <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid> - <source> - <host name="iscsi.example.com"/> - <device path="demo-target"/> - <auth type='chap' login='foobar' passwd='frobbar'/> - </source> - <target> - <path>/dev/disk/by-path</path> - <permissions> - <mode>0700</mode> - <owner>0</owner> - <group>0</group> - </permissions> - </target> -</pool> diff --git a/tests/storagepoolxml2xmlout/pool-iscsi-auth-login.xml b/tests/storagepoolxml2xmlout/pool-iscsi-auth-login.xml new file mode 100644 index 0000000..5e3e8a2 --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-iscsi-auth-login.xml @@ -0,0 +1,20 @@ +<pool type='iscsi'> + <name>virtimages</name> + <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid> + <capacity unit='bytes'>0</capacity> + <allocation unit='bytes'>0</allocation> + <available unit='bytes'>0</available> + <source> + <host name='iscsi.example.com'/> + <device path='demo-target'/> + <auth type='chap' username='foobar' passwd='frobbar'/> + </source> + <target> + <path>/dev/disk/by-path</path> + <permissions> + <mode>0700</mode> + <owner>0</owner> + <group>0</group> + </permissions> + </target> +</pool> diff --git a/tests/storagepoolxml2xmlout/pool-iscsi-auth-username.xml b/tests/storagepoolxml2xmlout/pool-iscsi-auth-username.xml new file mode 100644 index 0000000..5e3e8a2 --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-iscsi-auth-username.xml @@ -0,0 +1,20 @@ +<pool type='iscsi'> + <name>virtimages</name> + <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid> + <capacity unit='bytes'>0</capacity> + <allocation unit='bytes'>0</allocation> + <available unit='bytes'>0</available> + <source> + <host name='iscsi.example.com'/> + <device path='demo-target'/> + <auth type='chap' username='foobar' passwd='frobbar'/> + </source> + <target> + <path>/dev/disk/by-path</path> + <permissions> + <mode>0700</mode> + <owner>0</owner> + <group>0</group> + </permissions> + </target> +</pool> diff --git a/tests/storagepoolxml2xmlout/pool-iscsi-auth.xml b/tests/storagepoolxml2xmlout/pool-iscsi-auth.xml deleted file mode 100644 index 4fa8f64..0000000 --- a/tests/storagepoolxml2xmlout/pool-iscsi-auth.xml +++ /dev/null @@ -1,20 +0,0 @@ -<pool type='iscsi'> - <name>virtimages</name> - <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid> - <capacity unit='bytes'>0</capacity> - <allocation unit='bytes'>0</allocation> - <available unit='bytes'>0</available> - <source> - <host name='iscsi.example.com'/> - <device path='demo-target'/> - <auth type='chap' login='foobar' passwd='frobbar'/> - </source> - <target> - <path>/dev/disk/by-path</path> - <permissions> - <mode>0700</mode> - <owner>0</owner> - <group>0</group> - </permissions> - </target> -</pool> diff --git a/tests/storagepoolxml2xmlout/pool-iscsi-vendor-product.xml b/tests/storagepoolxml2xmlout/pool-iscsi-vendor-product.xml index 6ae1c39..9558e59 100644 --- a/tests/storagepoolxml2xmlout/pool-iscsi-vendor-product.xml +++ b/tests/storagepoolxml2xmlout/pool-iscsi-vendor-product.xml @@ -7,7 +7,7 @@ <source> <host name='iscsi.example.com'/> <device path='demo-target'/> - <auth type='chap' login='foobar' passwd='frobbar'/> + <auth type='chap' username='foobar' passwd='frobbar'/> <vendor name='test-vendor'/> <product name='test-product'/> </source> diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index 0376e63..b8935ee 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -88,7 +88,8 @@ mymain(void) DO_TEST("pool-logical-create"); DO_TEST("pool-disk"); DO_TEST("pool-iscsi"); - DO_TEST("pool-iscsi-auth"); + DO_TEST("pool-iscsi-auth-login"); + DO_TEST("pool-iscsi-auth-username"); DO_TEST("pool-netfs"); DO_TEST("pool-scsi"); DO_TEST("pool-scsi-type-scsi-host");

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... 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; 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 Depending on the rest of the review not sure if a v2 is necessary, but might be nice to post anyway. John

+ VIR_STORAGE_POOL_AUTH_CHAP_PLAIN_PASSWORD, + VIR_STORAGE_POOL_AUTH_CHAP_SECRET, +}; + typedef struct _virStoragePoolAuthChap virStoragePoolAuthChap; typedef virStoragePoolAuthChap *virStoragePoolAuthChapPtr; struct _virStoragePoolAuthChap { char *login; - char *passwd; + int type; /* enum virStoragePoolAuthChapType */ + union { + char *passwd; + virStoragePoolAuthSecret secret; + } u; };

typedef struct _virStoragePoolAuthCephx virStoragePoolAuthCephx; diff --git a/tests/storagepoolxml2xmlin/pool-iscsi-auth-secret.xml b/tests/storagepoolxml2xmlin/pool-iscsi-auth-secret.xml new file mode 100644 index 0000000..c897cc6 --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-iscsi-auth-secret.xml @@ -0,0 +1,19 @@ +<pool type='iscsi'> + <name>virtimages</name> + <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid> + <source> + <host name="iscsi.example.com"/> + <device path="demo-target"/> + <auth type='chap' username='foobar'> + <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/> + </auth> + </source> + <target> + <path>/dev/disk/by-path</path> + <permissions> + <mode>0700</mode> + <owner>0</owner> + <group>0</group> + </permissions> + </target> +</pool> diff --git a/tests/storagepoolxml2xmlout/pool-iscsi-auth-secret.xml b/tests/storagepoolxml2xmlout/pool-iscsi-auth-secret.xml new file mode 100644 index 0000000..0ab3b3d --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-iscsi-auth-secret.xml @@ -0,0 +1,22 @@ +<pool type='iscsi'> + <name>virtimages</name> + <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid> + <capacity unit='bytes'>0</capacity> + <allocation unit='bytes'>0</allocation> + <available unit='bytes'>0</available> + <source> + <host name='iscsi.example.com'/> + <device path='demo-target'/> + <auth type='chap' username='foobar'> + <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/> + </auth> + </source> + <target> + <path>/dev/disk/by-path</path> + <permissions> + <mode>0700</mode> + <owner>0</owner> + <group>0</group> + </permissions> + </target> +</pool>

On 06/20/2013 02:38 AM, Osier Yang wrote:

On 06/06/13 23:34, John Ferlan wrote:

...

On 05/28/2013 02:39 AM, Osier Yang wrote: ...snip...

...

...

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

It's been a bit since I reviewed, but I think the point was given the context of the code *as written*, then NONE could be defined and if set, sure would be an error. The order of the definitions and the "default" value of 0 (zero) is important since all allocations result in fields being initialized to zero. Thus the assumption 'could be' in places in the code that VIR_STORAGE_POOL_AUTH_CHAP_PLAIN_PASSWORD is the value for auth->type. If you have a NONE=0 definition, then code decisions can be made based upon that... John

On 05/28/2013 02:39 AM, Osier Yang wrote:

Though the XML for "chap" authentication with plain "password" was introduced long ago, the function was never implemented. This patch completes it.

There are two types of CHAP configurations supported for iSCSI authentication: * Initiator Authentication Forward, one-way; The initiator is authenticated by the target.

* Target Authentication Reverse, Bi-directional, mutual, two-way; The target is authenticated by the initiator; This method also requires Initiator Authentication

This only supports the "Initiator Authentication". (I don't have any enterprise iSCSI env for testing, only have a iSCSI target setup with tgtd, which doesn't support "Target Authentication").

"Discovery authentication" is not supported by tgt yet too. So this only setup the session authentication by executing 3 iscsiadm commands, E.g:

% iscsiadm -m node --target "iqn.2013-05.test:iscsi.foo" --name \ "node.session.auth.authmethod" -v "CHAP" --op update

% iscsiadm -m node --target "iqn.2013-05.test:iscsi.foo" --name \ "node.session.auth.username" -v "Jim" --op update

% iscsiadm -m node --target "iqn.2013-05.test:iscsi.foo" --name \ "node.session.auth.password" -v "Jimsecret" --op update --- src/storage/storage_backend_iscsi.c | 68 +++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+)

diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index ad38ab2..6a17b5a 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -713,6 +713,68 @@ virStorageBackendISCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, return ret; }

+static int +virStorageBackendISCSINodeUpdate(const char *portal, + const char *target, + const char *name, + const char *value) +{ + virCommandPtr cmd = NULL; + int status; + int ret = -1; + + cmd = virCommandNewArgList(ISCSIADM, + "--mode", "node", + "--portal", portal, + "--target", target, + "--op", "update", + "--name", name, + "--value", value, + NULL); + + if (virCommandRun(cmd, &status) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to update '%s' of node mode for target '%s'"), + name, target); + goto cleanup; + } + + ret = 0; +cleanup: + virCommandFree(cmd); + return ret; +} + +static int +virStorageBackendISCSISetAuth(virStoragePoolDefPtr def, + const char *portal,

[1] Any reason to not pass portal first like other API's? Not that matters..

+ const char *target) +{ + if (def->source.authType == VIR_STORAGE_POOL_AUTH_NONE) + return 0; + + if (def->source.authType != VIR_STORAGE_POOL_AUTH_CHAP) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("iscsi pool only supports 'chap' auth type")); + return -1; + } + + if (virStorageBackendISCSINodeUpdate(portal, + target, + "node.session.auth.authmethod", + "CHAP") < 0 || + virStorageBackendISCSINodeUpdate(portal, + target, + "node.session.auth.username", + def->source.auth.chap.login) < 0 || + virStorageBackendISCSINodeUpdate(portal, + target, + "node.session.auth.password", + def->source.auth.chap.u.passwd) < 0)

In 04/11, you added def->source.auth.type setting/checking which I don't see here... Or is there some magic happening where auth.u.secret can take the place of passwd...

+ return -1; + + return 0; +}

static int virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, @@ -745,6 +807,7 @@ virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, if ((session = virStorageBackendISCSISession(pool, 1)) == NULL) { if ((portal = virStorageBackendISCSIPortal(&pool->def->source)) == NULL) goto cleanup; + /* * iscsiadm doesn't let you login to a target, unless you've * first issued a 'sendtargets' command to the portal :-( @@ -754,6 +817,11 @@ virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, NULL, NULL) < 0) goto cleanup;

+ if (virStorageBackendISCSISetAuth(pool->def, + portal,

[1] Why not follow other code which passes portal first... Also why pass pool->def and pool->def->source.devices[0].path - it's not like you couldn't grab "source.devices[0].path" from the passed pool->def John

+ pool->def->source.devices[0].path) < 0) + goto cleanup; + if (virStorageBackendISCSIConnection(portal, pool->def->source.initiator.iqn, pool->def->source.devices[0].path,

On 07/06/13 00:51, John Ferlan wrote:

On 05/28/2013 02:39 AM, Osier Yang wrote:

...

Based on the plain password chap "auth" support, this gets the secret value (password) with the secret driver methods, and apply it for the "iscsiadm" update command. Ugh. Of course - it's the "next" patch (gets me every time).

In any case, since 6/11 cannot "assume" the passwd field is filled in, you probably need to combine the two or just make the check in 6/11 for type != PLAIN_PASSWORD - return 0;

Prefer the later.

...

--- src/storage/storage_backend_iscsi.c | 56 +++++++++++++++++++++++++++++++++---- 1 file changed, 50 insertions(+), 6 deletions(-)

diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 6a17b5a..516025a 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -35,6 +35,8 @@ #include <unistd.h> #include <sys/stat.h>

+#include "datatypes.h" +#include "driver.h" #include "virerror.h" #include "storage_backend_scsi.h" #include "storage_backend_iscsi.h" @@ -42,6 +44,7 @@ #include "virlog.h" #include "virfile.h" #include "vircommand.h" +#include "virobject.h" #include "virrandom.h" #include "virstring.h"

@@ -746,10 +749,17 @@ cleanup: }

static int -virStorageBackendISCSISetAuth(virStoragePoolDefPtr def, +virStorageBackendISCSISetAuth(virConnectPtr conn, + virStoragePoolDefPtr def, const char *portal, const char *target) { + virSecretPtr secret = NULL; + unsigned char *secret_value = NULL; + const char *passwd = NULL; + virStoragePoolAuthChap chap = def->source.auth.chap; + int ret = -1; + if (def->source.authType == VIR_STORAGE_POOL_AUTH_NONE) return 0;

@@ -759,6 +769,35 @@ virStorageBackendISCSISetAuth(virStoragePoolDefPtr def, return -1; }

+ if (chap.type == VIR_STORAGE_POOL_AUTH_CHAP_SECRET) { + if (chap.u.secret.uuidUsable) + secret = virSecretLookupByUUID(conn, chap.u.secret.uuid); + else + secret = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_ISCSI, + chap.u.secret.usage); + + if (secret) { + size_t secret_size; + secret_value = conn->secretDriver->secretGetValue(secret, &secret_size, 0, + VIR_SECRET_GET_VALUE_INTERNAL_CALL); + if (!secret_value) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("could not get the value of the secret " + "for username %s"), chap.login); + goto cleanup; + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("username '%s' specified but secret not found"), + chap.login); + goto cleanup; + } + + passwd = (const char *)secret_value; + } else if (chap.type == VIR_STORAGE_POOL_AUTH_CHAP_PLAIN_PASSWORD) { + passwd = chap.u.passwd; + } + if (virStorageBackendISCSINodeUpdate(portal, target, "node.session.auth.authmethod", @@ -770,14 +809,18 @@ virStorageBackendISCSISetAuth(virStoragePoolDefPtr def, virStorageBackendISCSINodeUpdate(portal, target, "node.session.auth.password", - def->source.auth.chap.u.passwd) < 0) - return -1; + passwd) < 0) + goto cleanup;

- return 0; + ret = 0; +cleanup: + virObjectUnref(secret); + VIR_FREE(secret_value); + return ret; }

static int -virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, +virStorageBackendISCSIStartPool(virConnectPtr conn, Seeing this change to used now makes me wonder... Do we need to now check that conn != NULL anywhere?

Since "virStorageBackendISCSIStartPool" is the "startPool" entry point - I used cscope and looked for "startPool", I found one reference where a NULL was passed:

src/storage/storage_driver.c storageDriverAutostart() ... if (backend->startPool && backend->startPool(NULL, pool) < 0) { virErrorPtr err = virGetLastError(); ...

If I'm reading things right, I think that will cause issues in virSecretLookupByUUID() and virSecretLookupByUsage() when called by virStorageBackendISCSISetAuth().

Reasonable, will update. Osier

On 05/28/2013 02:39 AM, Osier Yang wrote:

To mention that the secret type "iscsi" and "ceph" can be used to storage pool too. --- docs/formatsecret.html.in | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)

I think you should take the opportunity to describe the available options a little better in the formatdomain and formatstorage pages. In fact, 'auth' is not even mentioned on the formatstorage page. Once the latter is done you could provide the direct "#" link to the page tag. John

diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in index 50c9533..3e306b5 100644 --- a/docs/formatsecret.html.in +++ b/docs/formatsecret.html.in @@ -64,8 +64,9 @@ a single <code>name</code> element that specifies a usage name for the secret. The Ceph secret can then be used by UUID or by this usage name via the <code><auth></code> element of - a <a href="formatdomain.html#elementsDisks">disk - device</a>. <span class="since">Since 0.9.7</span>. + a <a href="formatdomain.html#elementsDisks">disk device</a> or + a <a href="formatstorage.html">storage pool (rbd)</a>. + <span class="since">Since 0.9.7</span>. </p>

<h3>Usage type "iscsi"</h3> @@ -76,8 +77,9 @@ a single <code>target</code> element that specifies a usage name for the secret. The iSCSI secret can then be used by UUID or by this usage name via the <code><auth></code> element of - a <a href="formatdomain.html#elementsDisks">disk - device</a>. <span class="since">Since 1.0.4</span>. + a <a href="formatdomain.html#elementsDisks">disk device</a> or + a <a href="formatstorage.html">storage pool (iscsi)</a>. + <span class="since">Since 1.0.4</span>. </p>

<h2><a name="example">Example</a></h2>

On 07/06/13 01:26, John Ferlan wrote:

On 05/28/2013 02:39 AM, Osier Yang wrote:

...

Without the flag VIR_SECRET_GET_VALUE_INTERNAL_CALL, there is no way to get the value of private secret. And error out if the secret value is not found. --- src/storage/storage_backend_rbd.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)

Is this patch separatable? That is - is it required for this set of changes or is it "out of band" enough to be its own patch.

separatable, it's the thing I found when coding on the way.

...

diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 953a8ee..d66d3f9 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -23,6 +23,7 @@

#include <config.h>

+#include "datatypes.h" #include "virerror.h" #include "storage_backend_rbd.h" #include "storage_conf.h" @@ -88,7 +89,17 @@ static int virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr *ptr, goto cleanup; }

- secret_value = virSecretGetValue(secret, &secret_value_size, 0); + secret_value = conn->secretDriver->secretGetValue(secret, &secret_value_size, 0, + VIR_SECRET_GET_VALUE_INTERNAL_CALL); + There are callers to this function that have set ATTRIBUTE_UNUSED on the 'conn' parameter. Now this code uses it - so it seems you have some more checking to do.

See virStorageBackendRBDRefreshPool() and virStorageBackendRBDResizeVol()

Using the same logic as before I see that storage_driver.c and storageDriverAutostart() will call the backend->refreshPool with NULL and that will cause you issues in this code.

Okay. Osier

On 28/05/13 14:39, Osier Yang wrote:

--- tests/storagepoolxml2xmlin/pool-rbd.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/storagepoolxml2xmlin/pool-rbd.xml b/tests/storagepoolxml2xmlin/pool-rbd.xml index 4ee8d56..056ba6e 100644 --- a/tests/storagepoolxml2xmlin/pool-rbd.xml +++ b/tests/storagepoolxml2xmlin/pool-rbd.xml @@ -5,7 +5,7 @@ <host name='localhost' port='6789'/> <host name='localhost' port='6790'/> <auth username='admin' type='ceph'> - <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/> + <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/> </auth> </source> </pool>

Pushed this independantly of the series under trivial rule. Osier