s3: gate POST Object acl fields behind PutObjectAcl (#11939)

* s3: gate POST Object acl fields behind PutObjectAcl

The POST Object handler promotes acl and x-amz-grant-* form fields onto
the upload's ACL headers, and resolveFileMode honors the promoted canned
ACL. The PUT path only applies such headers after an s3:PutObjectAcl
grant; mirror that on POST by running the shared per-key authorization
with the WriteAcp action whenever the request carries ACL headers.

* s3: cover the POST Object acl gate

Signed POST requests carrying an acl form field are denied for a writer
without the WriteAcp action and for a writer a bucket policy denies
s3:PutObjectAcl on the target key; the authorization boundary is
exercised directly as well.

* s3: cover x-amz-grant-* POST fields and the no-ACL case

* s3: encode POST policy conditions as JSON in the acl-gate test

weed/s3api/auth_credentials.go

@@ -3183,6 +3183,20 @@ func (iam *IdentityAccessManagement) AuthorizeObjectWrite(r *http.Request, ident
 	return iam.authorizeObjectKeyAction(r, identity, http.MethodPut, s3_constants.ACTION_WRITE, bucket, objectKey, "")
 }
 
+// AuthorizeObjectAclWrite authorizes setting an ACL on one key the request URL
+// does not name: a POST Object upload promotes its acl form field and any
+// x-amz-grant-* fields onto the upload's ACL headers, which the PUT path only
+// applies after an s3:PutObjectAcl grant.
+func (iam *IdentityAccessManagement) AuthorizeObjectAclWrite(r *http.Request, identity *Identity, bucket, objectKey string) s3err.ErrorCode {
+	if !iam.isEnabled() {
+		return s3err.ErrNone
+	}
+	if bucket == "" || objectKey == "" {
+		return s3err.ErrNone
+	}
+	return iam.authorizeObjectKeyAction(r, identity, http.MethodPut, s3_constants.ACTION_WRITE_ACP, bucket, objectKey, "")
+}
+
 // authorizeWithIAM authorizes requests using the IAM integration policy engine
 func (iam *IdentityAccessManagement) authorizeWithIAM(r *http.Request, identity *Identity, action Action, bucket string, object string) s3err.ErrorCode {
 	ctx := r.Context()

weed/s3api/s3api_object_handlers_postpolicy.go

@@ -144,6 +144,14 @@ func (s3a *S3ApiServer) PostPolicyBucketHandler(w http.ResponseWriter, r *http.R
 		s3err.WriteErrorResponse(w, r, refineDenialForBucketName(r, identity, errCode))
 		return
 	}
+	// An acl or x-amz-grant-* form field rides onto the upload as ACL headers,
+	// which the PUT path only applies under s3:PutObjectAcl.
+	if postRequestsAcl(r) {
+		if errCode := s3a.iam.AuthorizeObjectAclWrite(r, identity, bucket, object); errCode != s3err.ErrNone {
+			s3err.WriteErrorResponse(w, r, refineDenialForBucketName(r, identity, errCode))
+			return
+		}
+	}
 
 	versioningState, err := s3a.getVersioningState(bucket)
 	if err != nil {
@@ -275,6 +283,17 @@ func applyPostPolicyFormHeaders(r *http.Request, formValues http.Header) {
 	}
 }
 
+// postRequestsAcl reports whether the request carries ACL headers, whether
+// promoted from the POST form fields or supplied on the request itself.
+func postRequestsAcl(r *http.Request) bool {
+	for _, header := range []string{s3_constants.AmzCannedAcl, s3_constants.AmzAclFullControl, s3_constants.AmzAclRead, s3_constants.AmzAclReadAcp, s3_constants.AmzAclWrite, s3_constants.AmzAclWriteAcp} {
+		if r.Header.Get(header) != "" {
+			return true
+		}
+	}
+	return false
+}
+
 // Extract form fields and file data from a HTTP POST Policy
 func extractPostPolicyFormValues(form *multipart.Form) (filePart io.ReadCloser, fileName, fileContentType string, fileSize int64, formValues http.Header, err error) {
 	// / HTML Form values

weed/s3api/s3api_object_handlers_postpolicy_authz_test.go

@@ -3,10 +3,12 @@ package s3api
 import (
 	"bytes"
 	"encoding/base64"
+	"encoding/json"
 	"fmt"
 	"mime/multipart"
 	"net/http"
 	"net/http/httptest"
+	"sort"
 	"sync"
 	"testing"
 	"time"
@@ -26,6 +28,10 @@ const (
 	postAuthzService   = "s3"
 	postAuthzBucket    = "testbucket"
 	postAuthzPrincipal = "arn:aws:iam::000000000000:user/tester"
+
+	postAclAccessKey = "AKIAACLTESTTEST"
+	postAclSecretKey = "secret-key-for-acl-tests"
+	postAclPrincipal = "arn:aws:iam::000000000000:user/aclwriter"
 )
 
 // newPostAuthzServer builds an S3ApiServer whose non-admin "tester" identity
@@ -45,6 +51,11 @@ func newPostAuthzServer(t *testing.T) *S3ApiServer {
 			Account:     &iam_pb.Account{Id: "000000000000", DisplayName: "tester"},
 			Credentials: []*iam_pb.Credential{{AccessKey: postAuthzAccessKey, SecretKey: postAuthzSecretKey}},
 			Actions:     []string{"Read:" + postAuthzBucket, "Write:" + postAuthzBucket, "List:" + postAuthzBucket},
+		}, {
+			Name:        "aclwriter",
+			Account:     &iam_pb.Account{Id: "000000000000", DisplayName: "aclwriter"},
+			Credentials: []*iam_pb.Credential{{AccessKey: postAclAccessKey, SecretKey: postAclSecretKey}},
+			Actions:     []string{"Read:" + postAuthzBucket, "Write:" + postAuthzBucket, "WriteAcp:" + postAuthzBucket},
 		}},
 	})
 	require.NoError(t, err)
@@ -58,8 +69,14 @@ func newPostAuthzServer(t *testing.T) *S3ApiServer {
 			"Principal": {"AWS": "%s"},
 			"Action": ["s3:PutObject"],
 			"Resource": ["arn:aws:s3:::%s/denied/*"]
+		}, {
+			"Sid": "DenyAclWriterAclPrefix",
+			"Effect": "Deny",
+			"Principal": {"AWS": "%s"},
+			"Action": ["s3:PutObjectAcl"],
+			"Resource": ["arn:aws:s3:::%s/acl-denied/*"]
 		}]
-	}`, postAuthzPrincipal, postAuthzBucket))
+	}`, postAuthzPrincipal, postAuthzBucket, postAclPrincipal, postAuthzBucket))
 	require.NoError(t, err)
 	iam.policyEngine = policyEngine
 
@@ -79,26 +96,42 @@ func newPostAuthzServer(t *testing.T) *S3ApiServer {
 // policy conditions are satisfied by the form, so the handler passes signature
 // and CheckPostPolicy verification and reaches the write-authorization stage.
 func newSignedPostRequest(t *testing.T, key string) *http.Request {
+	return newSignedPostRequestFor(t, key, postAuthzAccessKey, postAuthzSecretKey, nil)
+}
+
+// extraFields are added to both the form and the policy conditions so
+// CheckPostPolicy accepts them.
+func newSignedPostRequestFor(t *testing.T, key, accessKey, secretKey string, extraFields map[string]string) *http.Request {
 	t.Helper()
 
 	now := time.Now().UTC()
 	amzDate := now.Format(iso8601Format)
 	yyyymmddStr := now.Format(yyyymmdd)
-	credential := fmt.Sprintf("%s/%s/%s/%s/aws4_request", postAuthzAccessKey, yyyymmddStr, postAuthzRegion, postAuthzService)
+	credential := fmt.Sprintf("%s/%s/%s/%s/aws4_request", accessKey, yyyymmddStr, postAuthzRegion, postAuthzService)
 	expiration := now.Add(1 * time.Hour).Format("2006-01-02T15:04:05.000Z")
 
-	policyJSON := fmt.Sprintf(
-		`{"expiration":"%s","conditions":[`+
-			`["eq","$bucket","%s"],`+
-			`["eq","$key","%s"],`+
-			`["eq","$x-amz-credential","%s"],`+
-			`["eq","$x-amz-algorithm","AWS4-HMAC-SHA256"],`+
-			`["eq","$x-amz-date","%s"]`+
-			`]}`,
-		expiration, postAuthzBucket, key, credential, amzDate,
-	)
-	encodedPolicy := base64.StdEncoding.EncodeToString([]byte(policyJSON))
-	signature := getSignature(getSigningKey(postAuthzSecretKey, yyyymmddStr, postAuthzRegion, postAuthzService), encodedPolicy)
+	extraKeys := make([]string, 0, len(extraFields))
+	for k := range extraFields {
+		extraKeys = append(extraKeys, k)
+	}
+	sort.Strings(extraKeys)
+	conditions := []any{
+		[]any{"eq", "$bucket", postAuthzBucket},
+		[]any{"eq", "$key", key},
+		[]any{"eq", "$x-amz-credential", credential},
+		[]any{"eq", "$x-amz-algorithm", "AWS4-HMAC-SHA256"},
+		[]any{"eq", "$x-amz-date", amzDate},
+	}
+	for _, k := range extraKeys {
+		conditions = append(conditions, []any{"eq", "$" + k, extraFields[k]})
+	}
+	policyJSON, err := json.Marshal(map[string]any{
+		"expiration": expiration,
+		"conditions": conditions,
+	})
+	require.NoError(t, err)
+	encodedPolicy := base64.StdEncoding.EncodeToString(policyJSON)
+	signature := getSignature(getSigningKey(secretKey, yyyymmddStr, postAuthzRegion, postAuthzService), encodedPolicy)
 
 	var buf bytes.Buffer
 	writer := multipart.NewWriter(&buf)
@@ -107,6 +140,9 @@ func newSignedPostRequest(t *testing.T, key string) *http.Request {
 	require.NoError(t, writer.WriteField("x-amz-credential", credential))
 	require.NoError(t, writer.WriteField("x-amz-algorithm", "AWS4-HMAC-SHA256"))
 	require.NoError(t, writer.WriteField("x-amz-date", amzDate))
+	for _, k := range extraKeys {
+		require.NoError(t, writer.WriteField(k, extraFields[k]))
+	}
 	require.NoError(t, writer.WriteField("policy", encodedPolicy))
 	require.NoError(t, writer.WriteField("x-amz-signature", signature))
 	filePart, err := writer.CreateFormFile("file", "payload.txt")
@@ -170,3 +206,71 @@ func TestAuthorizeObjectWrite(t *testing.T) {
 		s3a.iam.AuthorizeObjectWrite(req, admin, postAuthzBucket, "denied/secret.txt"),
 		"an admin is not confined by the bucket policy")
 }
+
+// TestPostPolicyBucketHandlerGatesAclFields drives signed POST uploads carrying
+// an acl form field and asserts the handler rejects them the way the PUT path
+// does: a writer without the WriteAcp action is refused, and so is a writer the
+// bucket policy denies s3:PutObjectAcl on the target key.
+func TestPostPolicyBucketHandlerGatesAclFields(t *testing.T) {
+	s3a := newPostAuthzServer(t)
+
+	rec := httptest.NewRecorder()
+	s3a.PostPolicyBucketHandler(rec, newSignedPostRequestFor(t, "allowed/report.txt",
+		postAuthzAccessKey, postAuthzSecretKey, map[string]string{"acl": "public-read"}))
+	assert.Equal(t, http.StatusForbidden, rec.Code,
+		"POST with an acl field needs WriteAcp like PUT, body: %s", rec.Body.String())
+
+	rec = httptest.NewRecorder()
+	s3a.PostPolicyBucketHandler(rec, newSignedPostRequestFor(t, "acl-denied/secret.txt",
+		postAclAccessKey, postAclSecretKey, map[string]string{"acl": "public-read"}))
+	assert.Equal(t, http.StatusForbidden, rec.Code,
+		"POST with an acl field must honor the bucket-policy PutObjectAcl deny, body: %s", rec.Body.String())
+
+	for _, field := range []string{"x-amz-grant-read", "x-amz-grant-write", "x-amz-grant-read-acp", "x-amz-grant-write-acp", "x-amz-grant-full-control"} {
+		rec = httptest.NewRecorder()
+		s3a.PostPolicyBucketHandler(rec, newSignedPostRequestFor(t, "allowed/report.txt",
+			postAuthzAccessKey, postAuthzSecretKey,
+			map[string]string{field: `id="0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"`}))
+		assert.Equal(t, http.StatusForbidden, rec.Code,
+			"POST with an %s field needs WriteAcp like PUT, body: %s", field, rec.Body.String())
+	}
+}
+
+func TestPostRequestsAcl(t *testing.T) {
+	for _, header := range []string{
+		s3_constants.AmzCannedAcl, s3_constants.AmzAclFullControl, s3_constants.AmzAclRead,
+		s3_constants.AmzAclReadAcp, s3_constants.AmzAclWrite, s3_constants.AmzAclWriteAcp,
+	} {
+		req := httptest.NewRequest(http.MethodPost, "/"+postAuthzBucket, nil)
+		req.Header.Set(header, "x")
+		assert.True(t, postRequestsAcl(req), "%s must trip the ACL gate", header)
+	}
+	assert.False(t, postRequestsAcl(httptest.NewRequest(http.MethodPost, "/"+postAuthzBucket, nil)))
+}
+
+// TestAuthorizeObjectAclWrite covers the acl authorization decision at the
+// boundary, for the same reason as TestAuthorizeObjectWrite.
+func TestAuthorizeObjectAclWrite(t *testing.T) {
+	s3a := newPostAuthzServer(t)
+	tester, _, found := s3a.iam.lookupByAccessKey(postAuthzAccessKey)
+	require.True(t, found)
+	aclwriter, _, found := s3a.iam.lookupByAccessKey(postAclAccessKey)
+	require.True(t, found)
+
+	req := httptest.NewRequest(http.MethodPost, "/"+postAuthzBucket, nil)
+
+	assert.Equal(t, s3err.ErrAccessDenied,
+		s3a.iam.AuthorizeObjectAclWrite(req, tester, postAuthzBucket, "allowed/report.txt"),
+		"a writer without the WriteAcp action cannot attach an ACL")
+	assert.Equal(t, s3err.ErrNone,
+		s3a.iam.AuthorizeObjectAclWrite(req, aclwriter, postAuthzBucket, "allowed/report.txt"),
+		"the WriteAcp holder clears keys the policy does not deny")
+	assert.Equal(t, s3err.ErrAccessDenied,
+		s3a.iam.AuthorizeObjectAclWrite(req, aclwriter, postAuthzBucket, "acl-denied/secret.txt"),
+		"a bucket-policy PutObjectAcl deny must block the ACL-bearing upload")
+
+	admin := &Identity{Name: "admin", Actions: []Action{s3_constants.ACTION_ADMIN}}
+	assert.Equal(t, s3err.ErrNone,
+		s3a.iam.AuthorizeObjectAclWrite(req, admin, postAuthzBucket, "acl-denied/secret.txt"),
+		"an admin is not confined by the bucket policy")
+}