-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Some improvements to safe.directory
on Windows
#1286
Conversation
/submit |
Submitted as pull.1286.git.1657700238.gitgitgadget@gmail.com To fetch this version into
To fetch this version to local tag
|
Documentation/config/safe.txt
Outdated
@@ -40,3 +40,9 @@ which id the original user has. | |||
If that is not what you would prefer and want git to only trust |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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`.
I can see how the change to compat/ part is useful, but ...
> diff --git a/setup.c b/setup.c
> index 9dcecda65b0..3ba42ffcb27 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1353,13 +1353,23 @@ 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
... I am not sure about this part. Do we have any other codepath to
show "to debug, run the program with this" suggestion? Adding it in
the documentation is probably good, but this is an extra message
that is much larger than the "owned by X but you are Y" message that
would be shown. With or without the environment set, the output
will become noisier with this patch. I wonder if we are better off
giving the information that is given in the warning (in compat/ part
of the patch) _unconditionally_ in the message, which would make it
less noisy overall.
Also what's our vision for non-Windows platform for "further
debugging aid" in this area? Would we perhaps want a similar
"warning" that says "owned by X but you are Y"?
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Junio C Hamano <gitster@pobox.com> writes:
> ... I am not sure about this part. Do we have any other codepath to
> show "to debug, run the program with this" suggestion? Adding it in
> the documentation is probably good, but this is an extra message
> that is much larger than the "owned by X but you are Y" message that
> would be shown. With or without the environment set, the output
> will become noisier with this patch. I wonder if we are better off
> giving the information that is given in the warning (in compat/ part
> of the patch) _unconditionally_ in the message, which would make it
> less noisy overall.
I am wondering if passing a struct to allow is_path_owned_by*()
helper(s) to report the detail of the failures they discover a
cleaner way to do this. To illustrate what I meant, along the
lines of the following patch, with any additional code to actually
stuff messages in the strbuf report, in the is_path_owned_by*()
implementation.
I am perfectly OK if we make it a debug-only option by hiding this
behind an environment variable, but if we were to do so, I do not
want to see us advertize the environment variable in the die()
message (a debug-only option can be documented, but not worth
surfacing in and contaminating the usual UI output).
Comments?
compat/mingw.c | 2 +-
git-compat-util.h | 3 ++-
setup.c | 12 +++++++++---
3 files changed, 12 insertions(+), 5 deletions(-)
diff --git c/compat/mingw.c w/compat/mingw.c
index 2607de93af..f12b7df16d 100644
--- c/compat/mingw.c
+++ w/compat/mingw.c
@@ -2673,7 +2673,7 @@ static PSID get_current_user_sid(void)
return result;
}
-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];
PSID sid = NULL;
diff --git c/git-compat-util.h w/git-compat-util.h
index 58d7708296..de34b0ea7e 100644
--- c/git-compat-util.h
+++ w/git-compat-util.h
@@ -487,7 +487,8 @@ static inline void extract_id_from_env(const char *env, uid_t *id)
}
}
-static inline int is_path_owned_by_current_uid(const char *path)
+struct strbuf;
+static inline int is_path_owned_by_current_uid(const char *path, struct strbuf *report)
{
struct stat st;
uid_t euid;
diff --git c/setup.c w/setup.c
index 09b6549ba9..ed823585f7 100644
--- c/setup.c
+++ w/setup.c
@@ -1143,12 +1143,18 @@ static int ensure_valid_ownership(const char *gitfile,
struct safe_directory_data data = {
.path = worktree ? worktree : gitdir
};
+ struct strbuf report = STRBUF_INIT;
if (!git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0) &&
- (!gitfile || is_path_owned_by_current_user(gitfile)) &&
- (!worktree || is_path_owned_by_current_user(worktree)) &&
- (!gitdir || is_path_owned_by_current_user(gitdir)))
+ (!gitfile || is_path_owned_by_current_user(gitfile, &report)) &&
+ (!worktree || is_path_owned_by_current_user(worktree, &report)) &&
+ (!gitdir || is_path_owned_by_current_user(gitdir, &report))) {
+ if (report.len) {
+ fputs(report.buf, stderr);
+ strbuf_release(&report);
+ }
return 1;
+ }
/*
* data.path is the "path" that identifies the repository and it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Junio,
On Thu, 14 Jul 2022, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > ... I am not sure about this part. Do we have any other codepath to
> > show "to debug, run the program with this" suggestion? Adding it in
> > the documentation is probably good, but this is an extra message
> > that is much larger than the "owned by X but you are Y" message that
> > would be shown. With or without the environment set, the output
> > will become noisier with this patch. I wonder if we are better off
> > giving the information that is given in the warning (in compat/ part
> > of the patch) _unconditionally_ in the message, which would make it
> > less noisy overall.
>
> I am wondering if passing a struct to allow is_path_owned_by*()
> helper(s) to report the detail of the failures they discover a
> cleaner way to do this. To illustrate what I meant, along the
> lines of the following patch, with any additional code to actually
> stuff messages in the strbuf report, in the is_path_owned_by*()
> implementation.
I like it! Let me play with this, after this coming week (during which I
plan to be mostly offline).
Thanks,
Dscho
>
> I am perfectly OK if we make it a debug-only option by hiding this
> behind an environment variable, but if we were to do so, I do not
> want to see us advertize the environment variable in the die()
> message (a debug-only option can be documented, but not worth
> surfacing in and contaminating the usual UI output).
>
> Comments?
>
> compat/mingw.c | 2 +-
> git-compat-util.h | 3 ++-
> setup.c | 12 +++++++++---
> 3 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git c/compat/mingw.c w/compat/mingw.c
> index 2607de93af..f12b7df16d 100644
> --- c/compat/mingw.c
> +++ w/compat/mingw.c
> @@ -2673,7 +2673,7 @@ static PSID get_current_user_sid(void)
> return result;
> }
>
> -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];
> PSID sid = NULL;
> diff --git c/git-compat-util.h w/git-compat-util.h
> index 58d7708296..de34b0ea7e 100644
> --- c/git-compat-util.h
> +++ w/git-compat-util.h
> @@ -487,7 +487,8 @@ static inline void extract_id_from_env(const char *env, uid_t *id)
> }
> }
>
> -static inline int is_path_owned_by_current_uid(const char *path)
> +struct strbuf;
> +static inline int is_path_owned_by_current_uid(const char *path, struct strbuf *report)
> {
> struct stat st;
> uid_t euid;
> diff --git c/setup.c w/setup.c
> index 09b6549ba9..ed823585f7 100644
> --- c/setup.c
> +++ w/setup.c
> @@ -1143,12 +1143,18 @@ static int ensure_valid_ownership(const char *gitfile,
> struct safe_directory_data data = {
> .path = worktree ? worktree : gitdir
> };
> + struct strbuf report = STRBUF_INIT;
>
> if (!git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0) &&
> - (!gitfile || is_path_owned_by_current_user(gitfile)) &&
> - (!worktree || is_path_owned_by_current_user(worktree)) &&
> - (!gitdir || is_path_owned_by_current_user(gitdir)))
> + (!gitfile || is_path_owned_by_current_user(gitfile, &report)) &&
> + (!worktree || is_path_owned_by_current_user(worktree, &report)) &&
> + (!gitdir || is_path_owned_by_current_user(gitdir, &report))) {
> + if (report.len) {
> + fputs(report.buf, stderr);
> + strbuf_release(&report);
> + }
> return 1;
> + }
>
> /*
> * data.path is the "path" that identifies the repository and it is
>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Junio,
On Fri, 15 Jul 2022, Johannes Schindelin wrote:
> On Thu, 14 Jul 2022, Junio C Hamano wrote:
>
> > Junio C Hamano <gitster@pobox.com> writes:
> >
> > > ... I am not sure about this part. Do we have any other codepath to
> > > show "to debug, run the program with this" suggestion? Adding it in
> > > the documentation is probably good, but this is an extra message
> > > that is much larger than the "owned by X but you are Y" message that
> > > would be shown. With or without the environment set, the output
> > > will become noisier with this patch. I wonder if we are better off
> > > giving the information that is given in the warning (in compat/ part
> > > of the patch) _unconditionally_ in the message, which would make it
> > > less noisy overall.
> >
> > I am wondering if passing a struct to allow is_path_owned_by*()
> > helper(s) to report the detail of the failures they discover a
> > cleaner way to do this. To illustrate what I meant, along the
> > lines of the following patch, with any additional code to actually
> > stuff messages in the strbuf report, in the is_path_owned_by*()
> > implementation.
>
> I like it! Let me play with this, after this coming week (during which I
> plan to be mostly offline).
I had to play with it for a while until I could make it work, but
eventually I managed to do it. The second iteration was just sent, and
implements this route.
Thank you,
Dscho
This branch is now known as |
This patch series was integrated into seen via git@91d08e9. |
There was a status update in the "New Topics" section about the branch Needs review. source: <pull.1286.git.1657700238.gitgitgadget@gmail.com> |
This patch series was integrated into seen via git@e74df35. |
This patch series was integrated into seen via git@3b8697f. |
This patch series was integrated into seen via git@fc99667. |
This patch series was integrated into seen via git@c1960c8. |
This patch series was integrated into seen via git@12790ab. |
This patch series was integrated into seen via git@e391ae3. |
This patch series was integrated into seen via git@6e07fc6. |
There was a status update in the "Cooking" section about the branch Needs review. source: <pull.1286.git.1657700238.gitgitgadget@gmail.com> |
This patch series was integrated into seen via git@bf09132. |
This patch series was integrated into seen via git@c36ac8b. |
This patch series was integrated into seen via git@b81a9ba. |
This patch series was integrated into seen via git@39c3fa4. |
This patch series was integrated into seen via git@9f3fa5c. |
This patch series was integrated into seen via git@6bab7f1. |
This patch series was integrated into seen via git@28fb1b2. |
This patch series was integrated into seen via git@fa2f3d0. |
This patch series was integrated into seen via git@608a718. |
This patch series was integrated into seen via git@c930fab. |
This patch series was integrated into seen via git@fd54995. |
There was a status update in the "Cooking" section about the branch Expecting a reroll. cf. <8rqqnqp1-q613-ron6-6q8s-n7sq57o980q9@tzk.qr> source: <pull.1286.git.1657700238.gitgitgadget@gmail.com> |
This patch series was integrated into seen via git@a5adafa. |
There was a status update in the "Cooking" section about the branch Expecting a reroll. cf. <8rqqnqp1-q613-ron6-6q8s-n7sq57o980q9@tzk.qr> source: <pull.1286.git.1657700238.gitgitgadget@gmail.com> |
There was a status update in the "Cooking" section about the branch Expecting a reroll. cf. <8rqqnqp1-q613-ron6-6q8s-n7sq57o980q9@tzk.qr> source: <pull.1286.git.1657700238.gitgitgadget@gmail.com> |
dae03f1
to
fd83ef6
Compare
In preparation for touching code that was introduced in 3b0bf27 (setup: tighten ownership checks post CVE-2022-24765, 2022-05-10) and that was formatted differently than preferred in the Git project, fix the indentation before actually modifying the code. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
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 from Unix'), but only when it is a appropriate to actually say something. To allow for that, collect that information and hand it back to the caller (whose responsibility it is to show it or not). Note: We do not actually fill in any platform-dependent information yet, this commit just adds the infrastructure to be able to do so. Based-on-an-idea-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
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 providing more detailed information. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The FAT file system has no concept of ACLs. Therefore, it cannot store 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. This addresses git-for-windows#3886 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
When an Administrator creates a file or directory, the created file/directory is owned not by the Administrator SID, but by the _Administrators Group_ SID. The reason is that users with administrator privileges usually run in unprivileged ("non-elevated") mode, and their user SID does not change when running in elevated mode. This is is relevant e.g. when running a GitHub workflow on a build agent, which runs in elevated mode: cloning a Git repository in a script step will cause the worktree to be owned by the Administrators Group SID, for example. Let's handle this case as following: if the current user is an administrator, Git should consider a worktree owned by the Administrators Group as if it were owned by said user. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
fd83ef6
to
fbfaff2
Compare
/submit |
Submitted as pull.1286.v2.git.1659965270.gitgitgadget@gmail.com To fetch this version into
To fetch this version to local tag
|
This patch series was integrated into seen via git@e092f84. |
On the Git mailing list, Junio C Hamano wrote (reply to this): "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.
|
This patch series was integrated into seen via git@ab5600e. |
There was a status update in the "Cooking" section about the branch Platform-specific code that determines if a directory is OK to use as a repository has been taught to report more details, especially on Windows. Will merge to 'next'. source: <pull.1286.v2.git.1659965270.gitgitgadget@gmail.com> |
On the Git mailing list, Johannes Schindelin wrote (reply to this): 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 |
This patch series was integrated into seen via git@033df56. |
This patch series was integrated into next via git@3d32a87. |
This patch series was integrated into seen via git@7a4c233. |
This patch series was integrated into seen via git@0d95bba. |
There was a status update in the "Cooking" section about the branch Platform-specific code that determines if a directory is OK to use as a repository has been taught to report more details, especially on Windows. Will merge to 'master'. source: <pull.1286.v2.git.1659965270.gitgitgadget@gmail.com> |
This patch series was integrated into seen via git@d311e76. |
This patch series was integrated into seen via git@5721a52. |
This patch series was integrated into seen via git@7fac7b5. |
This patch series was integrated into master via git@7fac7b5. |
This patch series was integrated into next via git@7fac7b5. |
Closed via 7fac7b5. |
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:
gc/bare-repo-discovery
to avoid a merge conflict.