Skip to content

git-sec: adopt git-for-windows exception rules #426

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

Merged
merged 1 commit into from
May 23, 2022

Conversation

davidkna
Copy link
Contributor

This PR implements the changes proposed in #424:

Git-for-Windows has some exceptions for the ownership checks. The home directory is accepted unconditionally (which is owned by admin instead of the user), and admin-group-owned folders are accepted if the user is an admin (helpful in environments like Github-Actions).

Git-for-Windows references:
https://github.com/git-for-windows/git/blob/0cd1496fc3dd78dadbdf3644ec4d384188573e6f/compat/mingw.c#L3503-L3515

https://github.com/git-for-windows/git/blob/0cd1496fc3dd78dadbdf3644ec4d384188573e6f/compat/mingw.c#L3535-L3543

I couldn't make CheckTokenMembership work with the token/handle already being opened, so I instead I made it forward a null value to have the Windows API handle the details. As part of that attempt, I also made the code prefer using a thread token instead of a process token, which seems to be preferred, and added a CloseHandle call for the old handle.

Byron added a commit that referenced this pull request May 23, 2022
- remove unused `Options` type
- use `expected_trust()` function consistently
Byron added a commit that referenced this pull request May 23, 2022
…#426)

As is the case with my VM setup that can't get full trust while shared
and it's not a great thing to get used to a certain set of tests to
fail.
Byron added a commit that referenced this pull request May 23, 2022
In 'git', the passed path for ownership check probably is absolute
for reasons, but that's not an assumption we can make in `gitoxide`.

Hence we resolve the path fully before comparing one path with another.
Without this, using `gitoxide` powered applications from the current
working dir set to the home (probably) wouldn't work.
@Byron Byron merged commit 136eb37 into GitoxideLabs:main May 23, 2022
@Byron
Copy link
Member

Byron commented May 23, 2022

Thanks a million! This looks good to me, but then again, I am not an authority on how to do these things. The group membership check was already implemented at some point and made by a Microsoft employee, hence it probably is the authoritative version: 9a3f0ba#diff-71eaf9b8d0991458e6650383da1d69a99dff38608f29fa716c03e67b9388c0ffR70 . If you find anything there to remove the TODO, you could do so in another PR.

Thanks for all your help with this, you are definitely pushing gitoxide along considerably!

@davidkna
Copy link
Contributor Author

Thanks to you too! It looks like they went with the same approach as me for the membership test.

Regarding 85ab096, to make ownership checks work with networks drives, mounting the network drive as a single-letter drive has worked for me in the past.

@davidkna davidkna deleted the admin-sec branch May 23, 2022 13:34
@Byron
Copy link
Member

Byron commented May 24, 2022

Thanks for the great suggestion! I tried it (actually the mount was already there as drive Z, I just didn't see it), but the same issue ensues. I am actually happy about it as it would be strange to have different permissions depending on the way one reaches the very same resource. Maybe there is more to mapping a drive though, after all this one was pre-created. For now it seems OK to keep this test-only environment variable switch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants