-
Notifications
You must be signed in to change notification settings - Fork 3k
Add --sign-by-sq-fingerprint #26618
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 --sign-by-sq-fingerprint #26618
Conversation
dcc86fe to
e546425
Compare
|
And FWIW, RHEL will probably not have Sequoia on board until at least 9.8 and 10.2. |
| DigestFile string | ||
| TLSVerifyCLI bool // CLI only | ||
| CredentialsCLI string | ||
| signing common.SigningCLIOnlyOptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a problem, just curious why this isn't "Signing"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think conceptually this is a package-private type / field, and in such a situation using a package-private symbol helps linters detect unused / write-only values. (Compare the unused fields in pkg/domain/entities in #26617, although it’s unclear whether such a policy would have helped there.)
But, also, I’m not currently very interested in consistently cleaning up the whole type.
If consistency is preferred, I can make it a Signing type.
| #### **--sign-by-sq-fingerprint**=*fingerprint* | ||
|
|
||
| Add a “simple signing” signature using a Sequoia-PGP key with the specified fingerprint. | ||
| (This option is not available with the remote Podman client, including Mac and Windows (excluding WSL2) machines) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| (This option is not available with the remote Podman client, including Mac and Windows (excluding WSL2) machines) | |
| This option is not available with the remote Podman client, including Mac and Windows (excluding WSL2) machines. |
Not a strong suggestion, but I'd remove the first set of parens here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Searching for “This option is not available with”, there are ~40 other instances which parenthesize this, or a similar, remark.
I have no preference at all as to whether this should be parenthesized.
|
Not a detailed review, but looks good over all. I would like to NOT merge this until after we move Podman v5.6 to it's own RHEL branch, probably mid to late August. This should be aimed at Podman v5.7 or 6.0 at the earliest. |
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
|
As of e546425 , the new integration test is correctly skipped because the Sequoia implementation is not enabled. |
1749d9e to
adec92c
Compare
b6aed2e to
6e2fd88
Compare
64717ad to
908fc92
Compare
deeb2b5 to
5d2682d
Compare
This build tag replaces the backend for _verification_ of GPG signatures, to use Sequoia-PGP instead of GNUPG. Do Rawhide builds with Sequoia; the podman-sequoia package exists in F43 and later, so we can't do it in earlier versions. This way we cover both variants (+ containers_image_openpgp in the podman-remote client, at least that it builds). Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This adds a new feature that allows signing using Sequoia-backed keys. The existing options to sign using GPG-backed keys (and sigstore) remain unchanged, and continue to use the same backends as usual. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
|
Ready for review, @containers/podman-maintainers PTAL. This works, but I’m not very familiar with the CI / build architecture, I might well be missing something. |
|
@containers/podman-maintainers review please. |
Luap99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
@containers/podman-maintainers please review+merge. |
aguidirh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Luap99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, mtrmac The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
On top of #26617 .
Right now, this does not build Podman with the Sequoia backend, and thus does not work; I’ll update this PR with build changes as well. Compare also the packaging / CI discussion in containers/skopeo#2645 .
Does this PR introduce a user-facing change?