[libvirt] [PATCH v2 2/7] storage: Support "username" for "chap" type "auth"

[Daniel P. Berrange]

On Wed, Jul 10, 2013 at 11:51:42AM -0400, John Ferlan wrote:
> On 07/10/2013 10:49 AM, Daniel P. Berrange wrote:
> > On Tue, Jul 09, 2013 at 03:10:46PM -0400, John Ferlan wrote:
> >> To be consistent with "ceph" types for storage "auth" elements, allow
> >> "username" to be used as an "auth" attribute name for "chap" types.
> >> Continue to allow "login" for backwards compatibility when reading the XML,
> >> but when writing the XML use "username".
> > 
> > Hmm, so the schema for 'chap' auth is utterly awful.
> > 
> > While we have parsed this schema for a while, nothing in the libvirt
> > codebase has ever used 'chap' auth.
> > 
> > As such I think we have reasonable grounds for just discarding the
> > existing code for parsing 'chap' auth and doing it right. ie use
> > the same terminology as 'ceph' and do not include the 'password'
> > value in the XML at all.
> > 
> > Thoughts ?
> > 
> > Daniel
> > 
> 
> Don't have the historical perspective w/r/t when it was added - even
> looking through gitk I can find that parsing in for quite a while (even
> in the src/storage_conf.c).  I admit I found it quite confusing to read
> and attempt to document.
> 
> Other than having to actually make the changes :-), I see no reason why
> the storage_conf needs to be different and support numerous combinations
> based on type...
> 
> To be sure we're on the same page, the storage_conf XML then becomes
> 
> <auth username='someuser'>
>   <secret type='[ceph|iscsi]' [usage='mypassed'|uuid='someuuid']/>
> </auth>

Actually I think the schema is:

 <auth username='someuser' type='[ceph|chap]'>
   <secret [usage='mypassed'|uuid='someuuid']/>
 </auth>

> That is there'd be no reason for 'type' in the XML nor 'password'.  The
> 'login' goes away and the 'username' becomes required.

I think you still want to keep 'type' in the XML, since if iSCSI adds
a different auth mechanism that isn't 'chap', then we have the ability
to represent that using a new type.

> Should we search for 'login' or 'password' when parsing and issue some
> sort of warning/message via VIR_WARN or virReportError?  Ditto for
> 'type'?  Or do we just silently ignore?  Cannot imagine someone putting
> a plain text password in the XML file, but then again I also know what
> happens when you assume something...

My suggestion is to just silently ignore it. Anyone who has been adding
'login' or 'password' to the storage pool XML has never had anything
which works. Thus just removing the XML parser code for this can't make
life any worse for them. They were broken before, and will remain broken
afterwards. Using the new schema will do the right thing for them.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|