Re: [libvirt] [PATCH v2 6/7] storage: Support "chap" authentication for iscsi pool

[Osier Yang]

On 15/07/13 23:01, John Ferlan wrote:

On 07/15/2013 10:16 AM, Osier Yang wrote:

On 10/07/13 03:10, John Ferlan wrote:

From: Osier Yang <jy...@redhat.com>

<...snip...>

diff --git a/src/storage/storage_backend_iscsi.c
b/src/storage/storage_backend_iscsi.c
index 0a4cd22..673ca16 100644
--- a/src/storage/storage_backend_iscsi.c
+++ b/src/storage/storage_backend_iscsi.c
@@ -32,6 +32,8 @@
   #include <unistd.h>
   #include <sys/stat.h>
   +#include "datatypes.h"
+#include "driver.h"
   #include "virerror.h"
   #include "storage_backend_scsi.h"
   #include "storage_backend_iscsi.h"
@@ -39,6 +41,7 @@
   #include "virlog.h"
   #include "virfile.h"
   #include "vircommand.h"
+#include "virobject.h"
   #include "virrandom.h"
   #include "virstring.h"
   @@ -545,9 +548,112 @@ cleanup:
       return ret;
   }
   +static int
+virStorageBackendISCSINodeUpdate(const char *portal,
+                                 const char *target,
+                                 const char *name,
+                                 const char *value)
+{
+     virCommandPtr cmd = NULL;
+     int status;
+     int ret = -1;
+
+     cmd = virCommandNewArgList(ISCSIADM,
+                                "--mode", "node",
+                                "--portal", portal,
+                                "--target", target,
+                                "--op", "update",
+                                "--name", name,
+                                "--value", value,
+                                NULL);
+
+    if (virCommandRun(cmd, &status) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Failed to update '%s' of node mode for
target '%s'"),
+                       name, target);
+        goto cleanup;
+    }
+
+    ret = 0;
+cleanup:
+    virCommandFree(cmd);
+    return ret;
+}
+
+static int
+virStorageBackendISCSISetAuth(const char *portal,
+                              virConnectPtr conn,
+                              virStoragePoolSourcePtr source)
+{
+    virSecretPtr secret = NULL;
+    unsigned char *secret_value = NULL;
+    const char *passwd = NULL;
+    virStoragePoolAuthChap chap;
+    int ret = -1;
+
+    if (source->authType == VIR_STORAGE_POOL_AUTH_NONE)
+        return 0;
+
+    if (source->authType != VIR_STORAGE_POOL_AUTH_CHAP) {
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("iscsi pool only supports 'chap' auth type"));
+        return -1;
+    }
+
+    chap = source->auth.chap;
+    if (chap.type == VIR_STORAGE_POOL_AUTH_CHAP_SECRET) {
+        if (chap.u.secret.uuidUsable)
+            secret = virSecretLookupByUUID(conn, chap.u.secret.uuid);
+        else
+            secret = virSecretLookupByUsage(conn,
VIR_SECRET_USAGE_TYPE_ISCSI,
+                                            chap.u.secret.usage);
+
+        if (secret) {
+            size_t secret_size;
+            secret_value =
+                conn->secretDriver->secretGetValue(secret,
&secret_size, 0,
+
VIR_SECRET_GET_VALUE_INTERNAL_CALL);
+            if (!secret_value) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("could not get the value of the
secret "
+                                 "for username %s"), chap.login);
+                goto cleanup;
+            }
+        } else {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("username '%s' specified but secret not
found"),
+                           chap.login);
+            goto cleanup;
+        }
+        passwd = (const char *)secret_value;
+    } else if (chap.type == VIR_STORAGE_POOL_AUTH_CHAP_PLAIN_PASSWORD) {
+        passwd = chap.u.passwd;
+    }
+
+    if (virStorageBackendISCSINodeUpdate(portal,
+                                         source->devices[0].path,
+                                         "node.session.auth.authmethod",
+                                         "CHAP") < 0 ||
+        virStorageBackendISCSINodeUpdate(portal,
+                                         source->devices[0].path,
+                                         "node.session.auth.username",
+                                         source->auth.chap.login) < 0 ||
+        virStorageBackendISCSINodeUpdate(portal,
+                                         source->devices[0].path,
+                                         "node.session.auth.password",
+                                         passwd) < 0)
+        goto cleanup;
+
+    ret = 0;
+
+cleanup:
+    virObjectUnref(secret);
+    VIR_FREE(secret_value);
+    return ret;
+}
     static char *
-virStorageBackendISCSIFindPoolSources(virConnectPtr conn
ATTRIBUTE_UNUSED,
+virStorageBackendISCSIFindPoolSources(virConnectPtr conn,
                                         const char *srcSpec,
                                         unsigned int flags)
   {
@@ -590,6 +696,9 @@
virStorageBackendISCSIFindPoolSources(virConnectPtr conn
ATTRIBUTE_UNUSED,
                                             &ntargets, &targets) < 0)
           goto cleanup;
   +    if (virStorageBackendISCSISetAuth(portal, conn, source) < 0)
+        goto cleanup;
+

i think the chap auth iscsi pool won't  be able to start with this change,
findPoolSources is an api for discovering the pool sources. however,
we want the chap auth are set up before connecting to the iscsi target
when starting the pool, otherwise the pool starting will fail on
authentication.
note that startPool and findPoolSources are independant apis, they don't
invoke each other.

with this change, if one wants to start the pool successfully, he has to
invoke the findPoolSources first, this dependancy is incorrect. as an
example, in virsh layer, one has to execute find-storage-pool-sources
or find-storage-pool-sources-as before pool-start.

as an alternative way to have the non-null 'conn' for startPool, we can
create a connection in storageDriverAutostart (like qemu driver does),
which is the only place pass an null 'conn' to startPool,

While there is a v3 posted - this code hasn't differed there, so I'll
just use this opportunity to ask you questions...

A 'conn' to what? Should we assume qemu like nwfilter_driver.c does?

     if (!driverState->privileged)
         return 0;

i think we don't need to restrict it to be a priviledged connection for
storage. otherwise there will be regression.

    conn = virConnectOpen("qemu:///system");

Do we further restrict the StartPool code to ensure there is a
privileged qemu connection?

yeah, we will want to get a connection object, but as said above, it
should work both priviledged or unpriviledged connection, and no
need to restrict startPool to ensure there is a connection, since
other storage backends may still work without a connection object

osier

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list