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

2013-07-18 Thread Osier Yang 

On 16/07/13 00:07, John Ferlan wrote:

On 07/15/2013 11:11 AM, Osier Yang wrote:

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...


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

Right and that's what I wasn't quite sure how to resolve in the
startPool path.  Furthermore a connection to what?


hm, it's a good question. see below


I guess I have to assume qemu because of the docs which have:
 disk type='volume' device='disk'
   driver name='qemu' type='raw'/


we only use qemu here because of 'volume' type disk only works
for qemu driver, and it's just a document example.  it doesn't relate
with how to set up the connection uri in storage driver. storage driver
should be independant with each hypervisor drivers (e.g lxc/xen/qemu),
but the question is how could the storage driver know which uri it
should use to get a connection? lxc:///, qemu:///[session|system],
or others? directly using qemu uri doesn't work, for instance, libvirtd
is running without qemu driver loaded.

but i don't have idea either. and also don't see any shortcut to
get the connection object.

Daniel, any thoughts?


   source pool='blk-pool0' volume='blk-pool0-vol0'/
   target dev='hda' bus='ide'/

(BTW: 'volume' is not listed as a valid 'disk type=value'...)

And it seems I'd need to a privileged field to _virStorageDriverState
since storageStateReload() doesn't have the privileged value like
storageStateInitialize() has.

In storageDriverAutostart() there'd need to be a virConnectOpen to
either qemu:///system or qemu:///session.  That 'conn' would be passed
to the startPool (but could also be passed along to checkPool and
refreshPool conceivably.

This is (more or less) like what qemuAutostartDomains() does with
respect to 'conn' (except it's using cfg-uri).  The comment in that
code is rather dubious too...

I'll redo v3 7/7 and repost those changes.

John


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

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

2013-07-15 Thread Osier Yang 

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

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

Although the XML for chap authentication with plain password
was introduced long ago, the function was never implemented.
This patch completes it by performing the authentication during
findPoolSources() processing of pools with an authType of
VIR_STORAGE_POOL_AUTH_CHAP specified.

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

Update storage_backend_rbd.c to use the internal API to get the secret
value instead and error out if the secret value is not found. Without the
flag VIR_SECRET_GET_VALUE_INTERNAL_CALL, there is no way to get the value
of private secret.

Update the formatstorage.html to describe the usage of the secret element
to mention that the secret type iscsi and ceph can be used
to storage pool too.

Update the formatsecret.html to include a reference to the storage pool
---
  docs/formatsecret.html.in   |  10 ++--
  docs/formatstorage.html.in  |  31 +-
  src/storage/storage_backend_iscsi.c | 113 +++-
  src/storage/storage_backend_rbd.c   |  13 -
  4 files changed, 159 insertions(+), 8 deletions(-)

diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in
index 50c9533..3e306b5 100644
--- a/docs/formatsecret.html.in
+++ b/docs/formatsecret.html.in
@@ -64,8 +64,9 @@
a single codename/code element that specifies a usage name
for the secret.  The Ceph secret can then be used by UUID or by
this usage name via the codelt;authgt;/code element of
-  a a href=formatdomain.html#elementsDisksdisk
-  device/a. span class=sinceSince 0.9.7/span.
+  a a href=formatdomain.html#elementsDisksdisk device/a or
+  a a href=formatstorage.htmlstorage pool (rbd)/a.
+  span class=sinceSince 0.9.7/span.
  /p
  
  h3Usage type iscsi/h3

@@ -76,8 +77,9 @@
a single codetarget/code element that specifies a usage name
for the secret.  The iSCSI secret can then be used by UUID or by
this usage name via the codelt;authgt;/code element of
-  a a href=formatdomain.html#elementsDisksdisk
-  device/a. span class=sinceSince 1.0.4/span.
+  a a href=formatdomain.html#elementsDisksdisk device/a or
+  a a href=formatstorage.htmlstorage pool (iscsi)/a.
+  span class=sinceSince 1.0.4/span.
  /p
  
  h2a name=exampleExample/a/h2

diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
index d702eb1..cf028dd 100644
--- a/docs/formatstorage.html.in
+++ b/docs/formatstorage.html.in
@@ -72,6 +72,9 @@
  lt;sourcegt;
lt;host name=iscsi.example.com/gt;
lt;device path=demo-target/gt;
+  lt;auth type='chap' username='myname'gt;
+lt;secret type='iscsi' usage='mycluster_myname'/gt;
+  lt;/authgt;
lt;vendor name=Acme/gt;
lt;product name=model/gt;
  lt;/sourcegt;
@@ -80,7 +83,6 @@
  pre
  ...
  lt;sourcegt;
-lt;sourcegt;
lt;adapter type='fc_host' parent='scsi_host5' wwnn='2000c9831b4b' 
wwpn='1000c9831b4b'/gt;
  lt;/sourcegt;
  .../pre
@@ -123,6 +125,33 @@
  which is the hostname or IP address of the server. May optionally
  contain a codeport/code attribute for the protocol specific
  port number. span class=sinceSince 0.4.1/span/dd
+  dtcodeauth/code/dt
+  ddIf present, the codeauth/code element provides the
+authentication credentials needed to access the source by the
+setting of the codetype/code attribute. The codetype/code
+must be either chap or ceph. For a chap codetype/code
+attribute, mandatory attributes codelogin/code or
+codeusername/code and either a codepasswd/code or a
+codesecret/code sub-element are used to provide the
+authentication identification. span class=sinceSince 1.1.1,/span
+when writing out the XML, libvirt will utilize the
+

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

2013-07-15 Thread John Ferlan 
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,

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

2013-07-15 Thread 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

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

2013-07-15 Thread John Ferlan 
On 07/15/2013 11:11 AM, Osier Yang wrote:
 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...


 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

Right and that's what I wasn't quite sure how to resolve in the
startPool path.  Furthermore a connection to what?

I guess I have to assume qemu because of the docs which have:

disk type='volume' device='disk'
  driver name='qemu' type='raw'/
  source pool='blk-pool0' volume='blk-pool0-vol0'/
  target dev='hda' bus='ide'/

(BTW: 'volume' is not listed as a valid 'disk type=value'...)

And it seems I'd need to a privileged field to _virStorageDriverState
since storageStateReload() doesn't have the privileged value like
storageStateInitialize() has.

In storageDriverAutostart() there'd need to be a virConnectOpen to
either qemu:///system or qemu:///session.  That 'conn' would be passed
to the startPool (but could also be passed along to checkPool and
refreshPool conceivably.

This is (more or less) like what qemuAutostartDomains() does with
respect to 'conn' (except it's using cfg-uri).  The comment in that
code is rather dubious too...

I'll redo v3 7/7 and repost those changes.

John

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