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

Re: [libvirt] [PATCH 01/20] Secret manipulation step 1: Public API



On Sun, Aug 16, 2009 at 10:47:54PM +0200, Miloslav Trmač wrote:
> This patch adds a "secret" as a separately managed object, using a
> special-purpose API to transfer the secret values between nodes and
> libvirt users.

  Okay, interesting...

> Rather than add explicit accessors for attributes of secrets, and
> hard-code the "secrets are related to storage volumes" association in
> the API, the API uses XML to manipulate the association as well as
> other attributes, similarly to other areas of libvirt.
> 
> The user can set attributes of the secret using XML, e.g.
>   <secret ephemeral='no' private='yes'>
>     <uuid>b8eecf55-798e-4db7-b2dd-025b0cf08a36</uuid>
>     <volume>/var/lib/libvirt/images/mail.img</volume>
>     <description>LUKS passphrase for our mail server</description>
>   </secret>
> If <uuid/> is not specified, it is chosen automatically.

  Should secret always be tied to volumes. The API is generic enough
that we should make sure we can use this later to get priviledged
access to other resources, though right now I don't have a good
example in mind.

> The secret value can be either generated and stored by libvirt during
> volume creation, or supplied by the user using virSecretSetValue().
> 
> A simple API is provided for enumeration of all secrets.  Very large
> deployments will manage secret IDs automatically, so it is probably not
> necessary to provide a specialized lookup function (allowing the volume
> key -> secret ID lookup in less than O(number of secrets)).  These
> functions can eventually be added later.
> 
> Changes since the second submission:
> - Changed API to revolve around a virSecretPtr.  The operations are now:
>   virSecretGetConnect
>   virConnectNumOfSecrets
>   virConnectListSecrets
>   virSecretLookupByUUIDString
>   virSecretDefineXML
>   virSecretGetUUIDString
>   virSecretGetXMLDesc
>   virSecretSetValue
>   virSecretGetValue
>   virSecretUndefine
>   virSecretRef
>   virSecretFree

  Okay that's completely generic, sounds fine !

> - Use an optional <uuid/> element inside <secret/> to pre-specify an
>   UUID in virSecretDefineXML() (default is to choose an UUID
>   automatically)
> - s/secret_id/uuid/g

  Not sure I catch that, 

> - use "unsigned char *" for secret value

  fine, assuming 0 terninated. though this may not work with everything
