[master] Prevent "_pygit2.GitError: error loading known_hosts" in some libgit2 versions.

[meaksh]

What does this PR do?

This PR should fix a problem that happens in some libgit2 versions, where there is an strict check for ssh "known_hosts" done by the "libgit2" library, in the context of fixing a CVE issue:

libgit2/libgit2@42e5db9

We noticed this CVE fix has been backported to older "libgit2" versions in some distributions.

Without the fix on this PR, we see _pygit2.GitError: error loading known_hosts errors when dealing with git ssh repositories.

What issues does this PR fix or reference?

Fixes: #64121

Previous Behavior

There are gitfs errors we can see in the Salt master logs.

[ERROR   ] Error occurred fetching gitfs remote 'ssh://[email protected]/myorg/mysalt.git': error loading known_hosts:
Traceback (most recent call last):
  File "/opt/saltstack/salt/lib/python3.10/site-packages/salt/utils/gitfs.py", line 1870, in _fetch
    fetch_results = origin.fetch(**fetch_kwargs)
  File "/opt/saltstack/salt/extras-3.10/pygit2/remote.py", line 146, in fetch
    payload.check_error(err)
  File "/opt/saltstack/salt/extras-3.10/pygit2/callbacks.py", line 98, in check_error
    check_error(error_code)
  File "/opt/saltstack/salt/extras-3.10/pygit2/errors.py", line 65, in check_error
    raise GitError(message)
_pygit2.GitError: error loading known_hosts:

New Behavior

There are no errors in Salt master logs and gitfs works as expected.

Merge requirements satisfied?

• Docs
• Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
• Tests written/updated

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

[meaksh]

I had to rebase this PR to get merge conflicts solved.

@Ch3LL @dwoz this should be ready for review. Thanks in advance!

[dwoz]

Since we're modifying the environment anyway can we do it higher up in the stack? Maybe when we run verify_user?

[Ch3LL]

@meaksh did you see @dwoz 's comment?

[meaksh]

@meaksh did you see @dwoz 's comment?

Hey @Ch3LL, sorry for the delay here.

We have noticed that, bringing the user switching that early on the stack is actually causing "Authentication denied" errors when you have configured publisher_acl.

So still a bit more time needs to be invested here to fix this other problem. Let see if we find some time in the following days.

In any case, the initial patch to fix the "pygit2" problem that motivated this PR seems to be working fine: 46e6ba3

[Ch3LL]

K thanks for the update. We are trying to tag the first 3007 RC next week sometime, so keep me updated on what you where able to find about the authentication denied error.

[meaksh]

@dwoz @Ch3LL it seems switching the user for "salt-master" that early on the stack is actually causing problems with "publisher_acl".

In a nutshell, "salt-master" needs to run as "root" during startup, and then it will switch to the configured user at some point during the startup. We cannot set "salt-master" systemd unit with User=salt and neither switching to the configured user at the begining of the initialization stack, as this is too early and will cause when creating user keys (as "chmod/chown" cannot be executed properly if not running as root). Switching the user that early on the stack seems problematic and not trivial.

The initial fix that motivated this PR was about setting HOME in the environment if not existing to solve the "gitfs" issue after a recent "pygit2" upgrade. During PR review, it was suggested to set this fix early in the stack and also set the user as part of systemd units, as mentioned, this seems to have unwanted implications and not really trivial without changes in other areas.

So, in order to fix the situation that is currently breaking the "gitfs" integration, maybe I would suggest to just introduce the initial fix, without doing the switch of the user and just setting the HOME in the scope of the gitfs backend, and then, if it applies, deal with the "user" switch in a different PR.

What is your take here? Thanks!

[dmurphy18]

Cannot assume 'salt' as the user in the systemd service files

[dmurphy18]

How will this accomodate users which don't use 'salt' as the user, I know of one large customer who uses a different name for the user.

Assuming salt is incorrect and have already had to fix that assumption for log rotation.

Question then is when will the value for 'User' get set, since have to read the configuration file etc. to know what user is set to, and then update the systemd service file, which is part of the install process, so installed before configuration read. Implies a need to update service file and restart with the new value, and additional checks in case the user changes the configuration value for 'user' too

[ITJamie]

this will cause issues with people using the 'root' user for pam auth funtionality

[dmurphy18]

see comment above

[dmurphy18]

see comment above

[meaksh]

I'm closing this PR for now, as commented on #64510 (comment), in favor of this other PR: #67186 (which is tackling the initial issue that motivated this PR)