[libvirt] [PATCH v4 0/3] Support CHAP authentication for iscsi pool
From what I see in just reading the code, this path would have sent a NULL 'conn' to the virStorageBackendRBDOpenRADOSConn() via the call from volStorageBackendRBDRefreshVolInfo() (eg, refreshPool()) and would have failed with a "failed to find the secret"). Whether this was a latent issue or not - I'm not quite sure. This code follows Osier's original change to be sure to call he secret driver 'secretGetValue' directly rather than
https://bugzilla.redhat.com/show_bug.cgi?id=957294 Based on reviews of the the v2/v3 changes to the storage backend drivers regarding the call to perform the authentication, see (and followups): https://www.redhat.com/archives/libvir-list/2013-July/msg00910.html I moved the authentication of the pool back into the startPool callback instread of the findPoolSources() as was done for the original patches, see: https://www.redhat.com/archives/libvir-list/2013-May/msg01887.html https://www.redhat.com/archives/libvir-list/2013-May/msg01886.html Although the plain text option was removed. In order to achieve the goal of getting the secret, the startPool path needed a connection to a driver, so like the nwfilter_driver I chose a qemu connection. Deciding whether to use a privileged connection or not was made based on the privileged value set during storage driver state initialization. Maintaining that state allows for the decision further in the storageDriverAutostart() code to make the connection. Adjusted the existing rbd authentication to take advantage of this as well. through the virSecretGetValue() API. Using the same connection paradigm for the chap/iscsi authentication path in order to access the secret driver 'secretGetValue' directly rather than through the virSecretGetValue() API. John Ferlan (3): Add a privileged field to storageDriverState Adjust 'ceph' authentication secret usage for rbd pool. storage: Support "chap" authentication for iscsi pool src/conf/storage_conf.h | 1 + src/storage/storage_backend_iscsi.c | 111 +++++++++++++++++++++++++++++++++++- src/storage/storage_backend_rbd.c | 21 ++++++- src/storage/storage_driver.c | 19 ++++-- 4 files changed, 145 insertions(+), 7 deletions(-) -- 1.8.1.4
Use the privileged value in order to generate a connection which could be passed to the various storage backend drivers. In particular, the iSCSI driver will need a connect in order to perform pool authentication using the 'chap' secrets. Additionally, the RBD backend utilizes the connection during pool refresh for pools using 'ceph' secrets. --- src/conf/storage_conf.h | 1 + src/storage/storage_driver.c | 19 +++++++++++++++---- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index fd9b2e7..62ff1fd 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -354,6 +354,7 @@ struct _virStorageDriverState { char *configDir; char *autostartDir; + bool privileged; }; typedef struct _virStoragePoolSourceList virStoragePoolSourceList; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index a8eb731..f38acef 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -68,6 +68,13 @@ static void storageDriverUnlock(virStorageDriverStatePtr driver) static void storageDriverAutostart(virStorageDriverStatePtr driver) { size_t i; + virConnectPtr conn = NULL; + + if (driverState->privileged) + conn = virConnectOpen("qemu:///system"); + else + conn = virConnectOpen("qemu:///session"); + /* Ignoring NULL conn - let backends decide */ for (i = 0; i < driver->pools.count; i++) { virStoragePoolObjPtr pool = driver->pools.objs[i]; @@ -82,7 +89,7 @@ storageDriverAutostart(virStorageDriverStatePtr driver) { } if (backend->checkPool && - backend->checkPool(NULL, pool, &started) < 0) { + backend->checkPool(conn, pool, &started) < 0) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to initialize storage pool '%s': %s"), pool->def->name, err ? err->message : @@ -95,7 +102,7 @@ storageDriverAutostart(virStorageDriverStatePtr driver) { pool->autostart && !virStoragePoolObjIsActive(pool)) { if (backend->startPool && - backend->startPool(NULL, pool) < 0) { + backend->startPool(conn, pool) < 0) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to autostart storage pool '%s': %s"), pool->def->name, err ? err->message : @@ -107,10 +114,10 @@ storageDriverAutostart(virStorageDriverStatePtr driver) { } if (started) { - if (backend->refreshPool(NULL, pool) < 0) { + if (backend->refreshPool(conn, pool) < 0) { virErrorPtr err = virGetLastError(); if (backend->stopPool) - backend->stopPool(NULL, pool); + backend->stopPool(conn, pool); VIR_ERROR(_("Failed to autostart storage pool '%s': %s"), pool->def->name, err ? err->message : _("no error message found")); @@ -121,6 +128,9 @@ storageDriverAutostart(virStorageDriverStatePtr driver) { } virStoragePoolObjUnlock(pool); } + + if (conn) + virConnectClose(conn); } /** @@ -152,6 +162,7 @@ storageStateInitialize(bool privileged, if (!base) goto error; } + driverState->privileged = privileged; /* Configuration paths are either $USER_CONFIG_HOME/libvirt/storage/... (session) or * /etc/libvirt/storage/... (system). -- 1.8.1.4
On Mon, Jul 15, 2013 at 04:26:10PM -0400, John Ferlan wrote:
Use the privileged value in order to generate a connection which could be passed to the various storage backend drivers.
In particular, the iSCSI driver will need a connect in order to perform pool authentication using the 'chap' secrets. Additionally, the RBD backend utilizes the connection during pool refresh for pools using 'ceph' secrets. --- src/conf/storage_conf.h | 1 + src/storage/storage_driver.c | 19 +++++++++++++++---- 2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index fd9b2e7..62ff1fd 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -354,6 +354,7 @@ struct _virStorageDriverState {
char *configDir; char *autostartDir; + bool privileged; };
typedef struct _virStoragePoolSourceList virStoragePoolSourceList; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index a8eb731..f38acef 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -68,6 +68,13 @@ static void storageDriverUnlock(virStorageDriverStatePtr driver) static void storageDriverAutostart(virStorageDriverStatePtr driver) { size_t i; + virConnectPtr conn = NULL; + + if (driverState->privileged) + conn = virConnectOpen("qemu:///system"); + else + conn = virConnectOpen("qemu:///session"); + /* Ignoring NULL conn - let backends decide */
Nope, this doesn't fly. The storage driver is shared across many other hypervisor drivers, and we can't assume that the QEMU driver is compiled into libvirt. IIUC, the reason we need the connection is to access the secrets driver. We should probably just make the storage driver directly call into the secrets driver, instead of going via a virConnectPtr object. 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 Tue, Jul 16, 2013 at 09:44:52AM +0100, Daniel P. Berrange wrote:
On Mon, Jul 15, 2013 at 04:26:10PM -0400, John Ferlan wrote:
Use the privileged value in order to generate a connection which could be passed to the various storage backend drivers.
In particular, the iSCSI driver will need a connect in order to perform pool authentication using the 'chap' secrets. Additionally, the RBD backend utilizes the connection during pool refresh for pools using 'ceph' secrets. --- src/conf/storage_conf.h | 1 + src/storage/storage_driver.c | 19 +++++++++++++++---- 2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index fd9b2e7..62ff1fd 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -354,6 +354,7 @@ struct _virStorageDriverState {
char *configDir; char *autostartDir; + bool privileged; };
typedef struct _virStoragePoolSourceList virStoragePoolSourceList; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index a8eb731..f38acef 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -68,6 +68,13 @@ static void storageDriverUnlock(virStorageDriverStatePtr driver) static void storageDriverAutostart(virStorageDriverStatePtr driver) { size_t i; + virConnectPtr conn = NULL; + + if (driverState->privileged) + conn = virConnectOpen("qemu:///system"); + else + conn = virConnectOpen("qemu:///session"); + /* Ignoring NULL conn - let backends decide */
Nope, this doesn't fly. The storage driver is shared across many other hypervisor drivers, and we can't assume that the QEMU driver is compiled into libvirt.
IIUC, the reason we need the connection is to access the secrets driver. We should probably just make the storage driver directly call into the secrets driver, instead of going via a virConnectPtr object.
Hmm, that is actually harder than I thought. I think perhaps we need a virConnectOpenDummy() which lets us open a connection to the secret's driver, without opening a hypervisor. 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/16/2013 05:31 AM, Daniel P. Berrange wrote:
On Tue, Jul 16, 2013 at 09:44:52AM +0100, Daniel P. Berrange wrote:
On Mon, Jul 15, 2013 at 04:26:10PM -0400, John Ferlan wrote:
Use the privileged value in order to generate a connection which could be passed to the various storage backend drivers.
In particular, the iSCSI driver will need a connect in order to perform pool authentication using the 'chap' secrets. Additionally, the RBD backend utilizes the connection during pool refresh for pools using 'ceph' secrets. --- src/conf/storage_conf.h | 1 + src/storage/storage_driver.c | 19 +++++++++++++++---- 2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index fd9b2e7..62ff1fd 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -354,6 +354,7 @@ struct _virStorageDriverState {
char *configDir; char *autostartDir; + bool privileged; };
typedef struct _virStoragePoolSourceList virStoragePoolSourceList; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index a8eb731..f38acef 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -68,6 +68,13 @@ static void storageDriverUnlock(virStorageDriverStatePtr driver) static void storageDriverAutostart(virStorageDriverStatePtr driver) { size_t i; + virConnectPtr conn = NULL; + + if (driverState->privileged) + conn = virConnectOpen("qemu:///system"); + else + conn = virConnectOpen("qemu:///session"); + /* Ignoring NULL conn - let backends decide */
Nope, this doesn't fly. The storage driver is shared across many other hypervisor drivers, and we can't assume that the QEMU driver is compiled into libvirt.
IIUC, the reason we need the connection is to access the secrets driver. We should probably just make the storage driver directly call into the secrets driver, instead of going via a virConnectPtr object.
Correct - we need access to the secrets driver via a virConnectPtr and as I pointed out in my response yesterday the reason why I chose the findPoolSources() as that vehicle. It was going to be tricky and controversial to do so in startPool() - first because "choosing" which driver then ties the storage pool to that driver and second because of the system vs. session connection. In using findPoolSources(), I figured the theory would be that disks within the pool would use the pool's authentication rather than having their own defined. I think that had to do with my reading of the <auth> description in the domain disk definition, where disk defs can have the <auth> attribute. In the pool case, I figured one wouldn't have to put an <auth> on a disk element, rather the disk could be in a pool that had the authentication. When first "found" is when the auth would take place rather than when the pool started. In the long run while Osier points out in his response that creating a dependency on findPoolSources being run before startPool is not desired, see: https://www.redhat.com/archives/libvir-list/2013-July/msg00910.html I wasn't viewing things that way. In order for a source disk to be used in the pool, how does an application discover it? I was assuming via the virsh mechanisms or through virConnectFindStoragePoolSources(). Does it matter that the pool is authenticated or that the volumes using the pool use the pool authentication? A pool could be authenticated "early" and never need it again. Elements in and added to or removed from the pool after that authentication wouldn't be authenticated. Is that the design goal? I don't have the answer in my head since I've jumped into the middle of this...
Hmm, that is actually harder than I thought.
I think perhaps we need a virConnectOpenDummy() which lets us open a connection to the secret's driver, without opening a hypervisor.
Oh joy - what did I get myself into :-) A secret backdoor, you hear a gurgling brook to your east... xyzzy.. poof you found what you're looking for. John
Update virStorageBackendRBDOpenRADOSConn() to use the internal API to the secret driver in order to get the secret value instead of the external virSecretGetValue() path. Without the flag VIR_SECRET_GET_VALUE_INTERNAL_CALL there is no way to get the value of private secret. This also requires ensuring there is a connection which wasn't true for for the refreshPool() path calls from storageDriverAutostart() prior to adding support for the connection to a qemu driver. It seems calls to virSecretLookupByUUIDString() and virSecretLookupByUsage() from the refreshPool() path would have failed with no way to find the secret - that is theoretically speaking since the 'conn' was NULL the failure would have been "failed to find the secret". --- src/storage/storage_backend_rbd.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 493e33b..8dc253f 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" @@ -71,6 +72,12 @@ static int virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr *ptr, goto cleanup; } + if (!conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("'ceph' authentication requires connection")); + return -1; + } + if (pool->def->source.auth.cephx.secret.uuidUsable) { virUUIDFormat(pool->def->source.auth.cephx.secret.uuid, secretUuid); VIR_DEBUG("Looking up secret by UUID: %s", secretUuid); @@ -88,7 +95,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); @@ -254,7 +271,7 @@ cleanup: return ret; } -static int virStorageBackendRBDRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, +static int virStorageBackendRBDRefreshPool(virConnectPtr conn, virStoragePoolObjPtr pool) { size_t max_size = 1024; -- 1.8.1.4
Although the XML for CHAP authentication with plain "password" was introduced long ago, the function was never implemented. This patch replaces the login/password mechanism by following the 'ceph' (or RBD) model of using a 'username' with a 'secret' which has the authentication information. This patch performs the authentication during startPool() processing of pools with an authType of VIR_STORAGE_POOL_AUTH_CHAP specified for iSCSI pools. There are two types of CHAP configurations supported for iSCSI authentication: * Initiator Authentication Forward, one-way; The initiator is authenticated by the target. * Target Authentication Reverse, Bi-directional, mutual, two-way; The target is authenticated by the initiator; This method also requires Initiator Authentication This only supports the "Initiator Authentication". (I don't have any enterprise iSCSI env for testing, only have a iSCSI target setup with tgtd, which doesn't support "Target Authentication"). "Discovery authentication" is not supported by tgt yet too. So this only setup the session authentication by executing 3 iscsiadm commands, E.g: % iscsiadm -m node --target "iqn.2013-05.test:iscsi.foo" --name \ "node.session.auth.authmethod" -v "CHAP" --op update % iscsiadm -m node --target "iqn.2013-05.test:iscsi.foo" --name \ "node.session.auth.username" -v "Jim" --op update % iscsiadm -m node --target "iqn.2013-05.test:iscsi.foo" --name \ "node.session.auth.password" -v "Jimsecret" --op update --- src/storage/storage_backend_iscsi.c | 111 +++++++++++++++++++++++++++++++++++- 1 file changed, 110 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index ba4f388..d8df5d8 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" @@ -655,9 +658,112 @@ virStorageBackendISCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, return ret; } +static int +virStorageBackendISCSINodeUpdate(const char *portal, + const char *target, + const char *name, + const char *value) +{ + virCommandPtr cmd = NULL; + int status; + int ret = -1; + + cmd = virCommandNewArgList(ISCSIADM, + "--mode", "node", + "--portal", portal, + "--target", target, + "--op", "update", + "--name", name, + "--value", value, + NULL); + + if (virCommandRun(cmd, &status) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to update '%s' of node mode for target '%s'"), + name, target); + goto cleanup; + } + + ret = 0; +cleanup: + virCommandFree(cmd); + return ret; +} static int -virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, +virStorageBackendISCSISetAuth(const char *portal, + virConnectPtr conn, + virStoragePoolDefPtr def) +{ + virSecretPtr secret = NULL; + unsigned char *secret_value = NULL; + virStoragePoolAuthChap chap; + int ret = -1; + + if (def->source.authType == VIR_STORAGE_POOL_AUTH_NONE) + return 0; + + if (def->source.authType != VIR_STORAGE_POOL_AUTH_CHAP) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("iscsi pool only supports 'chap' auth type")); + return -1; + } + + if (!conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("iscsi 'chap' authentication requires connection")); + return -1; + } + + chap = def->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, + def->source.devices[0].path, + "node.session.auth.authmethod", + "CHAP") < 0 || + virStorageBackendISCSINodeUpdate(portal, + def->source.devices[0].path, + "node.session.auth.username", + chap.username) < 0 || + virStorageBackendISCSINodeUpdate(portal, + def->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 int +virStorageBackendISCSIStartPool(virConnectPtr conn, virStoragePoolObjPtr pool) { char *portal = NULL; @@ -696,6 +802,9 @@ virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, NULL, NULL) < 0) goto cleanup; + if (virStorageBackendISCSISetAuth(portal, conn, pool->def) < 0) + goto cleanup; + if (virStorageBackendISCSIConnection(portal, pool->def->source.initiator.iqn, pool->def->source.devices[0].path, -- 1.8.1.4
participants (2)
-
Daniel P. Berrange -
John Ferlan