[libvirt PATCH 0/5] Formalize the deprecation of arguments in virsh

[Tim Wiederhake]

On Tue, 2021-03-23 at 16:55 +0000, Daniel P. Berrangé wrote:
> On Tue, Mar 23, 2021 at 05:23:38PM +0100, Tim Wiederhake wrote:
> > On Tue, 2021-03-23 at 15:28 +0100, Peter Krempa wrote:
> > > On Tue, Mar 23, 2021 at 15:19:44 +0100, Michal Privoznik wrote:
> > > > On 3/23/21 3:04 PM, Peter Krempa wrote:
> > > > > On Tue, Mar 23, 2021 at 14:50:09 +0100, Michal Privoznik
> > > > > wrote:
> > > > > > On 3/23/21 2:42 PM, Daniel P. Berrangé wrote:
> > > > > > > On Tue, Mar 23, 2021 at 02:36:19PM +0100, Michal
> > > > > > > Privoznik
> > > > > > > wrote:
> > > 
> > > [...]
> > > 
> > > > > The only thing that IMO should be removed but I didn't for
> > > > > compatibility
> > > > > is the 'secret-set-value's 'base64' parameter as that is
> > > > > insecure. There
> > > > > isn't a compatible replacement though.
> > > > > 
> > > > 
> > > > That's debatable. Its not much worse than reading from a file.
> > > > I
> > > > mean, who
> > > > has access to my $HISTFILE? Only me and root and in both cases
> > > > the
> > > > secret
> > > 
> > > It's not about HISTFILE, but about the process listing. On a
> > > default
> > > linux box, all users can list all other user's processes. If your
> > > password is an argument for a command, it will be readable for
> > > other
> > > users without the access to your directory.
> > > 
> > > Arguably, the lifetime of virsh is very short, so it's extremely
> > > unlikely for anyone to notice, but it's insecure regardless.
> > > 
> > > > can be changed or read from the file (if the file is not
> > > > deleted
> > > > right away,
> > > > and even then it could be recovered). Many tools accept
> > > > passwords
> > > > in clear
> > > > text on cmd line (e.g. curl, wget). If anything, we could
> > > > document
> > > > why
> > > 
> > > You should avoid use of those arguments if you are on a multi-
> > > user
> > > box.
> > > 
> > > > --base64 is dangerous (if we haven't done so yet).
> > > 
> > > It is documented as such and also prints a warning as pointed out
> > > in
> > > the
> > > other reply.
> > > 
> > 
> > Hi all,
> > 
> > thank you for your feedback!
> > 
> > My motivation for starting this patch series was the desire to
> > change
> > the behavior of the virsh commands "create", "define", "snapshot-
> > create", "cpu-compare", and "hypervisor-cpu-compare": Currently,
> > those
> > commands accept an "--validate" argument that enables RNG schema
> > validation for the provided XML. In my opinion, this should be the
> > default behavior. This series was supposed to lay the ground work,
> > a
> > formal deprecation system for commands and arguments.
> > 
> > I understand that suddenly rejecting invalid XML where it was
> > accepted
> > previously and seemingly did the right thing breaks backwards
> > compatibility, even if the only affected users are those with
> > invalid
> > XML which should be fixed anyway. To that end, I wrote a small set
> > of
> > patches that introduce a  "--no-validate" argument to these
> > commands,
> > mark that flag as deprecated, and only then make the switch for
> > enabling validation by default.
> > 
> > For users with valid XML, this change is invisible. Users with
> > invalid
> > XML are being made aware that their XML is broken, but given the
> > opportunity to continue operations by specifying "--no-validate"
> > until
> > their XML is fixed.
> 
> Their XML is not neccessarily broken. They may simply have included
> some XML elements from newer libvirt that are not relevant on the
> current libvirt and be fine with them being ignored.  Yes it would
> be better if they actually tailored the XML for the specific libvirt,
> but the kind of people using virsh often don't care about this level
> of perfection.

Good point, did not think about that possibility. Thanks!

> Note, that we automatically use validation with the 'virsh *edit'
> commands because those are for interactive users and they often
> make mistakes resulting in their changes silently disappearing.
> So in that case validating by default was a clear win with no
> real downside (assuming bug free schemas).
> 
> 
> > I will abandon this series and retry with a version that simply
> > outputs
> > a warning if the user provides XML data without also using "
> > --validate", similar to the one for "--base64".
> 
> This will result in warnings for almost everyone who uses virsh, so I
> would not be in favour of that.

I realized that too, once I hit "send". Let me see if something like
"always validate; '--validate' => error out, no '--validate' => warn
and continue" is feasible.

Cheers,
Tim