Disable local git hooks by default.

[rsandell]

Disable local git hooks by default.

The security fixes released on 2022-02-15 fixed all known ways of utilizing git hooks to do nefarious things on a Jenkins Controller. But there might be other ways in that we don't yet know of.
So this is a security hardening/feature that turns off the execution of local git hooks whenever the plugin initializes a new repository both on the controller and on agents.
Hooks can be enabled again by a configuration on the security page.

Checklist

• I have read the CONTRIBUTING doc
• I have referenced the Jira issue related to my changes in one or more commit messages
• I have added tests that verify my changes
• Unit tests pass locally with my changes
• I have added documentation as necessary
• No Javadoc warnings were introduced with my changes
• No spotbugs warnings were introduced with my changes
• Documentation in README has been updated as necessary
• Online help has been added and reviewed for any new or modified fields
• I have interactively tested my changes
• Any dependent changes have been merged and published in upstream modules (like git-client-plugin)

Types of changes

What types of changes does your code introduce?

• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Further comments

[jtnord]

couple of NITS, but otherwise LGTM.

tested the NUL: in a local clone and hooks are not run

[jtnord]

depending on:

• git-client because it won't always when using the jgit provider on windows, hooks are run on a posix file system, or if you are on a windows filesystem and cygwin or some other sh.exe is detected.

[rsandell]

@jtnord if you have a few minutes, could you help me figure out why the hook isn't working on the windows agents? There is something wrong with a path that I've managed to fix in an earlier version of the test, but it's broken again 😢

[rsandell]

So @yaroslavafenkin did some tests this weekend and apparently the tests passes when run with forkCount=1 so it might be some interfering with some static state or something. Though strange that it would only show on Windows.

[MarkEWaite]

So @yaroslavafenkin did some tests this weekend and apparently the tests passes when run with forkCount=1 so it might be some interfering with some static state or something. Though strange that it would only show on Windows.

That is enough justification for us to follow the recommendation from @jtnord and disable the forkCount setting that I added. I can always add the forkCount argument on my own development builds.

[rsandell]

Interesting...
I set the failing windows test to be skipped, and now the other windows test(s) starts to fail 😞

[rsandell]

Now the build is just failing because of timeout.

[rsandell]

So the linux-17 test failure is a bit weird. The test times out while waiting for the first build to finish. But the thread dump isn't showing any running executors. The thread dump is taken a few moments later though. But if the build was really stuck it would be really stuck I guess?

[MarkEWaite]

So the linux-17 test failure is a bit weird. The test times out while waiting for the first build to finish. But the thread dump isn't showing any running executors. The thread dump is taken a few moments later though. But if the build was really stuck it would be really stuck I guess?

Since the Java 17 test configuration is a very recent addition from #1249 , it may be a case of undetected unreliability. I've started another run of the master branch to provide another data point.

I've not seen Java 17 be unstable in multiple test runs in my home environment, but there are still many fewer test jobs that have run with Java 17 than with Java 8 and Java 11.

[rsandell]

Green build! Woohooo! 🚢

[rsandell]

@MarkEWaite any thoughts on when this could be merged? I don't know what is more in the pipe, so I'm not sure to merge and release it myself :)

[MarkEWaite]

@MarkEWaite any thoughts on when this could be merged? I don't know what is more in the pipe, so I'm not sure to merge and release it myself :)

Thanks for asking. I need to deliver a new git client plugin release with JGit 5.13.1 and can use that as an opportunity to deliver a new git plugin release as well. Let me check with others to assure

[rsandell]

@MarkEWaite Slightly strange thing; this PR isn't showing up in the draft release notes for the next release 🤔

[MarkEWaite]

@MarkEWaite Slightly strange thing; this PR isn't showing up in the draft release notes for the next release 🤔

Thanks for noting it. The release drafter application has been confused by the series of releases that have been generated from branches. I'll assemble the changelog interactively and assure that this is included in the final changelog.

