-
Notifications
You must be signed in to change notification settings - Fork 372
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
Check for unsigned images #2035
base: main
Are you sure you want to change the base?
Conversation
policy_eval: Checks for insecureAcceptAnything defined in policy.json for the target image Signed-off-by: RishabhSaini <rsaini@redhat.com>
@@ -253,6 +253,32 @@ func (pc *PolicyContext) GetSignaturesWithAcceptedAuthor(ctx context.Context, pu | |||
return res, nil | |||
} | |||
|
|||
// IsImagePolicySigned true iff the policy requirement for the image is not insecureAcceptAnything | |||
func (pc *PolicyContext) IsImagePolicySigned(publicImage types.UnparsedImage) (res bool, finalErr error) { |
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'd say let's not replicate the "bool and error" that we now think was a mistake.
IOW, let's call this RequireSignedPolicyFor
and have it just return error
.
@@ -253,6 +253,32 @@ func (pc *PolicyContext) GetSignaturesWithAcceptedAuthor(ctx context.Context, pu | |||
return res, nil | |||
} | |||
|
|||
// IsImagePolicySigned true iff the policy requirement for the image is not insecureAcceptAnything |
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 it'd be easy to add unit tests for this in policy_eval_test.go
reqs := pc.requirementsForImageRef(image.Reference()) | ||
|
||
for _, req := range reqs { | ||
if req == NewPRInsecureAcceptAnything() { |
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.
Super minor nit, I think we can avoid instantiating a new instance of this on each iteration of the loop. IOW something more like:
insecurePolicy := NewPRInsecureAcceptAnything()
for _, req := range reqs {
if req == insecurePolicy {
...
}
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 don’t know what’s a good API here. We need to precisely formulate the security requirement, and then turn that in to a precise predicate. This is a plausible implementation of something, but at least the predicate being implemented is unclear.
- Code in c/image/signature needs full unit test code coverage (or be clearly unreachable, potentially with a comment indicating why.)
@@ -253,6 +253,32 @@ func (pc *PolicyContext) GetSignaturesWithAcceptedAuthor(ctx context.Context, pu | |||
return res, nil | |||
} | |||
|
|||
// IsImagePolicySigned true iff the policy requirement for the image is not insecureAcceptAnything | |||
func (pc *PolicyContext) IsImagePolicySigned(publicImage types.UnparsedImage) (res bool, finalErr error) { |
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’m not really expressing an opinion on the API at this point. Should this be a flag to IsRunningImageAllowed
?)
But at the very least, the function name must not be misleading. This one would succeed on a “policy: reject”, so it is clearly not “is the policy requiring signed images”.
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.
Should this be a flag to IsRunningImageAllowed
Yes, I think that'd be better. Well, we can't change that API now, but hat we could do is introduce a new, IsRunningImageAllowedRequireSignature
right?
Or maybe what would be nicer even is something like EvaluatePolicyForImage()
and then we have a IsInsecure()
method on the returned policy object? (This looks like it'd require making the policy objects public, but maybe we could have a smaller API surface for that)
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.
IsRunningImageWithOptions
… “functional options” would probably allow more long-term extensibility, than adding a single option to the function name. (OTOH it requires tedious code to define the functional options.)
and then we have a IsInsecure() method on the returned policy object?
I’m not much of a fan of the policy evaluation returning a multi-valued object, with callers making their own policy decisions on top.
What if we flipped this around, and added a PolicyContext.Require{SignatureValidationToBeEnforced,InsecureAllowAnythingNotToBeUsed}
(whatever we actually want)? That would imply that a single caller with a single PolicyContext
always has the same opinion about this policy — and that actually seems appropriate. (A caller can always create two separate PolicyContext
objects.)
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 just discovered GetSignaturesWithAcceptedAuthor
- but apparently nothing uses it?
But it seems to be related in that what we want is "evaluate the policy and require signatures".
One thing I also want is something like a string
description for the successful case. If you look at e.g.
[root@cosa-devsh ~]# rpm-ostree status
State: idle
Deployments:
● fedora:fedora/x86_64/coreos/testing
Version: 38.20230514.2.0 (2023-05-15T17:23:12Z)
Commit: d640da041e110e1abaa8a88cf876d68165e02ad9e1294a6ba8babb9d7a32ed39
GPGSignature: Valid signature by 6A51BBABBA3D5467B6171221809A8D7CEB10B464
[root@cosa-devsh ~]#
The GPGSignature
thing there is the success output from gpg.
And that's a very useful thing to log for visibility.
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.
That would imply that a single caller with a single PolicyContext always has the same opinion about this policy — and that actually seems appropriate. (A caller can always create two separate PolicyContext objects.
I'm OK with that, yes.
So are we iterating towards two new APIs?
func (pc *PolicyContext) DisallowInsecure();
func (pc *PolicyContext) Evaluate(ctx context.Context, publicImage types.UnparsedImage) (string, error);
Where string
contains some sort of human-readable informational message like the GPG example above?
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 that’s really a “debug logging” feature;
I disagree. I want my tooling (at least bootc e.g.) to be able to highlight and make visible the signature state on success in a reliable fashion in the same way rpm-ostree does today.
Parsing log messages is not a reliable approach.
At a practical level because we use the proxy in skopeo as a subprocess, if we had "configure logrus so we can scrape out logs from success" the code would need to move there...
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.
(Going into the debug log would also make it hard to build a client-side policy overlay on top, which is an advantage in my view — the idea of
policy.json
is that all tools enforce the same policy.)
IOW, that IsRunningImageAllowed
is named like a predicate, and that it returns a bool without any more details, is a feature in my intuition. That could well be a wrong intuition, or a bad match for some use cases, I don't know.
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.
During a podman pull, all signatures are recorded locally. So the policy check is reproducible and can be investigated from raw data
We did the same in ostree, and it caused performance problems to be constantly re-verifying the signature to display this state.
Further, it's possible for the signature policy to change from when an image was pulled. So the policy check isn't actually quite reproducible.
I don’t think the key fingerprint really belongs in the UI.
I don't expect users to go to often verify that manually. But I do think it's reasonable for auditing/compliance tools to check the presence of this string. It acts as a "proof of work".
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.
It acts as a "proof of work".
BTW, rpm-software-management/dnf5#617 (comment) is a parallel in the dnf world, where also the signature verification state is not cached anywhere and tooling doesn't make it very visible. If dnf did something like rpm-ostree does, dnf status
would show the set of keys that were actually used to verify packages, and it would have been hopefully obvious that that set was empty...
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.
During a podman pull, all signatures are recorded locally. So the policy check is reproducible and can be investigated from raw data
We did the same in ostree, and it caused performance problems to be constantly re-verifying the signature to display this state.
It seems to me that “when a signature is verified / how that is recorded” is a fairly orthogonal discussion.
I don’t think the key fingerprint really belongs in the UI.
I don't expect users to go to often verify that manually. But I do think it's reasonable for auditing/compliance tools to check the presence of this string.
Sure, an audit log makes sense — that’s not a part of the UI; typically it would be reported to a dedicated machine ASAP.
A later “does this system host anything suspicious” check would better be implemented verifying the stored image against the stored signatures, rather than trusting some local “verified on X and it matched fingerprint Y at that time” record.
|
||
image := unparsedimage.FromPublic(publicImage) | ||
|
||
logrus.Debugf("IsImagePolicySigned for image %s", policyIdentityLogName(image.Reference())) |
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.
If this is logging an operation, it should also log the outcome and/or a reason. Or maybe log nothing and let the caller do that.
logrus.Debugf("IsImagePolicySigned for image %s", policyIdentityLogName(image.Reference())) | ||
reqs := pc.requirementsForImageRef(image.Reference()) | ||
|
||
for _, req := range reqs { |
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.
Multiple requirements can apply; why does it make sense to fail on e.g. a (signedBy
+ insecureAcceptAnything
) combination? (It might make sense, but I don’t see the argument right now — hence the question. See elsewhere about “what is the goal, and predicate being tested”.)
reqs := pc.requirementsForImageRef(image.Reference()) | ||
|
||
for _, req := range reqs { | ||
if req == NewPRInsecureAcceptAnything() { |
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 does not work at all, AFAICS. Nothing promises that NewPRInsecureAcceptAnything
returns a constant, and in fact it returns a new instance every time.
(Frankly, submitting a security PR that clearly fails leaves a bad impression.)
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.
.
xref: containers/skopeo#2029 (comment)
policy_eval: Checks for insecureAcceptAnything defined in policy.json for the target image