[libvirt] 'build' on FS pool now unconditionally formats?

[Daniel P. Berrange]

On Tue, Mar 16, 2010 at 11:01:10AM -0400, Dave Allan wrote:
> On 03/16/2010 06:22 AM, Daniel P. Berrange wrote:
> >On Mon, Mar 15, 2010 at 05:21:26PM -0400, Dave Allan wrote:
> >>
> >>Now that 0.7.7 is done, we need to revisit fs pool building.
> >>
> >>Because it is impossible to implement a check for arbitrary existing
> >>data, the only safe option is to require the overwrite flag in all
> >>cases.  If we do not require the flag in all cases, virt-manager and
> >>virsh will format unknown partitions without providing any indication to
> >>the user that a destructive operation is about to take place.  The only
> >>input from the user will be the selection of the partition.  All other
> >>instances of destructive behavior require explicit confirmation from the
> >>user, or as Cole aptly put it, are loudly warned about by virt-manager.
> >>  I wish that a safe alternative existed, but none does.
> >
> >There are two alternatives that are safe
> 
> Unfortunately, no.  There is no programatic way to detect the presence 
> of arbitrary data on a partition.  Any attempt to do so is false 
> security.  We can, as you point out, determine in some cases that the 
> user *is* going to overwrite something, but we cannot determine in all 
> cases that the user *is not* going to overwrite something.
> 
> >  1. Do a per filesystem magic check on the volume in question. libvirt has
> >     a list of filesystems that I knows about "ext2", "ext3", "ext4",
> >     "ufs", "iso9660", "udf", "gfs", "gfs2", "vfat", "hfs+", "xfs", "ocfs2"
> >     All of these will have an easily identified magic header that could
> >     be positively checked for.
> >
> >Or
> >
> >  2. Do a check for the first 512 or 4096 bytes being all zeros. This 
> >  should
> >     detect the absence of any filesystem AFAIK.
> 
> And that's the problem.  We can detect filesystems *that we know of*. 
> We do not and cannot know the universe of partition formats that exist. 
>  Again, if we pretend we do all we're going to do is give users a false 
> sense of security when the only real security is asking the user "You're 
> about to destroy data, are you certain you have the right partition?"

I don't think we need to care about all possible types of data in the
world. We already have exactly the issue you describe for the logical
and disk storage pools. We blindly run 'pvcreate' on all disk paths
you pass in the logical pool XML. pvcreate will check if it is alrady
a LVM physical volume. It will happily overwrite any other type of
data on that devices.  For the disk pool we just run 'parted mklabel'
on the path, again not checking for any random data that might be
there.

We don't need to protect against all possible data that might be on
the disk. Primarily we should be protecting against the execution of
virStoragePoolBuild() twice in a row. This implies the safety checks
that I outline above. This also protects against the most likely
common sources of data the user might have there already. 

I agree with you that this isn't 100% protection from all possible risks.
Adding a OVERWRITE flag on its own is not actually reducing the risk of 
the API though. It is merely moving the risk from one part of the API to
another. The overall risk remains the same, except it avoids one specific
virt-manager bug. Adding checking to the Build() API with flags=0 does
reduce the real risk in all scenarios that libvirt itself directly supports.


> 
> >The semantics we should have for these APIs are
> >
> >  - virStoragePoolBuild(flags=0)   - format the 
> >  filesystem/volgroup/partition table
> >                                     only if not already formatted.
> >  - virStoragePoolBuild(flags=OVERWRITE) - unconditionally format
> >  - virStoragePoolDelete()         - wipe data such that Build(flags=0) is 
> >  guaranteed
> >                                     to be successful next time.
> >
> >So I see several things that need to be implemented
> >
> >  - Make disk&  logical  pool backends check for existing formatted data
> >  - Implement the 'Delete' operation for all pool types,
> >  - Add (checked) formatting of filesystem pools
> >  - Add  OVERWRITE flag to disk, logical, filesystem pool types to skip 
> >  the check
> >
> >>The attached patch implements this behavior.
> >
> >NACK in this form.
> 
> We clearly disagree, so I think we need some additional voices to weigh 
> in here.

The most fundemental requirement here is that we provide API semantics
that are consistent across all pool types. Second, we need to reduce the
risk of this API bth in general, and to protect against the virt-manager
bug. This patch makes the FS pool even more of a special case, and does 
not do anything to address the identical risk scenario *already* present
in LVM & disk pool types impl of the virStoragePoolBuild().

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|