[libvirt] [PATCH v2 0/7] Support CHAP authentication for iscsi pool

https://bugzilla.redhat.com/show_bug.cgi?id=957294 Since Osier is out temporarily, I was asked to pick up this work since I initially reviewed the changes. Originally posted by Osier as: https://www.redhat.com/archives/libvir-list/2013-May/msg01879.html I reviewed the changes and had comments here: https://www.redhat.com/archives/libvir-list/2013-June/msg00286.html https://www.redhat.com/archives/libvir-list/2013-June/msg00288.html https://www.redhat.com/archives/libvir-list/2013-June/msg00290.html https://www.redhat.com/archives/libvir-list/2013-June/msg00294.html https://www.redhat.com/archives/libvir-list/2013-June/msg00298.html https://www.redhat.com/archives/libvir-list/2013-June/msg00303.html https://www.redhat.com/archives/libvir-list/2013-June/msg00301.html https://www.redhat.com/archives/libvir-list/2013-June/msg00302.html https://www.redhat.com/archives/libvir-list/2013-June/msg00304.html https://www.redhat.com/archives/libvir-list/2013-June/msg00305.html v2 changes * Adjust code/commit messages based on my comments and Osier's responses * Due to the issue noted in 7/11 from the original series regarding making the secret driver calls during the 'startPool()' path and not having a connection yet, I moved the calls to the 'findPoolSources()' entry since there is a connection provided then. Combined 6/11, 7/11, 8/11, & 9/11 into one commit. Since I complained about the lack of documentation that was also added to the formatstorage.html.in to describe the final level of changes. * My review of 9/11 missed that the two virSecret*() calls made above had to succeed prior to the direct reference of 'conn' in the secret driver secretGetValue() API. Still not convinced the code will work for the paths noted in my review, but either of the existing paths will return a NULL secret value thus there's no new regression. John Ferlan (2): storage: Support "username" for "chap" type "auth" storage: Introduce XMLs to use secret object for pool auth Osier Yang (5): storage: Refactor the rng schema for storage pool auth storage: Add a struct for auth secret storage: Output auth type before username storage: Support "chap" authentication for iscsi pool storage: Improve the pool auth type parsing and formatting docs/formatsecret.html.in | 10 +- docs/formatstorage.html.in | 31 +++- docs/schemas/storagepool.rng | 51 ++++--- src/conf/storage_conf.c | 170 ++++++++++++++------- src/conf/storage_conf.h | 28 +++- src/storage/storage_backend_iscsi.c | 113 +++++++++++++- src/storage/storage_backend_rbd.c | 13 +- .../storagepoolxml2xmlin/pool-iscsi-auth-login.xml | 17 +++ .../pool-iscsi-auth-secret.xml | 19 +++ .../pool-iscsi-auth-username.xml | 17 +++ tests/storagepoolxml2xmlin/pool-iscsi-auth.xml | 17 --- .../pool-iscsi-auth-login.xml | 20 +++ .../pool-iscsi-auth-secret.xml | 22 +++ .../pool-iscsi-auth-username.xml | 20 +++ tests/storagepoolxml2xmlout/pool-iscsi-auth.xml | 20 --- .../pool-iscsi-vendor-product.xml | 2 +- tests/storagepoolxml2xmlout/pool-rbd.xml | 2 +- tests/storagepoolxml2xmltest.c | 3 +- 18 files changed, 442 insertions(+), 133 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-iscsi-auth-login.xml create mode 100644 tests/storagepoolxml2xmlin/pool-iscsi-auth-secret.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-secret.xml create mode 100644 tests/storagepoolxml2xmlout/pool-iscsi-auth-username.xml delete mode 100644 tests/storagepoolxml2xmlout/pool-iscsi-auth.xml -- 1.8.1.4

From: Osier Yang <jyang@redhat.com> The attributes/elements for auth type "chap" and "ceph" are completely different, but the schema didn't reflect that. This patch separates each of the definitions into groups. Changed "chap" type "login" and "passwd" attributes to be be interleaved. Also adjusted the documentation of types to match code. The formatdomain.html page indicated the 'type' field could be 'ceph' or 'iscsi', when in fact it should have been 'chap'. --- 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 10/07/13 03:10, John Ferlan wrote:
From: Osier Yang <jyang@redhat.com>
The attributes/elements for auth type "chap" and "ceph" are completely different, but the schema didn't reflect that. This patch separates each of the definitions into groups.
Changed "chap" type "login" and "passwd" attributes to be be interleaved.
Also adjusted the documentation of types to match code. The formatdomain.html page indicated the 'type' field could be 'ceph' or 'iscsi', when in fact it should have been 'chap'.
i see no documentation change in this patch. is it missed?

To be consistent with "ceph" types for storage "auth" elements, allow "username" to be used as an "auth" attribute name for "chap" types. Continue to allow "login" for backwards compatibility when reading the XML, but when writing the XML use "username". --- docs/schemas/storagepool.rng | 12 ++++++++--- src/conf/storage_conf.c | 24 ++++++++++++++++++---- .../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, 106 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 2539c45..997df54 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -457,13 +457,29 @@ 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', both " + "cannot be supplied")); + VIR_FREE(login); + VIR_FREE(username); + return -1; + } + + auth->login = username ? username : login; + auth->passwd = virXPathString("string(./auth/@passwd)", ctxt); if (auth->passwd == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -1123,7 +1139,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

