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
