On Thu, May 5, 2022 at 6:44 AM Johannes Schindelin <Johannes.Schindelin@xxxxxx> wrote: > > On Mon, 2 May 2022, Carlo Marcelo Arenas Belón wrote: > > > > $ IKNOWWHATIAMDOING=YES ./t0034-root-safe-directory.sh > > Hmm. I would like to suggest that we can side-step all of these issues > (and the ones I outline below) by considering a similar approach to the > one Stolee took in t0033: use one or more `GIT_TEST_*` environment > variables to pretend the exact scenario we want to test for. I wish we could, but I think it is not possible in principle because it would break the trust chain that we are relying on here to make this work. As explained in the commit message from the next patch, for this to be useful as well as safe our ONLY chance is to trust SUDO_UID hasn't been tampered with, which also requires that we run through sudo so git is running as root and not as the regular user the test suite would use. If we remove the (I am really root, before I trust SUDO_UID) requirement from our code, we have just opened ourselves to a way to weaken the protections that were added with CVE-2022-24765. to be frank while Junio mention this "weakens" the checks, I consider I was strengthened them by introducing a mechanism the user could use (only when he is root) to safely tell us that he wants to trust a repository that is not owned by him without having to create an exception, and also improving the usability of it, but "magically" detecting which uid they are most likely to trust. > It took me a good chunk of time to figure out how to run these tests, and > I will have to remember to revert the temporary edit of `/etc/sudoers` > file. This is definitely not something I plan on doing often, so I wonder > how these regression tests can guarantee that no regressions are > introduced if they are so hard to run ;-) by running in the CI (at least the macOS hosts, and maybe others if we decide later to butcher their sudoers config as well) I am adding more instructions in the commit message from the next RFC to help anyone that might want to run this locally (which I wouldn't recommend myself) > This would be more canonical as `test_when_finished "sudo rm -rf root"` in > the preceding test cases. correct, but I was attempting not to do that to make it less of a pain to write more tests since (probably incorrectly) I assumed it would be simpler to remember that there is a test at the end that does the cleanup and at least one at the beginning that does the setup than probably having to take care of those 2 things on each test that you write. Ideally, the test framework would be able to know that this test creates files as root and cleanup itself, but that was specifically punted. I am keeping this for the next RFC, but I am open to changing it to whatever you would prefer, until a proper integration could be written to clean that mess up. Carlo
