[libvirt] [PATCH v3 0/7] Support CHAP authentication for iscsi pool
Based on review comments of v2 posting (see 2/7 for specifics): https://www.redhat.com/archives/libvir-list/2013-July/msg00554.html I rewrote the storage_conf.c chap parsing code. I think the first 6 patches could be squashed into 1 for a final submit, but figured I'd post in steps to make the reviews a bit easier on the eyes. Difference to v2: * Remove the 'login' and 'password' from the AuthChap structure * Reordered some of the steps taken in v2, code is essentially the same, but the process to get there a bit different. * The result is the 'ceph' and 'chap' <auth types are for all intents the same except for a few letters ('eph' and 'hap') * Updated Osier's change to storage_backend_iscsi.c in order to remove the 'login'/'passwd' options. Ran 'make' & 'make check' each step along the way. Also tested with valgrind with no new errors. John Ferlan (6): storage_conf: Adjust virStoragePoolAuthType enum storage_conf: Introduce virStoragePoolAuthSecretPtr storage_conf: Move auth processing into virStoragePoolDefParseAuth storage_pool: Rework chap XML to mimic ceph storage_conf: Move username processing into common function storage_conf: Merge AuthChap and AuthCephx into AuthSecret Osier Yang (1): storage: Support "chap" authentication for iscsi pool docs/formatsecret.html.in | 10 +- docs/formatstorage.html.in | 25 +++- docs/schemas/storagepool.rng | 20 +-- src/conf/storage_conf.c | 150 +++++++++++---------- src/conf/storage_conf.h | 21 ++- src/storage/storage_backend_iscsi.c | 107 ++++++++++++++- src/storage/storage_backend_rbd.c | 13 +- tests/storagepoolxml2xmlin/pool-iscsi-auth.xml | 4 +- .../pool-iscsi-vendor-product.xml | 4 +- tests/storagepoolxml2xmlout/pool-iscsi-auth.xml | 4 +- .../pool-iscsi-vendor-product.xml | 4 +- tests/storagepoolxml2xmlout/pool-rbd.xml | 2 +- 12 files changed, 254 insertions(+), 110 deletions(-) -- 1.8.1.4
Generate and use the virStoragePoolAuthTypeType{To|From}String helpers --- src/conf/storage_conf.c | 19 +++++++++++-------- src/conf/storage_conf.h | 3 +++ 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 524a4d6..9bcfced 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); @@ -676,11 +680,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); @@ -1117,13 +1118,15 @@ 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='%s' login='%s' passwd='%s'/>\n", + virStoragePoolAuthTypeTypeToString(src->authType), src->auth.chap.login, src->auth.chap.passwd); if (src->authType == VIR_STORAGE_POOL_AUTH_CEPHX) { - virBufferAsprintf(buf," <auth username='%s' type='ceph'>\n", - src->auth.cephx.username); + virBufferAsprintf(buf," <auth username='%s' type='%s'>\n", + src->auth.cephx.username, + virStoragePoolAuthTypeTypeToString(src->authType)); virBufferAddLit(buf," <secret"); if (src->auth.cephx.secret.uuidUsable) { diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index c183427..98339ef 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 _virStoragePoolAuthChap virStoragePoolAuthChap; typedef virStoragePoolAuthChap *virStoragePoolAuthChapPtr; -- 1.8.1.4
On Mon, Jul 15, 2013 at 09:04:29AM -0400, John Ferlan wrote:
Generate and use the virStoragePoolAuthTypeType{To|From}String helpers --- src/conf/storage_conf.c | 19 +++++++++++-------- src/conf/storage_conf.h | 3 +++ 2 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 524a4d6..9bcfced 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); @@ -676,11 +680,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); @@ -1117,13 +1118,15 @@ 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='%s' login='%s' passwd='%s'/>\n", + virStoragePoolAuthTypeTypeToString(src->authType), src->auth.chap.login, src->auth.chap.passwd);
if (src->authType == VIR_STORAGE_POOL_AUTH_CEPHX) { - virBufferAsprintf(buf," <auth username='%s' type='ceph'>\n", - src->auth.cephx.username); + virBufferAsprintf(buf," <auth username='%s' type='%s'>\n", + src->auth.cephx.username, + virStoragePoolAuthTypeTypeToString(src->authType));
virBufferAddLit(buf," <secret"); if (src->auth.cephx.secret.uuidUsable) { diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index c183427..98339ef 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 _virStoragePoolAuthChap virStoragePoolAuthChap; typedef virStoragePoolAuthChap *virStoragePoolAuthChapPtr;
ACK 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 :|
Split out the _virStoragePoolAuthSecret data from _virStoragePoolAuthCephx into its own structure --- src/conf/storage_conf.h | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 98339ef..5fbecf4 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -151,6 +151,14 @@ enum virStoragePoolAuthType { }; VIR_ENUM_DECL(virStoragePoolAuthType) +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 { @@ -162,11 +170,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
On Mon, Jul 15, 2013 at 09:04:30AM -0400, John Ferlan wrote:
Split out the _virStoragePoolAuthSecret data from _virStoragePoolAuthCephx into its own structure --- src/conf/storage_conf.h | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 98339ef..5fbecf4 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -151,6 +151,14 @@ enum virStoragePoolAuthType { }; VIR_ENUM_DECL(virStoragePoolAuthType)
+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 { @@ -162,11 +170,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; };
ACK 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 :|
Split processing of "<auth" into its own function --- src/conf/storage_conf.c | 65 +++++++++++++++++++++++++++++++------------------ 1 file changed, 41 insertions(+), 24 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 9bcfced..1097de8 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -523,6 +523,45 @@ cleanup: } static int +virStoragePoolDefParseAuth(xmlXPathContextPtr ctxt, + virStoragePoolSourcePtr source) +{ + int ret = -1; + char *authType = NULL; + + authType = virXPathString("string(./auth/@type)", ctxt); + if (authType == NULL) { + source->authType = VIR_STORAGE_POOL_AUTH_NONE; + ret = 0; + goto cleanup; + } + + if ((source->authType = + virStoragePoolAuthTypeTypeFromString(authType)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown auth type '%s'"), + authType); + goto cleanup; + } + + if (source->authType == VIR_STORAGE_POOL_AUTH_CHAP) { + if (virStoragePoolDefParseAuthChap(ctxt, &source->auth.chap) < 0) + goto cleanup; + } + + if (source->authType == VIR_STORAGE_POOL_AUTH_CEPHX) { + if (virStoragePoolDefParseAuthCephx(ctxt, &source->auth.cephx) < 0) + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(authType); + return ret; +} + +static int virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, virStoragePoolSourcePtr source, int pool_type, @@ -530,7 +569,6 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, { int ret = -1; xmlNodePtr relnode, *nodeset = NULL; - char *authType = NULL; int nsource; size_t i; virStoragePoolOptionsPtr options; @@ -676,28 +714,8 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST; } - authType = virXPathString("string(./auth/@type)", ctxt); - if (authType == NULL) { - source->authType = VIR_STORAGE_POOL_AUTH_NONE; - } else { - if ((source->authType = - virStoragePoolAuthTypeTypeFromString(authType)) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("unknown auth type '%s'"), - authType); - goto cleanup; - } - } - - if (source->authType == VIR_STORAGE_POOL_AUTH_CHAP) { - if (virStoragePoolDefParseAuthChap(ctxt, &source->auth.chap) < 0) - goto cleanup; - } - - if (source->authType == VIR_STORAGE_POOL_AUTH_CEPHX) { - if (virStoragePoolDefParseAuthCephx(ctxt, &source->auth.cephx) < 0) - goto cleanup; - } + if (virStoragePoolDefParseAuth(ctxt, source) < 0) + goto cleanup; source->vendor = virXPathString("string(./vendor/@name)", ctxt); source->product = virXPathString("string(./product/@name)", ctxt); @@ -707,7 +725,6 @@ cleanup: ctxt->node = relnode; VIR_FREE(port); - VIR_FREE(authType); VIR_FREE(nodeset); VIR_FREE(adapter_type); return ret; -- 1.8.1.4
On Mon, Jul 15, 2013 at 09:04:31AM -0400, John Ferlan wrote:
Split processing of "<auth" into its own function --- src/conf/storage_conf.c | 65 +++++++++++++++++++++++++++++++------------------ 1 file changed, 41 insertions(+), 24 deletions(-)
ACK 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 :|
The existing 'chap' XML logic was never used - just defined. Rather than try to insert a square peg into a round hole, blow it up and rewrite the logic to follow the 'ceph' format. Remove the former "chap.login" and "chap.passwd" fields and replace with "chap.username" and "chap.secret" in _virStoragePoolAuthChap. Adjust the virStoragePoolDefParseAuthChap() to process. Change the rng file to describe the new layout 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 Update tests to handle the changes from 'login' and 'passwd' to 'username' and '<secret>' format --- docs/formatsecret.html.in | 10 ++-- docs/formatstorage.html.in | 25 +++++++++- docs/schemas/storagepool.rng | 20 ++------ src/conf/storage_conf.c | 56 +++++++++++++++------- src/conf/storage_conf.h | 4 +- tests/storagepoolxml2xmlin/pool-iscsi-auth.xml | 4 +- .../pool-iscsi-vendor-product.xml | 4 +- tests/storagepoolxml2xmlout/pool-iscsi-auth.xml | 4 +- .../pool-iscsi-vendor-product.xml | 4 +- tests/storagepoolxml2xmlout/pool-rbd.xml | 2 +- 10 files changed, 87 insertions(+), 46 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..f4d561f 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,27 @@ 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". Additionally a mandatory attribute + <code>username</code> 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 manages 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 for "ceph" and + 1.1.1 for "chap"</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/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index 3c2158a..6da3c11 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -286,22 +286,10 @@ <value>ceph</value> </choice> </attribute> - <choice> - <attribute name='login'> - <text/> - </attribute> - <attribute name='username'> - <text/> - </attribute> - </choice> - <optional> - <attribute name='passwd'> - <text/> - </attribute> - </optional> - <optional> - <ref name='sourceinfoauthsecret'/> - </optional> + <attribute name='username'> + <text/> + </attribute> + <ref name='sourceinfoauthsecret'/> </element> </define> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 1097de8..404545a 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -365,8 +365,8 @@ virStoragePoolSourceClear(virStoragePoolSourcePtr source) VIR_FREE(source->product); if (source->authType == VIR_STORAGE_POOL_AUTH_CHAP) { - VIR_FREE(source->auth.chap.login); - VIR_FREE(source->auth.chap.passwd); + VIR_FREE(source->auth.chap.username); + VIR_FREE(source->auth.chap.secret.usage); } if (source->authType == VIR_STORAGE_POOL_AUTH_CEPHX) { @@ -461,21 +461,44 @@ static int virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt, virStoragePoolAuthChapPtr auth) { - auth->login = virXPathString("string(./auth/@login)", ctxt); - if (auth->login == NULL) { + char *uuid = NULL; + int ret = -1; + + auth->username = virXPathString("string(./auth/@username)", ctxt); + if (auth->username == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing auth login attribute")); + _("missing auth username attribute")); return -1; } - auth->passwd = virXPathString("string(./auth/@passwd)", ctxt); - if (auth->passwd == NULL) { + 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 passwd attribute")); + _("missing auth secret uuid or usage attribute")); return -1; } - return 0; + 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; } static int @@ -1134,16 +1157,13 @@ virStoragePoolSourceFormat(virBufferPtr buf, virBufferAsprintf(buf," <format type='%s'/>\n", format); } - if (src->authType == VIR_STORAGE_POOL_AUTH_CHAP) - virBufferAsprintf(buf," <auth type='%s' login='%s' passwd='%s'/>\n", + if (src->authType == VIR_STORAGE_POOL_AUTH_CHAP || + src->authType == VIR_STORAGE_POOL_AUTH_CEPHX) { + virBufferAsprintf(buf," <auth type='%s' username='%s'>\n", virStoragePoolAuthTypeTypeToString(src->authType), - src->auth.chap.login, - src->auth.chap.passwd); - - if (src->authType == VIR_STORAGE_POOL_AUTH_CEPHX) { - virBufferAsprintf(buf," <auth username='%s' type='%s'>\n", - src->auth.cephx.username, - virStoragePoolAuthTypeTypeToString(src->authType)); + (src->authType == VIR_STORAGE_POOL_AUTH_CHAP ? + src->auth.chap.username : + src->auth.cephx.username)); virBufferAddLit(buf," <secret"); if (src->auth.cephx.secret.uuidUsable) { diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 5fbecf4..fd9b2e7 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -162,8 +162,8 @@ struct _virStoragePoolAuthSecret { typedef struct _virStoragePoolAuthChap virStoragePoolAuthChap; typedef virStoragePoolAuthChap *virStoragePoolAuthChapPtr; struct _virStoragePoolAuthChap { - char *login; - char *passwd; + char *username; + virStoragePoolAuthSecret secret; }; typedef struct _virStoragePoolAuthCephx virStoragePoolAuthCephx; diff --git a/tests/storagepoolxml2xmlin/pool-iscsi-auth.xml b/tests/storagepoolxml2xmlin/pool-iscsi-auth.xml index f7d4d52..c81eb60 100644 --- a/tests/storagepoolxml2xmlin/pool-iscsi-auth.xml +++ b/tests/storagepoolxml2xmlin/pool-iscsi-auth.xml @@ -4,7 +4,9 @@ <source> <host name="iscsi.example.com"/> <device path="demo-target"/> - <auth type='chap' login='foobar' passwd='frobbar'/> + <auth type='chap' username='admin'> + <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/> + </auth> </source> <target> <path>/dev/disk/by-path</path> diff --git a/tests/storagepoolxml2xmlin/pool-iscsi-vendor-product.xml b/tests/storagepoolxml2xmlin/pool-iscsi-vendor-product.xml index 01fbd9b..821feb1 100644 --- a/tests/storagepoolxml2xmlin/pool-iscsi-vendor-product.xml +++ b/tests/storagepoolxml2xmlin/pool-iscsi-vendor-product.xml @@ -4,7 +4,9 @@ <source> <host name="iscsi.example.com"/> <device path="demo-target"/> - <auth type='chap' login='foobar' passwd='frobbar'/> + <auth type='chap' username='admin'> + <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/> + </auth> <vendor name='test-vendor'/> <product name='test-product'/> </source> diff --git a/tests/storagepoolxml2xmlout/pool-iscsi-auth.xml b/tests/storagepoolxml2xmlout/pool-iscsi-auth.xml index 4fa8f64..3d84c1c 100644 --- a/tests/storagepoolxml2xmlout/pool-iscsi-auth.xml +++ b/tests/storagepoolxml2xmlout/pool-iscsi-auth.xml @@ -7,7 +7,9 @@ <source> <host name='iscsi.example.com'/> <device path='demo-target'/> - <auth type='chap' login='foobar' passwd='frobbar'/> + <auth type='chap' username='admin'> + <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/> + </auth> </source> <target> <path>/dev/disk/by-path</path> diff --git a/tests/storagepoolxml2xmlout/pool-iscsi-vendor-product.xml b/tests/storagepoolxml2xmlout/pool-iscsi-vendor-product.xml index 6ae1c39..4fb19bb 100644 --- a/tests/storagepoolxml2xmlout/pool-iscsi-vendor-product.xml +++ b/tests/storagepoolxml2xmlout/pool-iscsi-vendor-product.xml @@ -7,7 +7,9 @@ <source> <host name='iscsi.example.com'/> <device path='demo-target'/> - <auth type='chap' login='foobar' passwd='frobbar'/> + <auth type='chap' username='admin'> + <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/> + </auth> <vendor name='test-vendor'/> <product name='test-product'/> </source> 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
On Mon, Jul 15, 2013 at 09:04:32AM -0400, John Ferlan wrote:
The existing 'chap' XML logic was never used - just defined. Rather than try to insert a square peg into a round hole, blow it up and rewrite the logic to follow the 'ceph' format.
Remove the former "chap.login" and "chap.passwd" fields and replace with "chap.username" and "chap.secret" in _virStoragePoolAuthChap. Adjust the virStoragePoolDefParseAuthChap() to process.
Change the rng file to describe the new layout
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
Update tests to handle the changes from 'login' and 'passwd' to 'username' and '<secret>' format --- docs/formatsecret.html.in | 10 ++-- docs/formatstorage.html.in | 25 +++++++++- docs/schemas/storagepool.rng | 20 ++------ src/conf/storage_conf.c | 56 +++++++++++++++------- src/conf/storage_conf.h | 4 +- tests/storagepoolxml2xmlin/pool-iscsi-auth.xml | 4 +- .../pool-iscsi-vendor-product.xml | 4 +- tests/storagepoolxml2xmlout/pool-iscsi-auth.xml | 4 +- .../pool-iscsi-vendor-product.xml | 4 +- tests/storagepoolxml2xmlout/pool-rbd.xml | 2 +- 10 files changed, 87 insertions(+), 46 deletions(-)
ACK 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 :|
Move the auth->username processing into virStoragePoolDefParseAuth save the resulting username into chap/cephx specific data --- src/conf/storage_conf.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 404545a..c89a5b4 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -464,13 +464,6 @@ virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt, char *uuid = NULL; int ret = -1; - auth->username = virXPathString("string(./auth/@username)", ctxt); - if (auth->username == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing auth username attribute")); - 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) { @@ -508,13 +501,6 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt, char *uuid = NULL; int ret = -1; - auth->username = virXPathString("string(./auth/@username)", ctxt); - if (auth->username == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing auth username attribute")); - 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) { @@ -551,6 +537,7 @@ virStoragePoolDefParseAuth(xmlXPathContextPtr ctxt, { int ret = -1; char *authType = NULL; + char *username = NULL; authType = virXPathString("string(./auth/@type)", ctxt); if (authType == NULL) { @@ -567,12 +554,22 @@ virStoragePoolDefParseAuth(xmlXPathContextPtr ctxt, goto cleanup; } + username = virXPathString("string(./auth/@username)", ctxt); + if (username == NULL) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing auth username attribute")); + goto cleanup; + } + if (source->authType == VIR_STORAGE_POOL_AUTH_CHAP) { + source->auth.chap.username = username; + username = NULL; if (virStoragePoolDefParseAuthChap(ctxt, &source->auth.chap) < 0) goto cleanup; } - - if (source->authType == VIR_STORAGE_POOL_AUTH_CEPHX) { + else if (source->authType == VIR_STORAGE_POOL_AUTH_CEPHX) { + source->auth.cephx.username = username; + username = NULL; if (virStoragePoolDefParseAuthCephx(ctxt, &source->auth.cephx) < 0) goto cleanup; } @@ -581,6 +578,7 @@ virStoragePoolDefParseAuth(xmlXPathContextPtr ctxt, cleanup: VIR_FREE(authType); + VIR_FREE(username); return ret; } -- 1.8.1.4
On Mon, Jul 15, 2013 at 09:04:33AM -0400, John Ferlan wrote:
Move the auth->username processing into virStoragePoolDefParseAuth save the resulting username into chap/cephx specific data --- src/conf/storage_conf.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-)
ACK 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 :|
Merge virStoragePoolDefParseAuthChap and virStoragePoolDefParseAuthCephx into a common virStoragePoolDefParseAuthSecret. Change the output to be common for both by putting 'type' first followed by 'username'. Need to adjust a test output for that too. --- src/conf/storage_conf.c | 60 ++++++++++--------------------------------------- 1 file changed, 12 insertions(+), 48 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index c89a5b4..d6df1ed 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -456,73 +456,35 @@ virStoragePoolObjRemove(virStoragePoolObjListPtr pools, } } - -static int -virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt, - virStoragePoolAuthChapPtr auth) -{ - char *uuid = NULL; - int ret = -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")); - 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; -} - static int -virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt, - virStoragePoolAuthCephxPtr auth) +virStoragePoolDefParseAuthSecret(xmlXPathContextPtr ctxt, + virStoragePoolAuthSecretPtr secret) { char *uuid = NULL; int ret = -1; uuid = virXPathString("string(./auth/secret/@uuid)", ctxt); - auth->secret.usage = virXPathString("string(./auth/secret/@usage)", ctxt); - if (uuid == NULL && auth->secret.usage == NULL) { + 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 (auth->secret.usage != NULL) { + if (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) { + if (virUUIDParse(uuid, secret->uuid) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("invalid auth secret uuid")); goto cleanup; } - auth->secret.uuidUsable = true; + secret->uuidUsable = true; } else { - auth->secret.uuidUsable = false; + secret->uuidUsable = false; } ret = 0; @@ -564,13 +526,15 @@ virStoragePoolDefParseAuth(xmlXPathContextPtr ctxt, if (source->authType == VIR_STORAGE_POOL_AUTH_CHAP) { source->auth.chap.username = username; username = NULL; - if (virStoragePoolDefParseAuthChap(ctxt, &source->auth.chap) < 0) + if (virStoragePoolDefParseAuthSecret(ctxt, + &source->auth.chap.secret) < 0) goto cleanup; } else if (source->authType == VIR_STORAGE_POOL_AUTH_CEPHX) { source->auth.cephx.username = username; username = NULL; - if (virStoragePoolDefParseAuthCephx(ctxt, &source->auth.cephx) < 0) + if (virStoragePoolDefParseAuthSecret(ctxt, + &source->auth.cephx.secret) < 0) goto cleanup; } -- 1.8.1.4
On Mon, Jul 15, 2013 at 09:04:34AM -0400, John Ferlan wrote:
Merge virStoragePoolDefParseAuthChap and virStoragePoolDefParseAuthCephx into a common virStoragePoolDefParseAuthSecret. Change the output to be common for both by putting 'type' first followed by 'username'.
Need to adjust a test output for that too.
Didn't seem like you had to - unles the test change was mistakenly added to a different commit in this series ?
--- src/conf/storage_conf.c | 60 ++++++++++--------------------------------------- 1 file changed, 12 insertions(+), 48 deletions(-)
ACK 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/15/2013 11:00 AM, Daniel P. Berrange wrote:
On Mon, Jul 15, 2013 at 09:04:34AM -0400, John Ferlan wrote:
Merge virStoragePoolDefParseAuthChap and virStoragePoolDefParseAuthCephx into a common virStoragePoolDefParseAuthSecret. Change the output to be common for both by putting 'type' first followed by 'username'.
Need to adjust a test output for that too.
Didn't seem like you had to - unles the test change was mistakenly added to a different commit in this series ?
d'oh had to move that earlier and forgot to adjust the text of this commit. John
--- src/conf/storage_conf.c | 60 ++++++++++--------------------------------------- 1 file changed, 12 insertions(+), 48 deletions(-)
ACK
Daniel
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. --- src/storage/storage_backend_iscsi.c | 107 +++++++++++++++++++++++++++++++++++- src/storage/storage_backend_rbd.c | 13 ++++- 2 files changed, 117 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index ba4f388..c0f6032 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" @@ -538,9 +541,106 @@ 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; + 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.secret.uuidUsable) + secret = virSecretLookupByUUID(conn, chap.secret.uuid); + else + secret = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_ISCSI, + chap.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.username); + goto cleanup; + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("username '%s' specified but secret not found"), + chap.username); + goto cleanup; + } + + if (virStorageBackendISCSINodeUpdate(portal, + source->devices[0].path, + "node.session.auth.authmethod", + "CHAP") < 0 || + virStorageBackendISCSINodeUpdate(portal, + source->devices[0].path, + "node.session.auth.username", + chap.username) < 0 || + virStorageBackendISCSINodeUpdate(portal, + source->devices[0].path, + "node.session.auth.password", + (const char *)secret_value) < 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) { @@ -583,6 +683,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) goto cleanup; @@ -655,7 +758,6 @@ virStorageBackendISCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, return ret; } - static int virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) @@ -687,6 +789,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 493e33b..bd96ddd 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 15/07/13 21:04, 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. --- src/storage/storage_backend_iscsi.c | 107 +++++++++++++++++++++++++++++++++++- src/storage/storage_backend_rbd.c | 13 ++++- 2 files changed, 117 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index ba4f388..c0f6032 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"
@@ -538,9 +541,106 @@ 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; + 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.secret.uuidUsable) + secret = virSecretLookupByUUID(conn, chap.secret.uuid); + else + secret = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_ISCSI, + chap.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.username); + goto cleanup; + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("username '%s' specified but secret not found"), + chap.username); + goto cleanup; + } + + if (virStorageBackendISCSINodeUpdate(portal, + source->devices[0].path, + "node.session.auth.authmethod", + "CHAP") < 0 || + virStorageBackendISCSINodeUpdate(portal, + source->devices[0].path, + "node.session.auth.username", + chap.username) < 0 || + virStorageBackendISCSINodeUpdate(portal, + source->devices[0].path, + "node.session.auth.password", + (const char *)secret_value) < 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) { @@ -583,6 +683,9 @@ virStorageBackendISCSIFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, &ntargets, &targets) < 0) goto cleanup;
+ if (virStorageBackendISCSISetAuth(portal, conn, source) < 0) + goto cleanup; +
is it a mistake or? since i just submitted the comments on v2, which says this will lead the pool starting to failure. :\ or there are differences with v2 in some other places? osier
On 15/07/13 22:47, Osier Yang wrote:
On 15/07/13 21:04, 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. --- src/storage/storage_backend_iscsi.c | 107 +++++++++++++++++++++++++++++++++++- src/storage/storage_backend_rbd.c | 13 ++++- 2 files changed, 117 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index ba4f388..c0f6032 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" @@ -538,9 +541,106 @@ 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; + 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.secret.uuidUsable) + secret = virSecretLookupByUUID(conn, chap.secret.uuid); + else + secret = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_ISCSI, + chap.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.username); + goto cleanup; + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("username '%s' specified but secret not found"), + chap.username); + goto cleanup; + } + + if (virStorageBackendISCSINodeUpdate(portal, + source->devices[0].path, + "node.session.auth.authmethod", + "CHAP") < 0 || + virStorageBackendISCSINodeUpdate(portal, + source->devices[0].path, + "node.session.auth.username", + chap.username) < 0 || + virStorageBackendISCSINodeUpdate(portal, + source->devices[0].path, + "node.session.auth.password", + (const char *)secret_value) < 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) { @@ -583,6 +683,9 @@ virStorageBackendISCSIFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, &ntargets, &targets) < 0) goto cleanup; + if (virStorageBackendISCSISetAuth(portal, conn, source) < 0) + goto cleanup; +
is it a mistake or? since i just submitted the comments on v2, which says this will lead the pool starting to failure. :\ or there are differences with v2 in some other places?
oh, i see, the delay is too much, you posted the v3 set before my comments on v2, however, it just showed up on my mailbox.
On Mon, Jul 15, 2013 at 09:04:35AM -0400, 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. --- src/storage/storage_backend_iscsi.c | 107 +++++++++++++++++++++++++++++++++++- src/storage/storage_backend_rbd.c | 13 ++++- 2 files changed, 117 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index ba4f388..c0f6032 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"
@@ -538,9 +541,106 @@ 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; + 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.secret.uuidUsable) + secret = virSecretLookupByUUID(conn, chap.secret.uuid); + else + secret = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_ISCSI, + chap.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.username); + goto cleanup; + } + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("username '%s' specified but secret not found"), + chap.username); + goto cleanup; + } + + if (virStorageBackendISCSINodeUpdate(portal, + source->devices[0].path, + "node.session.auth.authmethod", + "CHAP") < 0 || + virStorageBackendISCSINodeUpdate(portal, + source->devices[0].path, + "node.session.auth.username", + chap.username) < 0 || + virStorageBackendISCSINodeUpdate(portal, + source->devices[0].path, + "node.session.auth.password", + (const char *)secret_value) < 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) { @@ -583,6 +683,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) goto cleanup;
As Osier mentioned, I think you need to be calling this somewhere in the virStorageBackendISCSIStartPool method or its call path. 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/15/2013 09:04 AM, John Ferlan wrote:
Based on review comments of v2 posting (see 2/7 for specifics):
https://www.redhat.com/archives/libvir-list/2013-July/msg00554.html
I rewrote the storage_conf.c chap parsing code. I think the first 6 patches could be squashed into 1 for a final submit, but figured I'd post in steps to make the reviews a bit easier on the eyes.
Difference to v2: * Remove the 'login' and 'password' from the AuthChap structure * Reordered some of the steps taken in v2, code is essentially the same, but the process to get there a bit different. * The result is the 'ceph' and 'chap' <auth types are for all intents the same except for a few letters ('eph' and 'hap') * Updated Osier's change to storage_backend_iscsi.c in order to remove the 'login'/'passwd' options.
Ran 'make' & 'make check' each step along the way. Also tested with valgrind with no new errors.
John Ferlan (6): storage_conf: Adjust virStoragePoolAuthType enum storage_conf: Introduce virStoragePoolAuthSecretPtr storage_conf: Move auth processing into virStoragePoolDefParseAuth storage_pool: Rework chap XML to mimic ceph storage_conf: Move username processing into common function storage_conf: Merge AuthChap and AuthCephx into AuthSecret
Osier Yang (1): storage: Support "chap" authentication for iscsi pool
docs/formatsecret.html.in | 10 +- docs/formatstorage.html.in | 25 +++- docs/schemas/storagepool.rng | 20 +-- src/conf/storage_conf.c | 150 +++++++++++---------- src/conf/storage_conf.h | 21 ++- src/storage/storage_backend_iscsi.c | 107 ++++++++++++++- src/storage/storage_backend_rbd.c | 13 +- tests/storagepoolxml2xmlin/pool-iscsi-auth.xml | 4 +- .../pool-iscsi-vendor-product.xml | 4 +- tests/storagepoolxml2xmlout/pool-iscsi-auth.xml | 4 +- .../pool-iscsi-vendor-product.xml | 4 +- tests/storagepoolxml2xmlout/pool-rbd.xml | 2 +- 12 files changed, 254 insertions(+), 110 deletions(-)
I've pushed patches 1->6 and will rework patch 7 and post as a v4. Thanks for the reviews, John
participants (3)
-
Daniel P. Berrange -
John Ferlan -
Osier Yang