| Message ID | 20240723021900.388020-1-gitster@pobox.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | safe.directory clean-up | expand |
Hi Junio Thanks for picking this up, I think this looks like a good approach apart from resolving relative entries in safe.directory which is intrinsically unsafe as we don't know which directory the user wants to consider safe. With these changes it should not be necessary to add "." to safe.directory to get "git daemon" to work and all the other code paths use an absolute path from getcwd() so I don't think there is any need to support relative directories. I'll be off the list for the next couple of weeks Best Wishes Phillip On 23/07/2024 03:18, Junio C Hamano wrote: > Changes since v1: > > - two "extra" patches for optimization that are unnecessary have > been dropped. > - a patch to add test for safe.directory="." use case has been > added. > > Recently we discussed what we should do when either the path > configured in the safe.directory configuration or coming from > the caller of ensure_valid_ownership() function as a result of > repository discovery is not normalized and textual equality check is > not sufficient. See the thread the contains > > https://lore.kernel.org/git/6d5b75a6-639d-429b-bd37-232fc6f475af@gmail.com/ > > Here are three patches that implement the comparison between > normalized path and configuration value. > > Imagine that you have a repository at /mnt/disk4/repos/frotz > directory but in order to make it simpler to manage and use, you > have your users use /projects/frotz to access the repository. A > symlink /projects/frotz pointing at /mnt/disk4/repos/frotz directory > allows you to do so. > > - The first patch normalizes the path to the directory that we > suspect is a usable repository, before comparing it with the > safe.directory configuration variable. The safe.directory may > say /mnt/disk4/repos/frotz or /mnt/disk4/repos/*, but the path to > the repository for the users may be /mnt/disk4/repos/frotz or > /projects/frotz, depending on where they come from and what their > $PWD makes getcwd() to say. > > - The second patch normalizes the value of the safe.directory > variable. This allows safe.directory to say /projects/frotz > or /projects/* and have them match /mnt/disk4/repos/frotz (which > is how the first patch normalizes the repository path to). > > - The third patch only adds a test to illustrate what happens when > the safe.directory configuration is set to ".", which was a > plausible setting for those who run "git daemon". The > normalization done by the first two patches is sufficient to make > this just work. > > Junio C Hamano (3): > safe.directory: normalize the checked path > safe.directory: normalize the configured path > safe.directory: setting safe.directory="." allows the "current" > directory > > setup.c | 28 ++++++-- > t/t0033-safe-directory.sh | 146 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 170 insertions(+), 4 deletions(-) > > Range-diff against v1: > 1: 20e1160946 ! 1: 86d5c83ee7 safe.directory: normalize the checked path > @@ t/t0033-safe-directory.sh: test_expect_success 'local clone of unowned repo acce > + sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER && > + git config --global safe.directory "$(pwd)/repository" > + ) && > -+ git -C repository/ for-each-ref >/dev/null && > -+ git -C repo/ for-each-ref > ++ git -C repository for-each-ref && > ++ git -C repository/ for-each-ref && > ++ git -C repo for-each-ref && > ++ git -C repo/ for-each-ref && > ++ test_must_fail git -C repository/.git for-each-ref && > ++ test_must_fail git -C repository/.git/ for-each-ref && > ++ test_must_fail git -C repo/.git for-each-ref && > ++ test_must_fail git -C repo/.git/ for-each-ref > +' > + > +test_expect_success SYMLINKS 'checked leading paths are normalized' ' > @@ t/t0033-safe-directory.sh: test_expect_success 'local clone of unowned repo acce > + sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER && > + git config --global safe.directory "$(pwd)/repository/*" > + ) && > -+ git -C repository/s for-each-ref >/dev/null && > -+ git -C repo/s for-each-ref > ++ git -C repository/s for-each-ref && > ++ git -C repository/s/ for-each-ref && > ++ git -C repo/s for-each-ref && > ++ git -C repo/s/ for-each-ref && > ++ git -C repository/s/.git for-each-ref && > ++ git -C repository/s/.git/ for-each-ref && > ++ git -C repo/s/.git for-each-ref && > ++ git -C repo/s/.git/ for-each-ref > +' > + > test_done > 2: 06de9038b7 ! 2: c4998076da safe.directory: normalize the configured path > @@ setup.c: static int safe_directory_cb(const char *key, const char *value, > > ## t/t0033-safe-directory.sh ## > @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths are normalized' ' > - git -C repo/s for-each-ref > + git -C repo/s/.git/ for-each-ref > ' > > +test_expect_success SYMLINKS 'configured paths are normalized' ' > @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a > + sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER && > + git config --global safe.directory "$(pwd)/repo" > + ) && > -+ git -C repository/ for-each-ref >/dev/null && > -+ git -C repo/ for-each-ref > ++ git -C repository for-each-ref && > ++ git -C repository/ for-each-ref && > ++ git -C repo for-each-ref && > ++ git -C repo/ for-each-ref && > ++ test_must_fail git -C repository/.git for-each-ref && > ++ test_must_fail git -C repository/.git/ for-each-ref && > ++ test_must_fail git -C repo/.git for-each-ref && > ++ test_must_fail git -C repo/.git/ for-each-ref > +' > + > +test_expect_success SYMLINKS 'configured leading paths are normalized' ' > @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a > + git init repository/s && > + ln -s repository repo && > + ( > -+ cd repository && > ++ cd repository/s && > + sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER && > + test_commit sample > + ) && > @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a > + sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER && > + git config --global safe.directory "$(pwd)/repo/*" > + ) && > -+ git -C repository/s for-each-ref >/dev/null && > -+ git -C repo/s for-each-ref > ++ git -C repository/s for-each-ref && > ++ git -C repository/s/ for-each-ref && > ++ git -C repository/s/.git for-each-ref && > ++ git -C repository/s/.git/ for-each-ref && > ++ git -C repo/s for-each-ref && > ++ git -C repo/s/ for-each-ref && > ++ git -C repo/s/.git for-each-ref && > ++ git -C repo/s/.git/ for-each-ref > +' > + > test_done > -: ---------- > 3: ffc3f7364e safe.directory: setting safe.directory="." allows the "current" directory
Phillip Wood <phillip.wood123@gmail.com> writes: > Thanks for picking this up, I think this looks like a good approach > apart from resolving relative entries in safe.directory which is > intrinsically unsafe as we don't know which directory the user wants > to consider safe. With these changes it should not be necessary to add > "." to safe.directory to get "git daemon" to work and all the other > code paths use an absolute path from getcwd() so I don't think there > is any need to support relative directories. I agree that we could limit to absolute, but that would mean we would be breaking a configuration that used to work. I do not want to mix that into this topic. > I'll be off the list for the next couple of weeks I'd imagine it would be for a block of fun time? Enjoy. In any case, this topic won't be moving in the coming few weeks anyway ;-)
Changes since v1: - two "extra" patches for optimization that are unnecessary have been dropped. - a patch to add test for safe.directory="." use case has been added. Recently we discussed what we should do when either the path configured in the safe.directory configuration or coming from the caller of ensure_valid_ownership() function as a result of repository discovery is not normalized and textual equality check is not sufficient. See the thread the contains https://lore.kernel.org/git/6d5b75a6-639d-429b-bd37-232fc6f475af@gmail.com/ Here are three patches that implement the comparison between normalized path and configuration value. Imagine that you have a repository at /mnt/disk4/repos/frotz directory but in order to make it simpler to manage and use, you have your users use /projects/frotz to access the repository. A symlink /projects/frotz pointing at /mnt/disk4/repos/frotz directory allows you to do so. - The first patch normalizes the path to the directory that we suspect is a usable repository, before comparing it with the safe.directory configuration variable. The safe.directory may say /mnt/disk4/repos/frotz or /mnt/disk4/repos/*, but the path to the repository for the users may be /mnt/disk4/repos/frotz or /projects/frotz, depending on where they come from and what their $PWD makes getcwd() to say. - The second patch normalizes the value of the safe.directory variable. This allows safe.directory to say /projects/frotz or /projects/* and have them match /mnt/disk4/repos/frotz (which is how the first patch normalizes the repository path to). - The third patch only adds a test to illustrate what happens when the safe.directory configuration is set to ".", which was a plausible setting for those who run "git daemon". The normalization done by the first two patches is sufficient to make this just work. Junio C Hamano (3): safe.directory: normalize the checked path safe.directory: normalize the configured path safe.directory: setting safe.directory="." allows the "current" directory setup.c | 28 ++++++-- t/t0033-safe-directory.sh | 146 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 170 insertions(+), 4 deletions(-) Range-diff against v1: 1: 20e1160946 ! 1: 86d5c83ee7 safe.directory: normalize the checked path @@ t/t0033-safe-directory.sh: test_expect_success 'local clone of unowned repo acce + sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER && + git config --global safe.directory "$(pwd)/repository" + ) && -+ git -C repository/ for-each-ref >/dev/null && -+ git -C repo/ for-each-ref ++ git -C repository for-each-ref && ++ git -C repository/ for-each-ref && ++ git -C repo for-each-ref && ++ git -C repo/ for-each-ref && ++ test_must_fail git -C repository/.git for-each-ref && ++ test_must_fail git -C repository/.git/ for-each-ref && ++ test_must_fail git -C repo/.git for-each-ref && ++ test_must_fail git -C repo/.git/ for-each-ref +' + +test_expect_success SYMLINKS 'checked leading paths are normalized' ' @@ t/t0033-safe-directory.sh: test_expect_success 'local clone of unowned repo acce + sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER && + git config --global safe.directory "$(pwd)/repository/*" + ) && -+ git -C repository/s for-each-ref >/dev/null && -+ git -C repo/s for-each-ref ++ git -C repository/s for-each-ref && ++ git -C repository/s/ for-each-ref && ++ git -C repo/s for-each-ref && ++ git -C repo/s/ for-each-ref && ++ git -C repository/s/.git for-each-ref && ++ git -C repository/s/.git/ for-each-ref && ++ git -C repo/s/.git for-each-ref && ++ git -C repo/s/.git/ for-each-ref +' + test_done 2: 06de9038b7 ! 2: c4998076da safe.directory: normalize the configured path @@ setup.c: static int safe_directory_cb(const char *key, const char *value, ## t/t0033-safe-directory.sh ## @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths are normalized' ' - git -C repo/s for-each-ref + git -C repo/s/.git/ for-each-ref ' +test_expect_success SYMLINKS 'configured paths are normalized' ' @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a + sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER && + git config --global safe.directory "$(pwd)/repo" + ) && -+ git -C repository/ for-each-ref >/dev/null && -+ git -C repo/ for-each-ref ++ git -C repository for-each-ref && ++ git -C repository/ for-each-ref && ++ git -C repo for-each-ref && ++ git -C repo/ for-each-ref && ++ test_must_fail git -C repository/.git for-each-ref && ++ test_must_fail git -C repository/.git/ for-each-ref && ++ test_must_fail git -C repo/.git for-each-ref && ++ test_must_fail git -C repo/.git/ for-each-ref +' + +test_expect_success SYMLINKS 'configured leading paths are normalized' ' @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a + git init repository/s && + ln -s repository repo && + ( -+ cd repository && ++ cd repository/s && + sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER && + test_commit sample + ) && @@ t/t0033-safe-directory.sh: test_expect_success SYMLINKS 'checked leading paths a + sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER && + git config --global safe.directory "$(pwd)/repo/*" + ) && -+ git -C repository/s for-each-ref >/dev/null && -+ git -C repo/s for-each-ref ++ git -C repository/s for-each-ref && ++ git -C repository/s/ for-each-ref && ++ git -C repository/s/.git for-each-ref && ++ git -C repository/s/.git/ for-each-ref && ++ git -C repo/s for-each-ref && ++ git -C repo/s/ for-each-ref && ++ git -C repo/s/.git for-each-ref && ++ git -C repo/s/.git/ for-each-ref +' + test_done -: ---------- > 3: ffc3f7364e safe.directory: setting safe.directory="." allows the "current" directory