Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add signed_by_[any|all] parameters to gpg.verify #63168

Merged
merged 22 commits into from
May 9, 2023

Conversation

lkubb
Copy link
Contributor

@lkubb lkubb commented Dec 1, 2022

What does this PR do?

Allows gpg.verify to additionally verify that good signatures come from a set of predefined public keys.

What issues does this PR fix or reference?

Fixes: #63166

Previous Behavior

Any key present in the keyring can provide a valid signature for anything.

New Behavior

It is possible to restrict the accepted signers to a predefined set of keys.

Notes

This is based off of #63152, so should be merged after that one.

Merge requirements satisfied?

Commits signed with GPG?

Yes

@lkubb lkubb marked this pull request as ready for review December 1, 2022 18:44
@lkubb lkubb requested a review from a team as a code owner December 1, 2022 18:44
@lkubb lkubb requested review from Ch3LL and removed request for a team December 1, 2022 18:44
Copy link
Contributor

@Ch3LL Ch3LL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question

salt/modules/gpg.py Show resolved Hide resolved
@lkubb
Copy link
Contributor Author

lkubb commented Dec 14, 2022

Huh, I accidentally performed a squash merge from master, which blew something up on Github. In the meantime, I force-pushed a normal merge, but somehow the first one triggered another reviewer to be requested as well. Sorry! =/

The new commit only applies the workaround to versions 0.5.0 and below (all currently released ones) and sets versionadded back to 3006.0.

@lkubb lkubb requested review from Ch3LL and removed request for s0undt3ch December 16, 2022 21:45
Ch3LL
Ch3LL previously approved these changes Jan 4, 2023
@garethgreenaway
Copy link
Contributor

@lkubb Just a quick question, are the two options signed_by_any and signed_by_all mutually exclusive or are they able to be specified together in a function call? I didn't see a check for the scenario when they're both included but wanted to double check.

@lkubb
Copy link
Contributor Author

lkubb commented Jan 4, 2023

@garethgreenaway They are able to be specified together:

https://github.com/lkubb/salt/blob/809c146c5a062cd9cf3efb07bb95f13c78d55435/salt/modules/gpg.py#L1286-L1289

Edit: You're right, I did not include a test when both are specified. Would you prefer I add one?

garethgreenaway
garethgreenaway previously approved these changes Jan 4, 2023
@garethgreenaway
Copy link
Contributor

@lkubb if it's not a issue to include both I think it's fine.

sig_info["fingerprint"] contains the actual signing key's fingerprint,
which might be a subkey. The primary key's fingerprint is always found in
sig_info["pubkey_fingerprint"]. In cases where a signing subkey was
used, the intended behavior is still comparison with the primary key.
@lkubb lkubb dismissed stale reviews from garethgreenaway and Ch3LL via f6b238e February 15, 2023 08:39
@lkubb
Copy link
Contributor Author

lkubb commented Feb 15, 2023

@garethgreenaway Sorry for dismissing the reviews, I had to push two slight modifications:

  • versionadded was 3006.0, but this PR is tagged with 3007.0
  • The comparison was done with the actual signing key's fingerprint which – in cases where a signing subkey was used – differs from the primary key's fingerprint. The expected behavior is to compare to the primary key, I assume. This just came up during usage.

@lkubb lkubb temporarily deployed to ci April 23, 2023 20:38 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci April 23, 2023 20:39 — with GitHub Actions Inactive
@lkubb lkubb temporarily deployed to ci April 23, 2023 20:39 — with GitHub Actions Inactive
@twangboy twangboy added this to the Chlorine v3007.0 milestone May 8, 2023
@Ch3LL Ch3LL merged commit 4d617dd into saltstack:master May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] Add parameters to limit gpg.verify to a set of explicit keys
4 participants