On Tue, Jul 09, 2013 at 03:10:46PM -0400, John Ferlan wrote:
To be consistent with "ceph" types for storage "auth" elements, allow "username" to be used as an "auth" attribute name for "chap" types. Continue to allow "login" for backwards compatibility when reading the XML, but when writing the XML use "username".
Hmm, so the schema for 'chap' auth is utterly awful. While we have parsed this schema for a while, nothing in the libvirt codebase has ever used 'chap' auth. As such I think we have reasonable grounds for just discarding the existing code for parsing 'chap' auth and doing it right. ie use the same terminology as 'ceph' and do not include the 'password' value in the XML at all. Thoughts ? Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 07/10/2013 10:49 AM, Daniel P. Berrange wrote:
On Tue, Jul 09, 2013 at 03:10:46PM -0400, John Ferlan wrote:
To be consistent with "ceph" types for storage "auth" elements, allow "username" to be used as an "auth" attribute name for "chap" types. Continue to allow "login" for backwards compatibility when reading the XML, but when writing the XML use "username".
Hmm, so the schema for 'chap' auth is utterly awful.
While we have parsed this schema for a while, nothing in the libvirt codebase has ever used 'chap' auth.
As such I think we have reasonable grounds for just discarding the existing code for parsing 'chap' auth and doing it right. ie use the same terminology as 'ceph' and do not include the 'password' value in the XML at all.
Thoughts ?
Daniel
Don't have the historical perspective w/r/t when it was added - even looking through gitk I can find that parsing in for quite a while (even in the src/storage_conf.c). I admit I found it quite confusing to read and attempt to document. Other than having to actually make the changes :-), I see no reason why the storage_conf needs to be different and support numerous combinations based on type... To be sure we're on the same page, the storage_conf XML then becomes <auth username='someuser'> <secret type='[ceph|iscsi]' [usage='mypassed'|uuid='someuuid']/> </auth> That is there'd be no reason for 'type' in the XML nor 'password'. The 'login' goes away and the 'username' becomes required. Should we search for 'login' or 'password' when parsing and issue some sort of warning/message via VIR_WARN or virReportError? Ditto for 'type'? Or do we just silently ignore? Cannot imagine someone putting a plain text password in the XML file, but then again I also know what happens when you assume something... John

On Wed, Jul 10, 2013 at 11:51:42AM -0400, John Ferlan wrote:
On 07/10/2013 10:49 AM, Daniel P. Berrange wrote:
On Tue, Jul 09, 2013 at 03:10:46PM -0400, John Ferlan wrote:
To be consistent with "ceph" types for storage "auth" elements, allow "username" to be used as an "auth" attribute name for "chap" types. Continue to allow "login" for backwards compatibility when reading the XML, but when writing the XML use "username".
Hmm, so the schema for 'chap' auth is utterly awful.
While we have parsed this schema for a while, nothing in the libvirt codebase has ever used 'chap' auth.
As such I think we have reasonable grounds for just discarding the existing code for parsing 'chap' auth and doing it right. ie use the same terminology as 'ceph' and do not include the 'password' value in the XML at all.
Thoughts ?
Daniel
Don't have the historical perspective w/r/t when it was added - even looking through gitk I can find that parsing in for quite a while (even in the src/storage_conf.c). I admit I found it quite confusing to read and attempt to document.
Other than having to actually make the changes :-), I see no reason why the storage_conf needs to be different and support numerous combinations based on type...
To be sure we're on the same page, the storage_conf XML then becomes
<auth username='someuser'> <secret type='[ceph|iscsi]' [usage='mypassed'|uuid='someuuid']/> </auth>
Actually I think the schema is: <auth username='someuser' type='[ceph|chap]'> <secret [usage='mypassed'|uuid='someuuid']/> </auth>
That is there'd be no reason for 'type' in the XML nor 'password'. The 'login' goes away and the 'username' becomes required.
I think you still want to keep 'type' in the XML, since if iSCSI adds a different auth mechanism that isn't 'chap', then we have the ability to represent that using a new type.
Should we search for 'login' or 'password' when parsing and issue some sort of warning/message via VIR_WARN or virReportError? Ditto for 'type'? Or do we just silently ignore? Cannot imagine someone putting a plain text password in the XML file, but then again I also know what happens when you assume something...
My suggestion is to just silently ignore it. Anyone who has been adding 'login' or 'password' to the storage pool XML has never had anything which works. Thus just removing the XML parser code for this can't make life any worse for them. They were broken before, and will remain broken afterwards. Using the new schema will do the right thing for them. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 07/10/2013 12:04 PM, Daniel P. Berrange wrote:
On Wed, Jul 10, 2013 at 11:51:42AM -0400, John Ferlan wrote:
On 07/10/2013 10:49 AM, Daniel P. Berrange wrote:
On Tue, Jul 09, 2013 at 03:10:46PM -0400, John Ferlan wrote: ...<snip>
To be sure we're on the same page, the storage_conf XML then becomes
<auth username='someuser'> <secret type='[ceph|iscsi]' [usage='mypassed'|uuid='someuuid']/> </auth>
Actually I think the schema is:
<auth username='someuser' type='[ceph|chap]'> <secret [usage='mypassed'|uuid='someuuid']/> </auth>
That is there'd be no reason for 'type' in the XML nor 'password'. The 'login' goes away and the 'username' becomes required.
I think you still want to keep 'type' in the XML, since if iSCSI adds a different auth mechanism that isn't 'chap', then we have the ability to represent that using a new type.
The <secret> XML has an existing 'type' which is 'ceph', 'iscsi' (eg, CHAP), or 'volume' and I just figured that since the <auth> XML requires 'username' and the <secret> subelement with a mandatory 'type' attribute, then that would cover the needs. See: http://libvirt.org/formatdomain.html#elementsDisks I'm not objecting to using the 'type' in the <auth> XML, just noting that's it's a duplication of an attribute, although I suppose if some authentication mechanism was added in the future that didn't use the secret element, then having a type present make things easier. John

