PolicyContext: add new RequireSignatureVerification method#355
Conversation
I initially went that way so have code for this as well if we want to compare. Keeping it in draft for now until there's agreement on the approach. |
mtrmac
left a comment
There was a problem hiding this comment.
Thanks!
Just a very quick drive-by, looking at the implementation. The much more important part is designing the semantics of the new option, and I don’t have an opinion on that yet.
|
Thanks for starting this! I think in the general case what we also want here is something like |
I did briefly look at that. Worth noting that e.g. skopeo today has an |
|
There’s a degree of implementation difficulty for Podman: Skopeo has the policy configuration centralized, as a top-level option; AFAIK Podman does not, really. So, an option would have to be added to each subcommand individually, or the policy setup would need to be fairly significantly refactored. (Complicating this even more, for Podman, is the “remote” mode where the CLI is an API client to a server on a different VM / machine. Even if we did centralize the CLI handling, we would still need to add the “reject insecure” field (and the pre-existing “signature policy path”) to every single API operation individually. That’s one of the reasons the |
|
✅ A new PR has been created in buildah to vendor these changes: podman-container-tools/buildah#6409 |
aguidirh
left a comment
There was a problem hiding this comment.
LGTM, I left only a nit.
ef7b873 to
3b44b0b
Compare
|
✅ A new PR has been created in buildah to vendor these changes: podman-container-tools/buildah#6504 |
|
Thanks for the feedback! I changed tactics and flipped this around. I.e. rather than forbidding insecure policy requirements (which is/may become ambiguous), the new knob now requires signature verification policy requirements (and I tried to define it clearly in the docstrings to imply the signature must cover the entire contents). I also rebased. (Sorry, I should've done it in two separate pushes to make it easier to see the real changes.) |
3b44b0b to
47ee7e4
Compare
SetRejectInsecure methodRequireSignatureVerification method
mtrmac
left a comment
There was a problem hiding this comment.
I’ll re-read this more carefully next week, but looks great from a quick skim.
mtrmac
left a comment
There was a problem hiding this comment.
One last thing:
Just to hold the line on “c/image/signature, at the very least in the policy enforcement code, should have code coverage whenever reasonably possible”, please add a bunch of one-line TestPR…VerifiesSignatures tests.
In bootc, we want the ability to assert that signature verification is enforced, but there are no mechanisms for this in the library. Add a new `RequireSignatureVerification` method on the `PolicyContext` object which would allow this. Add a new `isSigned` method on the `PolicyRequirement` interface which then allows `IsRunningImageAllowed` to detect if at least one requirement performed signature verification. Test generation was `Assisted-by: Claude Code v1.0.120`. Part of podman-container-tools/skopeo#1829. Signed-off-by: Jonathan Lebon <jonathan@jlebon.com>
47ee7e4 to
9ccdd04
Compare
|
Updated! |
In bootc, we want the ability to assert that signature verification is
enforced, but there are no mechanisms for this in the library.
Add a new
RequireSignatureVerificationmethod on thePolicyContextobject which would allow this.
Add a new
isSignedmethod on thePolicyRequirementinterfacewhich then allows
IsRunningImageAllowedto detect if at least onerequirement performed signature verification.
Test generation was
Assisted-by: Claude Code v1.0.120.Part of podman-container-tools/skopeo#1829.