On Mon, Aug 10, 2026 at 12:28 PM Daniel P. Berrangé <[email protected]> wrote:
> On Mon, Aug 10, 2026 at 11:58:44AM +0300, Kostiantyn Kostiuk wrote: > > On Mon, Aug 10, 2026 at 11:55 AM Daniel P. Berrangé <[email protected] > > > > wrote: > > > > > On Mon, Aug 10, 2026 at 11:37:34AM +0300, Kostiantyn Kostiuk wrote: > > > > On Mon, Aug 10, 2026 at 11:20 AM Daniel P. Berrangé < > [email protected] > > > > > > > > wrote: > > > > > > > > > On Mon, Aug 10, 2026 at 11:06:27AM +0300, Kostiantyn Kostiuk wrote: > > > > > > Hi Valentino, > > > > > > > > > > > > Sorry for the long delay. > > > > > > I have a question for you > > > > > > > > > > > > On Tue, Jul 14, 2026 at 7:10 PM Valentino Paulon < > > > > > > [email protected]> wrote: > > > > > > > > > > > > > > > So, you propose to call seteuid/setegid before any I/O > > > operation? > > > > > > > > > > > > > > > > Yes, specifically for the SSH commands, because they're > unusual > > > in > > > > > > > > that we're doing stuff on behalf of an unprivileged user. > > > > > > > > > > > > > > +1 on the euid/egid direction -- as the reporter that's the > > > outcome I > > > > > > > was hoping for. It confines every step (read, mkdir, create, > > > rename, > > > > > > > chown) to what the target user could already do himself, which > > > removes > > > > > > > the whole TOCTOU class instead of chasing one instance of it. > > > > > > > > > > > > > > A few implementation details that tend to bite with this > pattern, > > > in > > > > > > > case they save a round-trip: > > > > > > > > > > > > > > - drop the supplementary groups too, not just egid: > initgroups() > > > (or > > > > > > > setgroups() with the user's list) while still root, otherwise > the > > > > > > > agent's effective access won't match the user's in either > direction > > > > > > > > > > > > > > > > > > > Technically, there is no limitation to running QGA in > unprivileged > > > mode. > > > > > > Normally, it is a system daemon, but this is not mandatory. > > > > > > In this case, we can not call initgroups because the caller must > > > > > > have CAP_SETGID. > > > > > > We can call initgroups only when QGA runs as root. What do you > think? > > > > > > > > > > In any practical sense it is a system daemon given the set of > commands > > > > > it is exposing. Thus, IMHO, deploying as an unprivileged user > should > > > > > be considered a broken deployment. We could make that explicit by > > > > > refusing to launch. > > > > > > > > > > > > > Makes sense, but we have one more issue with QGA unit tests in this > case. > > > > They are called by an unprivileged user (build user), and initgroups > > > broke > > > > it. > > > > Is it possible to enter the namespace automatically by the test > engine? > > > > > > We can't assume that is possible todo in all build/test environments. > > > > > > Doing things as different user accounts feels pretty dubious for the > > > scope of a unit test too - that is really functional or integration > > > testing. > > > > > > Can we somehow restructure the tests and/or agent code to avoid the > > > changing groups stuff in unit tests. > > > > > > > There are two possible options: > > 1. add #ifdef QGA_BUILD_UNIT_TEST into qmp_guest_ssh_* functions > > 2. check the current uid and skip initgroups if it is zero (my initial > > question) > > Surely you mean "if it is non-zero" here, as it is with uid!=0 that > the UID changes will fail. > yes. So what sounds better: "ifdef" or "uid!=0"? Best Regards, Kostiantyn Kostiuk. > > With regards, > Daniel > -- > |: https://berrange.com ~~ https://hachyderm.io/@berrange :| > |: https://libvirt.org ~~ https://entangle-photo.org :| > |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :| > >