On Thu, Jul 11, 2013 at 10:11:16AM -0400, John Ferlan wrote:
On 07/10/2013 12:04 PM, Daniel P. Berrange wrote:
On Wed, Jul 10, 2013 at 11:51:42AM -0400, John Ferlan wrote:
On 07/10/2013 10:49 AM, Daniel P. Berrange wrote:
On Tue, Jul 09, 2013 at 03:10:46PM -0400, John Ferlan wrote: ...<snip>
To be sure we're on the same page, the storage_conf XML then becomes
<auth username='someuser'> <secret type='[ceph|iscsi]' [usage='mypassed'|uuid='someuuid']/> </auth>
Actually I think the schema is:
<auth username='someuser' type='[ceph|chap]'> <secret [usage='mypassed'|uuid='someuuid']/> </auth>
That is there'd be no reason for 'type' in the XML nor 'password'. The 'login' goes away and the 'username' becomes required.
I think you still want to keep 'type' in the XML, since if iSCSI adds a different auth mechanism that isn't 'chap', then we have the ability to represent that using a new type.
The <secret> XML has an existing 'type' which is 'ceph', 'iscsi' (eg, CHAP), or 'volume' and I just figured that since the <auth> XML requires 'username' and the <secret> subelement with a mandatory 'type' attribute, then that would cover the needs.
See: http://libvirt.org/formatdomain.html#elementsDisks
I'm not objecting to using the 'type' in the <auth> XML, just noting that's it's a duplication of an attribute, although I suppose if some authentication mechanism was added in the future that didn't use the secret element, then having a type present make things easier.
Hmm, we're comparing two different XML schemas. You're quoting the schema for the <disk> element's <auth> tag in the guest XML. I'm referring to the schema for the <auth> tag in the storage pool XML Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 11/07/13 00:04, Daniel P. Berrange wrote:
On Wed, Jul 10, 2013 at 11:51:42AM -0400, John Ferlan wrote:
On 07/10/2013 10:49 AM, Daniel P. Berrange wrote:
On Tue, Jul 09, 2013 at 03:10:46PM -0400, John Ferlan wrote:
To be consistent with "ceph" types for storage "auth" elements, allow "username" to be used as an "auth" attribute name for "chap" types. Continue to allow "login" for backwards compatibility when reading the XML, but when writing the XML use "username". Hmm, so the schema for 'chap' auth is utterly awful.
While we have parsed this schema for a while, nothing in the libvirt codebase has ever used 'chap' auth.
As such I think we have reasonable grounds for just discarding the existing code for parsing 'chap' auth and doing it right. ie use the same terminology as 'ceph' and do not include the 'password' value in the XML at all.
Thoughts ?
Daniel
Don't have the historical perspective w/r/t when it was added - even looking through gitk I can find that parsing in for quite a while (even in the src/storage_conf.c). I admit I found it quite confusing to read and attempt to document.
Other than having to actually make the changes :-), I see no reason why the storage_conf needs to be different and support numerous combinations based on type...
To be sure we're on the same page, the storage_conf XML then becomes
<auth username='someuser'> <secret type='[ceph|iscsi]' [usage='mypassed'|uuid='someuuid']/> </auth> Actually I think the schema is:
<auth username='someuser' type='[ceph|chap]'>
as a habit, it should be <auth type='[ceph|chap]' username='someuser'>

On 10/07/13 22:49, Daniel P. Berrange wrote:
On Tue, Jul 09, 2013 at 03:10:46PM -0400, John Ferlan wrote:
To be consistent with "ceph" types for storage "auth" elements, allow "username" to be used as an "auth" attribute name for "chap" types. Continue to allow "login" for backwards compatibility when reading the XML, but when writing the XML use "username". Hmm, so the schema for 'chap' auth is utterly awful.
While we have parsed this schema for a while, nothing in the libvirt codebase has ever used 'chap' auth.
i didn't realize this.
As such I think we have reasonable grounds for just discarding the existing code for parsing 'chap' auth and doing it right. ie use the same terminology as 'ceph' and do not include the 'password' value in the XML at all.
agreed. silently ignoring the 'login'and 'passwd' is fine, there are no documentations for the two attrs too.
Thoughts ?
Daniel

From: Osier Yang <jyang@redhat.com> Create _virStoragePoolAuthSecret for the _virStoragePoolAuthCephx to abstract parsing the XML secret object. --- 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 997df54..3b789f7 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -452,6 +452,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, @@ -494,9 +531,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", @@ -504,34 +538,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 c183427..452f583 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -148,6 +148,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 { @@ -159,11 +167,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

Allow using a secret object type for 'chap' authentication attributes. Using plain password is still supported for back-compat reason. Introduce virStoragePoolAuthChapType enum to allow either "plain password" or "chap secret" as options keeping the plain text as the first option, even though it's preferable to use the secret type. When parsing XML check for secret first, but fall back to plain passwd. 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 | 69 ++++++++++++++-------- src/conf/storage_conf.h | 11 +++- .../pool-iscsi-auth-secret.xml | 19 ++++++ .../pool-iscsi-auth-secret.xml | 22 +++++++ 5 files changed, 103 insertions(+), 27 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 3b789f7..1e75ba8 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -362,7 +362,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) { @@ -517,11 +520,17 @@ virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt, auth->login = username ? username : login; - auth->passwd = virXPathString("string(./auth/@passwd)", ctxt); - if (auth->passwd == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing auth passwd attribute")); - return -1; + if (virStoragePoolDefParseAuthSecret(ctxt, &auth->u.secret) < 0) { + auth->u.passwd = virXPathString("string(./auth/@passwd)", ctxt); + if (!auth->u.passwd) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Either 'passwd' attribute or 'secret' element " + "must be specified")); + return -1; + } + auth->type = VIR_STORAGE_POOL_AUTH_CHAP_PLAIN_PASSWORD; + } else { + auth->type = VIR_STORAGE_POOL_AUTH_CHAP_SECRET; } return 0; @@ -1067,13 +1076,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) { @@ -1148,26 +1174,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 452f583..233340e 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -156,11 +156,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

From: Osier Yang <jyang@redhat.com> Change the order of output to print the 'type' first in the XML --- 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 1e75ba8..ba71f42 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1188,7 +1188,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

From: Osier Yang <jyang@redhat.com> Although the XML for "chap" authentication with plain "password" was introduced long ago, the function was never implemented. This patch completes it by performing the authentication during findPoolSources() processing of pools with an authType of VIR_STORAGE_POOL_AUTH_CHAP specified. 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 Update storage_backend_rbd.c to use the internal API to get the secret value instead and error out if the secret value is not found. Without the flag VIR_SECRET_GET_VALUE_INTERNAL_CALL, there is no way to get the value of private secret. Update the formatstorage.html to describe the usage of the secret element to mention that the secret type "iscsi" and "ceph" can be used to storage pool too. Update the formatsecret.html to include a reference to the storage pool --- docs/formatsecret.html.in | 10 ++-- docs/formatstorage.html.in | 31 +++++++++- src/storage/storage_backend_iscsi.c | 113 +++++++++++++++++++++++++++++++++++- src/storage/storage_backend_rbd.c | 13 ++++- 4 files changed, 159 insertions(+), 8 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><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> diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index d702eb1..cf028dd 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -72,6 +72,9 @@ <source> <host name="iscsi.example.com"/> <device path="demo-target"/> + <auth type='chap' username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> <vendor name="Acme"/> <product name="model"/> </source> @@ -80,7 +83,6 @@ <pre> ... <source> - <source> <adapter type='fc_host' parent='scsi_host5' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/> </source> ...</pre> @@ -123,6 +125,33 @@ which is the hostname or IP address of the server. May optionally contain a <code>port</code> attribute for the protocol specific port number. <span class="since">Since 0.4.1</span></dd> + <dt><code>auth</code></dt> + <dd>If present, the <code>auth</code> element provides the + authentication credentials needed to access the source by the + setting of the <code>type</code> attribute. The <code>type</code> + must be either "chap" or "ceph". For a "chap" <code>type</code> + attribute, mandatory attributes <code>login</code> or + <code>username</code> and either a <code>passwd</code> or a + <code>secret</code> sub-element are used to provide the + authentication identification. <span class="since">Since 1.1.1,</span> + when writing out the XML, libvirt will utilize the + <code>username</code> instead of <code>login</code>. + For a "ceph" <code>type</code> attribute a mandatory attribute + <code>username</code>, which identifies the username to use during + authentication as well as a sub-element <code>secret</code> with + a mandatory attribute <code>type</code>, to tie back to a + <a href="formatsecret.html">libvirt secret object</a> that + holds the actual password or other credentials. The domain XML + intentionally does not expose the password, only the reference + to the object that does manage the password. The secret element + <code>type</code> must be either "ceph" or "iscsi". Use "ceph" for + Ceph RBD (rados block device) network sources and use "iscsi" for CHAP + (Challenge-Handshake Authentication Protocol) iSCSI targets. + The <code>secret</code> element requires either a <code>uuid</code> + attribute with the UUID of the secret object or a <code>usage</code> + attribute matching the key that was specified in the + secret object. <span class="since">Since 0.9.7</span> + </dd> <dt><code>name</code></dt> <dd>Provides the source for pools backed by storage from a named element (e.g., a logical volume group name). diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 0a4cd22..673ca16 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -32,6 +32,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" @@ -39,6 +41,7 @@ #include "virlog.h" #include "virfile.h" #include "vircommand.h" +#include "virobject.h" #include "virrandom.h" #include "virstring.h" @@ -545,9 +548,112 @@ cleanup: 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(const char *portal, + virConnectPtr conn, + virStoragePoolSourcePtr source) +{ + virSecretPtr secret = NULL; + unsigned char *secret_value = NULL; + const char *passwd = NULL; + virStoragePoolAuthChap chap; + int ret = -1; + + if (source->authType == VIR_STORAGE_POOL_AUTH_NONE) + return 0; + + if (source->authType != VIR_STORAGE_POOL_AUTH_CHAP) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("iscsi pool only supports 'chap' auth type")); + return -1; + } + + chap = source->auth.chap; + 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, + source->devices[0].path, + "node.session.auth.authmethod", + "CHAP") < 0 || + virStorageBackendISCSINodeUpdate(portal, + source->devices[0].path, + "node.session.auth.username", + source->auth.chap.login) < 0 || + virStorageBackendISCSINodeUpdate(portal, + source->devices[0].path, + "node.session.auth.password", + passwd) < 0) + goto cleanup; + + ret = 0; + +cleanup: + virObjectUnref(secret); + VIR_FREE(secret_value); + return ret; +} static char * -virStorageBackendISCSIFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, +virStorageBackendISCSIFindPoolSources(virConnectPtr conn, const char *srcSpec, unsigned int flags) { @@ -590,6 +696,9 @@ virStorageBackendISCSIFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, &ntargets, &targets) < 0) goto cleanup; + if (virStorageBackendISCSISetAuth(portal, conn, source) < 0) + goto cleanup; + if (VIR_ALLOC_N(list.sources, ntargets) < 0) { virReportOOMError(); goto cleanup; @@ -668,7 +777,6 @@ virStorageBackendISCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, return ret; } - static int virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) @@ -700,6 +808,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 :-( 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); + + if (!secret_value) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("could not get the value of the secret " + "for username %s"), + pool->def->source.auth.cephx.username); + goto cleanup; + } + base64_encode_alloc((char *)secret_value, secret_value_size, &rados_key); memset(secret_value, 0, secret_value_size); -- 1.8.1.4

