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

Fix git permission issue in CI build #5089

Merged
merged 1 commit into from Dec 14, 2022

Conversation

jnidzwetzki
Copy link
Member

@jnidzwetzki jnidzwetzki commented Dec 14, 2022

The new permissions checks to fix CVE-2022-29187 in Git caused some issues in our CI pipeline. This patch adds the checkout directory to Git's "safe.directory" setting.

Failed CI run: https://github.com/timescale/timescaledb/actions/runs/3690974658/jobs/6248545042

@codecov
Copy link

codecov bot commented Dec 14, 2022

Codecov Report

Merging #5089 (1593e4f) into main (558688c) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5089   +/-   ##
=======================================
  Coverage   89.60%   89.61%           
=======================================
  Files         227      227           
  Lines       51620    51619    -1     
=======================================
+ Hits        46253    46256    +3     
+ Misses       5367     5363    -4     
Impacted Files Coverage Δ
src/loader/bgw_message_queue.c 86.36% <0.00%> (-2.85%) ⬇️
tsl/src/nodes/data_node_dispatch.c 93.03% <0.00%> (-0.22%) ⬇️
tsl/src/bgw_policy/job.c 88.31% <0.00%> (-0.05%) ⬇️
src/bgw/job.c 93.12% <0.00%> (+0.16%) ⬆️
src/bgw/scheduler.c 86.18% <0.00%> (+2.30%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 558688c...1593e4f. Read the comment docs.

@jnidzwetzki jnidzwetzki force-pushed the fix_ci_git branch 3 times, most recently from e07d50a to ca29c2d Compare December 14, 2022 09:44
@jnidzwetzki jnidzwetzki marked this pull request as ready for review December 14, 2022 10:01
@jnidzwetzki jnidzwetzki changed the title Fix git permission issue during CI build Fix git permission issue in CI build Dec 14, 2022
@jnidzwetzki jnidzwetzki self-assigned this Dec 14, 2022
Copy link
Member

@svenklemm svenklemm left a comment

Choose a reason for hiding this comment

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

I think the better approach here is to make git skip that check by adjusting the configuration as we do in other places. E.g. 17844a4087

@jnidzwetzki
Copy link
Member Author

I am in favor of fixing the permission problem than disabling the new Git security checks. In this specific case, we have a mismatch between the owner of the checkout directory and the checkout. Is there any technical problem with this solution?

You mentioned in Slack, that the user is controlled by the GitHub runner. This is true, but if GitHub decides to let the workflow run as another user as root, a lot of things (e.g., apt-get) would also break. I could not find a specification for it. But I don't suspect that this will change unannounced.

@svenklemm
Copy link
Member

I am in favor of fixing the permission problem than disabling the new Git security checks. In this specific case, we have a mismatch between the owner of the checkout directory and the checkout. Is there any technical problem with this solution?

You mentioned in Slack, that the user is controlled by the GitHub runner. This is true, but if GitHub decides to let the workflow run as another user as root, a lot of things (e.g., apt-get) would also break. I could not find a specification for it. But I don't suspect that this will change unannounced.

Running a recursive chown is several orders of magnitude slower than adjusting the git config and this does not account for the additional runtime of the check itself. The suggested solution with chown only works for that specific user and prevents sharing the checkout between multiple steps when the checkout directory is bind mounted into docker containers as we do in some of the other workflows.

@jnidzwetzki jnidzwetzki force-pushed the fix_ci_git branch 5 times, most recently from cd3ba04 to 1568cfa Compare December 14, 2022 12:38
The new permissions checks to fix CVE-2022-29187 in Git caused some
issues in our CI pipeline. This patch adds the checkout directory to
Git's "safe.directory" setting.
@jnidzwetzki
Copy link
Member Author

@svenklemm Changed, let's do in that way.

@jnidzwetzki jnidzwetzki merged commit 940626b into timescale:main Dec 14, 2022
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.

None yet

3 participants