but since we need to import/export to XML, unless uuencoding the value
we won't be able to embed something which is not part of XML character
range (#x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] |
[#x10000-#x10FFFF]) unicode code points.

> - Use "Secret XML" instead of "Secret attributes XML" in the HTML
>   documentation now that the XML contains the UUID as well.
[...]
> diff --git a/docs/format.html b/docs/format.html

  No need to send the generated files in the patch :-)

[..]
> diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in
> new file mode 100644
> index 0000000..7471bf7
> --- /dev/null
> +++ b/docs/formatsecret.html.in
> @@ -0,0 +1,52 @@
> +<html>
> +  <body>
> +    <h1>Secret XML format</h1>
> +
> +    <ul id="toc"></ul>

  it's good to have a document from the start :-) but this lacks a bit
  of the intended use for the API, i.e. where this may be required to
  use it.

> +    <h2><a name="SecretAttributes">Secret XML</a></h2>
> +
> +    <p>
> +      Secrets stored by libvirt may have attributes associated with them, using
> +      the <code>secret</code> element.  The <code>secret</code> element has two
> +      optional attributes, each with values '<code>yes</code>' and
> +      '<code>no</code>', and defaulting to '<code>no</code>':
> +    </p>
> +    <dl>
> +      <dt><code>ephemeral</code></dt>
> +      <dd>This secret must only be kept in memory, never stored persistently.
> +      </dd>
> +      <dt><code>private</code></dt>
> +      <dd>The value of the secret must not be revealed to any caller of libvirt,
> +        nor to any other node.
> +      </dd>
> +    </dl>
> +    <p>
> +      The top-level <code>secret</code> element may contain the following
> +      elements:
> +    </p>
> +    <dl>
> +      <dt><code>uuid</code></dt>
> +      <dd>
> +        An unique identifier for this secret (not necessarily in the UUID
> +        format).  If omitted when defining a new secret, a random UUID is
> +        generated.
> +      </dd>
> +      <dt><code>volume</code></dt>
> +      <dd>Key of a volume this secret is associated with.  It is safe to delete
> +        the secret after the volume is deleted.
> +      </dd>
> +      <dt><code>description</code></dt>
> +      <dd>A human-readable description of the purpose of the secret.
> +      </dd>
> +    </dl>
> +
> +    <h2><a name="example">Example</a></h2>
> +
> +    <pre>
> +      &lt;secret ephemeral='no' private='yes'&gt;
> +         &lt;volume&gt;/var/lib/libvirt/images/mail.img&lt;/volume&gt;
> +         &lt;description&gt;LUKS passphrase for the main hard drive of our mail server&lt;/description&gt;
> +      &lt;/secret&gt;</pre>
> +  </body>
> +</html>
[...]
> --- /dev/null
> +++ b/docs/schemas/secret.rng
> @@ -0,0 +1,44 @@
> +<!-- A Relax NG schema for the libvirt secret properties XML format -->
> +<grammar xmlns="http://relaxng.org/ns/structure/1.0";>
> +  <start>
> +    <ref name='secret'/>
> +  </start>
> +
> +  <define name='secret'>
> +    <element name='secret'>
> +      <optional>
> +	<attribute name='ephemeral'>
> +	  <choice>
> +	    <value>yes</value>
> +	    <value>no</value>
> +	  </choice>
> +	</attribute>
> +      </optional>
> +      <optional>
> +	<attribute name='private'>
> +	  <choice>
> +	    <value>yes</value>
> +	    <value>no</value>
> +	  </choice>
> +	</attribute>
> +      </optional>
> +      <interleave>
> +	<optional>
> +	  <element name='uuid'>
> +	    <text/>
> +	  </element>
> +	</optional>
> +	<optional>
> +	  <element name='description'>
> +	    <text/>
> +	  </element>
> +	</optional>
> +	<optional>
> +	  <element name='volume'>
> +	    <text/>
> +	  </element>
> +	</optional>
> +      </interleave>
> +    </element>
> +  </define>
> +</grammar>

  Schemas looks fine,I'm just wondering if at some point we may not have
to allow something else than <volume> as the associated target... But
ensuing backward compatibility should be no problem if we extend uses.

> diff --git a/include/libvirt/libvirt.h b/include/libvirt/libvirt.h
> index 855f755..fd5a70f 100644
> --- a/include/libvirt/libvirt.h
> +++ b/include/libvirt/libvirt.h
> @@ -1448,6 +1448,40 @@ void virEventRegisterImpl(virEventAddHandleFunc addHandle,
>                            virEventAddTimeoutFunc addTimeout,
>                            virEventUpdateTimeoutFunc updateTimeout,
>                            virEventRemoveTimeoutFunc removeTimeout);
> +
> +/*
> + * Secret manipulation API
> + */
> +
> +/**
> + * virSecret:
> + *
> + * A virSecret stores a secret value (e.g. a passphrase or encryption key)
> + * and associated metadata.
> + */
> +typedef struct _virSecret virSecret;
> +typedef virSecret *virSecretPtr;
> +
> +virConnectPtr           virSecretGetConnect     (virSecretPtr secret);
> +int                     virConnectNumOfSecrets  (virConnectPtr conn);
> +int                     virConnectListSecrets   (virConnectPtr conn,
> +                                                 char **uuids,
> +                                                 int maxuuids);
> +virSecretPtr            virSecretLookupByUUIDString(virConnectPtr conn,
> +                                                    const char *uuid);
> +virSecretPtr            virSecretDefineXML      (virConnectPtr conn,
> +                                                 const char *xml);

 Let's add an "unsigned int flags" to virSecretDefineXML() especially
as we don't know yet the scope of future usages.

> +char *                  virSecretGetUUIDString  (virSecretPtr secret);
> +char *                  virSecretGetXMLDesc     (virSecretPtr secret);

  And also add an "unsigned int flags" to virSecretGetXMLDesc

> +int                     virSecretSetValue       (virSecretPtr secret,
> +                                                 const unsigned char *value,
> +                                                 size_t value_size);
> +unsigned char *         virSecretGetValue       (virSecretPtr secret,
> +                                                 size_t *value_size);

  Ah, so we aren't relying on 0 termination, but in that case the value
need to be uuencoded in the XML, and that should be made clear in the
API description. Actually not having the secret completely in the clear
in the XML dump sounds like a good idea.

> +int                     virSecretUndefine       (virSecretPtr secret);
> +int                     virSecretRef            (virSecretPtr secret);
> +int                     virSecretFree           (virSecretPtr secret);
> +
>  #ifdef __cplusplus
>  }
>  #endif

  Okay, looks fine but I still need to look at the libvirt.c entry point
to check comment, and there is a couple of fixes needed. I don't have
a good entry point to use for uuecoding/decoding of the secret for XML
input/outut.
  Note that uuecode is really the most common way in XML to pass
a random set of bytes, there is even support at some schemas level. We
would need to add that check to the rng too (I will look at it),

  thanks !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


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