On 10/07/13 03:10, John Ferlan wrote:
From: Osier Yang <jyang@redhat.com>
Although the XML for "chap" authentication with plain "password" was introduced long ago, the function was never implemented. This patch completes it by performing the authentication during findPoolSources() processing of pools with an authType of VIR_STORAGE_POOL_AUTH_CHAP specified.
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
Update storage_backend_rbd.c to use the internal API to get the secret value instead and error out if the secret value is not found. Without the flag VIR_SECRET_GET_VALUE_INTERNAL_CALL, there is no way to get the value of private secret.
Update the formatstorage.html to describe the usage of the secret element to mention that the secret type "iscsi" and "ceph" can be used to storage pool too.
Update the formatsecret.html to include a reference to the storage pool --- docs/formatsecret.html.in | 10 ++-- docs/formatstorage.html.in | 31 +++++++++- src/storage/storage_backend_iscsi.c | 113 +++++++++++++++++++++++++++++++++++- src/storage/storage_backend_rbd.c | 13 ++++- 4 files changed, 159 insertions(+), 8 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><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> diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index d702eb1..cf028dd 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -72,6 +72,9 @@ <source> <host name="iscsi.example.com"/> <device path="demo-target"/> + <auth type='chap' username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> <vendor name="Acme"/> <product name="model"/> </source> @@ -80,7 +83,6 @@ <pre> ... <source> - <source> <adapter type='fc_host' parent='scsi_host5' wwnn='20000000c9831b4b' wwpn='10000000c9831b4b'/> </source> ...</pre> @@ -123,6 +125,33 @@ which is the hostname or IP address of the server. May optionally contain a <code>port</code> attribute for the protocol specific port number. <span class="since">Since 0.4.1</span></dd> + <dt><code>auth</code></dt> + <dd>If present, the <code>auth</code> element provides the + authentication credentials needed to access the source by the + setting of the <code>type</code> attribute. The <code>type</code> + must be either "chap" or "ceph". For a "chap" <code>type</code> + attribute, mandatory attributes <code>login</code> or + <code>username</code> and either a <code>passwd</code> or a + <code>secret</code> sub-element are used to provide the + authentication identification. <span class="since">Since 1.1.1,</span> + when writing out the XML, libvirt will utilize the + <code>username</code> instead of <code>login</code>. + For a "ceph" <code>type</code> attribute a mandatory attribute + <code>username</code>, which identifies the username to use during + authentication as well as a sub-element <code>secret</code> with + a mandatory attribute <code>type</code>, to tie back to a + <a href="formatsecret.html">libvirt secret object</a> that + holds the actual password or other credentials. The domain XML + intentionally does not expose the password, only the reference + to the object that does manage the password. The secret element + <code>type</code> must be either "ceph" or "iscsi". Use "ceph" for + Ceph RBD (rados block device) network sources and use "iscsi" for CHAP + (Challenge-Handshake Authentication Protocol) iSCSI targets. + The <code>secret</code> element requires either a <code>uuid</code> + attribute with the UUID of the secret object or a <code>usage</code> + attribute matching the key that was specified in the + secret object. <span class="since">Since 0.9.7</span> + </dd> <dt><code>name</code></dt> <dd>Provides the source for pools backed by storage from a named element (e.g., a logical volume group name). diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 0a4cd22..673ca16 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -32,6 +32,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" @@ -39,6 +41,7 @@ #include "virlog.h" #include "virfile.h" #include "vircommand.h" +#include "virobject.h" #include "virrandom.h" #include "virstring.h"
@@ -545,9 +548,112 @@ cleanup: 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(const char *portal, + virConnectPtr conn, + virStoragePoolSourcePtr source) +{ + virSecretPtr secret = NULL; + unsigned char *secret_value = NULL; + const char *passwd = NULL; + virStoragePoolAuthChap chap; + int ret = -1; + + if (source->authType == VIR_STORAGE_POOL_AUTH_NONE) + return 0; + + if (source->authType != VIR_STORAGE_POOL_AUTH_CHAP) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("iscsi pool only supports 'chap' auth type")); + return -1; + } + + chap = source->auth.chap; + 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, + source->devices[0].path, + "node.session.auth.authmethod", + "CHAP") < 0 || + virStorageBackendISCSINodeUpdate(portal, + source->devices[0].path, + "node.session.auth.username", + source->auth.chap.login) < 0 || + virStorageBackendISCSINodeUpdate(portal, + source->devices[0].path, + "node.session.auth.password", + passwd) < 0) + goto cleanup; + + ret = 0; + +cleanup: + virObjectUnref(secret); + VIR_FREE(secret_value); + return ret; +}
static char * -virStorageBackendISCSIFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, +virStorageBackendISCSIFindPoolSources(virConnectPtr conn, const char *srcSpec, unsigned int flags) { @@ -590,6 +696,9 @@ virStorageBackendISCSIFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, &ntargets, &targets) < 0) goto cleanup;
+ if (virStorageBackendISCSISetAuth(portal, conn, source) < 0) + goto cleanup; +
i think the chap auth iscsi pool won't be able to start with this change, findPoolSources is an api for discovering the pool sources. however, we want the chap auth are set up before connecting to the iscsi target when starting the pool, otherwise the pool starting will fail on authentication. note that startPool and findPoolSources are independant apis, they don't invoke each other. with this change, if one wants to start the pool successfully, he has to invoke the findPoolSources first, this dependancy is incorrect. as an example, in virsh layer, one has to execute find-storage-pool-sources or find-storage-pool-sources-as before pool-start. as an alternative way to have the non-null 'conn' for startPool, we can create a connection in storageDriverAutostart (like qemu driver does), which is the only place pass an null 'conn' to startPool, regards osier