[pbrzica]

Hi,

This change breaks some tools (e.g. https://github.com/elasticdog/transcrypt ) breaks because it has the following contained in it:

readonly GIT_HOOKS=$(git config core.hooksPath | sed 's:/*$::' 2>/dev/null || printf "%s/hooks" "${RELATIVE_GIT_DIR}")

Later on in the script it contains:

[[ ! -d "${GIT_HOOKS}" ]] && mkdir -p "${GIT_HOOKS}"

As this change sets hooksPath to /dev/null explicitly when disabled this results in the following error:

mkdir: cannot create directory ‘/dev/null’: File exists

Is it possible to have an option to change this value in the security configuration to something other than /dev/null?

[MarkEWaite]

Is it possible to have an option to change this value in the security configuration to something other than /dev/null?

There is already a security configuration setting that will allow the administrator to allow hooks to be executed on checkout. If you need something like transcrypt, allow repository hooks to be executed by enabling them on agents. See the documentation for more details.

If that doesn't address your question, could you explain further what you are requesting?

[pbrzica]

Yeah I've read it there. The problem is I can now only choose between disabling all hooks or enabling all of them.

If I could specify a custom directory in the settings I could keep only approved hooks in that directory, which would give a bit more leeway.

[MarkEWaite]

Yeah I've read it there. The problem is I can now only choose between disabling all hooks or enabling all of them.

If I could specify a custom directory in the settings I could keep only approved hooks in that directory, which would give a bit more leeway.

Sorry, but I'm not willing to add that level of configuration to the plugin.

[pbrzica]

I understand.
Thanks for the fast replies!

[nhatkhai]

Are we aware doing this also brake git lfs install command ?

[MarkEWaite]

Are we aware doing this also brake git lfs install command ?

See JENKINS-69760 for the details related to git lfs install and a workaround (run git lfs install once per agent instead of running it once per job)

[nhatkhai]

FYI: The solution in JENKINS-69760 and its assumption excluding these case:

1. ALL 1 years old commits cannot be rebuild without manually modify each rebuild per commit this broke the concept of code as infrastructure concept. They call git install because not all agent preconfig git lfs! (infrastructure as code)
2. Project that need fine grain control behavior of git lfs for which object for when and which lfs object need to be smudge would broke. The need to remove that hookPath after this checkout to work again. Which also run into issue fixed HUDSON-6203 #1 (ALL Old commits break)

[MarkEWaite]

FYI: The solution in JENKINS-69760 and its assumption excluding these case:

1. ALL 1 years old commits cannot be rebuild without manually modify each rebuild per commit this broke the concept of code as infrastructure concept. They call git install because not all agent preconfig git lfs! (infrastructure as code)

Yes, agents that do not pre-configure git lfs can either preconfigure git lfs or can enable the switch that allows allows git hook scripts to be executed.

Yes, there are times when we make changes that break existing users in order to better protect those users from newly discovered threats. Disabling hook scripts is a safeguard against malicious hooks. Administrators that don't want the safeguard can allow hook scripts. The default of disallowing hook scripts is the safest for the user community as a whole.

2. Project that need fine grain control behavior of git lfs for which object for when and which lfs object need to be smudge would broke. The need to remove that hookPath after this checkout to work again. Which also run into issue fixed HUDSON-6203 #1 (ALL Old commits break)

Projects that need fine grain control of the behavior of git lfs should probably switch to wrap their calls to git in withCredentials blocks so that they can take complete control of the entire git command line. The git plugin does not plan to provide fine grained control of git lfs behavior.

[nhatkhai]

Thank Mark for explain, in fact I had attempt doing all of that. However, your plug-in have nice logic that notify the correct BB server.... if I don't use it, I muse make my own that would be in sane.
So I would end up run your plugin-in checkout out, right after that I run a git config --unset core.hookPath. This would make me think I'm crazy lol... I reverted what you had done, and cumbersome for me to future proof. So I would actually inject event more arbitrary execution script down from a server do run "clean up" and guard around the plug-in call to against future broken of old build/commits! Isn't this some thing go wrong here with architecture ?

[MarkEWaite]

Isn't this some thing go wrong here with architecture ?

It seems to me that you're refusing to take the easy choices and seem to be focused on choosing the difficult paths unnecessarily.

Easy paths:

• Configure Jenkins to allow all git hooks. Single configuration, allows you to continue to use git lfs install on every job, even though it is only needed once per agent instance
• Configure your agents to call git lfs install before any Jenkins job is run. Then you can remove it from your custom configuration

Instead, you said

I reverted what you had done

Reverting a change to a plugin is a lot of work. While it is open source and you're welcome to do that, I see no reason why you would do that. Use the settings that already exist in the plugin global configuration to allow git hooks to be executed.

[nhatkhai]

May be what I try to said is all most issues that I have boil down to this cool plug-in :-). And the solution for my use case is allow environment configurations to overwrite the default behaviors just like the way git been doing with gitconfig files (and backward compatible git config commands), and the environment variables.

