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

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.

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.