Verified Commit 71ff9d70 authored by Zed's avatar Zed Committed by GitLab
Browse files

fix(driver/s3-v2): strip request checksum and inject content-md5 when checksum_disabled

parent 2c69136b
Loading
Loading
Loading
Loading
+1 −1
Original line number Diff line number Diff line
@@ -46,7 +46,7 @@ This method uses an access key and secret key to authenticate with S3:
| `objectacl` | no | The S3 Canned ACL for objects. The default value is "private". If you are using a bucket owned by another AWS account, it is recommended that you set this to `bucket-owner-full-control` so that the bucket owner can access your objects. |
| `objectownership` | no | Indicates whether the S3 storage bucket to be used by the registry disabled access control lists (ACLs). The default value is `false`. This parameter cannot be `true` if the `objectacl` parameter is also set. |
| `checksum_algorithm` | no | The algorithm to use for checksums when uploading objects to S3. Can be one of: `CRC32`, `CRC32C`, `SHA1`, `SHA256`, or `CRC64NVME`. Defaults to `CRC64NVME`. |
| `checksum_disabled` | no | If set to true, disables checksum calculation for S3 objects. Defaults to false. This parameter takes precedence over `checksum_algorithm` if both are provided. |
| `checksum_disabled` | no | If set to true, disables checksum calculation for S3 objects. Defaults to false. This parameter takes precedence over `checksum_algorithm` if both are provided. It also makes the driver work with strict S3-compatible backends (e.g. QingStor) that reject `DeleteObjects` requests carrying an `x-amz-checksum-crc32` header: the driver strips the request checksum and sends a `Content-MD5` header instead. |

## SeaweedFS

