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.

Sign up for GitHub

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

Check for unsigned images #2035

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions signature/policy_eval.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

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.

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 •
edited
Loading

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.

if err := pc.changeState(pcReady, pcInUse); err != nil {
return false, err
}
defer func() {
if err := pc.changeState(pcInUse, pcReady); err != nil {
res = false
finalErr = err
}
}()

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.

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

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

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

return false, PolicyRequirementError("The policy defined for the image must not be InsecureAcceptAnything")
}
}

return true, nil
}

// IsRunningImageAllowed returns true iff the policy allows running the image.
// If it returns false, err must be non-nil, and should be an PolicyRequirementError if evaluation
// succeeded but the result was rejection.
Expand Down