pacman_key: support checking for expired and untrusted keys #9950

AlD · 2025-03-30T23:20:28Z

Adds state trusted.

SUMMARY

Adds functionality for pacman_key to check for key validity in terms of expiration and signature of the pacman machine key.

Introduces a new trusted state for the new functionality in order to preserve current behavior of present, although this should arguably be merged into present.

Moves gpg/pacman-key parsing into a trivial wrapper (GpgListResult) and adds two helpers that either return a single match or a collection of all matches for a certain gpg output item type.

Moves binary invocations with common flags into module methods.

Replaces --keyring with equivalent --homedir flag in order to allow for signature checks to work.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

pacman_key

ADDITIONAL INFORMATION

The existing test output already used an expired key, hence the GPG_SHOWKEY_OUTPUT{ → _EXPIRED} renaming of the variable holding the unchanged test data .

ansibullbot · 2025-03-30T23:25:44Z

cc @grawlinson
click here for bot help

felixfontein

Thanks for your contribution! I've added some first comments.

Regarding the interface: I'm not sure whether a new state is the best way to do this. I think it would be better to be able to specify the update mode, because what you want to do is tell the module to update the key if the fingerprint matches, but other parameters do not.

Right now the module supports two update modes: "always" (force_update=true), and "compare fingerprint" (force_update=false). Having another mode "full idempotent" (checks ID and other parameters) is basically what you want, I think?

changelogs/fragments/9950-pacman_key-verify-key-validity.yml

plugins/modules/pacman_key.py

AlD · 2025-03-31T22:17:32Z

Thanks for the quick review, Felix.

Regarding the interface: I'm not sure whether a new state is the best way to do this. I think it would be better to be able to specify the update mode, because what you want to do is tell the module to update the key if the fingerprint matches, but other parameters do not.

I agree that a separate state is not very intuitive. IMHO this should just be the default behavior of the present state. I can't really think of a realistic scenario where a user would prefer using force_update to update an expired key.

In that vein I wouldn't even make this new functionality configurable, because even if a user prefers to keep an expired key, having an import attempt of the expired key on each run is likely not noticeable.

grawlinson · 2025-04-01T04:39:59Z

I'm happy with this PR. :)

russoz

Small suggestion on handling the gpg command, other than that I do not have any strong opinion about solving this with an extra state or some other way, so I'll say it LGTM

russoz · 2025-04-05T01:19:37Z

plugins/modules/pacman_key.py

+    def gpg(self, args, keyring=None, **kwargs):
+        cmd = [self.gpg_binary]
+        if keyring:
+            cmd.append('--homedir={keyring}'.format(keyring=keyring))
+        cmd.extend(['--no-permission-warning', '--with-colons', '--quiet', '--batch', '--no-tty'])
+        return self.module.run_command(cmd + args, **kwargs)


You might want to consider using CmdRunner. See https://docs.ansible.com/ansible/latest/collections/community/general/docsite/guide_cmdrunner.html for more details.

felixfontein · 2025-04-12T16:04:54Z

plugins/modules/pacman_key.py

@@ -78,9 +78,9 @@
    default: /etc/pacman.d/gnupg
  state:
    description:
-      - Ensures that the key is present (added) or absent (revoked).
+      - Ensures that the key is present (added), trusted (signed and not expired) or absent (revoked).


How does the PR ensure that the key is not expired? If the key provided to the module is already expired, I don't see how the module can change that.

It can't extend keys of course, you're right. It just won't return a success status if the key is expired.

Maybe "Ensures/checks" would be clearer? How long are these descriptions supposed to be?

The descriptions should be long enough that it's clear what the option actually does.

I would move the description for trusted into a new paragraph (instead of cramming everything into a single paragraph).

felixfontein · 2025-04-12T16:05:44Z

plugins/modules/pacman_key.py

@@ -78,9 +78,9 @@
    default: /etc/pacman.d/gnupg
  state:
    description:
-      - Ensures that the key is present (added) or absent (revoked).
+      - Ensures that the key is present (added), trusted (signed and not expired) or absent (revoked).


You should mention when trusted was added:

Suggested change

- Ensures that the key is present (added), trusted (signed and not expired) or absent (revoked).

- Ensures that the key is V(present) (added), V(trusted) (signed and not expired) or V(absent) (revoked).

- The state V(trusted) has been added in community.general 10.6.0.

Also formatting the state names as values is IMO a good idea.

If you're not opposed to folding the new functionality into the present state, I think that would be preferable. IMHO the currently implemented functionality of the present state does not solve a real life use case.

I think this would be a breaking change, since this considerably changes the behavior of state=present. Since we don't accept breaking changes this isn't really acceptable.

