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

Check for unsigned images #2035

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

RishabhSaini
Copy link

xref: containers/skopeo#2029 (comment)

policy_eval: Checks for insecureAcceptAnything defined in policy.json for the target image

policy_eval: Checks for insecureAcceptAnything defined in policy.json
for the target image

Signed-off-by: RishabhSaini <[email protected]>
@@ -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) {
Copy link
Contributor

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
Copy link
Contributor

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() {
Copy link
Contributor

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 {
                 ...
}

Copy link
Collaborator

@mtrmac mtrmac left a 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) {
Copy link
Collaborator

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”.

Copy link
Contributor

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)

Copy link
Collaborator

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.)

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor

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...

Copy link
Collaborator

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.

Copy link
Contributor

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".

Copy link
Contributor

@cgwalters cgwalters Jul 17, 2023

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...

Copy link
Collaborator

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()))
Copy link
Collaborator

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 {
Copy link
Collaborator

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() {
Copy link
Collaborator

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.)

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

.

@mtrmac mtrmac added the kind/feature A request for, or a PR adding, new functionality label Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature A request for, or a PR adding, new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants