diff --git a/cmd/auth-handler.go b/cmd/auth-handler.go index 6e824e312..e1e54d650 100644 --- a/cmd/auth-handler.go +++ b/cmd/auth-handler.go @@ -343,6 +343,26 @@ func checkRequestAuthType(ctx context.Context, r *http.Request, action policy.Ac return s3Err } +func checkRequestAuthTypeWithExistingTags(ctx context.Context, r *http.Request, action policy.Action, bucketName, objectName, existingTags string) (s3Err APIErrorCode) { + logger.GetReqInfo(ctx).BucketName = bucketName + logger.GetReqInfo(ctx).ObjectName = objectName + + if s3Err = authenticateRequest(ctx, r, action); s3Err != ErrNone { + return s3Err + } + return authorizeRequestWithExistingTags(ctx, r, action, existingTags) +} + +func checkRequestAuthTypeWithRequestTags(ctx context.Context, r *http.Request, action policy.Action, bucketName, objectName string, requestTags *string) (s3Err APIErrorCode) { + logger.GetReqInfo(ctx).BucketName = bucketName + logger.GetReqInfo(ctx).ObjectName = objectName + + if s3Err = authenticateRequest(ctx, r, action); s3Err != ErrNone { + return s3Err + } + return authorizeRequestWithTags(ctx, r, action, "", requestTags) +} + // checkRequestAuthTypeWithVID is similar to checkRequestAuthType // passes versionID additionally. func checkRequestAuthTypeWithVID(ctx context.Context, r *http.Request, action policy.Action, bucketName, objectName, versionID string) (s3Err APIErrorCode) { @@ -416,6 +436,14 @@ func authenticateRequest(ctx context.Context, r *http.Request, action policy.Act } func authorizeRequest(ctx context.Context, r *http.Request, action policy.Action) (s3Err APIErrorCode) { + return authorizeRequestWithExistingTags(ctx, r, action, "") +} + +func authorizeRequestWithExistingTags(ctx context.Context, r *http.Request, action policy.Action, existingTags string) (s3Err APIErrorCode) { + return authorizeRequestWithTags(ctx, r, action, existingTags, nil) +} + +func authorizeRequestWithTags(ctx context.Context, r *http.Request, action policy.Action, existingTags string, requestTags *string) (s3Err APIErrorCode) { reqInfo := logger.GetReqInfo(ctx) if reqInfo == nil { return ErrAccessDenied @@ -435,7 +463,7 @@ func authorizeRequest(ctx context.Context, r *http.Request, action policy.Action Groups: cred.Groups, Action: action, BucketName: bucket, - ConditionValues: getConditionValues(r, region, auth.AnonymousCredentials), + ConditionValues: getConditionValuesWithTags(r, region, auth.AnonymousCredentials, existingTags, requestTags), IsOwner: false, ObjectName: object, }) { @@ -451,7 +479,7 @@ func authorizeRequest(ctx context.Context, r *http.Request, action policy.Action Groups: cred.Groups, Action: policy.ListBucketAction, BucketName: bucket, - ConditionValues: getConditionValues(r, region, auth.AnonymousCredentials), + ConditionValues: getConditionValuesWithTags(r, region, auth.AnonymousCredentials, existingTags, requestTags), IsOwner: false, ObjectName: object, }) { @@ -468,7 +496,7 @@ func authorizeRequest(ctx context.Context, r *http.Request, action policy.Action Groups: cred.Groups, Action: policy.Action(policy.DeleteObjectVersionAction), BucketName: bucket, - ConditionValues: getConditionValues(r, "", cred), + ConditionValues: getConditionValuesWithTags(r, "", cred, existingTags, requestTags), ObjectName: object, IsOwner: owner, Claims: cred.Claims, @@ -482,7 +510,7 @@ func authorizeRequest(ctx context.Context, r *http.Request, action policy.Action Groups: cred.Groups, Action: action, BucketName: bucket, - ConditionValues: getConditionValues(r, "", cred), + ConditionValues: getConditionValuesWithTags(r, "", cred, existingTags, requestTags), ObjectName: object, IsOwner: owner, Claims: cred.Claims, @@ -499,7 +527,7 @@ func authorizeRequest(ctx context.Context, r *http.Request, action policy.Action Groups: cred.Groups, Action: policy.ListBucketAction, BucketName: bucket, - ConditionValues: getConditionValues(r, "", cred), + ConditionValues: getConditionValuesWithTags(r, "", cred, existingTags, requestTags), ObjectName: object, IsOwner: owner, Claims: cred.Claims, @@ -720,6 +748,10 @@ func isPutRetentionAllowed(bucketName, objectName string, retDays int, retDate t // call verifies bucket policies and IAM policies, supports multi user // checks etc. func isPutActionAllowed(ctx context.Context, atype authType, bucketName, objectName string, r *http.Request, action policy.Action) (s3Err APIErrorCode) { + return isPutActionAllowedWithRequestTags(ctx, atype, bucketName, objectName, r, action, nil) +} + +func isPutActionAllowedWithRequestTags(ctx context.Context, atype authType, bucketName, objectName string, r *http.Request, action policy.Action, requestTags *string) (s3Err APIErrorCode) { var cred auth.Credentials var owner bool region := globalSite.Region() @@ -760,7 +792,7 @@ func isPutActionAllowed(ctx context.Context, atype authType, bucketName, objectN Groups: cred.Groups, Action: action, BucketName: bucketName, - ConditionValues: getConditionValues(r, "", auth.AnonymousCredentials), + ConditionValues: getConditionValuesWithTags(r, "", auth.AnonymousCredentials, "", requestTags), IsOwner: false, ObjectName: objectName, }) { @@ -774,7 +806,7 @@ func isPutActionAllowed(ctx context.Context, atype authType, bucketName, objectN Groups: cred.Groups, Action: action, BucketName: bucketName, - ConditionValues: getConditionValues(r, "", cred), + ConditionValues: getConditionValuesWithTags(r, "", cred, "", requestTags), ObjectName: objectName, IsOwner: owner, Claims: cred.Claims, diff --git a/cmd/bucket-policy.go b/cmd/bucket-policy.go index 0bcbe2e66..d096705b2 100644 --- a/cmd/bucket-policy.go +++ b/cmd/bucket-policy.go @@ -34,6 +34,7 @@ import ( xhttp "github.com/minio/minio/internal/http" "github.com/minio/minio/internal/logger" "github.com/minio/pkg/v3/policy" + "github.com/minio/pkg/v3/policy/condition" ) // PolicySys - policy subsystem. @@ -75,7 +76,104 @@ func getSTSConditionValues(r *http.Request, lc string, cred auth.Credentials) ma return m } +type conditionValueSource uint8 + +const ( + conditionValueFromHeader conditionValueSource = 1 << iota + conditionValueFromQuery +) + +// clientSuppliedConditionKeys records where each request-derived condition +// value actually comes from. Most x-amz-* values are headers, list parameters +// are query-only, and storage class retains the compatible query form consumed +// by object operations. +var clientSuppliedConditionKeys = map[string]conditionValueSource{ + "prefix": conditionValueFromQuery, + "delimiter": conditionValueFromQuery, + "max-keys": conditionValueFromQuery, + // AWS explicitly excludes the query-string form from this policy key, + // even though MinIO may consume it separately while verifying a presign. + "x-amz-content-sha256": conditionValueFromHeader, + "x-amz-copy-source": conditionValueFromHeader, + "x-amz-metadata-directive": conditionValueFromHeader, + "x-amz-server-side-encryption": conditionValueFromHeader, + "x-amz-server-side-encryption-aws-kms-key-id": conditionValueFromHeader, + "x-amz-server-side-encryption-customer-algorithm": conditionValueFromHeader, + "x-amz-storage-class": conditionValueFromHeader | conditionValueFromQuery, +} + +func acceptsConditionValueSource(key string, source conditionValueSource) bool { + name := strings.ToLower(key) + allowed, ok := clientSuppliedConditionKeys[name] + if !ok { + return true + } + if allowed&source == 0 { + return false + } + return source != conditionValueFromQuery || key == name +} + +// internalConditionKeys holds every other name a condition key can resolve to. +// Those name values MinIO derives for itself - identity from the credential, +// time from the clock, transport from the connection - and a request must never +// write one, whether or not the server populated it this time round: a name the +// server left empty is as forgeable as one it filled in, and the condition +// reading it cannot tell the difference. +// +// Deriving the set from the condition keys rather than from what +// getConditionValues writes is what makes it complete. The engine reads by key +// name, so the key list is the attack surface; enumerating the writes misses +// every key the server has no value for, which is most of jwt: and ldap:. It +// also defaults new upstream keys to reserved, which is the safe direction. +// +// Reserving a name only removes it from the condition map. Request handling is +// untouched - a handler still reads its own query parameters and headers. +// +// aws:SourceIp is still only as trustworthy as the forwarding headers it is +// computed from, see the note on GetSourceIPFromHeaders. +var internalConditionKeys = func() map[string]struct{} { + keys := make(map[string]struct{}, 2*len(condition.AllSupportedKeys)) + for _, keyName := range condition.AllSupportedKeys { + name := keyName.ToKey().Name() + if _, clientSupplied := clientSuppliedConditionKeys[name]; clientSupplied { + continue + } + // A condition key resolves against its exact name and falls back to the + // canonical MIME form, so both spellings have to be held. This is also + // what covers object lock, stored as Object-Lock-Mode and read as + // s3:object-lock-mode. + keys[name] = struct{}{} + keys[http.CanonicalHeaderKey(name)] = struct{}{} + } + return keys +}() + +// Tag conditions name one tag key each, so the variable forms are reserved by +// prefix; the bare names come from the loop above. +var internalConditionKeyPrefixes = []string{"ExistingObjectTag/", "RequestObjectTag/"} + +func isInternalConditionKey(key string) bool { + if _, ok := internalConditionKeys[key]; ok { + return true + } + for _, prefix := range internalConditionKeyPrefixes { + if strings.HasPrefix(key, prefix) { + return true + } + } + return false +} + func getConditionValues(r *http.Request, lc string, cred auth.Credentials) map[string][]string { + return getConditionValuesWithExistingTags(r, lc, cred, "") +} + +func getConditionValuesWithExistingTags(r *http.Request, lc string, cred auth.Credentials, existingTags string) map[string][]string { + return getConditionValuesWithTags(r, lc, cred, existingTags, nil) +} + +func getConditionValuesWithTags(r *http.Request, lc string, cred auth.Credentials, existingTags string, requestTags *string) map[string][]string { currTime := UTCNow() var ( @@ -144,26 +242,44 @@ func getConditionValues(r *http.Request, lc string, cred auth.Credentials) map[s if lc != "" { args["LocationConstraint"] = []string{lc} } - - cloneHeader := r.Header.Clone() - if v := cloneHeader.Get("x-amz-signature-age"); v != "" { - args["signatureAge"] = []string{v} - cloneHeader.Del("x-amz-signature-age") + if storageClass, ok := getRequestHeaderOrQueryValue(r, xhttp.AmzStorageClass); ok { + args[strings.ToLower(xhttp.AmzStorageClass)] = []string{storageClass} } - if userTags := cloneHeader.Get(xhttp.AmzObjectTagging); userTags != "" { + cloneHeader := r.Header.Clone() + signatureAge := cloneHeader.Get("x-amz-signature-age") + cloneHeader.Del("x-amz-signature-age") + // The presigned V4 verifier overwrites this internal scratch header after + // validating the signature. Ignore a value supplied on every other request + // type, where it would otherwise synthesize s3:signatureAge. + if authType == authTypePresigned && signatureAge != "" { + args["signatureAge"] = []string{signatureAge} + } + + userTags := cloneHeader.Get(xhttp.AmzObjectTagging) + if requestTags != nil { + userTags = *requestTags + } + if userTags != "" { tag, _ := tags.ParseObjectTags(userTags) if tag != nil { tagMap := tag.ToMap() keys := make([]string, 0, len(tagMap)) for k, v := range tagMap { - args[pathJoin("ExistingObjectTag", k)] = []string{v} args[pathJoin("RequestObjectTag", k)] = []string{v} keys = append(keys, k) } args["RequestObjectTagKeys"] = keys } } + if existingTags != "" { + tag, _ := tags.ParseObjectTags(existingTags) + if tag != nil { + for k, v := range tag.ToMap() { + args[pathJoin("ExistingObjectTag", k)] = []string{v} + } + } + } for _, objLock := range []string{ xhttp.AmzObjectLockMode, @@ -176,8 +292,20 @@ func getConditionValues(r *http.Request, lc string, cred auth.Credentials) map[s cloneHeader.Del(objLock) } + // The two loops below fold raw header and query values into the same map + // the server just filled in. Anything they add is indistinguishable, to a + // condition, from a value the server derived - and they merge by appending, + // so a supplied entry sits alongside the real one rather than replacing it. + // The source check keeps headers and query parameters in their actual roles; + // isInternalConditionKey keeps both apart from server-derived values. for key, values := range cloneHeader { - if strings.EqualFold(key, xhttp.AmzObjectTagging) { + if strings.EqualFold(key, xhttp.AmzObjectTagging) || strings.EqualFold(key, xhttp.AmzStorageClass) { + continue + } + if !acceptsConditionValueSource(key, conditionValueFromHeader) { + continue + } + if isInternalConditionKey(key) { continue } if existingValues, found := args[key]; found { @@ -190,18 +318,16 @@ func getConditionValues(r *http.Request, lc string, cred auth.Credentials) map[s cloneURLValues := make(url.Values, len(r.Form)) maps.Copy(cloneURLValues, r.Form) - for _, objLock := range []string{ - xhttp.AmzObjectLockMode, - xhttp.AmzObjectLockLegalHold, - xhttp.AmzObjectLockRetainUntilDate, - } { - if values, ok := cloneURLValues[objLock]; ok { - args[strings.TrimPrefix(objLock, "X-Amz-")] = values - } - cloneURLValues.Del(objLock) - } - for key, values := range cloneURLValues { + if strings.EqualFold(key, xhttp.AmzObjectTagging) || strings.EqualFold(key, xhttp.AmzStorageClass) { + continue + } + if !acceptsConditionValueSource(key, conditionValueFromQuery) { + continue + } + if isInternalConditionKey(key) { + continue + } if existingValues, found := args[key]; found { args[key] = append(existingValues, values...) } else { diff --git a/cmd/bucket-policy_test.go b/cmd/bucket-policy_test.go new file mode 100644 index 000000000..4e24d3e6c --- /dev/null +++ b/cmd/bucket-policy_test.go @@ -0,0 +1,482 @@ +// Copyright (c) 2015-2026 MinIO, Inc. +// +// This file is part of MinIO Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import ( + "net" + "net/http" + "net/url" + "slices" + "strings" + "testing" + + "github.com/minio/minio/internal/auth" + xhttp "github.com/minio/minio/internal/http" + "github.com/minio/pkg/v3/policy" + "github.com/minio/pkg/v3/policy/condition" +) + +const ( + testCondSourceIP = "203.0.113.5" + testCondRemoteILP = testCondSourceIP + ":12345" +) + +func condValuesForRequest(t *testing.T, rawURL string, header map[string]string) map[string][]string { + return condValuesForRequestWithTags(t, rawURL, header, "", nil) +} + +func condValuesForRequestWithExistingTags(t *testing.T, rawURL string, header map[string]string, existingTags string) map[string][]string { + return condValuesForRequestWithTags(t, rawURL, header, existingTags, nil) +} + +func condValuesForRequestWithTags(t *testing.T, rawURL string, header map[string]string, existingTags string, requestTags *string) map[string][]string { + t.Helper() + r, err := http.NewRequest(http.MethodGet, rawURL, nil) + if err != nil { + t.Fatal(err) + } + r.RemoteAddr = testCondRemoteILP + for k, v := range header { + r.Header.Set(k, v) + } + if err := r.ParseForm(); err != nil { + t.Fatal(err) + } + return getConditionValuesWithTags(r, "us-east-1", auth.Credentials{AccessKey: "lowpriv"}, existingTags, requestTags) +} + +func resolvedConditionValues(values map[string][]string, name string) []string { + if v := values[name]; len(v) > 0 { + return v + } + return values[http.CanonicalHeaderKey(name)] +} + +// A client must not be able to reach a condition key that the server computes +// for itself. Both routes are covered: a header whose canonical spelling +// collides with the key name, and a query parameter that collides with it +// exactly. The query route is the sharper one, because the merge appended to +// the server's value rather than replacing it and a condition function matches +// when any single value matches. +func TestGetConditionValuesRejectsClientSuppliedServerKeys(t *testing.T) { + honest := condValuesForRequest(t, "http://minio.local/bkt/obj", nil) + + for _, kn := range condition.AllSupportedKeys { + name := kn.ToKey().Name() + if _, clientSupplied := clientSuppliedConditionKeys[name]; clientSupplied { + continue // the request is where this one is supposed to come from + } + // Deliberately not skipped when the server left the key empty. An empty + // name is exactly as forgeable as a populated one, and the keys the + // server has no value for - most of jwt: and ldap: - are the ones a + // resource variable expands. + want := honest[name] + canonical := http.CanonicalHeaderKey(name) + + t.Run("query/"+name, func(t *testing.T) { + got := condValuesForRequest(t, + "http://minio.local/bkt/obj?"+url.Values{name: {"ATTACKER"}}.Encode(), nil) + if slices.Contains(got[name], "ATTACKER") { + t.Errorf("?%s= reached %v, server computed %v", name, got[name], want) + } + if !slices.Equal(got[name], want) { + t.Errorf("%v changed to %v", want, got[name]) + } + }) + + // aws:Referer is read out of the Referer header, so the header is its + // source of truth rather than a way to forge it. aws:UserAgent is not + // in the same position: it comes from User-Agent, which does not + // canonicalise to "Useragent". + if kn == condition.AWSReferer { + continue + } + + t.Run("header/"+canonical, func(t *testing.T) { + got := condValuesForRequest(t, "http://minio.local/bkt/obj", + map[string]string{canonical: "ATTACKER"}) + // The lookup the policy engine itself performs, exact name first + // with the canonical form as fallback. + seen := got[name] + if len(seen) == 0 { + seen = got[canonical] + } + if slices.Contains(seen, "ATTACKER") { + t.Errorf("%s: header reached the lookup as %v, server computed %v", + canonical, seen, want) + } + }) + } +} + +func TestGetConditionValuesUsesActualRequestSource(t *testing.T) { + for name, source := range clientSuppliedConditionKeys { + t.Run(name, func(t *testing.T) { + fromHeader := condValuesForRequest(t, "http://minio.local/bkt/obj", + map[string]string{name: "HEADER"}) + fromQuery := condValuesForRequest(t, + "http://minio.local/bkt/obj?"+url.Values{name: {"QUERY"}}.Encode(), nil) + fromCanonicalQuery := condValuesForRequest(t, + "http://minio.local/bkt/obj?"+url.Values{http.CanonicalHeaderKey(name): {"QUERY"}}.Encode(), nil) + if got, want := slices.Contains(resolvedConditionValues(fromHeader, name), "HEADER"), source&conditionValueFromHeader != 0; got != want { + t.Errorf("header accepted=%v, want %v: %v", got, want, fromHeader) + } + if got, want := slices.Contains(resolvedConditionValues(fromQuery, name), "QUERY"), source&conditionValueFromQuery != 0; got != want { + t.Errorf("query accepted=%v, want %v: %v", got, want, fromQuery) + } + canonicalQueryAllowed := name == strings.ToLower(xhttp.AmzStorageClass) + if got := slices.Contains(resolvedConditionValues(fromCanonicalQuery, name), "QUERY"); got != canonicalQueryAllowed { + t.Errorf("case-variant query accepted=%v, want %v: %v", got, canonicalQueryAllowed, fromCanonicalQuery) + } + }) + } + + storageURL := "http://minio.local/bkt/obj?" + url.Values{ + strings.ToLower(xhttp.AmzStorageClass): {"QUERY"}, + }.Encode() + storageValues := condValuesForRequest(t, storageURL, map[string]string{xhttp.AmzStorageClass: "HEADER"}) + if got := resolvedConditionValues(storageValues, strings.ToLower(xhttp.AmzStorageClass)); !slices.Equal(got, []string{"HEADER"}) { + t.Errorf("storage class did not use header precedence: %v", got) + } + + fromQuery := condValuesForRequest(t, + "http://minio.local/bkt/obj?"+url.Values{xhttp.AmzObjectLockMode: {"COMPLIANCE"}}.Encode(), nil) + if got := resolvedConditionValues(fromQuery, "object-lock-mode"); len(got) != 0 { + t.Errorf("object-lock query value reached header condition as %v", got) + } +} + +func TestGetConditionValuesUsesEffectiveRequestTags(t *testing.T) { + rawURL := "http://minio.local/bkt/obj?" + url.Values{ + strings.ToLower(xhttp.AmzObjectTagging): {"security=public&virus=true"}, + }.Encode() + + // Generic operations such as CopyObject must not gain RequestObjectTag + // values from a query parameter they do not consume. + withoutEffectiveTags := condValuesForRequest(t, rawURL, nil) + if len(withoutEffectiveTags["RequestObjectTag/security"]) != 0 || len(withoutEffectiveTags["RequestObjectTagKeys"]) != 0 { + t.Fatalf("query tags leaked into a generic operation: %v", withoutEffectiveTags) + } + + effectiveTags := "security=public&virus=true" + withEffectiveTags := condValuesForRequestWithTags(t, rawURL, nil, "", &effectiveTags) + if !slices.Equal(withEffectiveTags["RequestObjectTag/security"], []string{"public"}) { + t.Fatalf("effective request tag missing: %v", withEffectiveTags) + } + if !slices.Contains(withEffectiveTags["RequestObjectTagKeys"], "security") || + !slices.Contains(withEffectiveTags["RequestObjectTagKeys"], "virus") { + t.Fatalf("effective request tag keys missing: %v", withEffectiveTags["RequestObjectTagKeys"]) + } + + security, err := condition.NewStringEqualsFunc("", condition.NewKey(condition.RequestObjectTag, "security"), "public") + if err != nil { + t.Fatal(err) + } + allowedKeys, err := condition.NewStringLikeFunc("ForAllValues", condition.RequestObjectTagKeys.ToKey(), "security", "virus") + if err != nil { + t.Fatal(err) + } + conditions := condition.NewFunctions(security, allowedKeys) + if conditions.Evaluate(withoutEffectiveTags) { + t.Fatal("query upload satisfied request-tag policy without effective tags") + } + if !conditions.Evaluate(withEffectiveTags) { + t.Fatal("effective query tags did not satisfy request-tag policy") + } +} + +func TestBucketPolicySSEConditionUsesHeader(t *testing.T) { + fn, err := condition.NewStringEqualsFunc("", condition.S3XAmzServerSideEncryption.ToKey(), "aws:kms") + if err != nil { + t.Fatal(err) + } + conditions := condition.NewFunctions(fn) + if conditions.Evaluate(condValuesForRequest(t, + "http://minio.local/bkt/obj?x-amz-server-side-encryption=aws%3Akms", nil)) { + t.Error("query parameter satisfied a condition on the SSE request header") + } + if !conditions.Evaluate(condValuesForRequest(t, "http://minio.local/bkt/obj", + map[string]string{xhttp.AmzServerSideEncryption: "aws:kms"})) { + t.Error("SSE request header did not satisfy its condition") + } +} + +// The end to end shape of the bypass: an IpAddress condition restricting a +// bucket to an internal range, against a request from outside it. +func TestBucketPolicySourceIPCannotBeForged(t *testing.T) { + _, cidr, err := net.ParseCIDR("10.0.0.0/8") + if err != nil { + t.Fatal(err) + } + fn, err := condition.NewIPAddressFunc(condition.AWSSourceIP.ToKey(), cidr) + if err != nil { + t.Fatal(err) + } + bp := policy.BucketPolicy{ + Version: policy.DefaultVersion, + Statements: []policy.BPStatement{{ + Effect: policy.Allow, + Principal: policy.NewPrincipal("*"), + Actions: policy.NewActionSet(policy.GetObjectAction), + Resources: policy.NewResourceSet(policy.NewResource("bkt/*")), + Conditions: condition.NewFunctions(fn), + }}, + } + allowed := func(rawURL string, header map[string]string) bool { + return bp.IsAllowed(policy.BucketPolicyArgs{ + Action: policy.GetObjectAction, + BucketName: "bkt", + ObjectName: "obj", + ConditionValues: condValuesForRequest(t, rawURL, header), + }) + } + + if allowed("http://minio.local/bkt/obj", nil) { + t.Fatal("baseline: an address outside 10.0.0.0/8 must not satisfy the condition") + } + if allowed("http://minio.local/bkt/obj?SourceIp=10.1.2.3", nil) { + t.Error("a query parameter forged aws:SourceIp") + } + if allowed("http://minio.local/bkt/obj", map[string]string{"Sourceip": "10.1.2.3"}) { + t.Error("a header forged aws:SourceIp") + } +} + +// "Deny unless the connection is TLS" is the usual hardening statement, and +// aws:SecureTransport is computed from r.TLS. +func TestBucketPolicySecureTransportCannotBeForged(t *testing.T) { + fn, err := condition.NewBoolFunc(condition.AWSSecureTransport.ToKey(), false) + if err != nil { + t.Fatal(err) + } + bp := policy.BucketPolicy{ + Version: policy.DefaultVersion, + Statements: []policy.BPStatement{ + { + Effect: policy.Allow, Principal: policy.NewPrincipal("*"), + Actions: policy.NewActionSet(policy.GetObjectAction), + Resources: policy.NewResourceSet(policy.NewResource("bkt/*")), + }, + { + Effect: policy.Deny, Principal: policy.NewPrincipal("*"), + Actions: policy.NewActionSet(policy.GetObjectAction), + Resources: policy.NewResourceSet(policy.NewResource("bkt/*")), + Conditions: condition.NewFunctions(fn), + }, + }, + } + allowed := func(rawURL string, header map[string]string) bool { + return bp.IsAllowed(policy.BucketPolicyArgs{ + Action: policy.GetObjectAction, + BucketName: "bkt", + ObjectName: "obj", + ConditionValues: condValuesForRequest(t, rawURL, header), + }) + } + + // r.TLS is nil throughout, so every one of these is a plaintext request. + if allowed("http://minio.local/bkt/obj", nil) { + t.Fatal("baseline: a plaintext request must be denied") + } + if allowed("http://minio.local/bkt/obj?SecureTransport=true", nil) { + t.Error("a query parameter forged aws:SecureTransport") + } + if allowed("http://minio.local/bkt/obj", map[string]string{"Securetransport": "true"}) { + t.Error("a header forged aws:SecureTransport") + } +} + +// Reserving the server's own keys must not stop the request from supplying the +// values that are client-derived by design. +func TestGetConditionValuesKeepsClientDerivedKeys(t *testing.T) { + got := condValuesForRequest(t, "http://minio.local/bkt/obj?prefix=team%2F", + map[string]string{ + xhttp.AmzObjectLockMode: "GOVERNANCE", + xhttp.AmzServerSideEncryption: "aws:kms", + "X-Amz-Meta-Team": "storage", + xhttp.AmzObjectTagging: "project=silo", + }) + + for _, tc := range []struct { + key string + want string + }{ + {"Object-Lock-Mode", "GOVERNANCE"}, + {xhttp.AmzServerSideEncryption, "aws:kms"}, + {"X-Amz-Meta-Team", "storage"}, + {"RequestObjectTag/project", "silo"}, + {"prefix", "team/"}, + } { + if !slices.Contains(got[tc.key], tc.want) { + t.Errorf("%s: expected %q, got %v", tc.key, tc.want, got[tc.key]) + } + } + if !slices.Contains(got["RequestObjectTagKeys"], "project") { + t.Errorf("RequestObjectTagKeys: expected project, got %v", got["RequestObjectTagKeys"]) + } + + if len(got["ExistingObjectTag/project"]) != 0 { + t.Errorf("request tags leaked into ExistingObjectTag: %v", got["ExistingObjectTag/project"]) + } +} + +func TestGetConditionValuesSeparatesRequestAndExistingTags(t *testing.T) { + got := condValuesForRequestWithExistingTags(t, "http://minio.local/bkt/obj", + map[string]string{xhttp.AmzObjectTagging: "project=request&new=yes"}, + "project=stored&old=yes") + + for _, tc := range []struct { + key string + want string + }{ + {"RequestObjectTag/project", "request"}, + {"RequestObjectTag/new", "yes"}, + {"ExistingObjectTag/project", "stored"}, + {"ExistingObjectTag/old", "yes"}, + } { + if !slices.Equal(got[tc.key], []string{tc.want}) { + t.Errorf("%s: expected %q, got %v", tc.key, tc.want, got[tc.key]) + } + } + if len(got["ExistingObjectTag/new"]) != 0 || len(got["RequestObjectTag/old"]) != 0 { + t.Errorf("tag sources crossed: request new=%v, existing old=%v", + got["ExistingObjectTag/new"], got["RequestObjectTag/old"]) + } +} + +// Keys the server did not populate for this request are as forgeable as ones it +// did, so the reservation cannot depend on presence. +func TestGetConditionValuesRejectsAbsentInternalKeys(t *testing.T) { + for _, key := range []string{ + "signatureAge", + "groups", + "DurationSeconds", + "ExistingObjectTag/security", + "RequestObjectTag/security", + "RequestObjectTagKeys", + "object-lock-mode", + "object-lock-remaining-retention-days", + } { + t.Run(key, func(t *testing.T) { + got := condValuesForRequest(t, + "http://minio.local/bkt/obj?"+url.Values{key: {"ATTACKER"}}.Encode(), nil) + if slices.Contains(got[key], "ATTACKER") { + t.Errorf("?%s= was accepted into the condition values as %v", key, got[key]) + } + }) + } +} + +func TestGetConditionValuesOnlyAcceptsPresignedSignatureAge(t *testing.T) { + const signatureAgeHeader = "x-amz-signature-age" + + for _, tc := range []struct { + name string + target string + headers map[string]string + want bool + }{ + { + name: "anonymous client header", + target: "http://minio.local/bkt/obj", + headers: map[string]string{signatureAgeHeader: "1"}, + }, + { + name: "header-signed client header", + target: "http://minio.local/bkt/obj", + headers: map[string]string{ + xhttp.Authorization: signV4Algorithm + " attacker", + signatureAgeHeader: "1", + }, + }, + { + name: "presigned verifier value", + target: "http://minio.local/bkt/obj?" + url.Values{ + xhttp.AmzCredential: {"access/20260803/us-east-1/s3/aws4_request"}, + }.Encode(), + headers: map[string]string{signatureAgeHeader: "250"}, + want: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + got := condValuesForRequest(t, tc.target, tc.headers) + _, ok := got["signatureAge"] + if ok != tc.want { + t.Fatalf("signatureAge presence: expected %v, got %v", tc.want, got["signatureAge"]) + } + }) + } +} + +// The object-lock value is stored under the header spelling while the policy key +// that reads it is lower case. Reserving only one spelling lets the other be +// supplied and resolved in its place - which the policy package's exact-name +// lookup then prefers over the real one. +func TestGetConditionValuesObjectLockSpelling(t *testing.T) { + got := condValuesForRequest(t, + "http://minio.local/bkt/obj?object-lock-mode=COMPLIANCE", + map[string]string{xhttp.AmzObjectLockMode: "GOVERNANCE"}) + + if v, ok := got["object-lock-mode"]; ok { + t.Errorf("the lower-case spelling was accepted: %v", v) + } + if !slices.Equal(got["Object-Lock-Mode"], []string{"GOVERNANCE"}) { + t.Errorf("expected the header value to stand, got %v", got["Object-Lock-Mode"]) + } + + fn, err := condition.NewStringEqualsFunc("", + condition.S3ObjectLockMode.ToKey(), "COMPLIANCE") + if err != nil { + t.Fatal(err) + } + if condition.NewFunctions(fn).Evaluate(got) { + t.Error("a policy requiring COMPLIANCE was satisfied by a GOVERNANCE request") + } +} + +// Resource variables read the condition map directly, so a forgeable key is a +// forgeable resource path. ${ldap:user} and ${jwt:preferred_username} are the +// home-directory idiom for LDAP and OIDC deployments; the server derives them +// from the credential, and a request must not be able to answer them. +func TestBucketPolicyResourceVariableCannotBeForged(t *testing.T) { + for _, tc := range []struct{ variable, param, value string }{ + {"${ldap:user}", "user", "alice"}, + {"${ldap:username}", "username", "alice"}, + {"${jwt:preferred_username}", "preferred_username", "alice"}, + {"${jwt:sub}", "sub", "alice"}, + {"${aws:username}", "username", "alice"}, + } { + t.Run(tc.variable, func(t *testing.T) { + bp := policy.BucketPolicy{Version: policy.DefaultVersion, Statements: []policy.BPStatement{{ + Effect: policy.Allow, + Principal: policy.NewPrincipal("*"), + Actions: policy.NewActionSet(policy.GetObjectAction), + Resources: policy.NewResourceSet(policy.NewResource("bkt/" + tc.variable + "/*")), + }}} + args := policy.BucketPolicyArgs{ + Action: policy.GetObjectAction, BucketName: "bkt", ObjectName: tc.value + "/secret", + } + args.ConditionValues = condValuesForRequest(t, + "http://minio.local/bkt/"+tc.value+"/secret?"+ + url.Values{tc.param: {tc.value}}.Encode(), nil) + if bp.IsAllowed(args) { + t.Errorf("?%s=%s expanded %s and granted the prefix", tc.param, tc.value, tc.variable) + } + }) + } +} diff --git a/cmd/handler-utils.go b/cmd/handler-utils.go index 7e70d5cb2..ad087dcf4 100644 --- a/cmd/handler-utils.go +++ b/cmd/handler-utils.go @@ -142,7 +142,62 @@ var userMetadataKeyPrefixes = []string{ // extractMetadataFromReq extracts metadata from HTTP header and HTTP queryString. func extractMetadataFromReq(ctx context.Context, r *http.Request) (metadata map[string]string, err error) { - return extractMetadata(ctx, textproto.MIMEHeader(r.Form), textproto.MIMEHeader(r.Header)) + metadata, err = extractMetadata(ctx, textproto.MIMEHeader(r.Form), textproto.MIMEHeader(r.Header)) + if err != nil { + return nil, err + } + + // Keep the metadata consumed by object operations in lock-step with policy + // conditions: an explicitly present header wins, otherwise use the query + // value accepted by the existing S3-compatible request path. + for _, name := range []string{xhttp.AmzStorageClass, xhttp.AmzObjectTagging} { + if value, ok := getRequestHeaderOrQueryValue(r, name); ok { + metadata[name] = value + } + } + return metadata, nil +} + +// getRequestHeaderOrQueryValue returns the effective value of a request field. +// Header presence takes precedence even when its value is empty. Query lookup +// remains case-insensitive for compatibility with extractMetadataFromReq. +func getRequestHeaderOrQueryValue(r *http.Request, name string) (string, bool) { + if values, ok := getRequestValues(r.Header, name, http.CanonicalHeaderKey(name)); ok { + return strings.Join(values, ","), true + } + if values, ok := getRequestValues(http.Header(r.Form), name, strings.ToLower(name)); ok { + return strings.Join(values, ","), true + } + return "", false +} + +func getRequestValues(values http.Header, name, preferred string) ([]string, bool) { + if value, ok := values[preferred]; ok { + return value, true + } + + canonical, lower := http.CanonicalHeaderKey(name), strings.ToLower(name) + for _, key := range []string{canonical, lower} { + if key == preferred { + continue + } + if value, ok := values[key]; ok { + return value, true + } + } + + // Multiple differently-cased spellings are malformed but were previously + // accepted. Pick one deterministically instead of depending on map order. + match := "" + for key := range values { + if strings.EqualFold(key, name) && (match == "" || key < match) { + match = key + } + } + if match != "" { + return values[match], true + } + return nil, false } func extractMetadata(ctx context.Context, mimesHeader ...textproto.MIMEHeader) (metadata map[string]string, err error) { diff --git a/cmd/handler-utils_test.go b/cmd/handler-utils_test.go index f6691d329..6a7bcd611 100644 --- a/cmd/handler-utils_test.go +++ b/cmd/handler-utils_test.go @@ -27,6 +27,7 @@ import ( "net/url" "os" "reflect" + "strings" "testing" "github.com/minio/minio/internal/config" @@ -194,6 +195,63 @@ func TestExtractMetadataHeaders(t *testing.T) { } } +func TestExtractMetadataFromRequestUsesHeaderPrecedence(t *testing.T) { + query := make(url.Values) + query.Set(strings.ToLower(xhttp.AmzStorageClass), "QUERY-CLASS") + query.Set(strings.ToLower(xhttp.AmzObjectTagging), "source=query") + req, err := http.NewRequest(http.MethodGet, "http://localhost/test?"+query.Encode(), nil) + if err != nil { + t.Fatal(err) + } + req.Header.Set(xhttp.AmzStorageClass, "HEADER-CLASS") + req.Header.Set(xhttp.AmzObjectTagging, "source=header") + if err = req.ParseForm(); err != nil { + t.Fatal(err) + } + + metadata, err := extractMetadataFromReq(t.Context(), req) + if err != nil { + t.Fatal(err) + } + if got := metadata[xhttp.AmzStorageClass]; got != "HEADER-CLASS" { + t.Fatalf("storage class: expected header, got %q", got) + } + if got := metadata[xhttp.AmzObjectTagging]; got != "source=header" { + t.Fatalf("tagging: expected header, got %q", got) + } + + // Presence, rather than a non-empty value, establishes precedence. This + // prevents a query value from taking over when a signed header is empty. + req.Header[xhttp.AmzObjectTagging] = []string{""} + if got, ok := getRequestHeaderOrQueryValue(req, xhttp.AmzObjectTagging); !ok || got != "" { + t.Fatalf("empty header did not override query: value=%q present=%v", got, ok) + } +} + +func TestExtractMetadataFromRequestKeepsQueryCompatibility(t *testing.T) { + query := make(url.Values) + query.Set(strings.ToLower(xhttp.AmzStorageClass), "REDUCED_REDUNDANCY") + query.Set(strings.ToLower(xhttp.AmzObjectTagging), "security=public") + req, err := http.NewRequest(http.MethodGet, "http://localhost/test?"+query.Encode(), nil) + if err != nil { + t.Fatal(err) + } + if err = req.ParseForm(); err != nil { + t.Fatal(err) + } + + metadata, err := extractMetadataFromReq(t.Context(), req) + if err != nil { + t.Fatal(err) + } + if got := metadata[xhttp.AmzStorageClass]; got != "REDUCED_REDUNDANCY" { + t.Fatalf("storage class query value lost: %q", got) + } + if got := metadata[xhttp.AmzObjectTagging]; got != "security=public" { + t.Fatalf("tagging query value lost: %q", got) + } +} + func TestExtractReplicationMetadataHeaders(t *testing.T) { header := http.Header{ "X-Minio-Replication-Server-Side-Encryption-Sealed-Key": []string{"sealed-key"}, diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index 7b04337bf..296817ed5 100644 --- a/cmd/object-handlers.go +++ b/cmd/object-handlers.go @@ -387,11 +387,7 @@ func (api objectAPIHandlers) getObjectHandler(ctx context.Context, objectAPI Obj return true } - if oi.UserTags != "" { - r.Header.Set(xhttp.AmzObjectTagging, oi.UserTags) - } - - if s3Error := authorizeRequest(ctx, r, policy.GetObjectAction); s3Error != ErrNone { + if s3Error := authorizeRequestWithExistingTags(ctx, r, policy.GetObjectAction, oi.UserTags); s3Error != ErrNone { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Error), r.URL) return true } @@ -429,15 +425,17 @@ func (api objectAPIHandlers) getObjectHandler(ctx context.Context, objectAPI Obj } } if reader == nil || !proxy.Proxy { + // The conditional callback has already written 304/412. Do not + // authorize again without the stored tags or write a second response. + if isErrPreconditionFailed(err) { + return + } // validate if the request indeed was authorized, if it wasn't we need to return "ErrAccessDenied" // instead of any namespace related error. if s3Error := authorizeRequest(ctx, r, policy.GetObjectAction); s3Error != ErrNone { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Error), r.URL) return } - if isErrPreconditionFailed(err) { - return - } if proxy.Err != nil { writeErrorResponse(ctx, w, toAPIError(ctx, proxy.Err), r.URL) return @@ -839,12 +837,7 @@ func (api objectAPIHandlers) headObjectHandler(ctx context.Context, objectAPI Ob } } - if objInfo.UserTags != "" { - // Set this such that authorization policies can be applied on the object tags. - r.Header.Set(xhttp.AmzObjectTagging, objInfo.UserTags) - } - - if s3Error := authorizeRequest(ctx, r, policy.GetObjectAction); s3Error != ErrNone { + if s3Error := authorizeRequestWithExistingTags(ctx, r, policy.GetObjectAction, objInfo.UserTags); s3Error != ErrNone { writeErrorResponseHeadersOnly(w, errorCodes.ToAPIErr(s3Error)) return } @@ -1055,10 +1048,7 @@ func getCpObjMetadataFromHeader(ctx context.Context, r *http.Request, userMeta m // Storage class is special, it can be replaced regardless of the // metadata directive, if set should be preserved and replaced // to the destination metadata. - sc := r.Header.Get(xhttp.AmzStorageClass) - if sc == "" { - sc = r.Form.Get(xhttp.AmzStorageClass) - } + sc, _ := getRequestHeaderOrQueryValue(r, xhttp.AmzStorageClass) // if x-amz-metadata-directive says REPLACE then // we extract metadata from the input headers. @@ -1256,9 +1246,10 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re return } - // Validate storage class metadata if present - dstSc := r.Header.Get(xhttp.AmzStorageClass) - if dstSc != "" && !storageclass.IsValid(dstSc) { + // Validate the storage class header if present. Query values retain the + // existing compatibility path, including its historical validation behavior. + dstSc, _ := getRequestHeaderOrQueryValue(r, xhttp.AmzStorageClass) + if headerStorageClass := r.Header.Get(xhttp.AmzStorageClass); headerStorageClass != "" && !storageclass.IsValid(headerStorageClass) { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrInvalidStorageClass), r.URL) return } @@ -1857,7 +1848,8 @@ func (api objectAPIHandlers) PutObjectHandler(w http.ResponseWriter, r *http.Req return } - // Validate storage class metadata if present + // Validate the storage class header if present. Query values retain the + // existing compatibility path, including its historical validation behavior. if sc := r.Header.Get(xhttp.AmzStorageClass); sc != "" { if !storageclass.IsValid(sc) { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrInvalidStorageClass), r.URL) @@ -1906,13 +1898,11 @@ func (api objectAPIHandlers) PutObjectHandler(w http.ResponseWriter, r *http.Req return } - if objTags := r.Header.Get(xhttp.AmzObjectTagging); objTags != "" { + if objTags := metadata[xhttp.AmzObjectTagging]; objTags != "" { if _, err := tags.ParseObjectTags(objTags); err != nil { writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL) return } - - metadata[xhttp.AmzObjectTagging] = objTags } var ( @@ -1924,7 +1914,12 @@ func (api objectAPIHandlers) PutObjectHandler(w http.ResponseWriter, r *http.Req ) // Check if put is allowed - if s3Err = isPutActionAllowed(ctx, rAuthType, bucket, object, r, policy.PutObjectAction); s3Err != ErrNone { + requestTags, hasRequestTags := metadata[xhttp.AmzObjectTagging] + var requestTagsPtr *string + if hasRequestTags { + requestTagsPtr = &requestTags + } + if s3Err = isPutActionAllowedWithRequestTags(ctx, rAuthType, bucket, object, r, policy.PutObjectAction, requestTagsPtr); s3Err != ErrNone { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Err), r.URL) return } @@ -2271,13 +2266,12 @@ func (api objectAPIHandlers) PutObjectExtractHandler(w http.ResponseWriter, r *h return } - // Validate storage class metadata if present - sc := r.Header.Get(xhttp.AmzStorageClass) - if sc != "" { - if !storageclass.IsValid(sc) { - writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrInvalidStorageClass), r.URL) - return - } + // Validate the storage class header if present. PutObjectExtract now also + // consumes the compatible query value so policy and operation stay aligned. + sc, _ := getRequestHeaderOrQueryValue(r, xhttp.AmzStorageClass) + if headerStorageClass := r.Header.Get(xhttp.AmzStorageClass); headerStorageClass != "" && !storageclass.IsValid(headerStorageClass) { + writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrInvalidStorageClass), r.URL) + return } clientETag, err := etag.FromContentMD5(r.Header) @@ -3205,12 +3199,7 @@ func (api objectAPIHandlers) GetObjectTaggingHandler(w http.ResponseWriter, r *h } } - // Set this such that authorization policies can be applied on the object tags. - if tags := ot.String(); tags != "" { - r.Header.Set(xhttp.AmzObjectTagging, tags) - } - - if s3Error := authorizeRequest(ctx, r, policy.GetObjectTaggingAction); s3Error != ErrNone { + if s3Error := authorizeRequestWithExistingTags(ctx, r, policy.GetObjectTaggingAction, ot.String()); s3Error != ErrNone { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Error), r.URL) return } @@ -3264,12 +3253,14 @@ func (api objectAPIHandlers) PutObjectTaggingHandler(w http.ResponseWriter, r *h writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL) return } + tagsStr := tags.String() // Set this such that authorization policies can be applied on the object tags. - r.Header.Set(xhttp.AmzObjectTagging, tags.String()) + r.Header.Set(xhttp.AmzObjectTagging, tagsStr) - // Allow putObjectTagging if policy action is set - if s3Error := checkRequestAuthType(ctx, r, policy.PutObjectTaggingAction, bucket, object); s3Error != ErrNone { + logger.GetReqInfo(ctx).BucketName = bucket + logger.GetReqInfo(ctx).ObjectName = object + if s3Error := authenticateRequest(ctx, r, policy.PutObjectTaggingAction); s3Error != ErrNone { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Error), r.URL) return } @@ -3281,6 +3272,14 @@ func (api objectAPIHandlers) PutObjectTaggingHandler(w http.ResponseWriter, r *h } objInfo, err := objAPI.GetObjectInfo(ctx, bucket, object, opts) + existingTags := "" + if err == nil { + existingTags = objInfo.UserTags + } + if s3Error := authorizeRequestWithExistingTags(ctx, r, policy.PutObjectTaggingAction, existingTags); s3Error != ErrNone { + writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Error), r.URL) + return + } if err != nil { // if object is not found locally, but exists on peer site - proxy // the tagging request to peer site. The response to client will @@ -3314,7 +3313,6 @@ func (api objectAPIHandlers) PutObjectTaggingHandler(w http.ResponseWriter, r *h writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL) return } - tagsStr := tags.String() dsc := mustReplicate(ctx, bucket, object, getMustReplicateOptions(objInfo.UserDefined, tagsStr, objInfo.ReplicationStatus, replication.MetadataReplicationType, opts)) if dsc.ReplicateAny() { @@ -3413,13 +3411,8 @@ func (api objectAPIHandlers) DeleteObjectTaggingHandler(w http.ResponseWriter, r return } - if userTags := oi.UserTags; userTags != "" { - // Set this such that authorization policies can be applied on the object tags. - r.Header.Set(xhttp.AmzObjectTagging, oi.UserTags) - } - // Allow deleteObjectTagging if policy action is set - if s3Error := checkRequestAuthType(ctx, r, policy.DeleteObjectTaggingAction, bucket, object); s3Error != ErrNone { + if s3Error := checkRequestAuthTypeWithExistingTags(ctx, r, policy.DeleteObjectTaggingAction, bucket, object, oi.UserTags); s3Error != ErrNone { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Error), r.URL) return } diff --git a/cmd/object-multipart-handlers.go b/cmd/object-multipart-handlers.go index 048bf7076..fcc6d5b64 100644 --- a/cmd/object-multipart-handlers.go +++ b/cmd/object-multipart-handlers.go @@ -80,7 +80,12 @@ func (api objectAPIHandlers) NewMultipartUploadHandler(w http.ResponseWriter, r return } - if s3Error := checkRequestAuthType(ctx, r, policy.PutObjectAction, bucket, object); s3Error != ErrNone { + requestTags, hasRequestTags := getRequestHeaderOrQueryValue(r, xhttp.AmzObjectTagging) + var requestTagsPtr *string + if hasRequestTags { + requestTagsPtr = &requestTags + } + if s3Error := checkRequestAuthTypeWithRequestTags(ctx, r, policy.PutObjectAction, bucket, object, requestTagsPtr); s3Error != ErrNone { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(s3Error), r.URL) return } @@ -91,7 +96,8 @@ func (api objectAPIHandlers) NewMultipartUploadHandler(w http.ResponseWriter, r AutoEncrypt: globalAutoEncryption, }) - // Validate storage class metadata if present + // Validate the storage class header if present. Query values retain the + // existing compatibility path, including its historical validation behavior. if sc := r.Header.Get(xhttp.AmzStorageClass); sc != "" { if !storageclass.IsValid(sc) { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrInvalidStorageClass), r.URL) @@ -148,13 +154,11 @@ func (api objectAPIHandlers) NewMultipartUploadHandler(w http.ResponseWriter, r return } - if objTags := r.Header.Get(xhttp.AmzObjectTagging); objTags != "" { + if objTags := metadata[xhttp.AmzObjectTagging]; objTags != "" { if _, err := tags.ParseObjectTags(objTags); err != nil { writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL) return } - - metadata[xhttp.AmzObjectTagging] = objTags } if r.Header.Get(xhttp.AmzBucketReplicationStatus) == replication.Replica.String() { if s3Err := isPutActionAllowed(ctx, getRequestAuthType(r), bucket, object, r, policy.ReplicateObjectAction); s3Err != ErrNone { @@ -968,7 +972,7 @@ func (api objectAPIHandlers) CompleteMultipartUploadHandler(w http.ResponseWrite // The parts list must be strictly increasing by part number. Gaps are // allowed, repeats are not - sort.SliceIsSorted() with a '<' predicate - // considers equal neighbours sorted, so it is checked explicitly here, + // considers equal neighbors sorted, so it is checked explicitly here, // before anything is assembled into the target object. for i := 1; i < len(complMultipartUpload.Parts); i++ { if complMultipartUpload.Parts[i-1].PartNumber >= complMultipartUpload.Parts[i].PartNumber { diff --git a/cmd/object-multipart-handlers_test.go b/cmd/object-multipart-handlers_test.go index 79189b92e..87bd2ef14 100644 --- a/cmd/object-multipart-handlers_test.go +++ b/cmd/object-multipart-handlers_test.go @@ -68,7 +68,7 @@ func testAPICompleteMultipartHandlerPartOrder(obj ObjectLayer, instanceType, buc }{ // Defect reproduction. A duplicated part number is the case that // sort.SliceIsSorted() used to accept, because its '<' predicate treats - // equal neighbours as sorted. Each of these assembled the same part into + // equal neighbors as sorted. Each of these assembled the same part into // the object more than once, inflating it past what was uploaded. { name: "duplicate-part", diff --git a/cmd/sts-handlers_test.go b/cmd/sts-handlers_test.go index f4aa65981..24882745a 100644 --- a/cmd/sts-handlers_test.go +++ b/cmd/sts-handlers_test.go @@ -41,6 +41,8 @@ import ( "github.com/minio/minio-go/v7" cr "github.com/minio/minio-go/v7/pkg/credentials" "github.com/minio/minio-go/v7/pkg/set" + "github.com/minio/minio-go/v7/pkg/tags" + xhttp "github.com/minio/minio/internal/http" "github.com/minio/pkg/v3/ldap" ) @@ -417,6 +419,15 @@ func (s *TestSuiteIAM) TestSTSWithTags(c *check) { "Resource": "arn:aws:s3:::%s/*", "Condition": { "StringEquals": {"s3:ExistingObjectTag/security": "public" } } }, + { + "Effect": "Allow", + "Action": "s3:PutObjectTagging", + "Resource": "arn:aws:s3:::%s/*", + "Condition": { "StringEquals": { + "s3:ExistingObjectTag/virus": "true", + "s3:RequestObjectTag/security": "public" + } } + }, { "Effect": "Allow", "Action": "s3:DeleteObject", @@ -431,6 +442,9 @@ func (s *TestSuiteIAM) TestSTSWithTags(c *check) { "arn:aws:s3:::%s/*" ], "Condition": { + "StringEquals": { + "s3:RequestObjectTag/security": "public" + }, "ForAllValues:StringLike": { "s3:RequestObjectTagKeys": [ "security", @@ -440,7 +454,7 @@ func (s *TestSuiteIAM) TestSTSWithTags(c *check) { } } ] -}`, bucket, bucket, bucket, bucket) +}`, bucket, bucket, bucket, bucket, bucket) err = s.adm.AddCannedPolicy(ctx, policy, policyBytes) if err != nil { c.Fatalf("policy add error: %v", err) @@ -462,8 +476,108 @@ func (s *TestSuiteIAM) TestSTSWithTags(c *check) { // confirm that the user is able to access the bucket uClient := s.getUserClient(c, accessKey, secretKey, "") + queryObject := object + "-query-tags" + queryTags := "security=public&virus=true" + // Query storage-class values were historically accepted without the strict + // header validation. Pin that compatibility while proving they are consumed. + presignedPut, err := uClient.Presign(ctx, http.MethodPut, bucket, queryObject, time.Minute, url.Values{ + "x-amz-storage-class": {"CUSTOM_COMPAT"}, + "x-amz-tagging": {queryTags}, + }) + if err != nil { + c.Fatalf("unable to presign query-tagged upload: %v", err) + } + putReq, err := http.NewRequestWithContext(ctx, http.MethodPut, presignedPut.String(), bytes.NewReader([]byte("stuff"))) + if err != nil { + c.Fatalf("unable to build query-tagged upload: %v", err) + } + putResp, err := s.TestSuiteCommon.client.Do(putReq) + if err != nil { + c.Fatalf("query-tagged upload failed: %v", err) + } + putBody, readErr := io.ReadAll(putResp.Body) + putResp.Body.Close() + if readErr != nil { + c.Fatalf("unable to read query-tagged upload response: %v", readErr) + } + if putResp.StatusCode != http.StatusOK { + c.Fatalf("query-tagged upload returned %s: %s", putResp.Status, putBody) + } + storedTags, err := s.client.GetObjectTagging(ctx, bucket, queryObject, minio.GetObjectTaggingOptions{}) + if err != nil { + c.Fatalf("unable to read persisted query tags: %v", err) + } + if got := storedTags.ToMap(); got["security"] != "public" || got["virus"] != "true" { + c.Fatalf("query tags were not persisted: %v", got) + } + queryObjectInfo, err := s.testServer.Obj.GetObjectInfo(ctx, bucket, queryObject, ObjectOptions{}) + if err != nil { + c.Fatalf("unable to inspect query-tagged object: %v", err) + } + if queryObjectInfo.StorageClass != "CUSTOM_COMPAT" { + c.Fatalf("query storage class was not persisted: %q", queryObjectInfo.StorageClass) + } + c.mustGetObject(ctx, uClient, bucket, queryObject) + + multipartObject := object + "-multipart-query-tags" + presignedMultipart, err := uClient.Presign(ctx, http.MethodPost, bucket, multipartObject, time.Minute, url.Values{ + "uploads": {""}, + "x-amz-tagging": {queryTags}, + }) + if err != nil { + c.Fatalf("unable to presign query-tagged multipart upload: %v", err) + } + multipartReq, err := http.NewRequestWithContext(ctx, http.MethodPost, presignedMultipart.String(), nil) + if err != nil { + c.Fatalf("unable to build query-tagged multipart upload: %v", err) + } + multipartResp, err := s.TestSuiteCommon.client.Do(multipartReq) + if err != nil { + c.Fatalf("query-tagged multipart upload failed: %v", err) + } + multipartBody, readErr := io.ReadAll(multipartResp.Body) + multipartResp.Body.Close() + if readErr != nil { + c.Fatalf("unable to read query-tagged multipart response: %v", readErr) + } + if multipartResp.StatusCode != http.StatusOK { + c.Fatalf("query-tagged multipart upload returned %s: %s", multipartResp.Status, multipartBody) + } + var multipartResult InitiateMultipartUploadResponse + if err = xml.Unmarshal(multipartBody, &multipartResult); err != nil || multipartResult.UploadID == "" { + c.Fatalf("invalid query-tagged multipart response: uploadID=%q err=%v", multipartResult.UploadID, err) + } + if err = (minio.Core{Client: s.client}).AbortMultipartUpload(ctx, bucket, multipartObject, multipartResult.UploadID); err != nil { + c.Fatalf("unable to clean up query-tagged multipart upload: %v", err) + } + c.mustPutObjectWithTags(ctx, uClient, bucket, object) c.mustGetObject(ctx, uClient, bucket, object) + objectInfo, err := s.client.StatObject(ctx, bucket, object, minio.StatObjectOptions{}) + if err != nil { + c.Fatalf("unable to stat object for conditional GET: %v", err) + } + presignedGet, err := uClient.PresignedGetObject(ctx, bucket, object, time.Minute, nil) + if err != nil { + c.Fatalf("unable to presign conditional GET: %v", err) + } + getReq, err := http.NewRequestWithContext(ctx, http.MethodGet, presignedGet.String(), nil) + if err != nil { + c.Fatalf("unable to build conditional GET: %v", err) + } + getReq.Header.Set(xhttp.IfNoneMatch, `"`+objectInfo.ETag+`"`) + getResp := httptest.NewRecorder() + s.testServer.Server.Config.Handler.ServeHTTP(getResp, getReq) + if getResp.Code != http.StatusNotModified || getResp.Body.Len() != 0 { + c.Fatalf("conditional GET returned status %d with body %q", getResp.Code, getResp.Body.String()) + } + replacementTags, err := tags.NewTags(map[string]string{"security": "public", "reviewed": "yes"}, true) + if err != nil { + c.Fatalf("unable to build replacement tags: %v", err) + } + if err = uClient.PutObjectTagging(ctx, bucket, object, replacementTags, minio.PutObjectTaggingOptions{}); err != nil { + c.Fatalf("user is unable to replace object tags: %v", err) + } assumeRole := cr.STSAssumeRole{ Client: s.TestSuiteCommon.client, @@ -502,6 +616,9 @@ func (s *TestSuiteIAM) TestSTSWithTags(c *check) { if err = minioClient.RemoveObject(ctx, bucket, object, minio.RemoveObjectOptions{}); err != nil { c.Fatalf("user is unable to delete the object: %v", err) } + if err = minioClient.RemoveObject(ctx, bucket, queryObject, minio.RemoveObjectOptions{}); err != nil { + c.Fatalf("user is unable to delete the query-tagged object: %v", err) + } } func (s *TestSuiteIAM) TestSTS(c *check) { @@ -1948,9 +2065,7 @@ func TestSTSLDAPLoginRateLimiterCleanup(t *testing.T) { } func TestWriteSTSThrottledResponse(t *testing.T) { - req := httptest.NewRequest(http.MethodPost, "http://minio.test", strings.NewReader("")) rr := httptest.NewRecorder() - req = req.WithContext(newContext(req, rr, "test-throttle")) writeSTSThrottledResponse(rr) diff --git a/internal/handlers/proxy.go b/internal/handlers/proxy.go index f095b1fdb..886101a62 100644 --- a/internal/handlers/proxy.go +++ b/internal/handlers/proxy.go @@ -85,6 +85,17 @@ func GetSourceScheme(r *http.Request) string { return scheme } +// SECURITY NOTE: these headers are trusted from any peer. There is no +// trusted-proxy boundary, X-Forwarded-For is honoured by default, and X-Real-IP +// and Forwarded are not gated at all, so any client that can reach the server +// directly can set the address the rest of the process believes it came from. +// That includes aws:SourceIp, which means an IpAddress policy condition is not +// enforceable on a directly reachable deployment - put MinIO behind a proxy +// that overwrites these headers, or set _MINIO_API_XFF_HEADER=off and keep the +// other two out at the edge. Adding a trusted-proxy allowlist here would change +// what every deployment behind a load balancer resolves to, so it is recorded +// rather than changed. +// // GetSourceIPFromHeaders retrieves the IP from the X-Forwarded-For, X-Real-IP // and RFC7239 Forwarded headers (in that order) func GetSourceIPFromHeaders(r *http.Request) string {