Revert "Use absolute in paths in salt scripts" (bsc#1253780)

[m-czernek]

What does this PR change?

In c4f0d68, we modified the SLS to use absolute paths. This was based on sonarcloud issue - https://github.com/SUSE/spacewalk/issues/24387

The problem is that older Ubuntu and Debian systems (that were updated and not reinstalled, e.g. 16.04 -> 18.04 -> 20.04 -> 22.04 -> 24.04) do not necessarily have usrmerge installed. Usrmerge is installed by default only on new installations.

This means that we cannot assume /usr/bin path for a lot of binaries on some Debians and Ubuntus, such as uname, cat, ls, etc.

Given that this change is related to sonarcloud, I propose reverting the change.

Codespace

Check if you already have a running container clicking on

GUI diff

No difference.

Before:

After:

• DONE

Documentation

• No documentation needed: add explanation. This can't be used if there is a GUI diff

• No documentation needed: only internal and user invisible changes

• Documentation issue was created: Link for SUSE Multi-Linux Manager contributors, Link for community contributors.

• API documentation added: please review the Wiki page Writing Documentation for the API if you have any changes to API documentation.

• (OPTIONAL) Documentation PR

• DONE

Test coverage

ℹ️ If a major new functionality is added, it is strongly recommended that tests for the new functionality are added to the Cucumber test suite

• No tests: add explanation

• No tests: already covered

• Unit tests were added

• Cucumber tests were added

• DONE

Links

Issue(s): #

• https://github.com/SUSE/spacewalk/issues/29075

• https://github.com/SUSE/spacewalk/issues/29295
  Port(s): # TODO

• DONE

Changelogs

Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository

If you don't need a changelog check, please mark this checkbox:

• No changelog needed

If you uncheck the checkbox after the PR is created, you will need to re-run changelog_test (see below)

Re-run a test

If you need to re-run a test, please mark the related checkbox, it will be unchecked automatically once it has re-run:

• Re-run test "changelog_test"
• Re-run test "backend_unittests_pgsql"
• Re-run test "java_pgsql_tests"
• Re-run test "schema_migration_test_pgsql"
• Re-run test "susemanager_unittests"
• Re-run test "frontend_checks"
• Re-run test "spacecmd_unittests"

Before you merge

Check How to branch and merge properly!

[rjmateus]

This also affects some older versions of SLES, like sles12: https://github.com/SUSE/spacewalk/issues/29295

[rjmateus]

@m-czernek I don't think this revert is needed. Thomas already fixed part of the issues, and this commit should fix the remaining missing parts: #11355

What do you think?

[m-czernek]

@rjmateus Sles 12 is unrelated to this issue. As in the description, certain set of Ubuntu/Debian systems, some binaries we assume to be in /usr/bin are in /bin.

I see, for example, that we hardcode /usr/bin/which at [0], which would fail on an old Ubuntu system that does not explicitly install usrmerge. Note btw, that newly deployed Ubuntu systems would require /usr/bin/which even though they do not have usrmerge installed.

I see two options basically:

• We revert the original PR to use full binary paths (from a security perspective, I've discussed this with Orion & Wictor, see [1] for more info)
• For every binary, we try any possible path where it could be (non-user writable). When I was still in the bug squad, I created [2] as a POC, but this could suffer from problems for ssh-only management.

Would love to hear your opinion here; for now, I'm still leaning towards reverting the full paths (especially since we use which, which negates any security gained from using full binary paths anyways).

[0] https://github.com/uyuni-project/uyuni/blob/master/susemanager-utils/susemanager-sls/salt/hardware/profileupdate.sls#L157
[1] https://github.com/SUSE/spacewalk/issues/29075#issuecomment-3646503605
[2] #11315

[vzhestkov]

I would vote for reverting it.

[rjmateus]

@m-czernek Thank you for the detailed reply. For SLES is relatively simple to have the full path, since we can easily compute the path based on the OS.
For Debian is also possible, but it looks much more complex and unpredictable, we can look in a set of locations as you have on the pr: #11315

It doesn't look as a security issue, as discussed in the slack thread. So if the change in bringing more problems then the ones it fix, I think we should consider reverting it.
However, if I get it right, the autoinstall doesn't apply to Debian, and all issues should be fixed now. So we should be able to keep the changes to that file.
for the bootstrap is a bit more complex, and we can revert it and revisit in the future if a security issue arise. Another option could be have a conditional execution and you propose in the draft PR

[agraul]

@m-czernek Thank you for the detailed reply. For SLES is relatively simple to have the full path, since we can easily compute the path based on the OS. For Debian is also possible, but it looks much more complex and unpredictable, we can look in a set of locations as you have on the pr: #11315

After looking into this a bit more, I tend to think that we don't need to revert the whole commit. There are not many places left that cause issues and it's good to be explicit where we can.

There are Salt states that should work on different OSes and where we don't know the location of all invoked commands. In this case, I propose to use command -p $cmd. It's a built-in command and uses a PATH similar to the search_paths list defined in #11315. From a security perspective it's better than running which (which uses a user-defined PATH) and we know that it's always available.

[agraul]

To bring the discussion to a single place, this is what @m-czernek wrote in #11315 as a reply to my comment to keep using the full path to snapper.

I'm not a fan of this approach. Yes, we know where snapper is. The point of the security PR was to always use full binary paths. The point of this PR [#11315] was to always use utils.find_binary. If we implement your command -p approach, I'm not a fan of having implicit reasons for where to use it and where not.

This kind of "use a security principle only when it would have been insecure otherwise" leads to inconsistencies in the future, when someone sees that snapper uses a full path instead of e.g. command -p, and will use full path for other binaries when modifying/creating a SLS.

Marek raises a good point, using the same approach everywhere will make it easier to maintain the SLS files in the future. We only touch these files from time to time and I share the concern that someone will pick the wrong implementation (full path where command -p would have been appropriate) in the future. Once we have reached a conclusion I'll put it in a README file in susemanager-sls for future reference, but documenting alone won't be enough. We all know how easy it is too miss the documentation, especially when you already know how to do something.

Is everyone fine with always using command -p <foo>? That will protect against shell functions, shell aliases and PATH modifications (in Bash command -p sets PATH to "/bin:/usr/bin:/sbin:/usr/sbin").

[rjmateus]

I prefer to have the same approach across all calls. We need to keep consistency as much as possible to make maintenance a bit easier.
From my point of view we can move forward with the approach @agraul sugested with the command -p <foo>.