Skip to content
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

Add portage.process.check_output() function and use it in GitSync class #818

Closed
wants to merge 2 commits into from

Conversation

floppym
Copy link
Contributor

@floppym floppym commented Apr 14, 2022

This allows us to utilize existing logic for changing the user id in the
called process.

Bug: https://bugs.gentoo.org/838223

This allows us to utilize existing logic for changing the user id in the
called process.

Bug: https://bugs.gentoo.org/838223
Signed-off-by: Mike Gilbert <floppym@gentoo.org>
@floppym
Copy link
Contributor Author

floppym commented Apr 14, 2022

In testing, I found that syncing with sync-git-verify-commit-signature fails when we run git as the "portage" user, probably due to a permissions issue on the temporary directory that we use for WKD key refresh. I'll have to look into that.

status = portage._unicode_decode(
subprocess.check_output(
portage.process.check_output(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call is problematic. The files gnupg files generated by gemato will be owned by root, but we need to run git as non-root to avoid the "'/var/db/repos/gentoo' is owned by someone else" error.

I think we would need to invoke the gemato setup code (self._get_openpgp_env()) as the appropriate non-root user.

@thesamesam
Copy link
Member

@ajakk

This ensures that any spawned gpg or git processes will have access to
files in GNUPGHOME.

Bug: https://bugs.gentoo.org/838223
Signed-off-by: Mike Gilbert <floppym@gentoo.org>
@floppym
Copy link
Contributor Author

floppym commented Apr 16, 2022

This PR breaks emerge --info when a git repo is configured.

% emerge --info
Traceback (most recent call last):
  File "/home/floppym/src/portage/bin/emerge", line 60, in <module>
    retval = emerge_main()
  File "/home/floppym/src/portage/lib/_emerge/main.py", line 1294, in emerge_main
    return run_action(emerge_config)
  File "/home/floppym/src/portage/lib/_emerge/actions.py", line 3942, in run_action
    return action_info(
  File "/home/floppym/src/portage/lib/_emerge/actions.py", line 1956, in action_info
    head_commit = sync.retrieve_head(options=options)
  File "/home/floppym/src/portage/lib/portage/sync/modules/git/git.py", line 367, in retrieve_head
    portage.process.check_output(
TypeError: portage.process.check_output() argument after ** must be a mapping, not NoneType

@ajakk
Copy link
Member

ajakk commented Apr 17, 2022

Lifting the uid/gid/etc grabbing functionality from controller.py, we can get something like:

diff --git a/lib/portage/sync/modules/git/git.py b/lib/portage/sync/modules/git/git.py
index 10dabb5a4..4b4e8cf45 100644
--- a/lib/portage/sync/modules/git/git.py
+++ b/lib/portage/sync/modules/git/git.py
@@ -3,6 +3,7 @@

 import io
 import logging
+import pwd
 import subprocess

 import portage
@@ -361,6 +362,27 @@ class GitSync(NewBase):
             return (1, False)
         rev_cmd = [self.bin_command, "rev-list", "--max-count=1", "HEAD"]
         try:
+            repo = kwargs['options']['repo']
+            st = os.stat(repo.location)
+            try:
+                pw = pwd.getpwuid(st.st_uid)
+            except KeyError:
+                pass
+            else:
+                # Drop privileges when syncing, in order to match
+                # existing uid/gid settings.
+                self.spawn_kwargs = {}
+                self.usersync_uid = st.st_uid
+                self.spawn_kwargs["uid"] = st.st_uid
+                self.spawn_kwargs["gid"] = st.st_gid
+                self.spawn_kwargs["groups"] = [st.st_gid]
+                self.spawn_kwargs["env"] = {}
+                self.spawn_kwargs["env"]["HOME"] = pw.pw_dir
+                self.spawn_kwargs["env"]["LOGNAME"] = pw.pw_name
+                umask = 0o002
+                if not st.st_mode & 0o020:
+                    umask = umask | 0o020
+                self.spawn_kwargs["umask"] = umask
             ret = (
                 os.EX_OK,
                 portage._unicode_decode(

Though that duplicates a bit of code and I haven't dug into trying to make it more generic to fit into its own function.

@floppym
Copy link
Contributor Author

floppym commented Jun 7, 2022

I'm really not sure where to go with this one. I feel like some code needs to be reworked (possibly in gemato), or we need to approach this problem from a different angle.

I wonder if we could just disable git's new behavior without re-introducing whatever security issue it "solves".

@thesamesam
Copy link
Member

We might be able to do something like git config --add safe.directory /path/to/sync/dir if sync-type = git.

@floppym floppym closed this Jan 2, 2023
@floppym floppym deleted the bug828223 branch January 2, 2023 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants