Add git_helpers.py w/ function for safe directory handling #35

taldcroft · 2023-07-02T20:53:19Z

Description

See https://chandramission.slack.com/archives/G01LN40AXPG/p1688129800711439 for context. See also this blog for more info. Long story short is that git made a security update which requires setting a git config safe.directory in order to do any git operations on a git repo that is not owned by the user running the git command. This would break or greatly slow down the current infrastructure for accessing chandra_models data.

Instead this PR provides a convenient mechanism to restore shared access for select git repos in CXC operations. In practice this is expected to apply only to the chandra_models shared repo. This implies that CXC users trust that the owners of the chandra_models repo to not be placing malicious content in the repo.

If the user config does not permit safe access to the requested chandra_models repo then the user ~/.gitconfig file will be updated (via git config --global) to mark that directory (and only that directory) as safe for access as a non-owner. A warning message to that effect is issued.

On HEAD linux this typically results in adding the following two lines to ~/.gitconfig:

[safe]
	directory = /proj/sot/ska/data/chandra_models

Bonus function `utils.get_owner(path)`

I needed a platform-independent way to get the user name, which landed in ska_helpers.utils. l'm not sure if this is the only or best way, but it appears to work. Interestingly, if I ask for the owner of a VM-shared directory on Windows it returns "Everyone".

Interface impacts

This may update the user's ~/.gitconfig. See above.

Testing

Unit and functional tests

Mac (at 35991b8)
Linux (on HEAD as user aldcroft with ska3-flight 2023.1 activated, otherwise vanilla) (at 35991b8)
Windows (at 21ba07c)

Mac

ska_helpers/tests/test_git_helpers.py::test_make_git_repo_safe SKIPPED (Chandra models dir ownership is OK)                         [ 43%]

Linux HEAD

The relevant unit test ran to completion and passed (indicating it found a chandra_models with a different owner, verified the not safe condition and then fixed it):

ska_helpers/tests/test_git_helpers.py::test_make_git_repo_safe PASSED                                 [100%]

Note that four tests of ska_helpers.chandra_models failed due the git safe problem. To test correcting this I did:

ipython
>>> from ska_helpers.git_helpers import make_git_repo_safe
>>> from ska_helpers import paths
>>> make_git_repo_safe(paths.chandra_models_repo_path())

This produced the expected warning and updated my ~/.gitconfig to add these two lines:

[safe]
	directory = /proj/sot/ska/data/chandra_models

After doing this then the original four failing tests now pass.

Windows

ska_helpers/tests/test_git_helpers.py::test_make_git_repo_safe PASSED                                            [100%]

Independent check of unit tests by Jean

Linux

Functional tests

Ran this on Windows where every VM-shared repo is considered unsafe. For example:

In [6]: from ska_helpers.git_helpers import make_git_repo_safe

In [7]: make_git_repo_safe(r'y:\data\chandra_models')
<ipython-input-7-3979d908222d>:1: UserWarning: Updating git config to allow read-only git operations on trusted repository y:\data\chandra_models. Contact Ska team for questions.
  make_git_repo_safe(r'y:\data\chandra_models')

In [8]: make_git_repo_safe(r'y:\data\chandra_models')
# Second time it is already safe

OSX - JC - I can confirm that after changing the permission on /Users/jean/git/chandra_models that this worked as expected

In [1]: from ska_helpers.git_helpers import make_git_repo_safe

In [2]: make_git_repo_safe('/Users/jean/git/chandra_models')
<ipython-input-2-3a5454f91216>:1: UserWarning: Updating git config to allow read-only git operations on trusted repository /Users/jean/git/chandra_models. Contact Ska team for questions.
  make_git_repo_safe('/Users/jean/git/chandra_models')

And that the correct safe directory entry was added to my .gitconfig.

taldcroft · 2023-07-03T12:45:43Z

As for a unit test, there appears to be no way to change the configuration directories that are read by git.

The only thing I can think of is a test only on HEAD which temporarily moves (in a context manager) the user config ~/.gitconfig off to the side. Then running make_git_repo_safe("/proj/sot/ska/data/chandra_models") by any user but aca should generate a new ~/.gitconfig with the expected safe directory setting.

javierggt · 2023-07-05T16:48:37Z

an alternative is to change the HOME variable within the context, instead of moving the user's .gitconfig

jeanconn · 2023-07-05T16:51:15Z