+128 −0
Original line number Diff line number Diff line
@@ -14,7 +14,9 @@ package v2
import (
	"bytes"
	"context"
	"crypto/md5" // nolint: gosec,revive // ok for content verification
	"crypto/tls"
	"encoding/base64"
	"errors"
	"fmt"
	"io"
@@ -36,7 +38,9 @@ import (
	"github.com/aws/aws-sdk-go-v2/service/s3/types"
	"github.com/aws/smithy-go"
	"github.com/aws/smithy-go/logging"
	smithymiddleware "github.com/aws/smithy-go/middleware"
	"github.com/aws/smithy-go/ptr"
	smithyhttp "github.com/aws/smithy-go/transport/http"
	dcontext "github.com/docker/distribution/context"
	dlog "github.com/docker/distribution/log"
	storagedriver "github.com/docker/distribution/registry/storage/driver"
@@ -197,6 +201,21 @@ func NewS3API(params *common.DriverParameters) (*s3.Client, error) {
		middleware.AddUserAgentKey("docker-distribution/"+version.Version+"/"+runtime.Version()),
	)

	// When the operator opts out of request checksums entirely
	// (checksum_disabled=true), strip SDK v2's request-side checksum middleware
	// from every operation's stack. See removeRequestChecksumMiddleware for the
	// full rationale.
	if params.ChecksumDisabled {
		dlog.GetLogger().WithFields(log.Fields{
			"component": "registry.storage.s3_v2.internal",
		}).Info(
			"request checksums disabled: stripping SDK v2 request-side checksums " +
				"and injecting Content-MD5 on DeleteObjects for strict S3-compatible backends",
		)
		cfg.APIOptions = append(cfg.APIOptions, removeRequestChecksumMiddleware)
		cfg.APIOptions = append(cfg.APIOptions, addContentMD5ForDeleteObjects)
	}

	if params.RegionEndpoint != "" {
		cfg.BaseEndpoint = ptr.String(params.RegionEndpoint)
	}
@@ -215,6 +234,115 @@ func NewS3API(params *common.DriverParameters) (*s3.Client, error) {
	return client, nil
}

// removeRequestChecksumMiddleware is an APIOption that removes SDK v2's
// request-side checksum middleware from every S3 operation's middleware stack.
// It is applied only when the operator sets checksum_disabled=true.
//
// Background: aws.Config.RequestChecksumCalculation=WhenRequired suppresses
// checksums only for operations whose smithy model does not set
// httpChecksumRequired. Operations that DO set it (notably DeleteObjects) are
// code-generated to force a CRC32 request checksum regardless of the config
// flag — see service/internal/checksum/middleware_setup_context.go. DeleteObjects
// is EnableTrailingChecksum=false, so the SDK sends this checksum as the
// x-amz-checksum-crc32 request header, not a trailer. Some S3-compatible backends
// reject any x-amz-checksum-* request header with HTTP 400, which breaks the
// multipart-upload cleanup path used by blob uploads.
//
// Up to three middleware are removed:
//   - AWSChecksum:SetupInputContext           (Initialize) — selects the algorithm
//   - AWSChecksum:ComputeInputPayloadChecksum (Finalize)   — writes header form
//   - addInputChecksumTrailer                 (Finalize)   — writes trailer form
//     (installed only when EnableTrailingChecksum=true, so never for DeleteObjects;
//     removed here purely as defense in depth)
//
// Removing the Initialize-stage middleware is what actually prevents the
// request-side checksum: AWSChecksum:SetupInputContext selects the algorithm,
// and the Finalize-stage writers no-op when no algorithm is in context (see
// service/internal/checksum/middleware_compute_input_checksum.go —
// getInputAlgorithm returns !ok and the finalizer passes the request through
// untouched). Any finalizers are stripped too as defense in depth.
//
// This runs as an APIOption, so it executes once per operation, against that
// operation's own stack. The SDK only installs the request-checksum middleware
// on operations whose smithy model requires it (RequireChecksum / a checksum
// input member) — DeleteObjects, PutObject, UploadPart, … — generated inline
// per operation (see addInputChecksumMiddleware in the s3 client). Read
// operations such as GetObject, HeadObject and ListObjectsV2 never carry it, so
// Remove returning "not found" on their stacks is normal and MUST be tolerated;
// failing there would break every read.
//
// The error is therefore enforced only on the DeleteObjects stack. DeleteObjects
// is RequireChecksum=true, so the SDK always installs AWSChecksum:SetupInputContext
// on it, and it is the operation strict S3-compatible backends (e.g. QingStor)
// reject when the CRC32 header is present — the exact path this driver also
// re-adds Content-MD5 to (see addContentMD5ForDeleteObjects). If that ID is
// absent on the DeleteObjects stack, a future SDK rename or refactor has moved
// the middleware and the request-side checksum would silently return and break
// those backends with HTTP 400 again; we surface that as an error so an SDK bump
// fails the DeleteObjects path loudly at runtime instead of regressing silently
// on master. (The unit tests pin this guard against a synthetic stack with the
// literal ID; they assert the contract but, on their own, would not catch an
// upstream rename — only a real DeleteObjects call exercises the live stack.)
// Everything else stays best-effort. Response-side checksum validation
// middleware is untouched.
func removeRequestChecksumMiddleware(stack *smithymiddleware.Stack) error {
	_, errSetup := stack.Initialize.Remove("AWSChecksum:SetupInputContext")
	_, _ = stack.Finalize.Remove("AWSChecksum:ComputeInputPayloadChecksum")
	_, _ = stack.Finalize.Remove("addInputChecksumTrailer")

	if stack.ID() == "DeleteObjects" && errSetup != nil {
		return fmt.Errorf("s3-v2: AWSChecksum:SetupInputContext not found on DeleteObjects stack (SDK checksum layout changed?): %w", errSetup)
	}
	return nil
}

// addContentMD5ForDeleteObjects injects an SDK-v1-style Content-MD5 header on
// DeleteObjects requests when removeRequestChecksumMiddleware has stripped the
// SDK v2 request-checksum middleware. Some S3-compatible backends (notably
// QingStor) strictly enforce the AWS S3 specification, which requires the
// DeleteObjects request to carry a Content-MD5 (or an x-amz-checksum-*
// header); a request with neither is rejected with HTTP 400
// InvalidRequest. The middleware runs at the end of the Build phase so the
// header is in place before SigV4 signing in Finalize includes it in
// SignedHeaders.
//
// Only DeleteObjects is targeted to avoid an extra body scan on hot paths
// like UploadPart / PutObject, which the AWS spec does not require to carry
// Content-MD5. The op-name check also means the middleware no-ops on the
// presigning client stack (which does not signal DeleteObjects).
func addContentMD5ForDeleteObjects(stack *smithymiddleware.Stack) error {
	return stack.Build.Add(
		smithymiddleware.BuildMiddlewareFunc("ContentMD5ForDeleteObjects",
			func(ctx context.Context, in smithymiddleware.BuildInput, next smithymiddleware.BuildHandler) (smithymiddleware.BuildOutput, smithymiddleware.Metadata, error) {
				if smithymiddleware.GetOperationName(ctx) != "DeleteObjects" {
					return next.HandleBuild(ctx, in)
				}
				req, ok := in.Request.(*smithyhttp.Request)
				if !ok {
					return next.HandleBuild(ctx, in)
				}
				stream := req.GetStream()
				if stream == nil {
					return next.HandleBuild(ctx, in)
				}
				body, err := io.ReadAll(stream)
				if err != nil {
					return smithymiddleware.BuildOutput{}, smithymiddleware.Metadata{}, fmt.Errorf("DeleteObjects content-md5: reading body: %w", err)
				}
				sum := md5.Sum(body) // nolint: gosec // ok for content verification
				req.Header.Set("Content-MD5", base64.StdEncoding.EncodeToString(sum[:]))
				newReq, err := req.SetStream(bytes.NewReader(body))
				if err != nil {
					return smithymiddleware.BuildOutput{}, smithymiddleware.Metadata{}, fmt.Errorf("DeleteObjects content-md5: restoring body: %w", err)
				}
				newReq.ContentLength = int64(len(body))
				in.Request = newReq
				return next.HandleBuild(ctx, in)
			}),
		smithymiddleware.After,
	)
}

func New(params *common.DriverParameters) (storagedriver.StorageDriver, error) {
	// TODO Currently multipart uploads have no timestamps, so this would be unwise
	// if you initiated a new s3driver while another one is running on the same bucket.
+192 −0
Original line number Diff line number Diff line
package v2

import (
	"bytes"
	"context"
	"crypto/md5" // nolint: gosec,revive // ok for content verification
	"encoding/base64"
	"io"
	"testing"

	smithymiddleware "github.com/aws/smithy-go/middleware"
	smithyhttp "github.com/aws/smithy-go/transport/http"
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
)

// TestRemoveRequestChecksumMiddlewareRemovesAllOnDeleteObjects verifies that on
// the DeleteObjects stack — the operation that carries the request-checksum
// middleware and that strict S3-compatible backends reject — all present
// request-side checksum middleware are removed by their published IDs while
// unrelated middleware are left untouched.
func TestRemoveRequestChecksumMiddlewareRemovesAllOnDeleteObjects(t *testing.T) {
	stack := smithymiddleware.NewStack("DeleteObjects", func() any { return nil })

	addInitializeStub(t, stack, "AWSChecksum:SetupInputContext")
	addInitializeStub(t, stack, "Unrelated:OtherInitialize")
	addFinalizeStub(t, stack, "AWSChecksum:ComputeInputPayloadChecksum")
	addFinalizeStub(t, stack, "addInputChecksumTrailer")
	addFinalizeStub(t, stack, "Unrelated:OtherFinalize")

	err := removeRequestChecksumMiddleware(stack)
	require.NoError(t, err)

	_, ok := stack.Initialize.Get("AWSChecksum:SetupInputContext")
	assert.False(t, ok, "SetupInputContext middleware should have been removed")
	_, ok = stack.Finalize.Get("AWSChecksum:ComputeInputPayloadChecksum")
	assert.False(t, ok, "ComputeInputPayloadChecksum middleware should have been removed")
	_, ok = stack.Finalize.Get("addInputChecksumTrailer")
	assert.False(t, ok, "addInputChecksumTrailer middleware should have been removed")

	_, ok = stack.Initialize.Get("Unrelated:OtherInitialize")
	assert.True(t, ok, "unrelated initialize middleware should remain")
	_, ok = stack.Finalize.Get("Unrelated:OtherFinalize")
	assert.True(t, ok, "unrelated finalize middleware should remain")
}

// TestRemoveRequestChecksumMiddlewareErrorsWhenSetupAbsentOnDeleteObjects
// verifies that removal fails loudly when AWSChecksum:SetupInputContext is
// missing from the DeleteObjects stack. DeleteObjects is RequireChecksum=true,
// so the SDK always installs that middleware; its ID disappearing means a future
// SDK has renamed or refactored the request-checksum path, so the checksum would
// silently return and break strict S3-compatible backends again. Surfacing an
// error makes an SDK bump fail the DeleteObjects path loudly at runtime instead
// of regressing silently on master. Note this test pins the guard's contract
// against a synthetic stack with the literal ID; a real upstream rename surfaces
// only on a live DeleteObjects call, not here.
func TestRemoveRequestChecksumMiddlewareErrorsWhenSetupAbsentOnDeleteObjects(t *testing.T) {
	stack := smithymiddleware.NewStack("DeleteObjects", func() any { return nil })
	addInitializeStub(t, stack, "Unrelated:OnlyInitialize")
	addFinalizeStub(t, stack, "AWSChecksum:ComputeInputPayloadChecksum")

	err := removeRequestChecksumMiddleware(stack)
	require.Error(t, err)
	assert.Contains(t, err.Error(), "AWSChecksum:SetupInputContext")
	assert.Contains(t, err.Error(), "DeleteObjects")

	_, ok := stack.Initialize.Get("Unrelated:OnlyInitialize")
	assert.True(t, ok, "unrelated initialize middleware should remain")
}

// TestRemoveRequestChecksumMiddlewareToleratesAbsentOnReadOps verifies that on a
// read operation's stack (e.g. GetObject) — which the SDK never equips with the
// request-checksum middleware — a missing AWSChecksum:SetupInputContext is NOT
// an error. This APIOption runs once per operation against that operation's own
// stack, so failing on read ops would break every read. This is the regression
// guard for treating absence as fatal everywhere.
func TestRemoveRequestChecksumMiddlewareToleratesAbsentOnReadOps(t *testing.T) {
	stack := smithymiddleware.NewStack("GetObject", func() any { return nil })
	addInitializeStub(t, stack, "Unrelated:OnlyInitialize")
	addFinalizeStub(t, stack, "Unrelated:OnlyFinalize")

	err := removeRequestChecksumMiddleware(stack)
	require.NoError(t, err)

	_, ok := stack.Initialize.Get("Unrelated:OnlyInitialize")
	assert.True(t, ok, "unrelated initialize middleware should remain")
	_, ok = stack.Finalize.Get("Unrelated:OnlyFinalize")
	assert.True(t, ok, "unrelated finalize middleware should remain")
}

// TestAddContentMD5ForDeleteObjects_AddsHeader verifies that for a DeleteObjects
// operation with a non-empty body, the middleware sets Content-MD5 to the
// base64-encoded MD5 of the body, restores the stream so downstream middleware
// can read it again, and sets ContentLength to the body's byte length.
func TestAddContentMD5ForDeleteObjects_AddsHeader(t *testing.T) {
	body := []byte(`<?xml version="1.0" encoding="UTF-8"?><Delete><Object><Key>a</Key></Object></Delete>`)
	expectedMD5 := md5.Sum(body)
	expectedHeader := base64.StdEncoding.EncodeToString(expectedMD5[:])

	req := smithyhttp.NewStackRequest().(*smithyhttp.Request)
	req, err := req.SetStream(bytes.NewReader(body))
	require.NoError(t, err)
	req.ContentLength = -1

	captured := invokeContentMD5Middleware(t, "DeleteObjects", req)

	require.NotNil(t, captured, "next handler must have been invoked")
	assert.Equal(t, expectedHeader, captured.Header.Get("Content-MD5"))
	assert.Equal(t, int64(len(body)), captured.ContentLength)

	roundtrip, err := io.ReadAll(captured.GetStream())
	require.NoError(t, err)
	assert.Equal(t, body, roundtrip, "body must be readable end-to-end after the middleware runs")
}

// TestAddContentMD5ForDeleteObjects_SkipsOtherOperations verifies that
// non-DeleteObjects operations pass through untouched: no Content-MD5 header is
// added and the stream is not consumed.
func TestAddContentMD5ForDeleteObjects_SkipsOtherOperations(t *testing.T) {
	body := []byte("not a delete-objects body")
	req := smithyhttp.NewStackRequest().(*smithyhttp.Request)
	req, err := req.SetStream(bytes.NewReader(body))
	require.NoError(t, err)

	captured := invokeContentMD5Middleware(t, "PutObject", req)

	require.NotNil(t, captured)
	assert.Empty(t, captured.Header.Get("Content-MD5"), "Content-MD5 must not be set for non-DeleteObjects ops")

	roundtrip, err := io.ReadAll(captured.GetStream())
	require.NoError(t, err)
	assert.Equal(t, body, roundtrip, "stream must not be consumed for non-DeleteObjects ops")
}

// TestAddContentMD5ForDeleteObjects_SkipsWhenNoStream verifies that the
// middleware safely no-ops on a DeleteObjects request with no body (defensive
// case — the SDK always sends a body, but the middleware should not panic if
// upstream behavior changes).
func TestAddContentMD5ForDeleteObjects_SkipsWhenNoStream(t *testing.T) {
	req := smithyhttp.NewStackRequest().(*smithyhttp.Request)

	captured := invokeContentMD5Middleware(t, "DeleteObjects", req)

	require.NotNil(t, captured)
	assert.Empty(t, captured.Header.Get("Content-MD5"))
	assert.Nil(t, captured.GetStream())
}

// invokeContentMD5Middleware registers addContentMD5ForDeleteObjects on a fresh
// stack, then drives only its Build phase with the supplied operation name and
// request. It returns the captured request: the in.Request value seen by the
// next handler, i.e. what the middleware passed downstream.
func invokeContentMD5Middleware(t *testing.T, opName string, req *smithyhttp.Request) *smithyhttp.Request {
	t.Helper()

	stack := smithymiddleware.NewStack("test", smithyhttp.NewStackRequest)
	require.NoError(t, addContentMD5ForDeleteObjects(stack))

	m, ok := stack.Build.Get("ContentMD5ForDeleteObjects")
	require.True(t, ok, "middleware must be registered on the Build step")

	var captured *smithyhttp.Request
	next := smithymiddleware.BuildHandlerFunc(func(_ context.Context, in smithymiddleware.BuildInput) (smithymiddleware.BuildOutput, smithymiddleware.Metadata, error) {
		captured = in.Request.(*smithyhttp.Request)
		return smithymiddleware.BuildOutput{}, smithymiddleware.Metadata{}, nil
	})

	ctx := smithymiddleware.WithOperationName(context.Background(), opName)
	_, _, err := m.HandleBuild(ctx, smithymiddleware.BuildInput{Request: req}, next)
	require.NoError(t, err)
	return captured
}

func addInitializeStub(t *testing.T, stack *smithymiddleware.Stack, id string) {
	t.Helper()
	m := smithymiddleware.InitializeMiddlewareFunc(id, func(
		ctx context.Context, in smithymiddleware.InitializeInput, next smithymiddleware.InitializeHandler,
	) (smithymiddleware.InitializeOutput, smithymiddleware.Metadata, error) {
		return next.HandleInitialize(ctx, in)
	})
	require.NoError(t, stack.Initialize.Add(m, smithymiddleware.After))
}

func addFinalizeStub(t *testing.T, stack *smithymiddleware.Stack, id string) {
	t.Helper()
	m := smithymiddleware.FinalizeMiddlewareFunc(id, func(
		ctx context.Context, in smithymiddleware.FinalizeInput, next smithymiddleware.FinalizeHandler,
	) (smithymiddleware.FinalizeOutput, smithymiddleware.Metadata, error) {
		return next.HandleFinalize(ctx, in)
	})
	require.NoError(t, stack.Finalize.Add(m, smithymiddleware.After))
}