Giving that I'm not Jenkins Admin, and I like my CI/CD jobs run on multiple environment (Jenkins, local developer machine, VM machine etc....) The simple way to solve environment difference or said infrastructure differences are via a Properties File that define a set of environment variables that guide CI/CD script run correctly. And git had been perfect for me. However, I recently discovered that may solution are stop working when using this cool plug-in.

1. While the plugin run local git.exe installed command, but it do not respect the current environment setting of current executing pipeline jobs. This mean typical git command and git-plugin are behavior differently. I'm currently have zero control on how the plugin work. For example, git command would use the .ssh, gitconfig in $HOME environment set by current pipeline, but the plug-in still use the old $HOME environment when operate checkout.
2. Now security fix are great, but I had seen git developer always introduce a right amount of environment variables for overwrite default behavior when needed.

So just via environment I could make all my 1 years old commits now still work within any modification into the repository by temporary tell them use a temporary configurations.

Same way I could have fine grain control how git work because the fact that git do not scale well in some areas (reference https://stackoverflow.com/questions/984707/what-are-the-file-limits-in-git-number-and-size).

I hope this explain it... and please forgive my language as I'm a terrible communication.

[nhatkhai]

Aside the above.
As far as my research, in my opinion, setting invalid value to disable the git hooks is a hack to git ecosystem not a real security fix For example, git lfs use hooks to operate with normal git commands, so this would corrupting git lfs functions. Who knowns how many other things it would messed up.
Would it be better is set the core.hooksPath to a known good folder or empty folder? at least this would less likely disrupting the git ecosystem...

[harryssuperman]

@nhatkhai Thanks for the comments, ticket and infos. That was our problem too updating our Jenkins. I think this #1237 should be in the release notes as a breaking changes.
For us we had to change the pipeline code in order to have same behaviour as before.

1. git config --unset core.hooksPath
2. git lfs install
3. git add
4. git push

[MattLud]

One note that Gerrit SCM requires a change-id that is typically generated via commit-hook - operations that are running on the agent and manually configuring this hook post-checkout are broken without configuring this to allow agents to use githooks.

[MarkEWaite]

Gerrit SCM requires a change-id that is typically generated via commit-hook - operations that are running on the agent and manually configuring this hook post-checkout are broken without configuring this to allow agents to use githooks.

I believe that means that users of Gerrit SCM will need to follow the instructions from the documentation and from the pull request description. The pull request description says:

Hooks can be enabled again by a configuration on the security page

[MattLud]

I believe that means that users of Gerrit SCM will need to follow the instructions from the documentation and from the pull request description. The pull request description says:

We figured that out as soon as we found this merge request.

If someone using Gerrit suddenly finds their operations broken as part of updates, they'll have to figure out which plugin is causing this issue - adding some commentary here will hopefully save them time when trying to figure it out.