Regarding "As for a unit test, there appears to be no way to change the configuration directories that are read by git", I thought the GIT_CONFIG_SYSTEM and GIT_CONFIG_something (GLOBAL) could be used, but I didn't explore that after we decided not to use use them for a solution.

jeanconn · 2023-07-05T17:30:46Z

I think this makes sense, so the boxes just need to get done and remove from WIP status and then ask for another review.

ska_helpers/git_helpers.py

javierggt · 2023-07-05T18:41:20Z

The following test works for me (using the match string from the previous comment). It works only on HEAD, so the test stills needs some changes.

import pytest
import tempfile
from ska_helpers import git_helpers
import git
import os


# @pytest.mark.filterwarnings("ignore:Updating git config")
def test_make_git_repo_safe():
    with tempfile.TemporaryDirectory() as d, pytest.warns(
        UserWarning, match="Updating git config"
    ):
        os.environ["HOME"] = d
        repo = git.Repo("/proj/sot/ska/data/chandra_models")
        with pytest.raises(git.exc.GitCommandError):
            repo.is_dirty()
        git_helpers.make_git_repo_safe("/proj/sot/ska/data/chandra_models")
        repo.is_dirty()

ska_helpers/git_helpers.py

ska_helpers/tests/test_git_helpers.py

taldcroft · 2023-07-06T13:10:54Z

Thanks for the updates @javierggt this is looking pretty close! Can you document your unit testing? I'll try to run the tests as well.

ska_helpers/tests/test_git_helpers.py

taldcroft · 2023-07-10T20:04:23Z

Note to @javierggt - os.getlogin() appears to be useless since it returns 'root' for me on Mac. Instead copilot chat told me to use getpass.getuser(), which rang a bell as something I've used before.

jeanconn · 2023-07-10T20:46:00Z

This didn't work for me on OSX but maybe we don't care. I set my chandra_models directory to be owned by the 'nx' user (because it was a user on my machine).

(ska3-matlab-2023.4rc6) spark:git jean$ ls -ld chandra_models
drwxr-xr-x  10 nx  staff  320 Jul 10 15:40 chandra_models
(ska3-matlab-2023.4rc6) spark:git jean$ cd chandra_models/
(ska3-matlab-2023.4rc6) spark:chandra_models jean$ git log
fatal: detected dubious ownership in repository at '/Users/jean/git/chandra_models'
To add an exception for this directory, call:

	git config --global --add safe.directory /Users/jean/git/chandra_models

and then tried to run the drift model test from chandra_aca with this version of ska_helpers in my path. And my git config hasn't been modified and the test failed.

========================================================================================= short test summary info =========================================================================================
FAILED chandra_aca/tests/test_drift.py::test_get_aca_offsets_legacy - git.exc.GitCommandError: Cmd('git') failed due to: exit code(129)
======================================================================== 1 failed, 16 passed, 165 deselected, 8 warnings in 6.11s =========================================================================
(ska3-matlab-2023.4rc6) spark:chandra_aca jean$

But trying to do this on OSX was a pretty contrived test anyway.

jeanconn · 2023-07-10T20:58:46Z

Oh wait... does this PR just add the helper function? If so, seems like we need another PR to actually do the handling.

taldcroft · 2023-07-10T21:26:20Z

Oh wait... does this PR just add the helper function? If so, seems like we need another PR to actually do the handling.

I was coming to this realization. I think in all the discussion this final connection to actually use the new functionality got lost. Do'oh! But maybe that is best left as a separate PR.

jeanconn · 2023-07-10T21:32:45Z

Well, it didn't get lost ... it just sounds like we didn't get around to defining requirements in its evolution from WIP. But that's fine. A new PR to use it is also fine and then functional testing like I tried above should work.

taldcroft · 2023-07-10T21:42:48Z

Being honest in my mind it really did get lost because 2 hours ago (before your comment) I asked myself the question "where is this used now"? There are so many PR's open that I thought it might be in one of the others but finally came to the conclusion that it was nowhere.

taldcroft · 2023-07-10T21:47:21Z

@jeanconn @javierggt - I believe this is ready for final review and testing.

jeanconn

LGTM

Add git_helpers.py w/ function for safe directory handling

960f9e1

taldcroft requested review from javierggt and jeanconn July 2, 2023 20:53

Cache function and add API docs

e22f7f3

javierggt reviewed Jul 5, 2023

View reviewed changes

ska_helpers/git_helpers.py Outdated Show resolved Hide resolved

