Pr1/3 : add GPG key management operations and facts #1460
Conversation
maisim
force-pushed
the
pr1-add-gpg-operations
branch
4 times, most recently
from
5585d21 to
8d3ea39
Compare
|
Hi @DonDebonair ! |
|
Thanks for the PR @maisim . I would not be afraid to use facts inside operations. I actually consider it a best practice to use facts to check things in operations. So instead of doing checks inside the shell commands you yield, there are probably places where you can rely on facts instead. That makes the yielded commands simpler. If you can change those already, I'll do a more full review this weekend. Caveat btw: I'm by no means a GPG expert. A short while ago, I wanted to use pyinfra to install Docker on a Debian host. I just created the keyring directory and downloaded the key directly into that, never touching GPG 😅 |
src/pyinfra/operations/gpg.py
Outdated
| for kid in keyid: | ||
| # Remove key from all matching keyrings | ||
| yield ( | ||
| f'for keyring in {pattern}; do [ -e "$keyring" ] && ' |
There was a problem hiding this comment.
Doesn't it make more sense to retrieve keyrings through a fact and then only try to remove the key from keyrings that actually exist?
Or maybe even better, create a fact that lists all known keyrings and then use the GpgKeys fact for each of those keyrings to get all the keys. Then you can specifically remove the keyid if it exists
There was a problem hiding this comment.
Good idea. Since there’s no global or common location for keyrings, this will allow specifying the directories to search in — for example, /etc/apt/trusted.gpg.d/ and /usr/share/keyrings/ in the APT context.
src/pyinfra/operations/gpg.py
Outdated
|
|
||
| # Clean up empty keyrings | ||
| yield ( | ||
| f'for keyring in {pattern}; do [ -e "$keyring" ] && ' |
There was a problem hiding this comment.
As referenced in the other comment, if you can list all keys by keyring, then you know if keyrings are going to be empty after removing keyid from those rings and you can remove them based on that
| yield (f'if [ -f "{dest}" ] && ({condition}); then ' f'rm -f "{dest}"; fi') | ||
| return | ||
|
|
||
| elif dest and not keyid: |
There was a problem hiding this comment.
This could be else instead of elif. To me, that seems cleaner because it clearly signals there are only 3 possible combinations of conditions: keyid is provided and dest isn't, keyid and dest are both provided and lastly, dest is provided and keyid isn't
There was a problem hiding this comment.
Yes, but it is more an "documentation" elif. To be more explicit.
What about add an else wich raise an exception if we go into an unhandle case?
src/pyinfra/operations/gpg.py
Outdated
| elif dest and keyid: | ||
| # For APT keyring files, use a simpler approach: | ||
| # Check if keys exist in file, and if so, remove the entire file | ||
| # This is appropriate for most APT use cases |
There was a problem hiding this comment.
I'm a bit concerned about "most APT use cases" here. Is there a situation we might be removed a keyfile/keyring that has multiple keys in it?
|
|
||
|
|
||
| @operation() | ||
| def key( |
There was a problem hiding this comment.
This is a very complex function that covers many different scenarios. I think that from a public API standpoint, that is fine, because we're dealing with 1 resource: keys. But you could break up the function into separate functions that are all called from the key() function, based on the scenario.
See the docker operations for an example of how this can be done
|
Left some more comments @maisim incl. some ideas on how to leverate facts. |
maisim
force-pushed
the
pr1-add-gpg-operations
branch
from
1f73888 to
55aca14
Compare
maisim
changed the title
- Add GpgFactBase, GpgKey, GpgKeys, GpgSecretKeys, and GpgKeyrings facts - Add gpg.key and gpg.dearmor operations for managing GPG keys - Support for keyserver fetching, local files, URLs, and key removal - Comprehensive test coverage for all GPG operations and facts
- Break down complex key() function into smaller, focused helper functions - Add _install_key_from_src() for file/URL installation - Add _install_key_from_keyserver() for keyserver installation - Add _remove_key_from_keyrings() for multi-keyring removal - Add _remove_key_from_keyring() for single keyring removal - Add _remove_keyring_file() for complete keyring removal - Add validation helper functions for parameters - Improve code readability and maintainability - Follow Docker operations pattern for function organization - Maintain 100% backward compatibility with existing API
GPG key IDs contain hexadecimal values that trigger false positives in typos (e.g., 'BA' in '1E9377A2BA9EF27F' is flagged as misspelled). These are valid hex values, not typos.
maisim
force-pushed
the
pr1-add-gpg-operations
branch
from
80ac55d to
2930027
Compare
2 participants
Add new gpg.key and gpg.dearmor operations to manage GPG keys and keyrings. These operations provide a modern alternative to apt-key for managing APT repository keys.
Features:
This is part 1/3 of modernizing APT key management.
3.xat this time)scripts/dev-test.sh)scripts/dev-lint.sh)