mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-11-05 11:37:44 +01:00
* 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>
150 lines
7.4 KiB
Go
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))
|
|
}
|