| Message ID | pull.1286.v2.git.1659965270.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Some improvements to safe.directory on Windows | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > Due to the semantics being substantially different from Unix, the > safe.directory feature presents its own set of problems on Windows. One > particular issue would have prevented it from working in GitHub Actions' > build agents, which we definitely rely on in the Git project itself. This > was addressed via the fifth patch, which had made it (in a slightly > different form) already into Git for Windows v2.35.2, and they are ready to > be applied to core Git, too. > > The FAT32 patch came in later, and was released as part of Git for Windows > v2.37.0, so I also have confidence that it is stable and ready to be > integrated into core Git, too. > > Changes since v1: > > * Restructured the patch series. > * Instead of an environment variable to turn on debugging, we now always > show the platform-dependent information together with the error message > about the dubious ownership (iff it is shown, that is), based on an idea > by Junio. > * Rebased onto gc/bare-repo-discovery to avoid a merge conflict. I actually had to rebase it back so that we could merge it to 'maint' for further 2.37.x releases. I'll refer to the original patches in this thread when I merge the result to 'seen', of course, to make sure the results do match. It would have been slightly less convenient if you did not do this rebase, but it would have allowed me to have much better confidence in the result that may eventually go to 'maint'. After all, mistakes in resolving merge conflicts on 'seen' can be corrected before the topic hits 'next'. Thanks. I do not know about the API calls mingw.c part of these patches make, but the overall structure looks sensible to me.
Hi Junio, On Mon, 8 Aug 2022, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > Due to the semantics being substantially different from Unix, the > > safe.directory feature presents its own set of problems on Windows. One > > particular issue would have prevented it from working in GitHub Actions' > > build agents, which we definitely rely on in the Git project itself. This > > was addressed via the fifth patch, which had made it (in a slightly > > different form) already into Git for Windows v2.35.2, and they are ready to > > be applied to core Git, too. > > > > The FAT32 patch came in later, and was released as part of Git for Windows > > v2.37.0, so I also have confidence that it is stable and ready to be > > integrated into core Git, too. > > > > Changes since v1: > > > > * Restructured the patch series. > > * Instead of an environment variable to turn on debugging, we now always > > show the platform-dependent information together with the error message > > about the dubious ownership (iff it is shown, that is), based on an idea > > by Junio. > > * Rebased onto gc/bare-repo-discovery to avoid a merge conflict. > > I actually had to rebase it back so that we could merge it to > 'maint' for further 2.37.x releases. I appreciate the effort you put into it, even if it is not your responsibility to take care of Git for Windows' releases. The range-diff shows that you snuck in a commit message change that I would have either not made at all (I think the original was fine) or would have made differently (because Access Control Lists are not specific to Windows, even if Windows is the most obvious example for this permission model): -- snip -- 1: 301d94f18f5 ! 1: d51e1dff980 setup: fix some formatting @@ Commit message the indentation before actually modifying the code. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> + Signed-off-by: Junio C Hamano <gitster@pobox.com> ## setup.c ## @@ setup.c: static int safe_directory_cb(const char *key, const char *value, void *d) 2: 8cc45e4922a ! 2: 17d3883fe9c Prepare for more detailed "dubious ownership" messages @@ Metadata Author: Johannes Schindelin <Johannes.Schindelin@gmx.de> ## Commit message ## - Prepare for more detailed "dubious ownership" messages + setup: prepare for more detailed "dubious ownership" messages When verifying the ownership of the Git directory, we sometimes would like to say a bit more about it, e.g. when using a platform-dependent - code path (think: Windows and the permission model that is so different + code path (think: Windows has the permission model that is so different from Unix'), but only when it is a appropriate to actually say something. @@ Commit message Based-on-an-idea-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> + Signed-off-by: Junio C Hamano <gitster@pobox.com> ## compat/mingw.c ## @@ compat/mingw.c: static PSID get_current_user_sid(void) @@ setup.c: static enum discovery_result setup_git_directory_gently_1(struct strbuf ret = GIT_DIR_DISCOVERED; } else @@ setup.c: static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir, + } + if (is_git_directory(dir->buf)) { - if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT) - return GIT_DIR_DISALLOWED_BARE; - if (!ensure_valid_ownership(NULL, NULL, dir->buf)) + if (!ensure_valid_ownership(NULL, NULL, dir->buf, report)) return GIT_DIR_INVALID_OWNERSHIP; 3: 63494818105 ! 3: e883e04b68b mingw: provide details about unsafe directories' ownership @@ Commit message Let's help with that by providing more detailed information. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> + Signed-off-by: Junio C Hamano <gitster@pobox.com> ## compat/mingw.c ## @@ 4: 7aaa6248dfe ! 4: 7c83470e64e mingw: be more informative when ownership check fails on FAT32 @@ Commit message This addresses https://github.com/git-for-windows/git/issues/3886 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> + Signed-off-by: Junio C Hamano <gitster@pobox.com> ## compat/mingw.c ## @@ compat/mingw.c: static PSID get_current_user_sid(void) 5: fbfaff2ec21 ! 5: 3f7207e2ea9 mingw: handle a file owned by the Administrators group correctly @@ Commit message Administrators Group as if it were owned by said user. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> + Signed-off-by: Junio C Hamano <gitster@pobox.com> ## compat/mingw.c ## @@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path, struct strbuf *report) -- snap -- Retitling the commit message was okay, of course, using the `setup:` prefix. > I'll refer to the original patches in this thread when I merge the > result to 'seen', of course, to make sure the results do match. It > would have been slightly less convenient if you did not do this rebase, > but it would have allowed me to have much better confidence in the > result that may eventually go to 'maint'. After all, mistakes in > resolving merge conflicts on 'seen' can be corrected before the topic > hits 'next'. Yes, mistakes can happen, and the more people work together the easier it is to avoid or fix them. > Thanks. I do not know about the API calls mingw.c part of these > patches make, but the overall structure looks sensible to me. I do not think that anybody expected you to know about Win32 API calls ;-) Ciao, Dscho
Due to the semantics being substantially different from Unix, the safe.directory feature presents its own set of problems on Windows. One particular issue would have prevented it from working in GitHub Actions' build agents, which we definitely rely on in the Git project itself. This was addressed via the fifth patch, which had made it (in a slightly different form) already into Git for Windows v2.35.2, and they are ready to be applied to core Git, too. The FAT32 patch came in later, and was released as part of Git for Windows v2.37.0, so I also have confidence that it is stable and ready to be integrated into core Git, too. Changes since v1: * Restructured the patch series. * Instead of an environment variable to turn on debugging, we now always show the platform-dependent information together with the error message about the dubious ownership (iff it is shown, that is), based on an idea by Junio. * Rebased onto gc/bare-repo-discovery to avoid a merge conflict. Johannes Schindelin (5): setup: fix some formatting Prepare for more detailed "dubious ownership" messages mingw: provide details about unsafe directories' ownership mingw: be more informative when ownership check fails on FAT32 mingw: handle a file owned by the Administrators group correctly compat/mingw.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++- compat/mingw.h | 2 +- git-compat-util.h | 5 +++- setup.c | 30 ++++++++++++++---------- 4 files changed, 81 insertions(+), 15 deletions(-) base-commit: 776f184893d2861a729aa4b91d69931036e03e4b Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1286%2Fdscho%2Fsafe.directory-and-windows-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1286/dscho/safe.directory-and-windows-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/1286 Range-diff vs v1: -: ----------- > 1: 301d94f18f5 setup: fix some formatting -: ----------- > 2: 8cc45e4922a Prepare for more detailed "dubious ownership" messages 1: 3480381b8b9 ! 3: 63494818105 Allow debugging unsafe directories' ownership @@ Metadata Author: Johannes Schindelin <Johannes.Schindelin@gmx.de> ## Commit message ## - Allow debugging unsafe directories' ownership + mingw: provide details about unsafe directories' ownership When Git refuses to use an existing repository because it is owned by someone else than the current user, it can be a bit tricky on Windows to figure out what is going on. - Let's help with that by offering some more information via the - environment variable `GIT_TEST_DEBUG_UNSAFE_DIRECTORIES`. + Let's help with that by providing more detailed information. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> - ## Documentation/config/safe.txt ## -@@ Documentation/config/safe.txt: which id the original user has. - If that is not what you would prefer and want git to only trust - repositories that are owned by root instead, then you can remove - the `SUDO_UID` variable from root's environment before invoking git. -++ -+Due to the permission model on Windows where ACLs are used instead of -+Unix' simpler permission model, it can be a bit tricky to figure out why -+a directory is considered unsafe. To help with this, Git will provide -+more detailed information when the environment variable -+`GIT_TEST_DEBUG_UNSAFE_DIRECTORIES` is set to `true`. - ## compat/mingw.c ## @@ #include "../git-compat-util.h" @@ compat/mingw.c #include <conio.h> #include <wchar.h> #include "../strbuf.h" -@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path) +@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path, struct strbuf *report) IsValidSid(current_user_sid) && EqualSid(sid, current_user_sid)) result = 1; -+ else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) { ++ else if (report) { + LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL; + + if (ConvertSidToStringSidA(sid, &str1)) @@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path) + to_free2 = str2; + else + str2 = "(inconvertible)"; -+ warning("'%s' is owned by:\n\t'%s'\nbut the current user is:\n\t'%s'", path, str1, str2); ++ strbuf_addf(report, ++ "'%s' is owned by:\n" ++ "\t'%s'\nbut the current user is:\n" ++ "\t'%s'\n", path, str1, str2); + LocalFree(to_free1); + LocalFree(to_free2); + } } /* - - ## setup.c ## -@@ setup.c: const char *setup_git_directory_gently(int *nongit_ok) - case GIT_DIR_INVALID_OWNERSHIP: - if (!nongit_ok) { - struct strbuf quoted = STRBUF_INIT; -+ struct strbuf hint = STRBUF_INIT; -+ -+#ifdef __MINGW32__ -+ if (!git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) -+ strbuf_addstr(&hint, -+ _("\n\nSet the environment variable " -+ "GIT_TEST_DEBUG_UNSAFE_DIRECTORIES=true " -+ "and run\n" -+ "again for more information.")); -+#endif - - sq_quote_buf_pretty("ed, dir.buf); - die(_("detected dubious ownership in repository at '%s'\n" - "To add an exception for this directory, call:\n" - "\n" -- "\tgit config --global --add safe.directory %s"), -- dir.buf, quoted.buf); -+ "\tgit config --global --add safe.directory %s%s"), -+ dir.buf, quoted.buf, hint.buf); - } - *nongit_ok = 1; - break; 3: dae03f1b204 ! 4: 7aaa6248dfe mingw: be more informative when ownership check fails on FAT32 @@ Commit message any ownership information anyway, and the `GetNamedSecurityInfoW()` call pretends that everything is owned "by the world". - Let's special-case that scenario and tell the user what's going on, at - least when they set `GIT_TEST_DEBUG_UNSAFE_DIRECTORIES`. + Let's special-case that scenario and tell the user what's going on. This addresses https://github.com/git-for-windows/git/issues/3886 @@ compat/mingw.c: static PSID get_current_user_sid(void) + return 0; +} + - int is_path_owned_by_current_sid(const char *path) + int is_path_owned_by_current_sid(const char *path, struct strbuf *report) { WCHAR wpath[MAX_PATH]; -@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path) - * okay, too. - */ +@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path, struct strbuf *report) + IsValidSid(current_user_sid) && + EqualSid(sid, current_user_sid)) result = 1; -- else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) { -+ else if (IsWellKnownSid(sid, WinWorldSid) && -+ git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0) && +- else if (report) { ++ else if (report && ++ IsWellKnownSid(sid, WinWorldSid) && + !acls_supported(path)) { + /* + * On FAT32 volumes, ownership is not actually recorded. + */ -+ warning("'%s' is on a file system that does not record ownership", path); -+ } else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) { ++ strbuf_addf(report, "'%s' is on a file system that does" ++ "not record ownership\n", path); ++ } else if (report) { LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL; if (ConvertSidToStringSidA(sid, &str1)) 2: be06d711a13 ! 5: fbfaff2ec21 mingw: handle a file owned by the Administrators group correctly @@ Commit message Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> ## compat/mingw.c ## -@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path) +@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path, struct strbuf *report) else if (sid && IsValidSid(sid)) { /* Now, verify that the SID matches the current user's */ static PSID current_user_sid; @@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path) if (!current_user_sid) current_user_sid = get_current_user_sid(); -@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path) +@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path, struct strbuf *report) IsValidSid(current_user_sid) && EqualSid(sid, current_user_sid)) result = 1; @@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path) + * okay, too. + */ + result = 1; - else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) { - LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL; - + else if (report && + IsWellKnownSid(sid, WinWorldSid) && + !acls_supported(path)) {