-
Notifications
You must be signed in to change notification settings - Fork 33
PolicyContext: add new SetRejectInsecure method
#355
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import ( | |
| "github.com/sirupsen/logrus" | ||
| "go.podman.io/image/v5/internal/private" | ||
| "go.podman.io/image/v5/internal/unparsedimage" | ||
| "go.podman.io/image/v5/transports" | ||
| "go.podman.io/image/v5/types" | ||
| ) | ||
|
|
||
|
|
@@ -65,6 +66,9 @@ type PolicyRequirement interface { | |
| // WARNING: This validates signatures and the manifest, but does not download or validate the | ||
| // layers. Users must validate that the layers match their expected digests. | ||
| isRunningImageAllowed(ctx context.Context, image private.UnparsedImage) (bool, error) | ||
|
|
||
| // isInsecure returns true if the requirement allows images without any signatures. | ||
| isInsecure() bool | ||
| } | ||
|
|
||
| // PolicyReferenceMatch specifies a set of image identities accepted in PolicyRequirement. | ||
|
|
@@ -79,8 +83,9 @@ type PolicyReferenceMatch interface { | |
| // PolicyContext encapsulates a policy and possible cached state | ||
| // for speeding up its evaluation. | ||
| type PolicyContext struct { | ||
| Policy *Policy | ||
| state policyContextState // Internal consistency checking | ||
| Policy *Policy | ||
| state policyContextState // Internal consistency checking | ||
| rejectInsecure bool | ||
| } | ||
|
|
||
| // policyContextState is used internally to verify the users are not misusing a PolicyContext. | ||
|
|
@@ -132,6 +137,13 @@ func policyIdentityLogName(ref types.ImageReference) string { | |
| return ref.Transport().Name() + ":" + ref.PolicyConfigurationIdentity() | ||
| } | ||
|
|
||
| // SetRejectInsecure modifies insecure policy requirement handling. If | ||
| // passed `true`, policy checking by IsRunningImageAllowed will ignore the | ||
| // "insecureAcceptAnything" policy type. | ||
| func (pc *PolicyContext) SetRejectInsecure(val bool) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The description (and name?) will need updating per the “authenticated contents”/“signed contents” discussion elsewhere. (Choose the semantics that bootc needs; we can always add one more option with some other semantics in the future if it turned out to be necessary for other users.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Thinking whether this should reject repeated calls, as an indication of a confused caller … we’d have to add an error return, and all callers would need to check it for all of this to make a difference — that wouldn’t really work, let’s not do that.) |
||
| pc.rejectInsecure = val | ||
| } | ||
|
|
||
| // requirementsForImageRef selects the appropriate requirements for ref. | ||
| func (pc *PolicyContext) requirementsForImageRef(ref types.ImageReference) PolicyRequirements { | ||
| // Do we have a PolicyTransportScopes for this transport? | ||
|
|
@@ -278,6 +290,7 @@ func (pc *PolicyContext) IsRunningImageAllowed(ctx context.Context, publicImage | |
| return false, PolicyRequirementError("List of verification policy requirements must not be empty") | ||
| } | ||
|
|
||
| wasSecure := false | ||
| for reqNumber, req := range reqs { | ||
| // FIXME: supply state | ||
| allowed, err := req.isRunningImageAllowed(ctx, image) | ||
|
|
@@ -286,7 +299,15 @@ func (pc *PolicyContext) IsRunningImageAllowed(ctx context.Context, publicImage | |
| return false, err | ||
| } | ||
| logrus.Debugf(" Requirement %d: allowed", reqNumber) | ||
| if !req.isInsecure() { | ||
| wasSecure = true | ||
| } | ||
| } | ||
|
|
||
| if pc.rejectInsecure && !wasSecure { | ||
| return false, PolicyRequirementError(fmt.Sprintf("No secure policy found for image %s.", transports.ImageName(image.Reference()))) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
|
|
||
| // We have tested that len(reqs) != 0, so at least one req must have explicitly allowed this image. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Is it needed to update this comment? It seems that having at least one req allowed is not enough to get to R312, since R299 will do a early return if at least one req is not allowed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Either way, I don’t think this PR changes the situation.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment is trying to say “if we get to this point, |
||
| logrus.Debugf("Overall: allowed") | ||
| return true, nil | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,3 +18,7 @@ func (pr *prSignedBaseLayer) isRunningImageAllowed(ctx context.Context, image pr | |
| logrus.Errorf("signedBaseLayer not implemented yet!") | ||
| return false, PolicyRequirementError("signedBaseLayer not implemented yet!") | ||
| } | ||
|
|
||
| func (pr *prSignedBaseLayer) isInsecure() bool { | ||
| return false | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I guess I should probably just flip this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reading #355 (comment) , if the semantics is “we authenticated the image at least once”, So I think this should just be |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -497,3 +497,79 @@ func assertRunningRejectedPolicyRequirement(t *testing.T, allowed bool, err erro | |
| assertRunningRejected(t, allowed, err) | ||
| assert.IsType(t, PolicyRequirementError(""), err) | ||
| } | ||
|
|
||
| func TestPolicyContextSetRejectInsecure(t *testing.T) { | ||
| pc, err := NewPolicyContext(&Policy{Default: PolicyRequirements{NewPRReject()}}) | ||
| require.NoError(t, err) | ||
| defer func() { | ||
| err := pc.Destroy() | ||
| require.NoError(t, err) | ||
| }() | ||
|
|
||
| // Test default value is false | ||
| assert.False(t, pc.rejectInsecure) | ||
|
|
||
| // Test setting to true | ||
| pc.SetRejectInsecure(true) | ||
| assert.True(t, pc.rejectInsecure) | ||
|
|
||
| // Test setting back to false | ||
| pc.SetRejectInsecure(false) | ||
| assert.False(t, pc.rejectInsecure) | ||
| } | ||
|
|
||
| func TestPolicyContextIsRunningImageAllowedWithRejectInsecure(t *testing.T) { | ||
| pc, err := NewPolicyContext(&Policy{ | ||
| Default: PolicyRequirements{NewPRReject()}, | ||
| Transports: map[string]PolicyTransportScopes{ | ||
| "docker": { | ||
| "docker.io/testing/manifest:insecureOnly": { | ||
| NewPRInsecureAcceptAnything(), | ||
| }, | ||
| "docker.io/testing/manifest:insecureWithOther": { | ||
| NewPRInsecureAcceptAnything(), | ||
| xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "fixtures/public-key.gpg", NewPRMMatchRepository()), | ||
| }, | ||
| "docker.io/testing/manifest:signedOnly": { | ||
| xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "fixtures/public-key.gpg", NewPRMMatchRepository()), | ||
| }, | ||
| }, | ||
| }, | ||
| }) | ||
| require.NoError(t, err) | ||
| defer func() { | ||
| err := pc.Destroy() | ||
| require.NoError(t, err) | ||
| }() | ||
|
|
||
| // Test with rejectInsecure=false (default behavior) | ||
| // insecureAcceptAnything should be accepted | ||
| img := pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:insecureOnly") | ||
| res, err := pc.IsRunningImageAllowed(context.Background(), img) | ||
| assertRunningAllowed(t, res, err) | ||
|
|
||
| // Test with rejectInsecure=true | ||
| pc.SetRejectInsecure(true) | ||
|
|
||
| // insecureAcceptAnything only: should be rejected (leaves no secure requirements) | ||
| img = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:insecureOnly") | ||
| res, err = pc.IsRunningImageAllowed(context.Background(), img) | ||
| assert.Equal(t, false, res) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| assert.Error(t, err) | ||
|
|
||
| // insecureAcceptAnything + signed requirement: first requirement has no effect, second is secure and valid | ||
| img = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:insecureWithOther") | ||
| res, err = pc.IsRunningImageAllowed(context.Background(), img) | ||
| assertRunningAllowed(t, res, err) | ||
|
|
||
| // signed requirement only: should work normally | ||
| img = pcImageMock(t, "fixtures/dir-img-valid", "testing/manifest:signedOnly") | ||
| res, err = pc.IsRunningImageAllowed(context.Background(), img) | ||
| assertRunningAllowed(t, res, err) | ||
|
|
||
| // Test with unsigned image and insecureAcceptAnything + signed requirement: first requirement has no effect, second is secure but rejects | ||
| img = pcImageMock(t, "fixtures/dir-img-unsigned", "testing/manifest:insecureWithOther") | ||
| res, err = pc.IsRunningImageAllowed(context.Background(), img) | ||
| assert.Equal(t, false, res) | ||
| assert.Error(t, err) | ||
| } | ||
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.
“allows images with unauthenticated contents”, per #355 (comment) ?
(A possible hypothetical to think about is a
PolicyRequirementthat “the input reference is a digested reference” — in that case the contents of the image would be authenticated, but by the caller-provided input, not cryptographically. “allows images with unsigned contents”, maybe? Which of the two?)