Files
seaweedfs/weed/s3api/s3tables/handler_policy_condition_test.go
T
15ff5c113a fix(s3tables): validate policy conditions and fail safe on unusable ones (#12389)
* s3tables: validate policy conditions and fail safe on unusable ones

* s3vectors: reject a condition operator with no keys

The shared evaluator now treats a keyless operator as unusable, so a
stored keyless Allow would never grant while the write succeeded.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3vectors: reject an Allow whose Condition is null

A null Condition decodes as no Condition, so the Allow granted
unconditionally. A null Condition on a Deny only widens the veto and
stays accepted, as in the S3 Tables validator.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3tables: treat a NaN numeric condition value as unreadable

strconv.ParseFloat accepts "NaN", but no comparison with it holds, so a
Deny on NumericLessThan never applied and an Allow on NumericNotEquals
matched every request.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3tables: reject condition keys the evaluator never supplies

A condition on a key such as aws:SourceIp reads no request value, so a
Deny on it never applied and an Allow never granted. Accept only the
keys getConditionContextValues resolves, as the vector validator does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3vectors: require Statement and Effect spelled as written

The evaluator decodes field names regardless of case, so an "effect" or
"statement" key slipped past the null Condition check and the other
raw-key checks while the stored Allow still granted.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3tables: treat an infinite numeric condition value as unreadable

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3tables: a stored Allow with a null Condition never grants

The typed decode reads "Condition": null as no Condition, so an Allow
stored before validation refused it still granted unconditionally, for
table and vector policies alike. Record the literal null while decoding
a Statement and skip such an Allow during evaluation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3tables: validate a null Condition from the decoded Statement

validatePolicyDocument re-parsed the document to find a literal null
Condition; the Statement decode now records it, so the validator reads
the same flag the evaluator does and nullConditionStatements goes away.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3tables: keep Condition objects merged across key spellings

Decoding Condition through a json.RawMessage kept only the last of
"Condition" and "condition", so an empty object under one spelling
erased the other's restriction. Decode the statement as before and
probe for the literal null separately.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3tables: refuse a Condition split across key spellings

Condition objects under differently cased keys decode into one map, and an
operator they share keeps only the last spelling's keys, so a restriction
under the other spelling was dropped. Validation now refuses a statement
whose Condition is under more than one key, and one stored before that
never grants on an Allow and vetoes on a Deny.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3tables: refuse a Condition key repeated under the same spelling

PolicyDocument re-marshaled each statement through interface{}, and
Statement.UnmarshalJSON counted keys from a map, so both kept only the
last of a repeated Condition key. "Condition":null,"Condition":{} or two
Condition objects sharing an operator passed validation with a restriction
dropped. Statements now decode from their original bytes and the keys are
counted while walking the object.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3vectors: refuse a repeated Condition key in a vector bucket policy

The validator read Condition from a map, which keeps only the last of a
repeated key, so it accepted a statement the evaluator never grants on.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3tables: keep the re-marshaled statement decode, count Condition keys apart

Decoding statements from their original bytes let document order pick
between spellings of every field, where the re-marshal sorted the keys
first, so a stored "effect":"Deny","Effect":"Allow" became an Allow.
Statements decode as before again; only the Condition keys are counted
from the original bytes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3tables: test that validation refuses a split Condition on a Deny

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* s3vectors: test that validation refuses a repeated Condition on a Deny

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2026-11-04 08:13:05 -08:00

150 lines
7.4 KiB
Go

package s3tables
import (
"testing"
"github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants"
"github.com/seaweedfs/seaweedfs/weed/s3api/s3tables/s3tablestest"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
const nullConditionDeny = `{"Statement":[{"Effect":"Deny","Principal":"*","Action":"s3tables:*","Resource":"*","Condition":{"StringEquals":{"aws:ResourceTag/env":null}}}]}`
// startConditionManager seeds startPolicyDenyManager's bucket, namespace and
// table with no table policy and bucketPolicy, if any, stored on the bucket
// the way a release without write-time condition checks could have left it.
func startConditionManager(t *testing.T, bucketPolicy string) (*s3tablestest.MemFiler, *Manager) {
t.Helper()
fs, m := startPolicyDenyManager(t)
bucket := fs.Get(TablesPath, policyDenyBucket)
require.NotNil(t, bucket)
delete(bucket.Extended, ExtendedKeyPolicy)
if bucketPolicy != "" {
bucket.Extended[ExtendedKeyPolicy] = []byte(bucketPolicy)
}
table := fs.Get(GetNamespacePath(policyDenyBucket, "ns"), "t")
require.NotNil(t, table)
delete(table.Extended, ExtendedKeyPolicy)
return fs, m
}
func requireS3TablesError(t *testing.T, err error, code string) *S3TablesError {
t.Helper()
var s3err *S3TablesError
require.ErrorAs(t, err, &s3err)
assert.Equal(t, code, s3err.Type, s3err.Message)
return s3err
}
// A condition the evaluator cannot compare is refused at write time, on the
// bucket and on the table, and nothing is stored.
func TestPutPolicyRejectsUnusableCondition(t *testing.T) {
fs, m := startConditionManager(t, "")
bucketARN, ns, name := deniedTableReq(t)
err := runPolicyOp(t, m, fs, "PutTableBucketPolicy", &PutTableBucketPolicyRequest{
TableBucketARN: bucketARN, ResourcePolicy: nullConditionDeny,
}, DefaultAccountID)
s3err := requireS3TablesError(t, err, ErrCodeInvalidRequest)
assert.Contains(t, s3err.Message, `"aws:ResourceTag/env"`)
_, stored := fs.Get(TablesPath, policyDenyBucket).Extended[ExtendedKeyPolicy]
assert.False(t, stored, "the bucket policy must not be written")
err = runPolicyOp(t, m, fs, "PutTablePolicy", &PutTablePolicyRequest{
TableBucketARN: bucketARN, Namespace: []string{ns}, Name: name, ResourcePolicy: nullConditionDeny,
}, DefaultAccountID)
s3err = requireS3TablesError(t, err, ErrCodeInvalidRequest)
assert.Contains(t, s3err.Message, `"aws:ResourceTag/env"`)
_, stored = fs.Get(GetNamespacePath(policyDenyBucket, ns), name).Extended[ExtendedKeyPolicy]
assert.False(t, stored, "the table policy must not be written")
// A string the numeric evaluator would skip is refused like a null one.
unreadable := `{"Statement":[{"Effect":"Deny","Principal":"*","Action":"s3tables:*","Resource":"*","Condition":{"NumericLessThan":{"aws:RequestTag/priority":"low"}}}]}`
err = runPolicyOp(t, m, fs, "PutTableBucketPolicy", &PutTableBucketPolicyRequest{
TableBucketARN: bucketARN, ResourcePolicy: unreadable,
}, DefaultAccountID)
s3err = requireS3TablesError(t, err, ErrCodeInvalidRequest)
assert.Contains(t, s3err.Message, "NumericLessThan")
_, stored = fs.Get(TablesPath, policyDenyBucket).Extended[ExtendedKeyPolicy]
assert.False(t, stored, "the bucket policy must not be written")
valid := `{"Statement":[{"Effect":"Deny","Principal":"*","Action":"s3tables:*","Resource":"*","Condition":{"StringEquals":{"aws:ResourceTag/env":"prod"}}}]}`
require.NoError(t, runPolicyOp(t, m, fs, "PutTableBucketPolicy", &PutTableBucketPolicyRequest{
TableBucketARN: bucketARN, ResourcePolicy: valid,
}, DefaultAccountID))
assert.Equal(t, valid, string(fs.Get(TablesPath, policyDenyBucket).Extended[ExtendedKeyPolicy]))
}
// The typed decode reads a null Condition as none, so an Allow carrying one is
// refused on the bucket and on the table, and nothing is stored.
func TestPutPolicyRejectsNullAllowCondition(t *testing.T) {
fs, m := startConditionManager(t, "")
bucketARN, ns, name := deniedTableReq(t)
allowNull := `{"Statement":[{"Effect":"Allow","Principal":"*","Action":"s3tables:*","Resource":"*","Condition":null}]}`
requireS3TablesError(t, runPolicyOp(t, m, fs, "PutTableBucketPolicy", &PutTableBucketPolicyRequest{
TableBucketARN: bucketARN, ResourcePolicy: allowNull,
}, DefaultAccountID), ErrCodeInvalidRequest)
_, stored := fs.Get(TablesPath, policyDenyBucket).Extended[ExtendedKeyPolicy]
assert.False(t, stored, "the bucket policy must not be written")
requireS3TablesError(t, runPolicyOp(t, m, fs, "PutTablePolicy", &PutTablePolicyRequest{
TableBucketARN: bucketARN, Namespace: []string{ns}, Name: name, ResourcePolicy: allowNull,
}, DefaultAccountID), ErrCodeInvalidRequest)
_, stored = fs.Get(GetNamespacePath(policyDenyBucket, ns), name).Extended[ExtendedKeyPolicy]
assert.False(t, stored, "the table policy must not be written")
}
// A keyless operator stored before write-time checks made its Allow grant
// everyone; it is now unusable, so a non-owner is turned away. GetTableBucket
// hides a bucket the caller may not read, so the refusal is NoSuchBucket,
// while GetTableBucketPolicy answers AccessDenied.
func TestStoredKeylessAllowDoesNotGrant(t *testing.T) {
keylessAllow := `{"Statement":[{"Effect":"Allow","Principal":"*","Action":"s3tables:*","Resource":"*","Condition":{"StringEquals":{}}}]}`
fs, m := startConditionManager(t, keylessAllow)
bucketARN := policyDenyBucketARN(t)
requireS3TablesError(t, runPolicyOp(t, m, fs, "GetTableBucket", &GetTableBucketRequest{
TableBucketARN: bucketARN,
}, "reader"), ErrCodeNoSuchBucket)
requireS3TablesError(t, runPolicyOp(t, m, fs, "GetTableBucketPolicy", &GetTableBucketPolicyRequest{
TableBucketARN: bucketARN,
}, "reader"), ErrCodeAccessDenied)
require.NoError(t, runPolicyOp(t, m, fs, "GetTableBucket", &GetTableBucketRequest{
TableBucketARN: bucketARN,
}, DefaultAccountID), "the owner keeps access")
}
// A bucket policy stored before conditions were validated must not cut the
// connection: the owner gets an authorization error, the admin can still
// remove the policy, and the owner is let back in once it is gone.
func TestStoredUnusableConditionPolicyIsRecoverable(t *testing.T) {
fs, m := startConditionManager(t, nullConditionDeny)
bucketARN, ns, name := deniedTableReq(t)
requireS3TablesError(t, runPolicyOp(t, m, fs, "GetTableBucket", &GetTableBucketRequest{
TableBucketARN: bucketARN,
}, DefaultAccountID), ErrCodeNoSuchBucket)
require.Error(t, runPolicyOp(t, m, fs, "GetTable", &GetTableRequest{
TableBucketARN: bucketARN, Namespace: []string{ns}, Name: name,
}, DefaultAccountID), "the broken bucket Deny must veto table reads")
requireS3TablesError(t, runPolicyOp(t, m, fs, "DeleteTableBucketPolicy", &DeleteTableBucketPolicyRequest{
TableBucketARN: bucketARN,
}, DefaultAccountID), ErrCodeAccessDenied)
assert.Equal(t, nullConditionDeny, string(fs.Get(TablesPath, policyDenyBucket).Extended[ExtendedKeyPolicy]))
require.NoError(t, runPolicyOp(t, m, fs, "DeleteTableBucketPolicy", &DeleteTableBucketPolicyRequest{
TableBucketARN: bucketARN,
}, s3_constants.AccountAdminId), "the admin must be able to remove the broken policy")
_, stored := fs.Get(TablesPath, policyDenyBucket).Extended[ExtendedKeyPolicy]
assert.False(t, stored, "the broken policy must be gone")
require.NoError(t, runPolicyOp(t, m, fs, "GetTableBucket", &GetTableBucketRequest{
TableBucketARN: bucketARN,
}, DefaultAccountID))
require.NoError(t, runPolicyOp(t, m, fs, "GetTable", &GetTableRequest{
TableBucketARN: bucketARN, Namespace: []string{ns}, Name: name,
}, DefaultAccountID))
}