Re: [libvirt] [PATCH 01/20] Secret manipulation step 1: Public API
----- "Daniel Veillard" <veillard@redhat.com> wrote:
On Sun, Aug 16, 2009 at 10:47:54PM +0200, Miloslav Trmač wrote:
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 is not "tied" to a volume - the volume is an optional attribute. A libvirt user can ignore the volume field completely, and manage the secrets separately - but other clients, e.g. virt-manager, can use the volume attribute to locate a secret for a given volume without storing any data outside of libvirt.
- s/secret_id/uuid/g
Not sure I catch that, An earlier patch was calling the ID of a secret object "secret_id" everywhere, leading to constructs like <secret secret_id=.../> In an earlier patch review I was asked to change 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. No, the value is an arbitrary set of bytes that is never passed through XML. Originally it was (void *value, size_t value_size), the change to (unsigned char *value, size_t value_size) was requested in an earlier review.
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. Those parts are documented in later patches that add references to secrets to the <volume> and <domain> XML formats.
+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 * virSecretGetXMLDesc (virSecretPtr secret);
And also add an "unsigned int flags" to virSecretGetXMLDesc
Will do.
+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. The values are never transferred in the XML, only using the virSecret[GS]etValue functions. (Storing secrets in XML was a major objection against the first version of my patch e.g. because XML tends to be revealed in various debugging dumps.) Mirek
On Wed, Aug 19, 2009 at 05:36:27AM -0400, Miloslav Trmac wrote:
----- "Daniel Veillard" <veillard@redhat.com> wrote:
On Sun, Aug 16, 2009 at 10:47:54PM +0200, Miloslav Trmač wrote:
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 is not "tied" to a volume - the volume is an optional attribute. A libvirt user can ignore the volume field completely, and manage the secrets separately - but other clients, e.g. virt-manager, can use the volume attribute to locate a secret for a given volume without storing any data outside of libvirt.
Okay, it may later be tied to something else but then we can expand the rng, fine !
- s/secret_id/uuid/g
Not sure I catch that, An earlier patch was calling the ID of a secret object "secret_id" everywhere, leading to constructs like <secret secret_id=.../> In an earlier patch review I was asked to change that.
Ah, right !
- 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. No, the value is an arbitrary set of bytes that is never passed through XML. Originally it was (void *value, size_t value_size), the change to (unsigned char *value, size_t value_size) was requested in an earlier review.
yes Dan pointed I was wrong too :-)
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.
Those parts are documented in later patches that add references to secrets to the <volume> and <domain> XML formats.
okay
+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 * virSecretGetXMLDesc (virSecretPtr secret);
And also add an "unsigned int flags" to virSecretGetXMLDesc
Will do.
thanks ! looks good then, 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/
On Wed, Aug 19, 2009 at 05:36:27AM -0400, Miloslav Trmac wrote:
----- "Daniel Veillard" <veillard@redhat.com> wrote:
+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 * virSecretGetXMLDesc (virSecretPtr secret);
And also add an "unsigned int flags" to virSecretGetXMLDesc
Will do.
Actually I think virConnectListSecrets should get one too :-) 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/
On Wed, Aug 19, 2009 at 03:19:07PM +0200, Daniel Veillard wrote:
On Wed, Aug 19, 2009 at 05:36:27AM -0400, Miloslav Trmac wrote:
----- "Daniel Veillard" <veillard@redhat.com> wrote:
+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 * virSecretGetXMLDesc (virSecretPtr secret);
And also add an "unsigned int flags" to virSecretGetXMLDesc
Will do.
Actually I think virConnectListSecrets should get one too :-)
I think that's overkill - none of the other virConnectList* methods have them - we only do it for Define/Create/GetXML in other APIs. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Miloslav Trmac