javierggt added 4 commits July 5, 2023 15:16

change match string in make_git_repo_safe

aa85968

add unit test for make_git_repo_safe

a543ffd

black

7373414

make test_make_git_repo_safe a bit more general

e957b86

jeanconn reviewed Jul 6, 2023

View reviewed changes

ska_helpers/git_helpers.py Show resolved Hide resolved

taldcroft commented Jul 6, 2023

View reviewed changes

ska_helpers/tests/test_git_helpers.py Outdated Show resolved Hide resolved

taldcroft commented Jul 6, 2023

View reviewed changes

ska_helpers/tests/test_git_helpers.py Outdated Show resolved Hide resolved

make sure environment i not changed inside test_make_git_repo_safe

d6703f8

javierggt force-pushed the git-safe-directory branch from cfa31b2 to d6703f8 Compare July 6, 2023 14:07

small edits and docs in test_make_git_repo_safe to make it more readable

7a9c572

javierggt force-pushed the git-safe-directory branch from 9aa30b3 to 7a9c572 Compare July 6, 2023 14:18

taldcroft changed the title ~~WIP: Add git_helpers.py w/ function for safe directory handling~~ Add git_helpers.py w/ function for safe directory handling Jul 10, 2023

javierggt mentioned this pull request Jul 10, 2023

Release ska3-flight 2023.3 sot/skare3#1089

Closed

41 tasks

jeanconn reviewed Jul 10, 2023

View reviewed changes

ska_helpers/tests/test_git_helpers.py Show resolved Hide resolved

Some fixes to the git_helpers test

35991b8

Fix test to work on Windows

21ba07c

taldcroft requested review from jeanconn and javierggt July 10, 2023 21:43

jeanconn approved these changes Jul 10, 2023

View reviewed changes

taldcroft merged commit 637145d into master Jul 10, 2023

taldcroft deleted the git-safe-directory branch July 10, 2023 22:28

This was referenced Jul 11, 2023

Update sot/ska_helpers to 0.10.1 sot/skare3#1117

Closed

Update sot/ska_helpers to 0.10.0 sot/skare3#1118

Closed

2023.3 sot/skare3#1088

Merged

javierggt mentioned this pull request Aug 9, 2023

2023.5 sot/skare3#1130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add git_helpers.py w/ function for safe directory handling #35

Add git_helpers.py w/ function for safe directory handling #35

taldcroft commented Jul 2, 2023 •

edited by jeanconn

Loading

taldcroft commented Jul 3, 2023 •

edited

Loading

javierggt commented Jul 5, 2023

jeanconn commented Jul 5, 2023 •

edited

Loading

jeanconn commented Jul 5, 2023

javierggt commented Jul 5, 2023 •

edited

Loading

taldcroft commented Jul 6, 2023

taldcroft commented Jul 10, 2023

jeanconn commented Jul 10, 2023

jeanconn commented Jul 10, 2023

taldcroft commented Jul 10, 2023

jeanconn commented Jul 10, 2023

taldcroft commented Jul 10, 2023

taldcroft commented Jul 10, 2023

jeanconn left a comment

Add git_helpers.py w/ function for safe directory handling #35

Add git_helpers.py w/ function for safe directory handling #35

Conversation

taldcroft commented Jul 2, 2023 • edited by jeanconn Loading

Description

Bonus function utils.get_owner(path)

Interface impacts

Testing

Unit and functional tests

Mac

Linux HEAD

Windows

Functional tests

taldcroft commented Jul 3, 2023 • edited Loading

javierggt commented Jul 5, 2023

jeanconn commented Jul 5, 2023 • edited Loading

jeanconn commented Jul 5, 2023

javierggt commented Jul 5, 2023 • edited Loading

taldcroft commented Jul 6, 2023

taldcroft commented Jul 10, 2023

jeanconn commented Jul 10, 2023

jeanconn commented Jul 10, 2023

taldcroft commented Jul 10, 2023

jeanconn commented Jul 10, 2023

taldcroft commented Jul 10, 2023

taldcroft commented Jul 10, 2023

jeanconn left a comment

Choose a reason for hiding this comment

taldcroft commented Jul 2, 2023 •

edited by jeanconn

Loading

Bonus function `utils.get_owner(path)`

taldcroft commented Jul 3, 2023 •

edited

Loading

jeanconn commented Jul 5, 2023 •

edited

Loading

javierggt commented Jul 5, 2023 •

edited

Loading