3:26 p.m.
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.
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
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
3:26 p.m.
New subject: [libvirt] [PATCH v4 1/3] Add a privileged field to storageDriverState
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
3:44 a.m.
New subject: [libvirt] [PATCH v4 1/3] Add a privileged field to storageDriverState
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 :|
4:31 a.m.
New subject: [libvirt] [PATCH v4 1/3] Add a privileged field to storageDriverState
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 :|
6:37 a.m.
New subject: [libvirt] [PATCH v4 1/3] Add a privileged field to storageDriverState
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
3:26 p.m.
New subject: [libvirt] [PATCH v4 2/3] Adjust 'ceph' authentication secret usage for rbd pool.
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
3:26 p.m.
New subject: [libvirt] [PATCH v4 3/3] storage: Support "chap" authentication for iscsi pool
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
4148
days inactive
4149
days old
6 comments
2 participants
participants (2)
-
Daniel P. Berrange -
John Ferlan