PolicyContext: add new SetRejectInsecure 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. |
| reqs = filteredReqs | ||
| } | ||
|
|
||
| if len(reqs) == 0 { |
There was a problem hiding this comment.
The filtering approach would hit this, and end up with a confusing error message.
Ultimately I think it would be simpler to pass the PolicyContext into the methods of PolicyRequirement — and it would be consistent with the wanted #279 . Then only the insecureAcceptAnything requirement would need its implementation extended.
(Alternatively, we could add a “filter against PolicyContext” method to PolicyRequirement, but I don’t think we need such a generalization yet. That does, though, depend on the semantics of the option.)
There was a problem hiding this comment.
Ultimately I think it would be simpler to pass the PolicyContext into the methods of PolicyRequirement. ... Then only the insecureAcceptAnything requirement would need its implementation extended.
I started down that road, but it's more subtle than that unfortunately. If we make insecureAcceptAnything deny the image when rejectInsecure is true, the policy check immediately ends. Whereas the semantic I'm going for here is more that e.g. if there is both a prInsecureAcceptAnything and a prSignedBy, the prSignedBy should still be evaluated. Which is closer to filtering conceptually.
Or did you mean something else?
(Alternatively, we could add a “filter against PolicyContext” method to PolicyRequirement, but I don’t think we need such a generalization yet. That does, though, depend on the semantics of the option.)
I went with a variant of this, but instead of literally filtering the requirements list, which felt awkward, I added a concept of a secure vs insecure requirement. Let me know what you think.
There was a problem hiding this comment.
Whereas the semantic I'm going for here is more that e.g. if there is both a
prInsecureAcceptAnythingand aprSignedBy, theprSignedByshould still be evaluated.
Interesting. I’d be tempted to declare that “unlikely to happen in practice, and indication of a possibly-confused user”, but, pedantically, accepting this situation does make sense.
This implementation approach LGTM, and this definition of “insecure” seems reasonable.
I’ll return to this in a few days — probably looking at maintainability concerns like option naming and tests; at a high level, this makes sense.
There was a problem hiding this comment.
Interesting. I’d be tempted to declare that “unlikely to happen in practice, and indication of a possibly-confused user”, but, pedantically, accepting this situation does make sense.
I could imagine that happening in the future if we support some sort of policy .d dropin merging.
|
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 |
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 `SetRejectInsecure` method on the `PolicyContext` object which would allow this. Add a new `isInsecure` method on the `PolicyRequirement` interface which then allows `IsRunningImageAllowed` to detect if at least one secure requirement was present. Test generation was `Assisted-by: Claude Code v1.0.120`. Part of containers/skopeo#1829. Signed-off-by: Jonathan Lebon <[email protected]>
|
✅ A new PR has been created in buildah to vendor these changes: containers/buildah#6409 |
| } | ||
|
|
||
| func (pr *prSignedBaseLayer) isInsecure() bool { | ||
| return false |
There was a problem hiding this comment.
Deciding whether “signed base layer” is “secure” is … a weird question. Not really an interesting question, given that this is an unusable stub…
There was a problem hiding this comment.
Yeah, I guess I should probably just flip this to true for now and add a FIXME here too to reconsider once it's implemented. It definitely stresses the binary secure vs insecure logic, so it might have to be reworked a bit at that point based on what semantics we want.
| reqs = filteredReqs | ||
| } | ||
|
|
||
| if len(reqs) == 0 { |
There was a problem hiding this comment.
Whereas the semantic I'm going for here is more that e.g. if there is both a
prInsecureAcceptAnythingand aprSignedBy, theprSignedByshould still be evaluated.
Interesting. I’d be tempted to declare that “unlikely to happen in practice, and indication of a possibly-confused user”, but, pedantically, accepting this situation does make sense.
This implementation approach LGTM, and this definition of “insecure” seems reasonable.
I’ll return to this in a few days — probably looking at maintainability concerns like option naming and tests; at a high level, this makes sense.
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
SetRejectInsecuremethod on thePolicyContextobject which would allow this.Since this only changes the behaviour of the
insecureAcceptAnythingpolicy requirement, rather than extending the policy requirement interface, I went with a filtering approach directly inIsRunningImageAllowed().Test generation was
Assisted-by: Claude Code v1.0.120.Part of containers/skopeo#1829.