On 07/15/2013 10:16 AM, Osier Yang wrote:
On 10/07/13 03:10, John Ferlan wrote:
From: Osier Yang <jyang@redhat.com>
<...snip...>
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 0a4cd22..673ca16 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -32,6 +32,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" @@ -39,6 +41,7 @@ #include "virlog.h" #include "virfile.h" #include "vircommand.h" +#include "virobject.h" #include "virrandom.h" #include "virstring.h" @@ -545,9 +548,112 @@ cleanup: 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(const char *portal, + virConnectPtr conn, + virStoragePoolSourcePtr source) +{ + virSecretPtr secret = NULL; + unsigned char *secret_value = NULL; + const char *passwd = NULL; + virStoragePoolAuthChap chap; + int ret = -1; + + if (source->authType == VIR_STORAGE_POOL_AUTH_NONE) + return 0; + + if (source->authType != VIR_STORAGE_POOL_AUTH_CHAP) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("iscsi pool only supports 'chap' auth type")); + return -1; + } + + chap = source->auth.chap; + 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, + source->devices[0].path, + "node.session.auth.authmethod", + "CHAP") < 0 || + virStorageBackendISCSINodeUpdate(portal, + source->devices[0].path, + "node.session.auth.username", + source->auth.chap.login) < 0 || + virStorageBackendISCSINodeUpdate(portal, + source->devices[0].path, + "node.session.auth.password", + passwd) < 0) + goto cleanup; + + ret = 0; + +cleanup: + virObjectUnref(secret); + VIR_FREE(secret_value); + return ret; +} static char * -virStorageBackendISCSIFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, +virStorageBackendISCSIFindPoolSources(virConnectPtr conn, const char *srcSpec, unsigned int flags) { @@ -590,6 +696,9 @@ virStorageBackendISCSIFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, &ntargets, &targets) < 0) goto cleanup; + if (virStorageBackendISCSISetAuth(portal, conn, source) < 0) + goto cleanup; +
i think the chap auth iscsi pool won't be able to start with this change, findPoolSources is an api for discovering the pool sources. however, we want the chap auth are set up before connecting to the iscsi target when starting the pool, otherwise the pool starting will fail on authentication. note that startPool and findPoolSources are independant apis, they don't invoke each other.
with this change, if one wants to start the pool successfully, he has to invoke the findPoolSources first, this dependancy is incorrect. as an example, in virsh layer, one has to execute find-storage-pool-sources or find-storage-pool-sources-as before pool-start.
as an alternative way to have the non-null 'conn' for startPool, we can create a connection in storageDriverAutostart (like qemu driver does), which is the only place pass an null 'conn' to startPool,
While there is a v3 posted - this code hasn't differed there, so I'll just use this opportunity to ask you questions... A 'conn' to what? Should we assume qemu like nwfilter_driver.c does? if (!driverState->privileged) return 0; conn = virConnectOpen("qemu:///system"); Do we further restrict the StartPool code to ensure there is a privileged qemu connection? This was the decision point I had to make... John
regards osier

On 15/07/13 23:01, John Ferlan wrote:
On 07/15/2013 10:16 AM, Osier Yang wrote:
On 10/07/13 03:10, John Ferlan wrote:
From: Osier Yang <jyang@redhat.com> <...snip...>
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 0a4cd22..673ca16 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -32,6 +32,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" @@ -39,6 +41,7 @@ #include "virlog.h" #include "virfile.h" #include "vircommand.h" +#include "virobject.h" #include "virrandom.h" #include "virstring.h" @@ -545,9 +548,112 @@ cleanup: 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(const char *portal, + virConnectPtr conn, + virStoragePoolSourcePtr source) +{ + virSecretPtr secret = NULL; + unsigned char *secret_value = NULL; + const char *passwd = NULL; + virStoragePoolAuthChap chap; + int ret = -1; + + if (source->authType == VIR_STORAGE_POOL_AUTH_NONE) + return 0; + + if (source->authType != VIR_STORAGE_POOL_AUTH_CHAP) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("iscsi pool only supports 'chap' auth type")); + return -1; + } + + chap = source->auth.chap; + 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, + source->devices[0].path, + "node.session.auth.authmethod", + "CHAP") < 0 || + virStorageBackendISCSINodeUpdate(portal, + source->devices[0].path, + "node.session.auth.username", + source->auth.chap.login) < 0 || + virStorageBackendISCSINodeUpdate(portal, + source->devices[0].path, + "node.session.auth.password", + passwd) < 0) + goto cleanup; + + ret = 0; + +cleanup: + virObjectUnref(secret); + VIR_FREE(secret_value); + return ret; +} static char * -virStorageBackendISCSIFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, +virStorageBackendISCSIFindPoolSources(virConnectPtr conn, const char *srcSpec, unsigned int flags) { @@ -590,6 +696,9 @@ virStorageBackendISCSIFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, &ntargets, &targets) < 0) goto cleanup; + if (virStorageBackendISCSISetAuth(portal, conn, source) < 0) + goto cleanup; + i think the chap auth iscsi pool won't be able to start with this change, findPoolSources is an api for discovering the pool sources. however, we want the chap auth are set up before connecting to the iscsi target when starting the pool, otherwise the pool starting will fail on authentication. note that startPool and findPoolSources are independant apis, they don't invoke each other.
with this change, if one wants to start the pool successfully, he has to invoke the findPoolSources first, this dependancy is incorrect. as an example, in virsh layer, one has to execute find-storage-pool-sources or find-storage-pool-sources-as before pool-start.
as an alternative way to have the non-null 'conn' for startPool, we can create a connection in storageDriverAutostart (like qemu driver does), which is the only place pass an null 'conn' to startPool, While there is a v3 posted - this code hasn't differed there, so I'll just use this opportunity to ask you questions...
A 'conn' to what? Should we assume qemu like nwfilter_driver.c does?
if (!driverState->privileged) return 0;
i think we don't need to restrict it to be a priviledged connection for storage. otherwise there will be regression.
conn = virConnectOpen("qemu:///system");
Do we further restrict the StartPool code to ensure there is a privileged qemu connection?
yeah, we will want to get a connection object, but as said above, it should work both priviledged or unpriviledged connection, and no need to restrict startPool to ensure there is a connection, since other storage backends may still work without a connection object osier