(That's why I would solve this not with a new state, but with an option. That way we can deprecate its default, and eventually change it to the better behavior, and fix the issue that way.)

Thanks! Finally got around to making the change to make it an option now.

I squashed all commits now, but I could restore the individual commits as well of course. Just let me know what's easier to review. Not super familiar with GitHub review workflows.

Adds option `ensure_trusted`. Fixes ansible-collections#9949

russoz

hi @AlD

A couple of suggestions, mostly about naming and readability, but also some simplifications.

plugins/modules/pacman_key.py

tests/unit/plugins/modules/test_pacman_key.py

russoz · 2025-05-10T10:13:11Z

plugins/modules/pacman_key.py

+    def key_is_trusted(self, keyring, keyid):
+        """Check if key is signed and not expired."""
+        unused_rc, stdout, unused_stderr = self.gpg(['--check-signatures', keyid], keyring=keyring)
+        return self.pacman_machine_key(keyring) in gpg_gather_all(stdout.splitlines(), 'sig', 'key')


The function gpg_gather_all() does more than just gathering, it also filters. TBH, I think that function is doing more than it should and the code would be leaner if it only returned the list of GpgListResult objects. Then, the code here could be changed to:

Suggested change

return self.pacman_machine_key(keyring) in gpg_gather_all(stdout.splitlines(), 'sig', 'key')

return self.pacman_machine_key(keyring) in [x.key for x in gpg_gather_all(stdout.splitlines() if x.kind == 'sig')

I think that makes it easier for the reader to understand what is really going on.

Same applies to line 341 above.

My thinking was that it gathers the key of all kind GPG results. I'm not really convinced that the list comprehension is easier to understand tbh.

Well, list comprehensions some people love them, some people don't. You can guess my type ;-).

Anyways, I'm OK without the comprehension, but simple fact that you had to add more words to explain what the function does is a good sign that its name could be improved. Maybe gather_key_from_all() ? Your thinking makes sense for you who wrote this about now. Someone else, one year from now, will read this code out of the blue and they will have to go up and down the file, read that function, see what it does, just to figure out what took a couple of words to explain. So, the point is, do the explaining now, save everybody else (including yourself 1, 3 or 5 years from now) that time later on.

russoz · 2025-05-10T10:15:49Z

plugins/modules/pacman_key.py

+def gpg_gather_all(lines, kind, attr):
+    result = []
+    for line in lines:
+        glr = GpgListResult(line)
+        if glr.kind == kind:
+            result.append(getattr(glr, attr))
+    return result


As suggested before, this could/should be simpler, as:

Suggested change

def gpg_gather_all(lines, kind, attr):

result = []

for line in lines:

glr = GpgListResult(line)

if glr.kind == kind:

result.append(getattr(glr, attr))

return result

def gpg_gather_all(lines):

return [GpgListResult(line) for line in lines]

russoz · 2025-05-10T10:17:06Z

plugins/modules/pacman_key.py

+def gpg_get_first(lines, kind, attr):
+    for line in lines:
+        glr = GpgListResult(line)
+        if glr.kind == kind:
+            return getattr(glr, attr)


And that would make this one:

Suggested change

def gpg_get_first(lines, kind, attr):

for line in lines:

glr = GpgListResult(line)

if glr.kind == kind:

return getattr(glr, attr)

def gpg_get_first(lines, kind, attr):

for glr in gpg_gather_all(lines):

if glr.kind == kind:

return getattr(glr, attr)

russoz · 2025-05-10T10:17:47Z

plugins/modules/pacman_key.py

+            return getattr(glr, attr)
+
+
+def gpg_gather_all(lines, kind, attr):


Maybe rename it to parse_gpg_list?

plugins/modules/pacman_key.py

russoz · 2025-05-10T10:25:54Z

plugins/modules/pacman_key.py

            if key_present:
                self.remove_key(keyring, keyid)
                module.exit_json(changed=True)
            module.exit_json(changed=False)

+    def gpg(self, args, keyring=None, **kwargs):


Apparently **kwargs is here just to handle the different cases of check_rc. That being the case, I would suggest to drop the ** thing and make it explicit:

Suggested change

def gpg(self, args, keyring=None, **kwargs):

def gpg(self, args, keyring=None, check_rc=True):

or whichever default value you prefer, then adjust the calls to that method accordingly - I mean, the calls will work as they are right now, but you might want to remove the extra parameter for the default value (True in my suggestion).

The function doesn't do anything with check_rc. IMHO **kwargs conveys that it's only passed through.

@russoz means that kwargs right now is only ever used for check_rc, so it's better to make this more explicit.

The same is true for pacman_key()'s **kwargs.

Yeah, I got that. What I was trying to say was that both functions are just wrappers around gpg/pacman-key via module.run_command, and having the parameters of the wrapped function not named explicitly helps conveying which function (wrapper/wrapped) owns which set of parameters. I acknowledge that the benefit may be slim as the size of each set is currently small (2 and 1).

For some empirical evidence, the top answers in https://stackoverflow.com/questions/1415812/why-use-kwargs-in-python-what-are-some-real-world-advantages-over-using-named for example mention wrappers as a valid use case for usage of **kwargs, so I think the use here matches common expectations.

In addition to the semantics it e.g. enables IDEs that support it to show all parameters + any present doc strings supported by run_command on these functions, without having to duplicate them.

That said, if you feel strongly about it, happy to change it. Just wanted to clarify that it was a deliberate choice.

Not that strong. I still think the readability would be improved by naming - or removing that parameter as it is not used - but happy to merge the PR as-is wrt this.

AlD · 2025-05-13T22:02:33Z

Thanks a lot for the suggestions. Happy to discuss everything further of course.

russoz · 2025-05-25T03:15:05Z

plugins/modules/pacman_key.py

+      - Ensure that the key is trusted (signed by the Pacman machine key and not expired).
+    type: bool
+    default: false
+    version_added: 10.7.0


That ship has sailed, please update to

Suggested change

version_added: 10.7.0

version_added: 11.0.0

AlD force-pushed the trusted branch from 2f106ff to fbf5f16 Compare March 30, 2025 23:23

ansibullbot added feature This issue/PR relates to a feature request module module new_contributor Help guide this first time contributor plugins plugin (any type) tests tests unit tests/unit labels Mar 30, 2025

This comment was marked as outdated.

Sign in to view

ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Mar 30, 2025

felixfontein reviewed Mar 31, 2025

View reviewed changes

changelogs/fragments/9950-pacman_key-verify-key-validity.yml Outdated Show resolved Hide resolved

plugins/modules/pacman_key.py Outdated Show resolved Hide resolved

plugins/modules/pacman_key.py Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Mar 31, 2025

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-10 Automatically create a backport for the stable-10 branch labels Apr 1, 2025

russoz reviewed Apr 5, 2025

View reviewed changes

felixfontein reviewed Apr 12, 2025

View reviewed changes

ansibullbot added stale_ci CI is older than 7 days, rerun before merging and removed stale_ci CI is older than 7 days, rerun before merging labels Apr 12, 2025

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Apr 23, 2025

Support checking for expired and untrusted keys

0c7d8bc

Adds option `ensure_trusted`. Fixes ansible-collections#9949

AlD force-pushed the trusted branch from 1b7df50 to 0c7d8bc Compare May 5, 2025 23:00

ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label May 5, 2025

russoz changed the title ~~Support checking for expired and untrusted keys~~ pacman_key: support checking for expired and untrusted keys May 10, 2025

russoz reviewed May 10, 2025

View reviewed changes

Update ensure_trusted option documentation

76c1dad

ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels May 13, 2025

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label May 23, 2025

felixfontein removed the backport-10 Automatically create a backport for the stable-10 branch label May 24, 2025

russoz reviewed May 25, 2025

View reviewed changes

	- Ensures that the key is present (added), trusted (signed and not expired) or absent (revoked).
	- Ensures that the key is V(present) (added), V(trusted) (signed and not expired) or V(absent) (revoked).
	- The state V(trusted) has been added in community.general 10.6.0.

	return self.pacman_machine_key(keyring) in gpg_gather_all(stdout.splitlines(), 'sig', 'key')
	return self.pacman_machine_key(keyring) in [x.key for x in gpg_gather_all(stdout.splitlines() if x.kind == 'sig')

		return getattr(glr, attr)


		def gpg_gather_all(lines, kind, attr):

	def gpg(self, args, keyring=None, **kwargs):
	def gpg(self, args, keyring=None, check_rc=True):

pacman_key: support checking for expired and untrusted keys #9950

Are you sure you want to change the base?

pacman_key: support checking for expired and untrusted keys #9950

Uh oh!

Conversation

AlD commented Mar 30, 2025

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

Uh oh!

ansibullbot commented Mar 30, 2025

Uh oh!

This comment was marked as outdated.

felixfontein left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlD commented Mar 31, 2025

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

grawlinson commented Apr 1, 2025

Uh oh!

russoz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

russoz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

russoz May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlD May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlD commented May 13, 2025

russoz May 23, 2025 •

edited

Loading

AlD May 20, 2025 •

edited

Loading