Re: [libvirt] [PATCH 1/9] Add volume encryption information handling.

----- "Daniel P. Berrange" <berrange(a)redhat.com&gt; wrote: > On Tue, Jul 21, 2009 at 01:11:57PM +0200, Miloslav Trma?? wrote: > > +#include <stdbool.h> > > +#include <libxml/tree.h> > > + > > +enum virStorageEncryptionFormat { > > + VIR_STORAGE_ENCRYPTION_FORMAT_UNENCRYPTED = 0, > > + VIR_STORAGE_ENCRYPTION_FORMAT_QCOW, /* Both qcow and qcow2 */ > > + > > + VIR_STORAGE_ENCRYPTION_FORMAT_LAST, > > +}; > > +VIR_ENUM_DECL(virStorageEncryptionFormat) > > + > > +typedef struct _virStorageEncryption virStorageEncryption; > > +typedef virStorageEncryption *virStorageEncryptionPtr; > > +struct _virStorageEncryption { > > + int format; /* enum virStorageEncryptionFormat */ > > + > > + union { /* Format-specific data */ > > + struct { > > + char *passphrase; > > + } qcow; > > + } v; > > +}; > > As with the XML format, I'd like to avoid encoding qcow as a > structural element here. Instead go for a generic storage of > secrets. > > enum virStorageEncryptionSecret { > VIR_STORAGE_ENCRYPTION_SECRET_PASSPHRASE, > }; > > struct virStorageSecret{ > int type; /* enum virStorageSecret */ > > union { > char *passphrase; > } data; > }; > > struct _virStorageEncryption { > unsigned encrypted : 1; > > int nsecrets; > virStorageSecret *secrets; > } > > This allows for > 1 secret should we need that (eg, for > LUKS/cryptsetup volume) As argued in the 0/9 e-mail, the "encryption format" is an independent piece of information that needs to be stored, and the set of other information that needs to be stored can differ among formats. Should we ever need support for more than one secret in LUKS, that a separate struct { ... } luks should be added to the union: it seems to me that support for storing more than one passphrase is not necessary for libvirt's use of LUKS (although LUKS supports it) - on the other hand, if such support would be necessary, it would most likely be necessary to store the slot used by each passphrase as well.

We know that the type and amount of information that will need to be stored will vary depending on the "encryption format"; on the other hand, expecting that the information consists of independent "secrets", each with a simple and format-independent definition, is probably too optimistic. (As mentioned above, we might need a key slot ID associated with a LUKS passphrase; we might also need a password hash algorithm associated with a dm-crypt passphrase. The encryption formats used in practice are often complex enough that a "simple passphrase" can not be used.) Once we have to assume that each secret can have an "encryption format"-dependent format, I think it is much clearer to use something like

enum { FORMAT_1, FORMAT_2, FORMAT_3 } format; union { struct { char *secret_1; } format_1; struct { struct { char *secret, id; } secrets[N]; } format_2; struct { int param_1, param_2, param_3; char *secret_1, *secret_2 }; format_2; } which explicitly defines what information is requested for each format, and how it is related, (and which allows showing the data related to a single format in a debugger by simply choosing a single member of the union) than something like struct { enum { SECRET_FMT_1, SECRET_FMT_2, PARAM_FMT_1, PARAM_FMT_2 } type; union { char *secret_fmt_1; struct { char *secret, id} secret_fmt_2; int param_fmt_1; char *param_fmt_2; } } [...] with implicit assumptions about the secret and parameter formats used by various "encryption formats".

Even if the XML uses one or more generic-looking <secret> elements, the information will have to be categorized depending on its meaning and relations before being used; it seems to me cleaner to do it once, when parsing the XML, than to parse the XML without assigning much semantic value to it, and then re-parse the internal representation to categorize the information. Categorizing the information already when parsing the XML will also make it easy to detect invalid or missing data already when parsing the XML, before any "real" operations that might be costly or difficult to undo are started, or before the information is used or stored by other components of libvirt.