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

[Osier Yang <jyang@xxxxxxxxxx>]

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 <jyang@xxxxxxxxxx>
> <...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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list