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

Re: [libvirt] [PATCH 04/11] storage: Introduce XMLs to use secret object for pool auth



On 06/06/13 23:34, John Ferlan wrote:
On 05/28/2013 02:39 AM, Osier Yang wrote:
Using plain password is still supported for back-compat reason.

Example XML:
     <auth type='chap' username='foo'>
       <secret uuid='48dcd4a4-b25f-4fc6-8874-84797c6e3678'/>
     </auth>

* docs/schemas/storagepool.rng (Add sourceinfoauthsecret as a choice)
* src/conf/storage_conf.h (union "passwd" and virStoragePoolAuthSecret)
* src/conf/storage_conf.c (s/chap\.passwd/chap\.u\.passwd/;
                            Add a helper virStoragePoolAuthDefFormat;
                            Parse the secret XMLs for "chap" auth)
* tests/storagepoolxml2xmlin/pool-iscsi-auth-secret.xml: (New tests)
* tests/storagepoolxml2xmlout/pool-iscsi-auth-secret.xml: (Likewise)
---
  docs/schemas/storagepool.rng                       |  9 ++-
  src/conf/storage_conf.c                            | 71 +++++++++++++++-------
  src/conf/storage_conf.h                            | 11 +++-
  .../pool-iscsi-auth-secret.xml                     | 19 ++++++
  .../pool-iscsi-auth-secret.xml                     | 22 +++++++
  5 files changed, 107 insertions(+), 25 deletions(-)
  create mode 100644 tests/storagepoolxml2xmlin/pool-iscsi-auth-secret.xml
  create mode 100644 tests/storagepoolxml2xmlout/pool-iscsi-auth-secret.xml

diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng
index ba6c741..386de1b 100644
--- a/docs/schemas/storagepool.rng
+++ b/docs/schemas/storagepool.rng
@@ -295,9 +295,12 @@
                  <text/>
                </attribute>
              </choice>
-            <attribute name='passwd'>
-              <text/>
-            </attribute>
+            <choice>
+              <attribute name='passwd'>
+                <text/>
+              </attribute>
+              <ref name='sourceinfoauthsecret'/>
+            </choice>
            </interleave>
          </group>
          <group>
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 0047372..8f83bae 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -349,7 +349,10 @@ virStoragePoolSourceClear(virStoragePoolSourcePtr source)
if (source->authType == VIR_STORAGE_POOL_AUTH_CHAP) {
          VIR_FREE(source->auth.chap.login);
-        VIR_FREE(source->auth.chap.passwd);
+        if (source->auth.chap.type == VIR_STORAGE_POOL_AUTH_CHAP_PLAIN_PASSWORD)
+            VIR_FREE(source->auth.chap.u.passwd);
+        else if (source->auth.chap.type == VIR_STORAGE_POOL_AUTH_CHAP_SECRET)
+            VIR_FREE(source->auth.chap.u.secret.usage);
See [1] below in 'def'...  It seems type must be one or the other *IF*
we get it defined.  As long as there's a "NONE" type option, then this
code will be OK; otherwise, you have a bit more refactoring to do.

      }
