The attributes/elements for auth type "chap" and "ceph" are complete different, this separates them into groups. And add "interleave" for "login" and "passwd" attributes of "chap" type auth. --- 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> </element> </define> -- 1.8.1.4

On 06/06/13 22:14, John Ferlan wrote: "passwd" is actually mandatory, see virStoragePoolDefParseAuthChap same for "sourceinfoauthsecret", see virStoragePoolDefParseAuthCephx 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

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")); + VIR_FREE(login); + VIR_FREE(username); + return -1; + } + + if (login) + auth->login = login; + else if (username) + auth->login = username; + auth->passwd = virXPathString("string(./auth/@passwd)", ctxt); if (auth->passwd == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -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"); -- 1.8.1.4

This also abstracts the code for parsing the XMLs to use the secret object as a helper. --- src/conf/storage_conf.c | 68 ++++++++++++++++++++++++++++--------------------- src/conf/storage_conf.h | 14 ++++++---- 2 files changed, 48 insertions(+), 34 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index c0bf084..0047372 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -439,6 +439,43 @@ virStoragePoolObjRemove(virStoragePoolObjListPtr pools, } } +static int +virStoragePoolDefParseAuthSecret(xmlXPathContextPtr ctxt, + virStoragePoolAuthSecretPtr secret) + +{ + char *uuid = NULL; + int ret = -1; + + uuid = virXPathString("string(./auth/secret/@uuid)", ctxt); + secret->usage = virXPathString("string(./auth/secret/@usage)", ctxt); + if (uuid == NULL && secret->usage == NULL) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing auth secret uuid or usage attribute")); + return -1; + } + + if (uuid != NULL) { + if (secret->usage != NULL) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("either auth secret uuid or usage expected")); + goto cleanup; + } + if (virUUIDParse(uuid, secret->uuid) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("invalid auth secret uuid")); + goto cleanup; + } + secret->uuidUsable = true; + } else { + secret->uuidUsable = false; + } + + ret = 0; +cleanup: + VIR_FREE(uuid); + return ret; +} static int virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt, @@ -484,9 +521,6 @@ static int virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt, virStoragePoolAuthCephxPtr auth) { - char *uuid = NULL; - int ret = -1; - auth->username = virXPathString("string(./auth/@username)", ctxt); if (auth->username == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -494,34 +528,10 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt, return -1; } - uuid = virXPathString("string(./auth/secret/@uuid)", ctxt); - auth->secret.usage = virXPathString("string(./auth/secret/@usage)", ctxt); - if (uuid == NULL && auth->secret.usage == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing auth secret uuid or usage attribute")); + if (virStoragePoolDefParseAuthSecret(ctxt, &auth->secret) < 0) return -1; - } - - if (uuid != NULL) { - if (auth->secret.usage != NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("either auth secret uuid or usage expected")); - goto cleanup; - } - if (virUUIDParse(uuid, auth->secret.uuid) < 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("invalid auth secret uuid")); - goto cleanup; - } - auth->secret.uuidUsable = true; - } else { - auth->secret.uuidUsable = false; - } - ret = 0; -cleanup: - VIR_FREE(uuid); - return ret; + return 0; } static int diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 8e739ff..aff1393 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -145,6 +145,14 @@ enum virStoragePoolAuthType { VIR_STORAGE_POOL_AUTH_CEPHX, }; +typedef struct _virStoragePoolAuthSecret virStoragePoolAuthSecret; +typedef virStoragePoolAuthSecret *virStoragePoolAuthSecretPtr; +struct _virStoragePoolAuthSecret { + unsigned char uuid[VIR_UUID_BUFLEN]; + char *usage; + bool uuidUsable; +}; + typedef struct _virStoragePoolAuthChap virStoragePoolAuthChap; typedef virStoragePoolAuthChap *virStoragePoolAuthChapPtr; struct _virStoragePoolAuthChap { @@ -156,11 +164,7 @@ typedef struct _virStoragePoolAuthCephx virStoragePoolAuthCephx; typedef virStoragePoolAuthCephx *virStoragePoolAuthCephxPtr; struct _virStoragePoolAuthCephx { char *username; - struct { - unsigned char uuid[VIR_UUID_BUFLEN]; - char *usage; - bool uuidUsable; - } secret; + virStoragePoolAuthSecret secret; }; /* -- 1.8.1.4

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); } 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; } @@ -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 { + 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> -- 1.8.1.4

On 06/06/13 23:34, John Ferlan wrote: Indeed. Agreed. 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

As a habit, "type" attribute is printed first. --- src/conf/storage_conf.c | 2 +- tests/storagepoolxml2xmlout/pool-rbd.xml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 8f83bae..a876332 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1175,7 +1175,7 @@ virStoragePoolSourceFormat(virBufferPtr buf, } if (src->authType == VIR_STORAGE_POOL_AUTH_CEPHX) { - virBufferAsprintf(buf," <auth username='%s' type='ceph'>\n", + virBufferAsprintf(buf," <auth type='ceph' username='%s'\n", src->auth.cephx.username); virStoragePoolAuthDefFormat(buf, src->auth.cephx.secret); virBufferAddLit(buf," </auth>\n"); diff --git a/tests/storagepoolxml2xmlout/pool-rbd.xml b/tests/storagepoolxml2xmlout/pool-rbd.xml index 309a6d9..4fe2fce 100644 --- a/tests/storagepoolxml2xmlout/pool-rbd.xml +++ b/tests/storagepoolxml2xmlout/pool-rbd.xml @@ -8,7 +8,7 @@ <name>rbd</name> <host name='localhost' port='6789'/> <host name='localhost' port='6790'/> - <auth username='admin' type='ceph'> + <auth type='ceph' username='admin'> <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/> </auth> </source> -- 1.8.1.4

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

On 07/06/13 00:31, John Ferlan wrote: No reason, will change. :-) It's handled in next patch. Commit log should be enough to clarify this patch only handles plain password: <...> Though the XML for "chap" authentication with plain "password" was introduced long ago, the function was never implemented. This patch completes it. </...> Will change. Sounds reasonable, I will see if there is requirement for it, if not will change too. Osier

On 05/28/2013 02:39 AM, Osier Yang wrote: 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; 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(). John

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(-) 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>&lt;auth&gt;</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>&lt;auth&gt;</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> -- 1.8.1.4

On 07/06/13 01:14, John Ferlan wrote: As said before, it's the things I don't want to touch at this stage, but since you mentioned it times, I will see if I can add the documents for pool auth (only).

On 05/28/2013 02:39 AM, Osier Yang wrote: 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. 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. John

Use the helpers Type{From,To}String instead. --- src/conf/storage_conf.c | 17 ++++++++++------- src/conf/storage_conf.h | 3 +++ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index a876332..bc8ca33 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -94,6 +94,10 @@ VIR_ENUM_IMPL(virStoragePoolSourceAdapterType, VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_LAST, "default", "scsi_host", "fc_host") +VIR_ENUM_IMPL(virStoragePoolAuthType, + VIR_STORAGE_POOL_AUTH_LAST, + "none", "chap", "ceph") + typedef const char *(*virStorageVolFormatToString)(int format); typedef int (*virStorageVolFormatFromString)(const char *format); @@ -709,11 +713,8 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, if (authType == NULL) { source->authType = VIR_STORAGE_POOL_AUTH_NONE; } else { - if (STREQ(authType, "chap")) { - source->authType = VIR_STORAGE_POOL_AUTH_CHAP; - } else if (STREQ(authType, "ceph")) { - source->authType = VIR_STORAGE_POOL_AUTH_CEPHX; - } else { + if ((source->authType = + virStoragePoolAuthTypeTypeFromString(authType)) < 0) { virReportError(VIR_ERR_XML_ERROR, _("unknown auth type '%s'"), authType); @@ -1162,7 +1163,8 @@ virStoragePoolSourceFormat(virBufferPtr buf, } if (src->authType == VIR_STORAGE_POOL_AUTH_CHAP) { - virBufferAsprintf(buf," <auth type='chap' username='%s'", + virBufferAsprintf(buf," <auth type='%s' username='%s'", + virStoragePoolAuthTypeTypeToString(src->authType), src->auth.chap.login); if (src->auth.chap.type == VIR_STORAGE_POOL_AUTH_CHAP_PLAIN_PASSWORD) { virBufferAsprintf(buf, " passwd='%s'/>\n", @@ -1175,7 +1177,8 @@ virStoragePoolSourceFormat(virBufferPtr buf, } if (src->authType == VIR_STORAGE_POOL_AUTH_CEPHX) { - virBufferAsprintf(buf," <auth type='ceph' username='%s'\n", + virBufferAsprintf(buf," <auth type='%s' username='%s'\n", + virStoragePoolAuthTypeTypeToString(src->authType), src->auth.cephx.username); 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 b52cdc4..06cd5c0 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -143,7 +143,10 @@ enum virStoragePoolAuthType { VIR_STORAGE_POOL_AUTH_NONE, VIR_STORAGE_POOL_AUTH_CHAP, VIR_STORAGE_POOL_AUTH_CEPHX, + + VIR_STORAGE_POOL_AUTH_LAST, }; +VIR_ENUM_DECL(virStoragePoolAuthType) typedef struct _virStoragePoolAuthSecret virStoragePoolAuthSecret; typedef virStoragePoolAuthSecret *virStoragePoolAuthSecretPtr; -- 1.8.1.4

--- 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> -- 1.8.1.4