rpm_key: support multiple siging keys

[t-kranz]

SUMMARY

Fixes # #83394

Took the changes from #75251 (for #50615) - which implement checking for multiple key ids when importing keys with the rpm_key module (see #75251 (comment)):

Make rpm_key module use lists for key ids since a single key file can contain multiple keys.

I also made the fingerprint option accept lists of fingerprints since they correspond with key ids. All fingerprints in a keyfile should be present when the fingerprints option is set for task success.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

rpm_key

ADDITIONAL INFORMATION

Fixed the concerns from @sivel regarding # #75251:

• comparing the keys against the actual content of keyids instead of asserting the lengths are equal (Make rpm_key aware of multiple keys in a keyfile #75251 (comment))

• keeping the normalize_keyid function and creating a normalize_keyids funtion that calls the singular version for unit testing (Make rpm_key aware of multiple keys in a keyfile #75251 (comment))

Additional Info from #75251

Example:

- name: Add Google GPG keys to system
  rpm_key:
    key: https://dl.google.com/linux/linux_signing_key.pub
    fingerprint:
      # First key in keyfile
      - 4CCA 1EAF 950C EE4A B839 76DC A040 830F 7FAC 5991
      # Second key in keyfile
      - EB4C 1BFD 4F04 2F6D DDCC EC91 7721 F63B D38B 4796

The rpm_key module only checked for the presence of the first key in a file when importing keys from files. This meant that in cases where a single file contained multiple keys it was possible for later keys to not be imported if the first key in a key file was already installed. It could also mean that multiple keys could be installed even when only expecting a single key in a file.

[ansibot]

The test ansible-test sanity --test pep8 [explain] failed with 4 errors:

lib/ansible/modules/rpm_key.py:123:23: E201: whitespace after '['
lib/ansible/modules/rpm_key.py:123:27: E202: whitespace before ']'
lib/ansible/modules/rpm_key.py:242:29: W291: trailing whitespace
lib/ansible/modules/rpm_key.py:247:1: W293: blank line contains whitespace

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/modules/rpm_key.py:242:28: trailing-whitespace: Trailing whitespace
lib/ansible/modules/rpm_key.py:247:0: trailing-whitespace: Trailing whitespace

click here for bot help

[t-kranz]

I would like to remove the RHEL9 SHA1 deprecation from /test/integration/targets/rpm_key/tasks/rpm_key.yaml

To be able to do that i guess a GPG key file would need to be added to https://ci-files.testing.ansible.com/test/integration/targets/

That file needs to contain keys with at least two different fingerprints and those keys have to comply with the SHA-1 deprecation in RHEL 9.

The example ('Google GPG keys') from the original PR will result in

"msg": "warning: Signature not supported. Hash algorithm SHA1 not available.\nerror: /tmp/tmpuc4h77a5: key 2 import failed.\n"

[s-hertel]

Thanks for working on this. You should be able to swap this out for https://ansible-ci-files.s3.amazonaws.com/test/integration/targets/rpm_key/linux_signing_key.pub now and remove the few tasks preceding this one.

[webknjaz]

Could you rephrase this in non-imperative mood? The writing style is inconsistent with the following entry (and likely many other changelog fragments).
This is going to show up in the aggregated change log document on release. That document refers to the changes that have already happened since the previous release, so it's natural to describe things as if they are already merged. Imperative mood is a better fit for Git commit summary lines and TODO entries, but change logs have different style, purpose and audience.

[webknjaz]

Since this is now a list, seeing code iterating over a variable called as a single item reads somewhat counterintuitive.
May I suggest renaming the variable to fingerprints? Even if you keep the public interface with the original name. This will make the code easier to reason about.

[webknjaz]

Or even expected_fingerprints?

[webknjaz]

This var should probably be called differently. Currently, set(has_fingerprint) looks as if a boolean is passed to the set() constructor. Perhaps, computed_fingerprint?

[webknjaz]

Should this be something along the lines of

Suggested change

	if set(fingerprint) != set(has_fingerprint):
	if any(fgrprnt not in computed_fingerprint for fgrprnt in fingerprints):

?

Perhaps, this entire expression could be assigned to a variable, so its intended purpose could be labeled via a variable name?

[webknjaz]

Pro tip: mixing formatting changes with functional ones is discouraged because it makes it harder for the reviewers to identify which places in the patch are useful payload. These should always be separated.

[webknjaz]

This is what people usually expect in Python:

Suggested change

	if keyids == []:
	if not keyids:

[webknjaz]

this could be

Suggested change

	return len(missing_keys) == 0
	return not missing_keys

[webknjaz]

Though, I'd probably label the expression with what it intends to compute and return that:

Suggested change

	missing_keys = set(keyids).difference(imported_ids)
	return len(missing_keys) == 0
	some_gpg_keys_missing = bool(set(keyids) - imported_ids)
	return some_gpg_keys_missing

(I think that “return whether the length of a set of missing keys is zero” might be more difficult to parse vs. “return whether some GPG keys are missing” — it's low level vs. high level)

[deriamis]

Correct me if I'm wrong, but isn't this a breaking change?

I think the current case is that users would only need to list one of the keys which appears in the import, and this test suggests that all keys would need to be listed for verification. I can imagine scenarios where users upload all their signing keys to a single file and then check for different keys in different modules. This change would break that usage if I have how it currently works right.