if (source->authType == VIR_STORAGE_POOL_AUTH_CEPHX) {
@@ -483,6 +486,8 @@ virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt,
  {
      char *login = NULL;
      char *username = NULL;
+    char *passwd = NULL;
+    int rc;
login = virXPathString("string(./auth/@login)", ctxt);
      username = virXPathString("string(./auth/@username)", ctxt);
@@ -507,10 +512,20 @@ virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt,
      else if (username)
          auth->login = username;
- auth->passwd = virXPathString("string(./auth/@passwd)", ctxt);
-    if (auth->passwd == NULL) {
+    passwd = virXPathString("string(./auth/@passwd)", ctxt);
+    rc = virStoragePoolDefParseAuthSecret(ctxt, &auth->u.secret);
+
+    if (passwd) {
+        auth->type = VIR_STORAGE_POOL_AUTH_CHAP_PLAIN_PASSWORD;
+        auth->u.passwd = passwd;
+    } else if (rc == 0) {
+        auth->type = VIR_STORAGE_POOL_AUTH_CHAP_SECRET;
+    } else if (rc < 0) {
+        return -1;
+    } else {
          virReportError(VIR_ERR_XML_ERROR, "%s",
-                       _("missing auth passwd attribute"));
+                       _("Either 'passwd' attribute or 'secret' element "
+                         "should be specified"));
          return -1;
      }
[1] According to this code - either we have passwd or secret being used;
however, there's no check if both were supplied, thus making the default
be passwd and ignoring the secret - which will be confusing in other
checks... such as a memory leak on the free side...

Indeed.

  Additionally
because this code may not be reached the 'type' would need a "NONE"
possibility.

If neither is supplied, then we have an error, so at least one must be
used.  I would think you'd prefer secret over plaintext password..

Thus, consider:
     if (virStoragePoolDefParseAuthSecret(ctxt, &auth->u.secret) < 0) {
         auth->passwd = virXPathString("string(./auth/@passwd)", ctxt);
         if (!auth->passwd) {
             error condition;
             return -1;
         }
         auth->type = VIR_STORAGE_POOL_AUTH_CHAP_PLAIN_PASSWORD;
     } else {
         auth->type = VIR_STORAGE_POOL_AUTH_CHAP_SECRET;
     }
     return 0;

Agreed.


NOTE: No need for locals passwd and rc.

@@ -1048,13 +1063,30 @@ virStoragePoolDefParseFile(const char *filename)
      return virStoragePoolDefParse(NULL, filename);
  }
+static void
+virStoragePoolAuthDefFormat(virBufferPtr buf,
+                            virStoragePoolAuthSecret secret)
+{
+    char uuid[VIR_UUID_STRING_BUFLEN];
+
+    virBufferAddLit(buf, "      <secret");
+    if (secret.uuidUsable) {
+        virUUIDFormat(secret.uuid, uuid);
+        virBufferAsprintf(buf, " uuid='%s'", uuid);
+    }
+
+    if (secret.usage != NULL) {
+        virBufferAsprintf(buf, " usage='%s'", secret.usage);
+    }
+    virBufferAddLit(buf, "/>\n");
+}
+
  static int
  virStoragePoolSourceFormat(virBufferPtr buf,
                             virStoragePoolOptionsPtr options,
                             virStoragePoolSourcePtr src)
  {
      int i, j;
-    char uuid[VIR_UUID_STRING_BUFLEN];
virBufferAddLit(buf," <source>\n");
      if ((options->flags & VIR_STORAGE_POOL_SOURCE_HOST) && src->nhost) {
@@ -1129,26 +1161,23 @@ virStoragePoolSourceFormat(virBufferPtr buf,
          virBufferAsprintf(buf,"    <format type='%s'/>\n", format);
      }
- if (src->authType == VIR_STORAGE_POOL_AUTH_CHAP)
-        virBufferAsprintf(buf,"    <auth type='chap' username='%s' passwd='%s'/>\n",
-                          src->auth.chap.login,
-                          src->auth.chap.passwd);
+    if (src->authType == VIR_STORAGE_POOL_AUTH_CHAP) {
+        virBufferAsprintf(buf,"    <auth type='chap' username='%s'",
+                          src->auth.chap.login);
+        if (src->auth.chap.type == VIR_STORAGE_POOL_AUTH_CHAP_PLAIN_PASSWORD) {
+            virBufferAsprintf(buf, " passwd='%s'/>\n",
+                              src->auth.chap.u.passwd);
+        } else if (src->auth.chap.type == VIR_STORAGE_POOL_AUTH_CHAP_SECRET) {
+            virBufferAddLit(buf, ">\n");
+            virStoragePoolAuthDefFormat(buf, src->auth.chap.u.secret);
+            virBufferAddLit(buf,"    </auth>\n");
+        }
+    }
if (src->authType == VIR_STORAGE_POOL_AUTH_CEPHX) {
          virBufferAsprintf(buf,"    <auth username='%s' type='ceph'>\n",
                            src->auth.cephx.username);
-
-        virBufferAddLit(buf,"      <secret");
-        if (src->auth.cephx.secret.uuidUsable) {
-            virUUIDFormat(src->auth.cephx.secret.uuid, uuid);
-            virBufferAsprintf(buf," uuid='%s'", uuid);
-        }
-
-        if (src->auth.cephx.secret.usage != NULL) {
-            virBufferAsprintf(buf," usage='%s'", src->auth.cephx.secret.usage);
-        }
-        virBufferAddLit(buf,"/>\n");
-
+        virStoragePoolAuthDefFormat(buf, src->auth.cephx.secret);
          virBufferAddLit(buf,"    </auth>\n");
      }
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
index aff1393..b52cdc4 100644
--- a/src/conf/storage_conf.h
+++ b/src/conf/storage_conf.h
@@ -153,11 +153,20 @@ struct _virStoragePoolAuthSecret {
      bool uuidUsable;
  };
+enum virStoragePoolAuthChapType {
I think you need a VIR_STORAGE_POOL_AUTH_CHAP_NONE = 0, here


I see no need, since it must be either PASSWORD or SECRET,
otherwise there is error, it's mandatory to have either a 'passwd'
or 'secret'.

Osier


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