[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 06/11] storage: Support "chap" authentication for iscsi pool



On 07/06/13 00:31, John Ferlan wrote:
On 05/28/2013 02:39 AM, Osier Yang wrote:
Though the XML for "chap" authentication with plain "password"
was introduced long ago, the function was never implemented.
This patch completes it.

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 | 68 +++++++++++++++++++++++++++++++++++++
  1 file changed, 68 insertions(+)

diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
index ad38ab2..6a17b5a 100644
--- a/src/storage/storage_backend_iscsi.c
+++ b/src/storage/storage_backend_iscsi.c
@@ -713,6 +713,68 @@ 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
+virStorageBackendISCSISetAuth(virStoragePoolDefPtr def,
+                              const char *portal,
[1] Any reason to not pass portal first like other API's? Not that matters..

No reason, will change. :-)



+                              const char *target)
+{
+    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 (virStorageBackendISCSINodeUpdate(portal,
+                                         target,
+                                         "node.session.auth.authmethod",
+                                         "CHAP") < 0 ||
+        virStorageBackendISCSINodeUpdate(portal,
+                                         target,
+                                         "node.session.auth.username",
+                                         def->source.auth.chap.login) < 0 ||
+        virStorageBackendISCSINodeUpdate(portal,
+                                         target,
+                                         "node.session.auth.password",
+                                         def->source.auth.chap.u.passwd) < 0)
In 04/11, you added def->source.auth.type setting/checking which I don't
see here... Or is there some magic happening where auth.u.secret can
take the place of passwd...

It's handled in next patch. Commit log should be enough to clarify this
patch only handles plain password:

<...>
Though the XML for "chap" authentication with plain "password" was introduced long ago, the function was never implemented. This patch completes it.
</...>



+        return -1;
+
+    return 0;
+}
static int
  virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED,
@@ -745,6 +807,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 :-(
@@ -754,6 +817,11 @@ virStorageBackendISCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED,
                                                NULL, NULL) < 0)
              goto cleanup;
+ if (virStorageBackendISCSISetAuth(pool->def,
+                                          portal,
[1] Why not follow other code which passes portal first...

Will change.

Also why pass
pool->def and pool->def->source.devices[0].path - it's not like you
couldn't grab "source.devices[0].path" from the passed pool->def

Sounds reasonable, I will see if there is requirement for it, if not
will change too.

Osier


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]