Skip to content

[WIP] Update a cancelableReader importing gpg keys #489

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+59 −13
Draft

[WIP] Update a cancelableReader importing gpg keys #489

Show file tree
Hide file tree
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
9 changes: 8 additions & 1 deletion image/copy/single.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"slices"
"strings"
"sync"
"time"

digest "github.com/opencontainers/go-digest"
imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1"
Expand All @@ -30,6 +31,8 @@ import (
chunkedToc "go.podman.io/storage/pkg/chunked/toc"
)

const policyEvaluationTimeout = 60 * time.Second

// imageCopier tracks state specific to a single image (possibly an item of a manifest list)
type imageCopier struct {
c *copier
Expand Down Expand Up @@ -75,7 +78,11 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar
// Please keep this policy check BEFORE reading any other information about the image.
// (The multiImage check above only matches the MIME type, which we have received anyway.
// Actual parsing of anything should be deferred.)
if allowed, err := c.policyContext.IsRunningImageAllowed(ctx, unparsedImage); !allowed || err != nil { // Be paranoid and fail if either return value indicates so.
// Apply a timeout to avoid hanging indefinitely on GPG key import operations. see issues.redhat.com/browse/OCPBUGS-57893
// The timeout context will be used by the cancelable reader to potentially abort GPGME operations.
policyCtx, cancel := context.WithTimeout(ctx, policyEvaluationTimeout)
defer cancel()
if allowed, err := c.policyContext.IsRunningImageAllowed(policyCtx, unparsedImage); !allowed || err != nil { // Be paranoid and fail if either return value indicates so.
return copySingleImageResult{}, fmt.Errorf("Source image rejected: %w", err)
}
src, err := image.FromUnparsedImage(ctx, c.options.SourceCtx, unparsedImage)
Expand Down
3 changes: 2 additions & 1 deletion image/signature/mechanism.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package signature

import (
"bytes"
"context"
"errors"
"fmt"
"io"
Expand Down Expand Up @@ -78,7 +79,7 @@ func NewGPGSigningMechanism() (SigningMechanism, error) {
// of these keys.
// The caller must call .Close() on the returned SigningMechanism.
func NewEphemeralGPGSigningMechanism(blob []byte) (SigningMechanism, []string, error) {
return newEphemeralGPGSigningMechanism([][]byte{blob})
return newEphemeralGPGSigningMechanism(context.Background(), [][]byte{blob})
}

// gpgUntrustedSignatureContents returns UNTRUSTED contents of the signature WITHOUT ANY VERIFICATION,
Expand Down
44 changes: 37 additions & 7 deletions image/signature/mechanism_gpgme_only.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
package signature

import (
"bytes"
"context"
"io"
"os"

"github.com/proglottis/gpgme"
Expand All @@ -12,7 +15,7 @@ import (
// recognizes _only_ public keys from the supplied blobs, and returns the identities
// of these keys.
// The caller must call .Close() on the returned SigningMechanism.
func newEphemeralGPGSigningMechanism(blobs [][]byte) (signingMechanismWithPassphrase, []string, error) {
func newEphemeralGPGSigningMechanism(ctx context.Context, blobs [][]byte) (signingMechanismWithPassphrase, []string, error) {
dir, err := os.MkdirTemp("", "containers-ephemeral-gpg-")
if err != nil {
return nil, nil, err
Expand All @@ -23,34 +26,61 @@ func newEphemeralGPGSigningMechanism(blobs [][]byte) (signingMechanismWithPassph
os.RemoveAll(dir)
}
}()
ctx, err := newGPGMEContext(dir)
gpgmeCtx, err := newGPGMEContext(dir)
if err != nil {
return nil, nil, err
}
keyIdentities := []string{}
for _, blob := range blobs {
ki, err := importKeysFromBytes(ctx, blob)
ki, err := importKeysFromBytes(ctx, gpgmeCtx, blob)
if err != nil {
return nil, nil, err
}
keyIdentities = append(keyIdentities, ki...)
}

mech := newGPGMESigningMechanism(ctx, dir)
mech := newGPGMESigningMechanism(gpgmeCtx, dir)
removeDir = false
return mech, keyIdentities, nil
}

// cancelableReader wraps an io.Reader and checks context cancellation on each Read call.
type cancelableReader struct {
ctx context.Context
reader io.Reader
}

func (r *cancelableReader) Read(p []byte) (int, error) {
// Check if context is cancelled before each read
if err := r.ctx.Err(); err != nil {
return 0, err
}
n, err := r.reader.Read(p)
// Check again after read in case cancellation happened during the read
if err == nil && r.ctx.Err() != nil {
return n, r.ctx.Err()
}
Comment on lines +54 to +62
Copy link
Member

@Luap99 Luap99 Nov 28, 2025

Choose a reason for hiding this comment

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

Does this even work/address the problem in any case? The issue is the Read is blocking so checking the context before/after will not solve the issue if the bug is is read() blocks forever.

I mean sure it does work if the the problem is that it keep reading indefinitely over and over in some loop and we want to timeout that but per #421 I got the impression that is not the case.

If we use a real pipe as reader go exposes SetDeadline() so that could be used to limit the actual time the read would block.

Copy link
Contributor

@mtrmac mtrmac Nov 28, 2025

Choose a reason for hiding this comment

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

GPGME is a C library running gnupg as a subprocess, and communicating with it using a ~poll loop — and something goes wrong and hangs there. (My limited investigation suggests that this centers on a control FD which is not visible in the GPGME API at all, but that might be wrong or a consequence of some earlier failure.)

Replacing one of the data sources with something that can indicate failure is hypothetically a way to interrupt that poll loop — if the code has a reason to execute one more iteration of the poll. There’s not much reason to think that the code will actually have a chance to poll again, but, also, that’s the ~only cancellation mechanism we have. That would absolutely not be a fix but it might be a pragmatic workaround if it happened to be proven to affect things.

The issue is the Read is blocking so checking the context before/after will not solve the issue if the bug is is read() blocks forever.

In this PR, the cancellation is on a source that is passed as a byte array, so it should never block. OTOH that also makes it fairly unlikely that the cancellation opportunity would manifest.

Copy link
Member

@Luap99 Luap99 Nov 28, 2025

Choose a reason for hiding this comment

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

ack, then I don't think the context check on read can help to work around it.

return n, err
}

// importKeysFromBytes imports public keys from the supplied blob and returns their identities.
// The blob is assumed to have an appropriate format (the caller is expected to know which one).
// NOTE: This may modify long-term state (e.g. key storage in a directory underlying the mechanism);
// but we do not make this public, it can only be used through newEphemeralGPGSigningMechanism.
func importKeysFromBytes(ctx *gpgme.Context, blob []byte) ([]string, error) {
inputData, err := gpgme.NewDataBytes(blob)
// The context can be used to cancel the operation; if cancelled, the reader will return an error
// which may allow the GPGME operation to abort (though there's no guarantee the C library will
// respect this immediately).
func importKeysFromBytes(ctx context.Context, gpgmeCtx *gpgme.Context, blob []byte) ([]string, error) {
// Create a cancelable reader that checks context on each Read call
reader := &cancelableReader{
ctx: ctx,
reader: bytes.NewReader(blob),
}
inputData, err := gpgme.NewDataReader(reader)
if err != nil {
return nil, err
}
res, err := ctx.Import(inputData)
res, err := gpgmeCtx.Import(inputData)
if err != nil {
return nil, err
}
Expand Down
5 changes: 4 additions & 1 deletion image/signature/mechanism_openpgp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package signature

import (
"bytes"
"context"
"errors"
"fmt"
"io"
Expand Down Expand Up @@ -63,7 +64,9 @@ func newGPGSigningMechanismInDirectory(optionalDir string) (signingMechanismWith
// recognizes _only_ public keys from the supplied blob, and returns the identities
// of these keys.
// The caller must call .Close() on the returned SigningMechanism.
func newEphemeralGPGSigningMechanism(blobs [][]byte) (signingMechanismWithPassphrase, []string, error) {
// The context parameter is accepted for API consistency but is not used by this implementation.
func newEphemeralGPGSigningMechanism(ctx context.Context, blobs [][]byte) (signingMechanismWithPassphrase, []string, error) {
_ = ctx // Context not used in this implementation
m := &openpgpSigningMechanism{
keyring: openpgp.EntityList{},
}
Expand Down
6 changes: 5 additions & 1 deletion image/signature/mechanism_sequoia.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
package signature

import (
"context"

"go.podman.io/image/v5/signature/internal/sequoia"
)

Expand All @@ -18,7 +20,9 @@ type sequoiaEphemeralSigningMechanism struct {
// recognizes _only_ public keys from the supplied blobs, and returns the identities
// of these keys.
// The caller must call .Close() on the returned SigningMechanism.
func newEphemeralGPGSigningMechanism(blobs [][]byte) (signingMechanismWithPassphrase, []string, error) {
// The context parameter is accepted for API consistency but is not used by this implementation.
func newEphemeralGPGSigningMechanism(ctx context.Context, blobs [][]byte) (signingMechanismWithPassphrase, []string, error) {
_ = ctx // Context not used in this implementation
if err := sequoia.Init(); err != nil {
return nil, nil, err // Coverage: This is impractical to test in-process, with the static go_sequoia_dlhandle.
}
Expand Down
3 changes: 2 additions & 1 deletion image/signature/mechanism_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package signature

import (
"bytes"
"context"
"os"
"path/filepath"
"slices"
Expand Down Expand Up @@ -175,7 +176,7 @@ func TestNewEphemeralGPGSigningMechanism(t *testing.T) {
require.NoError(t, err)
keyBlob2, err := os.ReadFile("./fixtures/public-key-2.gpg")
require.NoError(t, err)
mech, keyIdentities, err = newEphemeralGPGSigningMechanism([][]byte{keyBlob1, keyBlob2})
mech, keyIdentities, err = newEphemeralGPGSigningMechanism(context.Background(), [][]byte{keyBlob1, keyBlob2})
require.NoError(t, err)
defer mech.Close()
assert.Equal(t, []string{TestKeyFingerprint, TestKeyFingerprintWithPassphrase}, keyIdentities)
Expand Down
2 changes: 1 addition & 1 deletion image/signature/policy_eval_signedby.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ func (pr *prSignedBy) isSignatureAuthorAccepted(ctx context.Context, image priva
}

// FIXME: move this to per-context initialization
mech, trustedIdentities, err := newEphemeralGPGSigningMechanism(data)
mech, trustedIdentities, err := newEphemeralGPGSigningMechanism(ctx, data)
Copy link
Member Author

@QiWang19 QiWang19 Nov 25, 2025

Choose a reason for hiding this comment

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

Suggested change
mech, trustedIdentities, err := newEphemeralGPGSigningMechanism(ctx, data)
importCtx, cancel := context.WithTimeout(ctx, importKeyTimeOut)
defer cancel()
mech, trustedIdentities, err := newEphemeralGPGSigningMechanism(importCtx, data)

@mtrmac Updated the code. Do we still need to add timeout context pass down, so the cancellable reader to work? if not hardcode a timeout here, will the caller cancel the context?

Copy link
Contributor

@mtrmac mtrmac Nov 25, 2025

Choose a reason for hiding this comment

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

I’m afraid I can’t parse the question. Are you asking whether the CRI-O server already has a timeout / request cancellation logic of some kind (maybe triggering when/if the Kubelet client gives up)? That would be a CRI-O question.

And if CRI-O did pass to c/image/copy.Image a context that would in some situations be canceled, yes, I think such a context should be forwarded all the way to this code.

I would, by default, expect the timeout, if any, to be set at a pretty high-level; I was thinking one for the whole RPC operation served by CRI-O … except that that doesn’t work well for pulls, which, on slow links, may be making steady progress over many minutes, so there is no good timeout that can be set for the whole pull by default.

So, I guess, in the caller of IsRunningImageAllowed in c/image would be at least better than here prSignedBy; IsRunningImageAllowed is not really constant-time, but it is fairly bounded, and it would mean less new code in c/image/signature.

[There’s also CRI-O’s PullProgressTimeout which, last time I looked at it, made unwarranted assumptions about how c/image reports progress, and thus was ~unusable. But if it, hypothetically, did not trigger on false positives where c/image does not report progress but may spend a lot of time, it would trigger cancellation of the context and that should be visible in the GPGME mechanism.]

if err != nil {
return sarRejected, nil, err
}
Expand Down
Loading