On 07/15/2013 11:11 AM, Osier Yang wrote:
On 15/07/13 23:01, John Ferlan wrote:
On 07/15/2013 10:16 AM, Osier Yang wrote:
On 10/07/13 03:10, John Ferlan wrote:
From: Osier Yang <jyang@redhat.com> <...snip...>
i think the chap auth iscsi pool won't be able to start with this change, findPoolSources is an api for discovering the pool sources. however, we want the chap auth are set up before connecting to the iscsi target when starting the pool, otherwise the pool starting will fail on authentication. note that startPool and findPoolSources are independant apis, they don't invoke each other.
with this change, if one wants to start the pool successfully, he has to invoke the findPoolSources first, this dependancy is incorrect. as an example, in virsh layer, one has to execute find-storage-pool-sources or find-storage-pool-sources-as before pool-start.
as an alternative way to have the non-null 'conn' for startPool, we can create a connection in storageDriverAutostart (like qemu driver does), which is the only place pass an null 'conn' to startPool, While there is a v3 posted - this code hasn't differed there, so I'll just use this opportunity to ask you questions...
A 'conn' to what? Should we assume qemu like nwfilter_driver.c does?
if (!driverState->privileged) return 0;
i think we don't need to restrict it to be a priviledged connection for storage. otherwise there will be regression.
conn = virConnectOpen("qemu:///system");
Do we further restrict the StartPool code to ensure there is a privileged qemu connection?
yeah, we will want to get a connection object, but as said above, it should work both priviledged or unpriviledged connection, and no need to restrict startPool to ensure there is a connection, since other storage backends may still work without a connection object
osier
Right and that's what I wasn't quite sure how to resolve in the startPool path. Furthermore a connection to what? I guess I have to assume qemu because of the docs which have: <disk type='volume' device='disk'> <driver name='qemu' type='raw'/> <source pool='blk-pool0' volume='blk-pool0-vol0'/> <target dev='hda' bus='ide'/> (BTW: 'volume' is not listed as a valid 'disk type=<value>'...) And it seems I'd need to a privileged field to _virStorageDriverState since storageStateReload() doesn't have the privileged value like storageStateInitialize() has. In storageDriverAutostart() there'd need to be a virConnectOpen to either qemu:///system or qemu:///session. That 'conn' would be passed to the startPool (but could also be passed along to checkPool and refreshPool conceivably. This is (more or less) like what qemuAutostartDomains() does with respect to 'conn' (except it's using cfg->uri). The comment in that code is rather dubious too... I'll redo v3 7/7 and repost those changes. John

On 16/07/13 00:07, John Ferlan wrote:
On 07/15/2013 11:11 AM, Osier Yang wrote:
On 15/07/13 23:01, John Ferlan wrote:
On 07/15/2013 10:16 AM, Osier Yang wrote:
On 10/07/13 03:10, John Ferlan wrote:
From: Osier Yang <jyang@redhat.com> <...snip...>
i think the chap auth iscsi pool won't be able to start with this change, findPoolSources is an api for discovering the pool sources. however, we want the chap auth are set up before connecting to the iscsi target when starting the pool, otherwise the pool starting will fail on authentication. note that startPool and findPoolSources are independant apis, they don't invoke each other.
with this change, if one wants to start the pool successfully, he has to invoke the findPoolSources first, this dependancy is incorrect. as an example, in virsh layer, one has to execute find-storage-pool-sources or find-storage-pool-sources-as before pool-start.
as an alternative way to have the non-null 'conn' for startPool, we can create a connection in storageDriverAutostart (like qemu driver does), which is the only place pass an null 'conn' to startPool, While there is a v3 posted - this code hasn't differed there, so I'll just use this opportunity to ask you questions...
A 'conn' to what? Should we assume qemu like nwfilter_driver.c does?
if (!driverState->privileged) return 0; i think we don't need to restrict it to be a priviledged connection for storage. otherwise there will be regression.
conn = virConnectOpen("qemu:///system");
Do we further restrict the StartPool code to ensure there is a privileged qemu connection?
yeah, we will want to get a connection object, but as said above, it should work both priviledged or unpriviledged connection, and no need to restrict startPool to ensure there is a connection, since other storage backends may still work without a connection object
osier Right and that's what I wasn't quite sure how to resolve in the startPool path. Furthermore a connection to what?
hm, it's a good question. see below
I guess I have to assume qemu because of the docs which have: <disk type='volume' device='disk'> <driver name='qemu' type='raw'/>
we only use "qemu" here because of 'volume' type disk only works for qemu driver, and it's just a document example. it doesn't relate with how to set up the connection uri in storage driver. storage driver should be independant with each hypervisor drivers (e.g lxc/xen/qemu), but the question is how could the storage driver know which uri it should use to get a connection? lxc:///, qemu:///[session|system], or others? directly using qemu uri doesn't work, for instance, libvirtd is running without qemu driver loaded. but i don't have idea either. and also don't see any shortcut to get the connection object. Daniel, any thoughts?
<source pool='blk-pool0' volume='blk-pool0-vol0'/> <target dev='hda' bus='ide'/>
(BTW: 'volume' is not listed as a valid 'disk type=<value>'...)
And it seems I'd need to a privileged field to _virStorageDriverState since storageStateReload() doesn't have the privileged value like storageStateInitialize() has.
In storageDriverAutostart() there'd need to be a virConnectOpen to either qemu:///system or qemu:///session. That 'conn' would be passed to the startPool (but could also be passed along to checkPool and refreshPool conceivably.
This is (more or less) like what qemuAutostartDomains() does with respect to 'conn' (except it's using cfg->uri). The comment in that code is rather dubious too...
I'll redo v3 7/7 and repost those changes.
John

From: Osier Yang <jyang@redhat.com> 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 ba71f42..edec86b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -95,6 +95,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); typedef const char *(*virStorageVolFeatureToString)(int feature); @@ -713,11 +717,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); @@ -1175,7 +1176,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", @@ -1188,7 +1190,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 233340e..601f68b 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -146,7 +146,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

On 10/07/13 03:10, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=957294
Since Osier is out temporarily, I was asked to pick up this work since I initially reviewed the changes.
thanks for picking these work first.
participants (3)
-
Daniel P. Berrange
-
John Ferlan
-
Osier Yang