Support checking for expired and untrusted keys #9950
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
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?
|
Thanks for the quick review, Felix.
I agree that a separate state is not very intuitive. IMHO this should just be the default behavior of the 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. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
I'm happy with this PR. :) |
| 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) |
There was a problem hiding this comment.
You might want to consider using CmdRunner. See https://docs.ansible.com/ansible/latest/collections/community/general/docsite/guide_cmdrunner.html for more details.
plugins/modules/pacman_key.py
Outdated
| @@ -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). | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
plugins/modules/pacman_key.py
Outdated
| @@ -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). | |||
There was a problem hiding this comment.
You should mention when trusted was added:
| - 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
Adds state
trusted.Fixes #9949
SUMMARY
Adds functionality for
pacman_keyto check for key validity in terms of expiration and signature of the pacman machine key.Introduces a new
trustedstate for the new functionality in order to preserve current behavior ofpresent, although this should arguably be merged intopresent.Moves
gpg/pacman-keyparsing into a trivial wrapper (GpgListResult) and adds two helpers that either return a single match or a collection of all matches for a certaingpgoutput item type.Moves binary invocations with common flags into module methods.
Replaces
--keyringwith equivalent--homedirflag in order to allow for signature checks to work.ISSUE TYPE
COMPONENT NAME
pacman_keyADDITIONAL 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 .