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.

Sign up for GitHub

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

Jump to bottom

Fix git-init for Git 2.35.2 #4756

Merged
merged 1 commit into from
Apr 13, 2022
Merged

Fix git-init for Git 2.35.2 #4756

merged 1 commit into from
Apr 13, 2022

Conversation

abayer
Copy link
Contributor

@abayer abayer commented Apr 13, 2022

Changes

https://github.blog/2022-04-12-git-security-vulnerability-announced/ was announced
and then fixed in Git 2.35.2. This ends up creating issues like actions/checkout#760,
where the directory the git repo lives isn't owned by the same user performing the
git operations. This does appear to be affecting us - see https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_pipeline/4750/pull-tekton-pipeline-integration-tests/1514143139193950210/
for example, which has the telltale error message of fatal: unsafe repository ('/workspace/go/src/github.com/GoogleContainerTools/skaffold' is owned by someone else).

This is an attempt to fix that by having git-init call git config --global --add safe.directory [repo dir]
before fetching, etc.

/kind bug

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • Docs included if any changes are user facing
  • Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including
    functionality, content, code)
  • Release notes block below has been filled in
    (if there are no user facing changes, use release note "NONE")

Release Notes

Fixed git-init behavior to work with Git 2.35.2 changes.

@abayer
Fix git-init for Git 2.35.2
197115c 
https://github.blog/2022-04-12-git-security-vulnerability-announced/ was announced
and then fixed in Git 2.35.2. This ends up creating issues like actions/checkout#760,
where the directory the git repo lives isn't owned by the same user performing the
git operations. This does appear to be affecting us - see https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_pipeline/4750/pull-tekton-pipeline-integration-tests/1514143139193950210/
for example, which has the telltale error message of `fatal: unsafe repository ('/workspace/go/src/github.com/GoogleContainerTools/skaffold' is owned by someone else)`.

This is an attempt to fix that by having `git-init` call `git config --global --add safe.directory [repo dir]`
before fetching, etc.

Signed-off-by: Andrew Bayer <[email protected]>
@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. labels Apr 13, 2022
@tekton-robot tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 13, 2022
abayer
abayer commented Apr 13, 2022
View reviewed changes
pkg/git/git.go
if _, err := run(logger, "", "init"); err != nil {
return err
}
if _, err := run(logger, "", "config", "--add", "--global", "safe.directory", "/"); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When spec.Path is empty, I'm not sure exactly what we should be adding as a safe.directory here instead. I'm guessing that it's the root directory, but I could very well be wrong.

Copy link
Member

Choose a reason for hiding this comment

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

/ sounds safe to me. AIUI, the CVE specifically impacts shared environments, where one user can create e.g., /.git and trick subdirectory git checkouts to using the root-level git config. Tekton doesn't have that problem (or else you're using Tekton very weirdly, and you get what you get!)

I think if this causes any problems we could just uniformly configure / as a safe directory. But being conservative sounds smart until then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I saw no reason not to just go as broad as we have to - my uncertainty is whether / is actually where we're running git init from when we don't have spec.Path.

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/git/git.go 69.1% 67.9% -1.3

@abayer
Copy link
Contributor Author

abayer commented Apr 13, 2022

/test pull-tekton-pipeline-build-tests

@abayer
Copy link
Contributor Author

abayer commented Apr 13, 2022

/test pull-tekton-pipeline-integration-tests

@abayer
Copy link
Contributor Author

abayer commented Apr 13, 2022

/assign @wlynch
/assign @imjasonh
/assign @mattmoor

Assigning people who I see have done stuff in pkg/git/git.go in the past and may feel qualified to review this. =)

@abayer
Copy link
Contributor Author

abayer commented Apr 13, 2022

I think this works - the alpha integration tests passed (build and integration jobs never properly launched, so I'm re-running them), but they fail consistently on other PRs, like #4750's https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_pipeline/4750/pull-tekton-pipeline-alpha-integration-tests/1514243048417005569/

imjasonh
imjasonh reviewed Apr 13, 2022
View reviewed changes
Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2022
@abayer
Copy link
Contributor Author

abayer commented Apr 13, 2022

And fwiw, I'm more than a little confused as to how git-init is ending up with Git 2.35.2 on it - when I build it locally, it's Git 2.32.1, but that error message is new in 2.35.2, as seen at git/git@8959555#diff-c4fe8341fe7387f617eb30b841ca114b8b62ccb94403fc6a04cd7fc188dc2961R1303

@mattmoor
Copy link
Member

Just as a datapoint (context #4752):

# docker run -ti ghcr.io/distroless/git --version
Unable to find image 'ghcr.io/distroless/git:latest' locally
latest: Pulling from distroless/git
6c6f69aa2550: Pull complete
Digest: sha256:107c3bcf9a5d92c88e1085cb949d247ebe95cfbf6235d4a4307d129d2874de71
Status: Downloaded newer image for ghcr.io/distroless/git:latest
git version 2.35.1

Was this introduced in .2? 🤔

@mattmoor
Copy link
Member

I misread the title, I see fixed in .2 ok 👍

I'll talk to Ariadne about where that is in Alpine edge.

@abayer
Copy link
Contributor Author

abayer commented Apr 13, 2022 •
edited
Loading

@mattmoor Huh, looks like git/git@8959555 has actually been in for a while, but didn't behave the same way until 2.35.2, I guess? Weird in general for sure, but hey.

EDIT: Ah, interesting - it looks like that commit is in fact in 2.32.1, not just 2.35.2. This makes more sense now.

EDIT AGAIN: It looks like Alpine 3.14, 3.15, and Edge all have git package versions with the fix in, fwiw.

@abayer
Copy link
Contributor Author

abayer commented Apr 13, 2022

/retest

Quota flakiness on pipeline-integration-tests this time.

@mattmoor
Copy link
Member

mattmoor commented Apr 13, 2022 •
edited
Loading

It’s already in edge, I’ll rebuild and check the version again before attempting the switch 👍

I think it’s just bad timing with our nightly cron

@abayer
Copy link
Contributor Author

abayer commented Apr 13, 2022

/retest

workspace-in-sidecar integration test flakiness.

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 13, 2022
@tekton-robot tekton-robot merged commit be58566 into tektoncd:main Apr 13, 2022
@aceeric aceeric mentioned this pull request Jun 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imjasonh imjasonh imjasonh left review comments

@vdemeester vdemeester vdemeester approved these changes

@jerop jerop Awaiting requested review from jerop

@pritidesai pritidesai Awaiting requested review from pritidesai

Assignees

@imjasonh imjasonh

@wlynch wlynch

@mattmoor mattmoor

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@abayer @tekton-robot @mattmoor @vdemeester @imjasonh @wlynch