Use more secure random generation in Python code

[meaksh]

What does this PR change?

This PR makes random generation more secure in certain parts of Python code, as warned by SonarQube analysis:

• https://sonarcloud.io/project/security_hotspots?id=uyuni-project_uyuni&hotspots=AYMYhbVO5TQr2KFg-1yn
• https://sonarcloud.io/project/security_hotspots?id=uyuni-project_uyuni&hotspots=AYMYhbVO5TQr2KFg-1yo
• https://sonarcloud.io/project/security_hotspots?id=uyuni-project_uyuni&hotspots=AYMYhb5p5TQr2KFg-2VI
• https://sonarcloud.io/project/security_hotspots?id=uyuni-project_uyuni&hotspots=AY1Basdxy5vaCD3iDu6p
• https://sonarcloud.io/project/security_hotspots?id=uyuni-project_uyuni&hotspots=AY1Barc9y5vaCD3iDu1i

GUI diff

No difference.

• DONE

Documentation

• No documentation needed: only internal and user invisible changes

• 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: no tests around randomness

• DONE

Links

• 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 "javascript_lint"
• Re-run test "spacecmd_unittests"

Before you merge

Check How to branch and merge properly!

[m-czernek]

+1, thank you for looking into this!

[agraul]

LGTM, but I have two questions.

1. Do we need to invalidate caches for the Proxy or will this work automatically on updade + service restart?

2. Do we make sure that all Salt event queues are empty before updating the queues that events go into?

The reason we use a checksum for the event queues is just this: We want to have all events from the same minion in the same queue. This approach distributes the minions across the available queues. I suspect that the first 4 chars of a hex-sha256sum is generally different from the first 4 chars of a hex-md5sum of the same input. When this patch is installed, we can have a race condition if old events are still queued. For example, let's say minion "foo"'s queue changes from 1 to 3. If 1 still has event data for "foo" (but other event data ahead in the queue), new event data added to queue 3 could be processed before the old data in queue 1.

[agraul]

Can we check if this comment can be removed?

~uyuni/uy-work[pylint-everything]
% bb pylint --help-msg=E0611
:no-name-in-module (E0611): *No name %r in module %r*
  Used when a name cannot be found in a module. This message belongs to the
  variables checker.

[meaksh]

Yeah, it can be removed.

[meaksh]

LGTM, but I have two questions.

1. Do we need to invalidate caches for the Proxy or will this work automatically on updade + service restart?

Not really, this cache is actually persisted on the filesystem. We might end up with some stale cache files.

2. Do we make sure that all Salt event queues are empty before updating the queues that events go into?

We should not really make the mgr_events engine to wait until the Salt event queues are empty as the engine needs to keep processing other events. I understand the posibility of the race condition you mentioned, but that is very unlikely to happen, as spacewalk services are stopped during the upgrade, and I'd expect not too many events in the DB queue at the time of restarting the services. So, in case it happen for a couple of events, I think it won't cause any major issue. Do you have any idea in mind?

[agraul]

LGTM, but I have two questions.

1. Do we need to invalidate caches for the Proxy or will this work automatically on updade + service restart?

Not really, this cache is actually persisted on the filesystem. We might end up with some stale cache files.

So we need to invalidate caches?

2. Do we make sure that all Salt event queues are empty before updating the queues that events go into?

We should not really make the mgr_events engine to wait until the Salt event queues are empty as the engine needs to keep processing other events.

The point is that we're changing the logic and we know this is a race condition. If you think the likelihood of this affecting anything negatively is too low to worry about it, then proceed as planned. I just wanted to raise this point and make sure this is a conscious decision and we're prepared if this goes wrong for someone.

[meaksh]

@agraul this should be now performing the invalidation/deletion of possible old stale proxy auth cache files.

About the race condition, as mentioned, I think we should be good. In case the old events are unrelated to the new events, and the new events are processed before the old ones, then we would just have the new action results shown first in the event history.

If the old and new events are somehow related, for instance, because we are in the middle of a registration/action chain execution, then I would expect that the action could fail, but I'd also expect that anyway (as you are restarting the "spacewalk-services" in the middle of the execution of those actions).

[github-actions]

⚠️ Some changelog tests have failed. @meaksh, please review and fix the changelog issues with an additional PR.