fix(s3): omit NotResource:null from bucket policy JSON response (#8658)
* fix(s3): omit NotResource:null from bucket policy JSON response (#8657) Change NotResource from value type to pointer (*StringOrStringSlice) so that omitempty properly omits it when unset, matching the existing Principal field pattern. This prevents IaC tools (Terraform, Ansible) from detecting false configuration drift. Add bucket policy round-trip idempotency integration tests. * simplify JSON comparison in bucket policy idempotency test Use require.JSONEq directly on the raw JSON strings instead of round-tripping through unmarshal/marshal, since JSONEq already handles normalization internally. * fix bucket policy test cases that locked out the admin user The Deny+NotResource test cases used Action:"s3:*" which denied the admin's own GetBucketPolicy call. Scope deny to s3:GetObject only, and add an Allow+NotResource variant instead. * fix(s3): also make Resource a pointer to fix empty string in JSON Apply the same omitempty pointer fix to the Resource field, which was emitting "Resource":"" when only NotResource was set. Add NewStringOrStringSlicePtr helper, make Strings() nil-safe, and handle *StringOrStringSlice in normalizeToStringSliceWithError. * improve bucket policy integration tests per review feedback - Replace time.Sleep with waitForClusterReady using ListBuckets - Use structural hasKey check instead of brittle substring NotContains - Assert specific NoSuchBucketPolicy error code after delete - Handle single-statement policies in hasKey helper
This commit is contained in:
320
test/s3/policy/bucket_policy_idempotency_test.go
Normal file
320
test/s3/policy/bucket_policy_idempotency_test.go
Normal file
@@ -0,0 +1,320 @@
|
|||||||
|
package policy
|
||||||
|
|
||||||
|
// Tests for S3 bucket policy JSON round-trip idempotency.
|
||||||
|
// These validate behavior that IaC tools (Terraform, Ansible) depend on:
|
||||||
|
// PUT a policy, GET it back, and verify the JSON matches exactly.
|
||||||
|
// See https://github.com/seaweedfs/seaweedfs/issues/8657
|
||||||
|
|
||||||
|
import (
|
||||||
|
"encoding/json"
|
||||||
|
"testing"
|
||||||
|
|
||||||
|
"github.com/aws/aws-sdk-go/aws"
|
||||||
|
"github.com/aws/aws-sdk-go/aws/awserr"
|
||||||
|
"github.com/aws/aws-sdk-go/aws/credentials"
|
||||||
|
"github.com/aws/aws-sdk-go/aws/session"
|
||||||
|
"github.com/aws/aws-sdk-go/service/s3"
|
||||||
|
"github.com/stretchr/testify/require"
|
||||||
|
)
|
||||||
|
|
||||||
|
func waitForClusterReady(t *testing.T, s3Client *s3.S3) {
|
||||||
|
t.Helper()
|
||||||
|
// ListBuckets is a lightweight call that confirms the S3 API is serving.
|
||||||
|
_, err := s3Client.ListBuckets(&s3.ListBucketsInput{})
|
||||||
|
require.NoError(t, err, "S3 endpoint not ready")
|
||||||
|
}
|
||||||
|
|
||||||
|
func newS3ClientForCluster(t *testing.T, cluster *TestCluster) *s3.S3 {
|
||||||
|
t.Helper()
|
||||||
|
sess, err := session.NewSession(&aws.Config{
|
||||||
|
Region: aws.String("us-east-1"),
|
||||||
|
Endpoint: aws.String(cluster.s3Endpoint),
|
||||||
|
DisableSSL: aws.Bool(true),
|
||||||
|
S3ForcePathStyle: aws.Bool(true),
|
||||||
|
Credentials: credentials.NewStaticCredentials("admin", "admin", ""),
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
return s3.New(sess)
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestBucketPolicyRoundTrip verifies that GetBucketPolicy returns exactly what
|
||||||
|
// was submitted via PutBucketPolicy, without adding spurious fields like
|
||||||
|
// "NotResource": null. This is the core issue from #8657.
|
||||||
|
func TestBucketPolicyRoundTrip(t *testing.T) {
|
||||||
|
if testing.Short() {
|
||||||
|
t.Skip("Skipping integration test in short mode")
|
||||||
|
}
|
||||||
|
|
||||||
|
cluster, err := startMiniCluster(t)
|
||||||
|
require.NoError(t, err)
|
||||||
|
defer cluster.Stop()
|
||||||
|
|
||||||
|
s3Client := newS3ClientForCluster(t, cluster)
|
||||||
|
waitForClusterReady(t, s3Client)
|
||||||
|
bucket := uniqueName("policy-rt")
|
||||||
|
|
||||||
|
_, err = s3Client.CreateBucket(&s3.CreateBucketInput{Bucket: aws.String(bucket)})
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
policy map[string]interface{}
|
||||||
|
}{
|
||||||
|
{
|
||||||
|
name: "simple allow without NotResource",
|
||||||
|
policy: map[string]interface{}{
|
||||||
|
"Version": "2012-10-17",
|
||||||
|
"Statement": []interface{}{
|
||||||
|
map[string]interface{}{
|
||||||
|
"Sid": "AllowPublicRead",
|
||||||
|
"Effect": "Allow",
|
||||||
|
"Principal": "*",
|
||||||
|
"Action": "s3:GetObject",
|
||||||
|
"Resource": "arn:aws:s3:::" + bucket + "/*",
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "multiple actions without NotResource",
|
||||||
|
policy: map[string]interface{}{
|
||||||
|
"Version": "2012-10-17",
|
||||||
|
"Statement": []interface{}{
|
||||||
|
map[string]interface{}{
|
||||||
|
"Sid": "ReadWrite",
|
||||||
|
"Effect": "Allow",
|
||||||
|
"Principal": "*",
|
||||||
|
"Action": []interface{}{"s3:GetObject", "s3:PutObject"},
|
||||||
|
"Resource": "arn:aws:s3:::" + bucket + "/*",
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "allow with NotResource",
|
||||||
|
policy: map[string]interface{}{
|
||||||
|
"Version": "2012-10-17",
|
||||||
|
"Statement": []interface{}{
|
||||||
|
map[string]interface{}{
|
||||||
|
"Sid": "AllowOutsidePublic",
|
||||||
|
"Effect": "Allow",
|
||||||
|
"Principal": "*",
|
||||||
|
"Action": "s3:GetObject",
|
||||||
|
"NotResource": "arn:aws:s3:::" + bucket + "/private/*",
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "multiple statements with NotResource",
|
||||||
|
policy: map[string]interface{}{
|
||||||
|
"Version": "2012-10-17",
|
||||||
|
"Statement": []interface{}{
|
||||||
|
map[string]interface{}{
|
||||||
|
"Sid": "AllowRead",
|
||||||
|
"Effect": "Allow",
|
||||||
|
"Principal": "*",
|
||||||
|
"Action": "s3:GetObject",
|
||||||
|
"Resource": "arn:aws:s3:::" + bucket + "/*",
|
||||||
|
},
|
||||||
|
map[string]interface{}{
|
||||||
|
"Sid": "DenyPrivateObjects",
|
||||||
|
"Effect": "Deny",
|
||||||
|
"Principal": "*",
|
||||||
|
"Action": "s3:GetObject",
|
||||||
|
"NotResource": "arn:aws:s3:::" + bucket + "/public/*",
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tc := range tests {
|
||||||
|
t.Run(tc.name, func(t *testing.T) {
|
||||||
|
policyJSON, err := json.Marshal(tc.policy)
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
_, err = s3Client.PutBucketPolicy(&s3.PutBucketPolicyInput{
|
||||||
|
Bucket: aws.String(bucket),
|
||||||
|
Policy: aws.String(string(policyJSON)),
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
getOut, err := s3Client.GetBucketPolicy(&s3.GetBucketPolicyInput{
|
||||||
|
Bucket: aws.String(bucket),
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
require.NotNil(t, getOut.Policy)
|
||||||
|
|
||||||
|
// The returned policy must not contain fields that were not submitted.
|
||||||
|
// This is the exact issue from #8657: NotResource:null was being added.
|
||||||
|
returnedJSON := *getOut.Policy
|
||||||
|
var returnedPolicy map[string]interface{}
|
||||||
|
require.NoError(t, json.Unmarshal([]byte(returnedJSON), &returnedPolicy))
|
||||||
|
if !hasKey(tc.policy, "NotResource") {
|
||||||
|
require.False(t, hasKey(returnedPolicy, "NotResource"),
|
||||||
|
"returned policy must not contain NotResource when it was not submitted")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Semantic comparison of all submitted fields
|
||||||
|
require.JSONEq(t, string(policyJSON), returnedJSON,
|
||||||
|
"GET should return semantically identical policy to what was PUT")
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestBucketPolicyIdempotentPut verifies that putting the same policy twice
|
||||||
|
// does not change the returned value — the behavior IaC tools rely on.
|
||||||
|
func TestBucketPolicyIdempotentPut(t *testing.T) {
|
||||||
|
if testing.Short() {
|
||||||
|
t.Skip("Skipping integration test in short mode")
|
||||||
|
}
|
||||||
|
|
||||||
|
cluster, err := startMiniCluster(t)
|
||||||
|
require.NoError(t, err)
|
||||||
|
defer cluster.Stop()
|
||||||
|
|
||||||
|
s3Client := newS3ClientForCluster(t, cluster)
|
||||||
|
waitForClusterReady(t, s3Client)
|
||||||
|
bucket := uniqueName("policy-idem")
|
||||||
|
|
||||||
|
_, err = s3Client.CreateBucket(&s3.CreateBucketInput{Bucket: aws.String(bucket)})
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
policyJSON := `{
|
||||||
|
"Version": "2012-10-17",
|
||||||
|
"Statement": [{
|
||||||
|
"Sid": "AllowRead",
|
||||||
|
"Effect": "Allow",
|
||||||
|
"Principal": "*",
|
||||||
|
"Action": "s3:GetObject",
|
||||||
|
"Resource": "arn:aws:s3:::` + bucket + `/*"
|
||||||
|
}]
|
||||||
|
}`
|
||||||
|
|
||||||
|
// PUT, then GET, then PUT the returned value, then GET again.
|
||||||
|
// Both GETs should return the same result.
|
||||||
|
_, err = s3Client.PutBucketPolicy(&s3.PutBucketPolicyInput{
|
||||||
|
Bucket: aws.String(bucket),
|
||||||
|
Policy: aws.String(policyJSON),
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
getOut1, err := s3Client.GetBucketPolicy(&s3.GetBucketPolicyInput{
|
||||||
|
Bucket: aws.String(bucket),
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
// Re-PUT the policy that was returned by GET (what Terraform does on update)
|
||||||
|
_, err = s3Client.PutBucketPolicy(&s3.PutBucketPolicyInput{
|
||||||
|
Bucket: aws.String(bucket),
|
||||||
|
Policy: getOut1.Policy,
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
getOut2, err := s3Client.GetBucketPolicy(&s3.GetBucketPolicyInput{
|
||||||
|
Bucket: aws.String(bucket),
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
require.JSONEq(t, *getOut1.Policy, *getOut2.Policy,
|
||||||
|
"re-PUTting the GET result must produce identical output (idempotency)")
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestBucketPolicyDeleteAndRecreate verifies clean lifecycle.
|
||||||
|
func TestBucketPolicyDeleteAndRecreate(t *testing.T) {
|
||||||
|
if testing.Short() {
|
||||||
|
t.Skip("Skipping integration test in short mode")
|
||||||
|
}
|
||||||
|
|
||||||
|
cluster, err := startMiniCluster(t)
|
||||||
|
require.NoError(t, err)
|
||||||
|
defer cluster.Stop()
|
||||||
|
|
||||||
|
s3Client := newS3ClientForCluster(t, cluster)
|
||||||
|
waitForClusterReady(t, s3Client)
|
||||||
|
bucket := uniqueName("policy-del")
|
||||||
|
|
||||||
|
_, err = s3Client.CreateBucket(&s3.CreateBucketInput{Bucket: aws.String(bucket)})
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
policyJSON := `{
|
||||||
|
"Version": "2012-10-17",
|
||||||
|
"Statement": [{
|
||||||
|
"Effect": "Allow",
|
||||||
|
"Principal": "*",
|
||||||
|
"Action": "s3:GetObject",
|
||||||
|
"Resource": "arn:aws:s3:::` + bucket + `/*"
|
||||||
|
}]
|
||||||
|
}`
|
||||||
|
|
||||||
|
// PUT
|
||||||
|
_, err = s3Client.PutBucketPolicy(&s3.PutBucketPolicyInput{
|
||||||
|
Bucket: aws.String(bucket),
|
||||||
|
Policy: aws.String(policyJSON),
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
// DELETE
|
||||||
|
_, err = s3Client.DeleteBucketPolicy(&s3.DeleteBucketPolicyInput{
|
||||||
|
Bucket: aws.String(bucket),
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
// GET should fail with NoSuchBucketPolicy
|
||||||
|
_, err = s3Client.GetBucketPolicy(&s3.GetBucketPolicyInput{
|
||||||
|
Bucket: aws.String(bucket),
|
||||||
|
})
|
||||||
|
require.Error(t, err)
|
||||||
|
awsErr, ok := err.(awserr.Error)
|
||||||
|
require.True(t, ok, "expected AWS error, got %T: %v", err, err)
|
||||||
|
require.Equal(t, "NoSuchBucketPolicy", awsErr.Code())
|
||||||
|
|
||||||
|
// Re-PUT same policy
|
||||||
|
_, err = s3Client.PutBucketPolicy(&s3.PutBucketPolicyInput{
|
||||||
|
Bucket: aws.String(bucket),
|
||||||
|
Policy: aws.String(policyJSON),
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
// GET should succeed and be clean
|
||||||
|
getOut, err := s3Client.GetBucketPolicy(&s3.GetBucketPolicyInput{
|
||||||
|
Bucket: aws.String(bucket),
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
var recreatedPolicy map[string]interface{}
|
||||||
|
require.NoError(t, json.Unmarshal([]byte(*getOut.Policy), &recreatedPolicy))
|
||||||
|
require.False(t, hasKey(recreatedPolicy, "NotResource"),
|
||||||
|
"recreated policy must not contain spurious NotResource")
|
||||||
|
}
|
||||||
|
|
||||||
|
// hasKey checks whether any Statement in the policy map contains the given key.
|
||||||
|
// Handles both single-statement objects and arrays of statements.
|
||||||
|
func hasKey(policy map[string]interface{}, key string) bool {
|
||||||
|
stmtsRaw, ok := policy["Statement"]
|
||||||
|
if !ok {
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
|
||||||
|
// Single statement object
|
||||||
|
if stmt, ok := stmtsRaw.(map[string]interface{}); ok {
|
||||||
|
_, exists := stmt[key]
|
||||||
|
return exists
|
||||||
|
}
|
||||||
|
|
||||||
|
// Array of statements
|
||||||
|
if stmts, ok := stmtsRaw.([]interface{}); ok {
|
||||||
|
for _, s := range stmts {
|
||||||
|
stmt, ok := s.(map[string]interface{})
|
||||||
|
if !ok {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
if _, exists := stmt[key]; exists {
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
|
||||||
@@ -320,7 +320,7 @@ func testPolicyDocument(action string, resource string) policy_engine.PolicyDocu
|
|||||||
{
|
{
|
||||||
Effect: policy_engine.PolicyEffectAllow,
|
Effect: policy_engine.PolicyEffectAllow,
|
||||||
Action: policy_engine.NewStringOrStringSlice(action),
|
Action: policy_engine.NewStringOrStringSlice(action),
|
||||||
Resource: policy_engine.NewStringOrStringSlice(resource),
|
Resource: policy_engine.NewStringOrStringSlicePtr(resource),
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -53,7 +53,7 @@ func testPolicyOperations(t *testing.T, ctx context.Context, credentialManager *
|
|||||||
{
|
{
|
||||||
Effect: policy_engine.PolicyEffectAllow,
|
Effect: policy_engine.PolicyEffectAllow,
|
||||||
Action: policy_engine.NewStringOrStringSlice("s3:GetObject"),
|
Action: policy_engine.NewStringOrStringSlice("s3:GetObject"),
|
||||||
Resource: policy_engine.NewStringOrStringSlice("arn:aws:s3:::test-bucket/*"),
|
Resource: policy_engine.NewStringOrStringSlicePtr("arn:aws:s3:::test-bucket/*"),
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
@@ -91,7 +91,7 @@ func testPolicyOperations(t *testing.T, ctx context.Context, credentialManager *
|
|||||||
{
|
{
|
||||||
Effect: policy_engine.PolicyEffectAllow,
|
Effect: policy_engine.PolicyEffectAllow,
|
||||||
Action: policy_engine.NewStringOrStringSlice("s3:GetObject", "s3:PutObject"),
|
Action: policy_engine.NewStringOrStringSlice("s3:GetObject", "s3:PutObject"),
|
||||||
Resource: policy_engine.NewStringOrStringSlice("arn:aws:s3:::test-bucket/*"),
|
Resource: policy_engine.NewStringOrStringSlicePtr("arn:aws:s3:::test-bucket/*"),
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -459,7 +459,7 @@ func (iama *IamApiServer) GetUserPolicy(s3cfg *iam_pb.S3ApiConfiguration, values
|
|||||||
for i, statement := range policyDocument.Statement {
|
for i, statement := range policyDocument.Statement {
|
||||||
// Use order-independent comparison to avoid duplicates from different action orderings
|
// Use order-independent comparison to avoid duplicates from different action orderings
|
||||||
if stringSlicesEqual(statement.Action.Strings(), actions) {
|
if stringSlicesEqual(statement.Action.Strings(), actions) {
|
||||||
policyDocument.Statement[i].Resource = policy_engine.NewStringOrStringSlice(append(
|
policyDocument.Statement[i].Resource = policy_engine.NewStringOrStringSlicePtr(append(
|
||||||
policyDocument.Statement[i].Resource.Strings(), resource)...)
|
policyDocument.Statement[i].Resource.Strings(), resource)...)
|
||||||
isEqAction = true
|
isEqAction = true
|
||||||
break
|
break
|
||||||
@@ -471,7 +471,7 @@ func (iama *IamApiServer) GetUserPolicy(s3cfg *iam_pb.S3ApiConfiguration, values
|
|||||||
policyDocumentStatement := policy_engine.PolicyStatement{
|
policyDocumentStatement := policy_engine.PolicyStatement{
|
||||||
Effect: policy_engine.PolicyEffectAllow,
|
Effect: policy_engine.PolicyEffectAllow,
|
||||||
Action: policy_engine.NewStringOrStringSlice(actions...),
|
Action: policy_engine.NewStringOrStringSlice(actions...),
|
||||||
Resource: policy_engine.NewStringOrStringSlice(resource),
|
Resource: policy_engine.NewStringOrStringSlicePtr(resource),
|
||||||
}
|
}
|
||||||
policyDocument.Statement = append(policyDocument.Statement, policyDocumentStatement)
|
policyDocument.Statement = append(policyDocument.Statement, policyDocumentStatement)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -46,7 +46,7 @@ func TestGetActionsUserPath(t *testing.T) {
|
|||||||
{
|
{
|
||||||
Effect: policy_engine.PolicyEffectAllow,
|
Effect: policy_engine.PolicyEffectAllow,
|
||||||
Action: policy_engine.NewStringOrStringSlice("s3:Put*", "s3:PutBucketAcl", "s3:Get*", "s3:GetBucketAcl", "s3:List*", "s3:Tagging*", "s3:DeleteBucket*"),
|
Action: policy_engine.NewStringOrStringSlice("s3:Put*", "s3:PutBucketAcl", "s3:Get*", "s3:GetBucketAcl", "s3:List*", "s3:Tagging*", "s3:DeleteBucket*"),
|
||||||
Resource: policy_engine.NewStringOrStringSlice("arn:aws:s3:::shared/user-Alice/*"),
|
Resource: policy_engine.NewStringOrStringSlicePtr("arn:aws:s3:::shared/user-Alice/*"),
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
@@ -73,7 +73,7 @@ func TestGetActionsWildcardPath(t *testing.T) {
|
|||||||
{
|
{
|
||||||
Effect: policy_engine.PolicyEffectAllow,
|
Effect: policy_engine.PolicyEffectAllow,
|
||||||
Action: policy_engine.NewStringOrStringSlice("s3:Get*", "s3:PutBucketAcl"),
|
Action: policy_engine.NewStringOrStringSlice("s3:Get*", "s3:PutBucketAcl"),
|
||||||
Resource: policy_engine.NewStringOrStringSlice("arn:aws:s3:::*"),
|
Resource: policy_engine.NewStringOrStringSlicePtr("arn:aws:s3:::*"),
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
@@ -94,7 +94,7 @@ func TestGetActionsInvalidAction(t *testing.T) {
|
|||||||
{
|
{
|
||||||
Effect: policy_engine.PolicyEffectAllow,
|
Effect: policy_engine.PolicyEffectAllow,
|
||||||
Action: policy_engine.NewStringOrStringSlice("s3:InvalidAction"),
|
Action: policy_engine.NewStringOrStringSlice("s3:InvalidAction"),
|
||||||
Resource: policy_engine.NewStringOrStringSlice("arn:aws:s3:::shared/user-Alice/*"),
|
Resource: policy_engine.NewStringOrStringSlicePtr("arn:aws:s3:::shared/user-Alice/*"),
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
@@ -278,7 +278,7 @@ func TestGetPolicy(t *testing.T) {
|
|||||||
{
|
{
|
||||||
Effect: policy_engine.PolicyEffectAllow,
|
Effect: policy_engine.PolicyEffectAllow,
|
||||||
Action: policy_engine.NewStringOrStringSlice("s3:GetObject"),
|
Action: policy_engine.NewStringOrStringSlice("s3:GetObject"),
|
||||||
Resource: policy_engine.NewStringOrStringSlice("arn:aws:s3:::*"),
|
Resource: policy_engine.NewStringOrStringSlicePtr("arn:aws:s3:::*"),
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
@@ -323,7 +323,7 @@ func TestDeletePolicy(t *testing.T) {
|
|||||||
{
|
{
|
||||||
Effect: policy_engine.PolicyEffectAllow,
|
Effect: policy_engine.PolicyEffectAllow,
|
||||||
Action: policy_engine.NewStringOrStringSlice("s3:GetObject"),
|
Action: policy_engine.NewStringOrStringSlice("s3:GetObject"),
|
||||||
Resource: policy_engine.NewStringOrStringSlice("arn:aws:s3:::*"),
|
Resource: policy_engine.NewStringOrStringSlicePtr("arn:aws:s3:::*"),
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
@@ -371,7 +371,7 @@ func TestListPolicies(t *testing.T) {
|
|||||||
{
|
{
|
||||||
Effect: policy_engine.PolicyEffectAllow,
|
Effect: policy_engine.PolicyEffectAllow,
|
||||||
Action: policy_engine.NewStringOrStringSlice("s3:GetObject"),
|
Action: policy_engine.NewStringOrStringSlice("s3:GetObject"),
|
||||||
Resource: policy_engine.NewStringOrStringSlice("arn:aws:s3:::*"),
|
Resource: policy_engine.NewStringOrStringSlicePtr("arn:aws:s3:::*"),
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
@@ -399,7 +399,7 @@ func TestAttachUserPolicy(t *testing.T) {
|
|||||||
{
|
{
|
||||||
Effect: policy_engine.PolicyEffectAllow,
|
Effect: policy_engine.PolicyEffectAllow,
|
||||||
Action: policy_engine.NewStringOrStringSlice("s3:GetObject"),
|
Action: policy_engine.NewStringOrStringSlice("s3:GetObject"),
|
||||||
Resource: policy_engine.NewStringOrStringSlice("arn:aws:s3:::*"),
|
Resource: policy_engine.NewStringOrStringSlicePtr("arn:aws:s3:::*"),
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
@@ -459,7 +459,7 @@ func TestManagedPolicyActionsPreservedAcrossInlineMutations(t *testing.T) {
|
|||||||
{
|
{
|
||||||
Effect: policy_engine.PolicyEffectAllow,
|
Effect: policy_engine.PolicyEffectAllow,
|
||||||
Action: policy_engine.NewStringOrStringSlice("s3:GetObject"),
|
Action: policy_engine.NewStringOrStringSlice("s3:GetObject"),
|
||||||
Resource: policy_engine.NewStringOrStringSlice("arn:aws:s3:::*"),
|
Resource: policy_engine.NewStringOrStringSlicePtr("arn:aws:s3:::*"),
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
@@ -514,7 +514,7 @@ func TestDetachUserPolicy(t *testing.T) {
|
|||||||
{
|
{
|
||||||
Effect: policy_engine.PolicyEffectAllow,
|
Effect: policy_engine.PolicyEffectAllow,
|
||||||
Action: policy_engine.NewStringOrStringSlice("s3:GetObject"),
|
Action: policy_engine.NewStringOrStringSlice("s3:GetObject"),
|
||||||
Resource: policy_engine.NewStringOrStringSlice("arn:aws:s3:::*"),
|
Resource: policy_engine.NewStringOrStringSlicePtr("arn:aws:s3:::*"),
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -619,7 +619,7 @@ func TestLoadS3ApiConfigurationFromCredentialManagerHydratesInlinePolicies(t *te
|
|||||||
{
|
{
|
||||||
Effect: policy_engine.PolicyEffectAllow,
|
Effect: policy_engine.PolicyEffectAllow,
|
||||||
Action: policy_engine.NewStringOrStringSlice("s3:PutObject"),
|
Action: policy_engine.NewStringOrStringSlice("s3:PutObject"),
|
||||||
Resource: policy_engine.NewStringOrStringSlice("arn:aws:s3:::test-bucket/*"),
|
Resource: policy_engine.NewStringOrStringSlicePtr("arn:aws:s3:::test-bucket/*"),
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
@@ -673,7 +673,7 @@ func TestLoadS3ApiConfigurationFromCredentialManagerHydratesInlinePoliciesThroug
|
|||||||
{
|
{
|
||||||
Effect: policy_engine.PolicyEffectAllow,
|
Effect: policy_engine.PolicyEffectAllow,
|
||||||
Action: policy_engine.NewStringOrStringSlice("s3:PutObject"),
|
Action: policy_engine.NewStringOrStringSlice("s3:PutObject"),
|
||||||
Resource: policy_engine.NewStringOrStringSlice("arn:aws:s3:::test-bucket/*"),
|
Resource: policy_engine.NewStringOrStringSlicePtr("arn:aws:s3:::test-bucket/*"),
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -382,7 +382,7 @@ func convertSingleAction(action string) (*PolicyStatement, error) {
|
|||||||
return &PolicyStatement{
|
return &PolicyStatement{
|
||||||
Effect: PolicyEffectAllow,
|
Effect: PolicyEffectAllow,
|
||||||
Action: NewStringOrStringSlice(s3Actions...),
|
Action: NewStringOrStringSlice(s3Actions...),
|
||||||
Resource: NewStringOrStringSlice(resources...),
|
Resource: NewStringOrStringSlicePtr(resources...),
|
||||||
}, nil
|
}, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -615,7 +615,7 @@ func CreatePolicyFromLegacyIdentity(identityName string, actions []string) (*Pol
|
|||||||
Sid: fmt.Sprintf("%s-%s", identityName, strings.ReplaceAll(resourcePattern, "/", "-")),
|
Sid: fmt.Sprintf("%s-%s", identityName, strings.ReplaceAll(resourcePattern, "/", "-")),
|
||||||
Effect: PolicyEffectAllow,
|
Effect: PolicyEffectAllow,
|
||||||
Action: NewStringOrStringSlice(s3Actions...),
|
Action: NewStringOrStringSlice(s3Actions...),
|
||||||
Resource: NewStringOrStringSlice(resources...),
|
Resource: NewStringOrStringSlicePtr(resources...),
|
||||||
}
|
}
|
||||||
|
|
||||||
statements = append(statements, statement)
|
statements = append(statements, statement)
|
||||||
|
|||||||
@@ -82,8 +82,11 @@ func (s StringOrStringSlice) MarshalJSON() ([]byte, error) {
|
|||||||
return json.Marshal(s.values)
|
return json.Marshal(s.values)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Strings returns the slice of strings
|
// Strings returns the slice of strings. Nil-safe for pointer receivers.
|
||||||
func (s StringOrStringSlice) Strings() []string {
|
func (s *StringOrStringSlice) Strings() []string {
|
||||||
|
if s == nil {
|
||||||
|
return nil
|
||||||
|
}
|
||||||
return s.values
|
return s.values
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -92,6 +95,11 @@ func NewStringOrStringSlice(values ...string) StringOrStringSlice {
|
|||||||
return StringOrStringSlice{values: values}
|
return StringOrStringSlice{values: values}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// NewStringOrStringSlicePtr creates a new *StringOrStringSlice from strings
|
||||||
|
func NewStringOrStringSlicePtr(values ...string) *StringOrStringSlice {
|
||||||
|
return &StringOrStringSlice{values: values}
|
||||||
|
}
|
||||||
|
|
||||||
// PolicyConditions represents policy conditions with proper typing
|
// PolicyConditions represents policy conditions with proper typing
|
||||||
type PolicyConditions map[string]map[string]StringOrStringSlice
|
type PolicyConditions map[string]map[string]StringOrStringSlice
|
||||||
|
|
||||||
@@ -138,8 +146,8 @@ type PolicyStatement struct {
|
|||||||
Effect PolicyEffect `json:"Effect"`
|
Effect PolicyEffect `json:"Effect"`
|
||||||
Principal *StringOrStringSlice `json:"Principal,omitempty"`
|
Principal *StringOrStringSlice `json:"Principal,omitempty"`
|
||||||
Action StringOrStringSlice `json:"Action"`
|
Action StringOrStringSlice `json:"Action"`
|
||||||
Resource StringOrStringSlice `json:"Resource,omitempty"`
|
Resource *StringOrStringSlice `json:"Resource,omitempty"`
|
||||||
NotResource StringOrStringSlice `json:"NotResource,omitempty"`
|
NotResource *StringOrStringSlice `json:"NotResource,omitempty"`
|
||||||
Condition PolicyConditions `json:"Condition,omitempty"`
|
Condition PolicyConditions `json:"Condition,omitempty"`
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -296,8 +304,16 @@ func compileStatement(stmt *PolicyStatement) (*CompiledStatement, error) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Deep clone Resource/NotResource into the internal statement as well for completeness
|
// Deep clone Resource/NotResource into the internal statement as well for completeness
|
||||||
compiled.Statement.Resource.values = slices.Clone(stmt.Resource.values)
|
if stmt.Resource != nil {
|
||||||
compiled.Statement.NotResource.values = slices.Clone(stmt.NotResource.values)
|
resourceClone := *stmt.Resource
|
||||||
|
resourceClone.values = slices.Clone(stmt.Resource.values)
|
||||||
|
compiled.Statement.Resource = &resourceClone
|
||||||
|
}
|
||||||
|
if stmt.NotResource != nil {
|
||||||
|
notResourceClone := *stmt.NotResource
|
||||||
|
notResourceClone.values = slices.Clone(stmt.NotResource.values)
|
||||||
|
compiled.Statement.NotResource = ¬ResourceClone
|
||||||
|
}
|
||||||
compiled.Statement.Action.values = slices.Clone(stmt.Action.values)
|
compiled.Statement.Action.values = slices.Clone(stmt.Action.values)
|
||||||
|
|
||||||
// Deep clone Condition map
|
// Deep clone Condition map
|
||||||
@@ -448,6 +464,8 @@ func normalizeToStringSliceWithError(value interface{}) ([]string, error) {
|
|||||||
return result, nil
|
return result, nil
|
||||||
case StringOrStringSlice:
|
case StringOrStringSlice:
|
||||||
return v.Strings(), nil
|
return v.Strings(), nil
|
||||||
|
case *StringOrStringSlice:
|
||||||
|
return v.Strings(), nil
|
||||||
default:
|
default:
|
||||||
return nil, fmt.Errorf("unexpected type for policy value: %T", v)
|
return nil, fmt.Errorf("unexpected type for policy value: %T", v)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -317,9 +317,11 @@ func (s3a *S3ApiServer) validateBucketPolicy(policyDoc *policy_engine.PolicyDocu
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Validate NotResources refer to this bucket
|
// Validate NotResources refer to this bucket
|
||||||
for _, notResource := range statement.NotResource.Strings() {
|
if statement.NotResource != nil {
|
||||||
if !s3a.validateResourceForBucket(notResource, bucket) {
|
for _, notResource := range statement.NotResource.Strings() {
|
||||||
return fmt.Errorf("statement %d: NotResource %s does not match bucket %s", i, notResource, bucket)
|
if !s3a.validateResourceForBucket(notResource, bucket) {
|
||||||
|
return fmt.Errorf("statement %d: NotResource %s does not match bucket %s", i, notResource, bucket)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -898,7 +898,7 @@ func (e *EmbeddedIamApi) GetUserPolicy(s3cfg *iam_pb.S3ApiConfiguration, values
|
|||||||
for i, statement := range policyDocument.Statement {
|
for i, statement := range policyDocument.Statement {
|
||||||
// Use order-independent comparison to avoid duplicates from different action orderings
|
// Use order-independent comparison to avoid duplicates from different action orderings
|
||||||
if iamStringSlicesEqual(statement.Action.Strings(), actions) {
|
if iamStringSlicesEqual(statement.Action.Strings(), actions) {
|
||||||
policyDocument.Statement[i].Resource = policy_engine.NewStringOrStringSlice(append(
|
policyDocument.Statement[i].Resource = policy_engine.NewStringOrStringSlicePtr(append(
|
||||||
policyDocument.Statement[i].Resource.Strings(), resource)...)
|
policyDocument.Statement[i].Resource.Strings(), resource)...)
|
||||||
isEqAction = true
|
isEqAction = true
|
||||||
break
|
break
|
||||||
@@ -910,7 +910,7 @@ func (e *EmbeddedIamApi) GetUserPolicy(s3cfg *iam_pb.S3ApiConfiguration, values
|
|||||||
policyDocumentStatement := policy_engine.PolicyStatement{
|
policyDocumentStatement := policy_engine.PolicyStatement{
|
||||||
Effect: policy_engine.PolicyEffectAllow,
|
Effect: policy_engine.PolicyEffectAllow,
|
||||||
Action: policy_engine.NewStringOrStringSlice(actions...),
|
Action: policy_engine.NewStringOrStringSlice(actions...),
|
||||||
Resource: policy_engine.NewStringOrStringSlice(resource),
|
Resource: policy_engine.NewStringOrStringSlicePtr(resource),
|
||||||
}
|
}
|
||||||
policyDocument.Statement = append(policyDocument.Statement, policyDocumentStatement)
|
policyDocument.Statement = append(policyDocument.Statement, policyDocumentStatement)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -504,7 +504,7 @@ func TestEmbeddedIamAttachUserPolicyRefreshesIAM(t *testing.T) {
|
|||||||
{
|
{
|
||||||
Effect: policy_engine.PolicyEffectAllow,
|
Effect: policy_engine.PolicyEffectAllow,
|
||||||
Action: policy_engine.NewStringOrStringSlice("s3:GetObject"),
|
Action: policy_engine.NewStringOrStringSlice("s3:GetObject"),
|
||||||
Resource: policy_engine.NewStringOrStringSlice("arn:aws:s3:::bucket/*"),
|
Resource: policy_engine.NewStringOrStringSlicePtr("arn:aws:s3:::bucket/*"),
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user