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

Enforce safe directory

Merged
merged 2 commits into from Apr 14, 2022
Merged

Enforce safe directory #762

merged 2 commits into from Apr 14, 2022

Conversation

thboop
Copy link
Collaborator

@thboop thboop commented Apr 13, 2022

This pr fixes #760 by setting the repositories path as a safe directory when running checkout and removing that config on cleanup.

We aren't able to set it as a local config, as this setting only works for global configs.

So, using the standard we used for submodules, lets go ahead and copy the existing global config to a new location. Then we can go ahead and modify that global config (so we don't modify like a users global config if they test something on a self hosted runner real fast). Then, we can delete that global config at the end of the checkout step.

This does carry some limitations, it doesn't persist this configuration for the duration of the job mainly. So if your job pushes to git or something after checkout, that will continue to fail. We need to figure out how to address this at an ecosystem level, outside of the checkout action.

src/git-source-provider.ts Outdated Show resolved Hide resolved
src/git-command-manager.ts Outdated Show resolved Hide resolved
src/git-source-provider.ts Outdated Show resolved Hide resolved
src/git-source-provider.ts Outdated Show resolved Hide resolved
// Arrange
await setup(removeGlobalAuth_removesOverride)
Copy link
Collaborator Author

@thboop thboop Apr 13, 2022

Choose a reason for hiding this comment

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

Renamed this function, changes are a result of that

src/git-auth-helper.ts Outdated Show resolved Hide resolved
@cbalioglu
Copy link

cbalioglu commented Apr 13, 2022

@thboop thanks a lot for taking quick action! Do you have an ETA on when you plan to land this fix?

src/git-auth-helper.ts Outdated Show resolved Hide resolved
src/git-auth-helper.ts Outdated Show resolved Hide resolved
src/git-auth-helper.ts Outdated Show resolved Hide resolved
@thboop thboop force-pushed the thboop/fixSafeDirectory branch from c940690 to 5c1bbe6 Compare Apr 14, 2022
@thboop thboop force-pushed the thboop/fixSafeDirectory branch from 5c1bbe6 to 55fd82f Compare Apr 14, 2022
@thboop thboop mentioned this pull request Apr 14, 2022
ac000 added a commit to ac000/libmtdac that referenced this issue Apr 23, 2022
Git recently made some updates[0][1] fixing a CVE.

This has unfortunately caused the build on Alpine to fail with the
following

gcc -MT .objs/curler.o -MMD -MP -MF .d/curler.o.Td -Werror -Wall -Wextra -Wdeclaration-after-statement -Wvla -Wmissing-prototypes -Wstrict-prototypes -Wold-style-definition -std=gnu99 -g -O2 -Wp,-D_FORTIFY_SOURCE=2 --param=ssp-buffer-size=4 -fno-common -fstack-protector -fPIC -fexceptions -fvisibility=hidden -I../include/libmtdac -DLIBNAME=\"libmtdac\" -DGIT_VERSION=\"\" -pipe -c -o .objs/curler.o curler.c
curler.c: In function 'do_curl':
curler.c:292:2: error: offset '1' outside bounds of constant string [-Werror=array-bounds]
  292 |  snprintf(ua, 1024, "%s %d.%d.%d (%s) / (libcurl/%s)",
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  293 |    LIBNAME, LIBMTDAC_MAJOR_VERSION, LIBMTDAC_MINOR_VERSION,
      |    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  294 |    LIBMTDAC_MICRO_VERSION, GIT_VERSION + 1, verinfo->version);
      |    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The GIT_VERSION is empty! and just above that we have

  fatal: unsafe repository ('/__w/libmtdac/libmtdac' is owned by someone else)
  To add an exception for this directory, call:
  	git config --global --add safe.directory /__w/libmtdac/libmtdac

This is the result of trying to run

  git describe --long --dirty

GitHub have made some changes to the actions/checkout@...[2] to
workaround this, however this doesn't persist beyond the initial
checkout stage[3] and so the git command fails in the Makefile where we
try and get the version.

To fix this we need to set the safe.directory ourselves from the make
action. This updates all the builds as it won't harm older gits that
don't have this issue and don't know about safe.directory (they'll just
ignore that setting).

[0]: https://lore.kernel.org/git/xmqqv8veb5i6.fsf@gitster.g/
[1]: https://github.blog/2022-04-12-git-security-vulnerability-announced/
[2]: actions/checkout#762
[3]: actions/checkout#766

Signed-off-by: Andrew Clayton <andrew@digital-domain.net>
ac000 added a commit to ac000/libmtdac that referenced this issue Apr 23, 2022
Git recently made some updates[0][1] fixing a CVE.

This has unfortunately caused the build on Alpine to fail with the
following

gcc -MT .objs/curler.o -MMD -MP -MF .d/curler.o.Td -Werror -Wall -Wextra -Wdeclaration-after-statement -Wvla -Wmissing-prototypes -Wstrict-prototypes -Wold-style-definition -std=gnu99 -g -O2 -Wp,-D_FORTIFY_SOURCE=2 --param=ssp-buffer-size=4 -fno-common -fstack-protector -fPIC -fexceptions -fvisibility=hidden -I../include/libmtdac -DLIBNAME=\"libmtdac\" -DGIT_VERSION=\"\" -pipe -c -o .objs/curler.o curler.c
curler.c: In function 'do_curl':
curler.c:292:2: error: offset '1' outside bounds of constant string [-Werror=array-bounds]
  292 |  snprintf(ua, 1024, "%s %d.%d.%d (%s) / (libcurl/%s)",
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  293 |    LIBNAME, LIBMTDAC_MAJOR_VERSION, LIBMTDAC_MINOR_VERSION,
      |    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  294 |    LIBMTDAC_MICRO_VERSION, GIT_VERSION + 1, verinfo->version);
      |    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The GIT_VERSION is empty! and just above that we have

  fatal: unsafe repository ('/__w/libmtdac/libmtdac' is owned by someone else)
  To add an exception for this directory, call:
  	git config --global --add safe.directory /__w/libmtdac/libmtdac

This is the result of trying to run

  git describe --long --dirty

GitHub have made some changes to the actions/checkout@...[2] to
workaround this, however this doesn't persist beyond the initial
checkout stage[3] and so the git command fails in the Makefile where we
try and get the version.

To fix this we need to set the safe.directory ourselves from the make
action.

This updates all the builds, except centos_7 where we don't install git,
 as it won't harm older gits that don't have this issue and don't know
about safe.directory (they'll just ignore that setting).

[0]: https://lore.kernel.org/git/xmqqv8veb5i6.fsf@gitster.g/
[1]: https://github.blog/2022-04-12-git-security-vulnerability-announced/
[2]: actions/checkout#762
[3]: actions/checkout#766

Signed-off-by: Andrew Clayton <andrew@digital-domain.net>
@liskin
Copy link

liskin commented May 1, 2022

This does carry some limitations, it doesn't persist this configuration for the duration of the job mainly. So if your job pushes to git or something after checkout, that will continue to fail. We need to figure out how to address this at an ecosystem level, outside of the checkout action.

This doesn't just affect stuff like pushing to git, it breaks even relatively common things like "git status", "git describe", basically any git command at all. Therefore anyone who uses that sort of information in their jobs will see breakage. In my case, https://github.com/pypa/setuptools_scm no longer works. Is there an issue tracking the "addressing this at an ecosystem level" which I can watch and get notified when it's resolved? Thanks!

rcgoodfellow added a commit to oxidecomputer/dlpi-sys that referenced this issue May 5, 2022
chschuermann added a commit to dreipol/git-mirror-action that referenced this issue May 11, 2022
Error message was:
"""
fatal: unsafe repository ('/github/workspace' is owned by someone else)

To add an exception for this directory, call:

	git config --global --add safe.directory /github/workspace
"""

Reference:
actions/checkout#762
clebergnu added a commit to clebergnu/avocado that referenced this issue May 11, 2022
By bumping the version of this action to v3, it incorporates the
setting of safe mode on the repo directories, avoiding the error:

   fatal: unsafe repository ('/__w/avocado/avocado' is owned by someone else)
   To add an exception for this directory, call:

          git config --global --add safe.directory /__w/avocado/avocado
   Error: Process completed with exit code 128.

Reference: actions/checkout#762
Signed-off-by: Cleber Rosa <crosa@redhat.com>
clebergnu added a commit to clebergnu/avocado that referenced this issue May 11, 2022
By bumping the version of this action to v3, it incorporates the
setting of safe mode on the repo directories, avoiding the error:

   fatal: unsafe repository ('/__w/avocado/avocado' is owned by someone else)
   To add an exception for this directory, call:

          git config --global --add safe.directory /__w/avocado/avocado
   Error: Process completed with exit code 128.

Reference: actions/checkout#762
Signed-off-by: Cleber Rosa <crosa@redhat.com>
clebergnu added a commit to clebergnu/avocado that referenced this issue May 11, 2022
By bumping the version of this action to v3, it incorporates the
setting of safe mode on the repo directories, avoiding the error:

   fatal: unsafe repository ('/__w/avocado/avocado' is owned by someone else)
   To add an exception for this directory, call:

          git config --global --add safe.directory /__w/avocado/avocado
   Error: Process completed with exit code 128.

Reference: actions/checkout#762
Signed-off-by: Cleber Rosa <crosa@redhat.com>
clebergnu added a commit to clebergnu/avocado that referenced this issue May 11, 2022
This workflow is currently failing, giving the following error:

   fatal: unsafe repository ('/__w/avocado/avocado' is owned by someone else)
   To add an exception for this directory, call:

          git config --global --add safe.directory /__w/avocado/avocado
   Error: Process completed with exit code 128.

The checkout action, on version 3, has the capability to set the safe
mode on repos it checks out, but, it's limited to the scope of that
action.

Let's work around this limitation and more recent git behavior and set
the safe mode on the repo directory manually.

Reference: actions/checkout#762
Signed-off-by: Cleber Rosa <crosa@redhat.com>
clebergnu added a commit to clebergnu/avocado that referenced this issue May 13, 2022
This workflow is currently failing, giving the following error:

   fatal: unsafe repository ('/__w/avocado/avocado' is owned by someone else)
   To add an exception for this directory, call:

          git config --global --add safe.directory /__w/avocado/avocado
   Error: Process completed with exit code 128.

Let's work around this limitation and more recent git behavior and set
the safe mode on the repo directory manually.

Reference: actions/checkout#762
Signed-off-by: Cleber Rosa <crosa@redhat.com>
clebergnu added a commit to clebergnu/avocado that referenced this issue May 16, 2022
This workflow is currently failing, giving the following error:

   fatal: unsafe repository ('/__w/avocado/avocado' is owned by someone else)
   To add an exception for this directory, call:

          git config --global --add safe.directory /__w/avocado/avocado
   Error: Process completed with exit code 128.

Let's work around this limitation and more recent git behavior and set
the safe mode on the repo directory manually.

Reference: actions/checkout#762
Signed-off-by: Cleber Rosa <crosa@redhat.com>
liketechnik added a commit to Compilerbau/CB-Lecture-Bachelor that referenced this issue Jul 5, 2022
Fixes an issue introduced with a recent git update
(https://github.blog/2022-04-12-git-security-vulnerability-announced/)
with a common workaround (actions/checkout#762,
https://stackoverflow.com/questions/71901632/fatal-error-unsafe-repository-home-repon-is-owned-by-someone-else,
actions/checkout#760),
by marking the /data directory inside the container as safe for git
during the container build.
liketechnik added a commit to Compilerbau/CB-Lecture-Bachelor that referenced this issue Jul 5, 2022
Fixes an issue introduced with a recent git update
(https://github.blog/2022-04-12-git-security-vulnerability-announced/)
with a common workaround (actions/checkout#762,
https://stackoverflow.com/questions/71901632/fatal-error-unsafe-repository-home-repon-is-owned-by-someone-else,
actions/checkout#760),
by marking the /data directory inside the container as safe for git
during the container build.
cagix added a commit to Compilerbau/CB-Lecture-Bachelor that referenced this issue Jul 18, 2022
* tooling: git safe repo directory for docker image

Fixes an issue introduced with a recent git update
(https://github.blog/2022-04-12-git-security-vulnerability-announced/)
with a common workaround (actions/checkout#762,
https://stackoverflow.com/questions/71901632/fatal-error-unsafe-repository-home-repon-is-owned-by-someone-else,
actions/checkout#760),
by marking the /data directory inside the container as safe for git
during the container build.

* tooling: point git to directory instead of disabling security features

Easier to maintain version of 7c2b552
that additionally does not fiddle with security sensitive settings.

* style(Makefile): docker git env into separate variable

* tooling: extract repo location inside container into variable

* tooling: replace missing hardcoded /data with variable

Co-authored-by: Carsten Gips <cagix@fh-bielefeld.de>

* tooling(delete-rem-tags): pass git commit info (#19)

* tooling(delete-rem-tags): pass git commit info

Passes git author information via environment variables into the docker
container, in order to ensure commits done by the script have correct
author information.

* tooling(delete-rem-tags): pass git full commit info

Pass not only author information, but committer information too, since
git seems to be *sometimes* unhappy with only author information, for
whatever reason.

* tooling: makefile formatting

Co-authored-by: Carsten Gips <cagix@fh-bielefeld.de>

* tooling: makefile formatting

Co-authored-by: Carsten Gips <cagix@fh-bielefeld.de>

* tooling: makefile formatting

Co-authored-by: Carsten Gips <cagix@fh-bielefeld.de>

Co-authored-by: Carsten Gips <cagix@fh-bielefeld.de>
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.

5 participants