Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 17 additions & 1 deletion image/copy/blob.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"io"

"github.com/sirupsen/logrus"
"go.podman.io/image/v5/internal/digests"
"go.podman.io/image/v5/internal/private"
compressiontypes "go.podman.io/image/v5/pkg/compression/types"
"go.podman.io/image/v5/types"
Expand Down Expand Up @@ -101,6 +102,11 @@ func (ic *imageCopier) copyBlobFromStream(ctx context.Context, srcReader io.Read
Cache: ic.c.blobInfoCache,
IsConfig: isConfig,
EmptyLayer: emptyLayer,
Digests: ic.c.options.digestOptions,
// CannotChangeDigestReason requires stream.info.Digest to always be set, and it is:
// If ic.cannotModifyManifestReason, stream.info was not modified since its initialization at the top of this
// function, and the caller is required to provide a digest.
CannotChangeDigestReason: ic.cannotModifyManifestReason,
}
if !isConfig {
options.LayerIndex = &layerIndex
Expand Down Expand Up @@ -133,7 +139,17 @@ func (ic *imageCopier) copyBlobFromStream(ctx context.Context, srcReader io.Read
return types.BlobInfo{}, fmt.Errorf("Internal error writing blob %s, digest verification failed but was ignored", srcInfo.Digest)
}
if stream.info.Digest != "" && uploadedInfo.Digest != stream.info.Digest {
return types.BlobInfo{}, fmt.Errorf("Internal error writing blob %s, blob with digest %s saved with digest %s", srcInfo.Digest, stream.info.Digest, uploadedInfo.Digest)
expectedAlgo, err := ic.c.options.digestOptions.Choose(digests.Situation{
Copy link
Contributor

Choose a reason for hiding this comment

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

This is redundant / conflicting with PutBlob… making its own choice. If we had the generic code choosing a specific algorithm and expecting precisely that one to be used, then it should do that before PutBlob and provide that to transports as a parameter.

I’m not sure where the decision should be, at this point [and it’s a private interface, so we can change our mind] — I guess the transport might have some information (e.g. about remote registry capabilities) that the generic code doesn’t — but, for now, we don’t have all transports updated yet, either way…


AFAICS this only exists as a sanity check (both config and layer copies later enforce equality if cannotModifyManifest…), so I think an “if algorithms match the whole values must match” check here would be sufficient.

Preexisting: stream.info.Digest,
CannotChangeAlgorithmReason: ic.cannotModifyManifestReason,
})
if err != nil {
return types.BlobInfo{}, err
}
// If we're forcing a different algorithm and the uploaded digest uses that algorithm, it's acceptable
if uploadedInfo.Digest.Algorithm() != expectedAlgo {
return types.BlobInfo{}, fmt.Errorf("Internal error writing blob %s, blob with digest %s saved with digest %s", srcInfo.Digest, stream.info.Digest, uploadedInfo.Digest)
}
}
if digestingReader.validationSucceeded {
if err := compressionStep.recordValidatedDigestData(ic.c, uploadedInfo, srcInfo, encryptionStep, decryptionStep); err != nil {
Expand Down
32 changes: 32 additions & 0 deletions image/copy/copy.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/sirupsen/logrus"
"go.podman.io/image/v5/docker/reference"
internalblobinfocache "go.podman.io/image/v5/internal/blobinfocache"
"go.podman.io/image/v5/internal/digests"
"go.podman.io/image/v5/internal/image"
"go.podman.io/image/v5/internal/imagedestination"
"go.podman.io/image/v5/internal/imagesource"
Expand Down Expand Up @@ -155,6 +156,28 @@ type Options struct {
// In oci-archive: destinations, this will set the create/mod/access timestamps in each tar entry
// (but not a timestamp of the created archive file).
DestinationTimestamp *time.Time

// FIXME:
// - this reference to an internal type is unusable from the outside even if we made the field public
// - what is the actual semantics? Right now it is probably “choices to use when writing to the destination”, TBD
// - anyway do we want to expose _all_ of the digests.Options tunables, or fewer?
// - … do we want to expose _more_ granularity than that?
// - (“must have at least sha512 integrity when reading”, what does “at least” mean for random pairs of algorithms?)
// - should some of this be in config files, maybe ever per-registry?
digestOptions digests.Options
}

// SetForceDigestAlgorithm forces the use of a specific digest algorithm for this copy operation.
func (o *Options) SetForceDigestAlgorithm(algo digest.Algorithm) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

(Yes, I’m generally in favor of adding more options via methods rather than fields. And we can always add something vaguely like SetForceDigestOptions later if we decided to make that fully public.)

I’m wondering about naming this ForceDestinationDigest… — we might eventually want a “pulls must consume sha512 or stronger digests and refuse to work on sha256”, and that’s a fairly unrelated feature.

if !algo.Available() {
return fmt.Errorf("digest algorithm %q is not available", algo.String())
}
digestOpts, err := digests.MustUse(algo)
if err != nil {
return fmt.Errorf("failed to set force-digest algorithm: %w", err)
}
o.digestOptions = digestOpts
Copy link
Contributor

Choose a reason for hiding this comment

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

Fail if this is already set? It might protect against oversights. OTOH it can also prevent some code patterns — if that turned out to be troublesome, we can always remove that enforcement later.

return nil
}

// OptionCompressionVariant allows to supply information about
Expand Down Expand Up @@ -200,6 +223,15 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef,
if options == nil {
options = &Options{}
}
// FIXME: Currently, digestsOptions is not implemented at all, and exists in the codebase
// only to allow gradually building the feature set.
// After c/image/copy consistently implements it, provide a public digest options API of some kind.
optionsCopy := *options
// Only set default if not already configured
Copy link
Contributor

Choose a reason for hiding this comment

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

(We can avoid the whole optionsCopy if digestOptions != nil.)

if optionsCopy.digestOptions.MustUseSet() == "" {
optionsCopy.digestOptions = digests.CanonicalDefault()
}
options = &optionsCopy

if err := validateImageListSelection(options.ImageListSelection); err != nil {
return nil, err
Expand Down
55 changes: 49 additions & 6 deletions image/copy/single.go
Original file line number Diff line number Diff line change
Expand Up @@ -592,10 +592,19 @@ func (ic *imageCopier) copyUpdatedConfigAndManifest(ctx context.Context, instanc
return nil, "", fmt.Errorf("reading manifest: %w", err)
}

if err := ic.copyConfig(ctx, pendingImage); err != nil {
newConfigDigest, err := ic.copyConfig(ctx, pendingImage)
if err != nil {
return nil, "", err
}

// Config digest changed due to forcing a different digest algorithm
if newConfigDigest != nil {
man, err = ic.updateManifestConfigDigest(man, pendingImage, *newConfigDigest)
if err != nil {
return nil, "", fmt.Errorf("updating manifest config digest: %w", err)
}
}

ic.c.Printf("Writing manifest to image destination\n")
manifestDigest, err := manifest.Digest(man)
Copy link
Contributor

Choose a reason for hiding this comment

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

(This is another part that will need updating, for multi-arch images.)

if err != nil {
Expand All @@ -611,13 +620,41 @@ func (ic *imageCopier) copyUpdatedConfigAndManifest(ctx context.Context, instanc
return man, manifestDigest, nil
}

// updateManifestConfigDigest uses typed manifest structures instead of generic JSON manipulation.
// This leverages the existing manifest parsing and serialization infrastructure.
func (ic *imageCopier) updateManifestConfigDigest(manifestBlob []byte, src types.Image, newConfigDigest digest.Digest) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to go through the manifest abstraction in c/image/internal/image and not be hard-coded in c/image/copy.

(It can be a method on image.Sourced/genericManifest, like CanChangeLayerCompression, without making it a public API, at least for now.)

_, mt, err := src.Manifest(context.Background())
if err != nil {
return nil, fmt.Errorf("getting manifest type: %w", err)
}

m, err := manifest.FromBlob(manifestBlob, mt)
if err != nil {
return nil, fmt.Errorf("parsing manifest: %w", err)
}

switch typedManifest := m.(type) {
case *manifest.OCI1:
typedManifest.Config.Digest = newConfigDigest
return typedManifest.Serialize()
case *manifest.Schema2:
typedManifest.ConfigDescriptor.Digest = newConfigDigest
return typedManifest.Serialize()
case *manifest.Schema1:
return nil, fmt.Errorf("cannot update config digest for schema1 manifest")
default:
return nil, fmt.Errorf("unsupported manifest type for config digest update: %T", m)
}
}

// copyConfig copies config.json, if any, from src to dest.
func (ic *imageCopier) copyConfig(ctx context.Context, src types.Image) error {
// It returns the new config digest if it changed (due to digest algorithm forcing), or nil otherwise.
func (ic *imageCopier) copyConfig(ctx context.Context, src types.Image) (*digest.Digest, error) {
srcInfo := src.ConfigInfo()
if srcInfo.Digest != "" {
if err := ic.c.concurrentBlobCopiesSemaphore.Acquire(ctx, 1); err != nil {
// This can only fail with ctx.Err(), so no need to blame acquiring the semaphore.
return fmt.Errorf("copying config: %w", err)
return nil, fmt.Errorf("copying config: %w", err)
}
defer ic.c.concurrentBlobCopiesSemaphore.Release(1)

Expand Down Expand Up @@ -645,13 +682,19 @@ func (ic *imageCopier) copyConfig(ctx context.Context, src types.Image) error {
return destInfo, nil
}()
if err != nil {
return err
return nil, err
}
if destInfo.Digest != srcInfo.Digest {
return fmt.Errorf("Internal error: copying uncompressed config blob %s changed digest to %s", srcInfo.Digest, destInfo.Digest)
// Allow digest algorithm changes when forcing a specific digest algorithm
forcingDifferentAlgo := ic.c.options.digestOptions.MustUseSet() != "" &&
Copy link
Contributor

Choose a reason for hiding this comment

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

  • See the similar check copyBlobFromStream
  • “MustUse” Is not the only reason a copy might choose a different algorithm (e.g. we might be pushing to a sha256-only-transport or registry), hard-coding a check only for MustUse here is overly specific
  • Compare the end of copyLayers, where the equality check is immediately paired with manifest edits [and see elsewhere about manifest edits], so that we always set up edits if we know they are needed, and we minimize the risk of the two getting out of sync. I’m not sure whether the check+update should live in copyConfig or copyUpdatedConfigAndManifest (guessing the latter), but they should be together.

destInfo.Digest.Algorithm() != srcInfo.Digest.Algorithm()
if !forcingDifferentAlgo {
return nil, fmt.Errorf("Internal error: copying uncompressed config blob %s changed digest to %s", srcInfo.Digest, destInfo.Digest)
}
return &destInfo.Digest, nil
}
}
return nil
return nil, nil
}

// diffIDResult contains both a digest value and an error from diffIDComputationGoroutine.
Expand Down
7 changes: 6 additions & 1 deletion image/directory/directory_dest.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

"github.com/opencontainers/go-digest"
"github.com/sirupsen/logrus"
"go.podman.io/image/v5/internal/digests"
"go.podman.io/image/v5/internal/imagedestination/impl"
"go.podman.io/image/v5/internal/imagedestination/stubs"
"go.podman.io/image/v5/internal/private"
Expand Down Expand Up @@ -150,7 +151,11 @@ func (d *dirImageDestination) PutBlobWithOptions(ctx context.Context, stream io.
}
}()

digester, stream := putblobdigest.DigestIfUnknown(stream, inputInfo)
algorithm, err := options.Digests.Choose(digests.Situation{Preexisting: inputInfo.Digest, CannotChangeAlgorithmReason: options.CannotChangeDigestReason})
if err != nil {
return private.UploadedBlob{}, err
}
digester, stream := putblobdigest.DigestIfAlgorithmUnknown(stream, inputInfo, algorithm)

// TODO: This can take quite some time, and should ideally be cancellable using ctx.Done().
size, err := io.Copy(blobFile, stream)
Expand Down
146 changes: 146 additions & 0 deletions image/internal/digests/digests.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
// Package digests provides an internal representation of users’ digest use preferences.
//
// Something like this _might_ be eventually made available as a public API:
// before doing so, carefully think whether the API should be modified before we commit to it.

package digests

import (
"errors"
"fmt"

"github.com/opencontainers/go-digest"
)

// Options records users’ preferences for used digest algorithm usage.
// It is a value type and can be copied using ordinary assignment.
//
// It can only be created using one of the provided constructors.
type Options struct {
initialized bool // To prevent uses that don’t call a public constructor; this is necessary to enforce the .Available() promise.

// If any of the fields below is set, it is guaranteed to be .Available().

mustUse digest.Algorithm // If not "", written digests must use this algorithm.
prefer digest.Algorithm // If not "", use this algorithm whenever possible.
defaultAlgo digest.Algorithm // If not "", use this algorithm if there is no reason to use anything else.
}

// CanonicalDefault is Options which default to using digest.Canonical if there is no reason to use a different algorithm
// (e.g. when there is no pre-existing digest).
//
// The configuration can be customized using .WithPreferred() or .WithDefault().
func CanonicalDefault() Options {
// This does not set .defaultAlgo so that .WithDefault() can be called (once).
return Options{
initialized: true,
}
}

// MustUse constructs Options which always use algo.
func MustUse(algo digest.Algorithm) (Options, error) {
// We don’t provide Options.WithMustUse because there is no other option that makes a difference
// once .mustUse is set.
if !algo.Available() {
return Options{}, fmt.Errorf("attempt to use an unavailable digest algorithm %q", algo.String())
}
return Options{
initialized: true,
mustUse: algo,
}, nil
}

// WithPreferred returns a copy of o with a “preferred” algorithm set to algo.
// The preferred algorithm is used whenever possible (but if there is a strict requirement to use something else, it will be overridden).
func (o Options) WithPreferred(algo digest.Algorithm) (Options, error) {
if err := o.ensureInitialized(); err != nil {
return Options{}, err
}
if o.prefer != "" {
return Options{}, errors.New("digests.Options already have a 'prefer' algorithm configured")
}

if !algo.Available() {
return Options{}, fmt.Errorf("attempt to use an unavailable digest algorithm %q", algo.String())
}
o.prefer = algo
return o, nil
}

// WithDefault returns a copy of o with a “default” algorithm set to algo.
// The default algorithm is used if there is no reason to use anything else (e.g. when there is no pre-existing digest).
func (o Options) WithDefault(algo digest.Algorithm) (Options, error) {
if err := o.ensureInitialized(); err != nil {
return Options{}, err
}
if o.defaultAlgo != "" {
return Options{}, errors.New("digests.Options already have a 'default' algorithm configured")
}

if !algo.Available() {
return Options{}, fmt.Errorf("attempt to use an unavailable digest algorithm %q", algo.String())
}
o.defaultAlgo = algo
return o, nil
}

// ensureInitialized returns an error if o is not initialized.
func (o Options) ensureInitialized() error {
if !o.initialized {
return errors.New("internal error: use of uninitialized digests.Options")
}
return nil
}

// Situation records the context in which a digest is being chosen.
type Situation struct {
Preexisting digest.Digest // If not "", a pre-existing digest value (frequently one which is cheaper to use than others)
CannotChangeAlgorithmReason string // The reason why we must use Preexisting, or "" if we can use other algorithms.
}

// Choose chooses a digest algorithm based on the options and the situation.
func (o Options) Choose(s Situation) (digest.Algorithm, error) {
if err := o.ensureInitialized(); err != nil {
return "", err
}

if s.CannotChangeAlgorithmReason != "" && s.Preexisting == "" {
return "", fmt.Errorf("internal error: digests.Situation.CannotChangeAlgorithmReason is set but Preexisting is empty")
}

var choice digest.Algorithm // = what we want to use
switch {
case o.mustUse != "":
choice = o.mustUse
case s.CannotChangeAlgorithmReason != "":
choice = s.Preexisting.Algorithm()
if !choice.Available() {
return "", fmt.Errorf("existing digest uses unimplemented algorithm %s", choice)
}
case o.prefer != "":
choice = o.prefer
case s.Preexisting != "" && s.Preexisting.Algorithm().Available():
choice = s.Preexisting.Algorithm()
case o.defaultAlgo != "":
choice = o.defaultAlgo
default:
choice = digest.Canonical // We assume digest.Canonical is always available.
}

if s.CannotChangeAlgorithmReason != "" && choice != s.Preexisting.Algorithm() {
return "", fmt.Errorf("requested to always use digest algorithm %s but we cannot replace existing digest algorithm %s: %s",
choice, s.Preexisting.Algorithm(), s.CannotChangeAlgorithmReason)
}

return choice, nil
}

// MustUseSet returns an algorithm if o is set to always use a specific algorithm, "" if it is flexible.
func (o Options) MustUseSet() digest.Algorithm {
// We don’t do .ensureInitialized() because that would require an extra error value just for that.
// This should not be a part of any public API either way.
if o.mustUse != "" {
return o.mustUse
}
return ""
}
Loading
Loading