Skip to content

Commit edaee13

Browse files
nowinkeyynowinkeyy
committed
fix: tighten public read bucket policy
1 parent 7431b12 commit edaee13

3 files changed

Lines changed: 133 additions & 7 deletions

File tree

‎controllers/objectstorage/controllers/objectstoragebucket_controller.go‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -295,11 +295,10 @@ func buildPolicy(policy, bucketName string) string {
295295
case PrivateBucketPolicy:
296296
return `{"Version":"2012-10-17","Statement":[]}`
297297
case PublicReadBucketPolicy:
298-
return `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"AWS":["*"]},"Action":["s3:GetBucketLocation","s3:ListBucket"],"Resource":["arn:aws:s3:::` + bucketName + `"]},
299-
{"Effect":"Allow","Principal":{"AWS":["*"]},"Action":["s3:GetObject"],"Resource":["arn:aws:s3:::` + bucketName + `/*"]}]}`
298+
return `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"AWS":["*"]},"Action":["s3:GetObject"],"Resource":["arn:aws:s3:::` + bucketName + `/*"]}]}`
300299
case PublicReadwriteBucketPolicy:
301-
return `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"AWS":["*"]},"Action":["s3:ListBucketMultipartUploads","s3:GetBucketLocation","s3:ListBucket"],"Resource":["arn:aws:s3:::` + bucketName + `"]},
302-
{"Effect":"Allow","Principal":{"AWS":["*"]},"Action":["s3:PutObject","s3:AbortMultipartUpload","s3:DeleteObject","s3:GetObject","s3:ListMultipartUploadParts"],"Resource":["arn:aws:s3:::` + bucketName + `/*"]}]}`
300+
return `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"AWS":["*"]},"Action":["s3:GetBucketLocation","s3:ListBucket","s3:ListBucketMultipartUploads"],"Resource":["arn:aws:s3:::` + bucketName + `"]},
301+
{"Effect":"Allow","Principal":{"AWS":["*"]},"Action":["s3:GetObject","s3:PutObject","s3:DeleteObject","s3:AbortMultipartUpload","s3:ListMultipartUploadParts"],"Resource":["arn:aws:s3:::` + bucketName + `/*"]}]}`
303302
case BucketServiceAccountPolicy:
304303
return `{
305304
"Version": "2012-10-17",
@@ -309,14 +308,22 @@ func buildPolicy(policy, bucketName string) string {
309308
"Action": [
310309
"s3:ListBucket",
311310
"s3:ListBucketMultipartUploads",
312-
"s3:ListMultipartUploadParts",
313311
"s3:GetBucketPolicy",
314312
"s3:GetBucketLocation",
315313
"s3:GetBucketTagging",
316-
"s3:PutBucketTagging",
314+
"s3:PutBucketTagging"
315+
],
316+
"Resource": [
317+
"arn:aws:s3:::` + bucketName + `"
318+
]
319+
},
320+
{
321+
"Effect": "Allow",
322+
"Action": [
317323
"s3:GetObject",
318324
"s3:PutObject",
319-
"s3:DeleteObject"
325+
"s3:DeleteObject",
326+
"s3:ListMultipartUploadParts"
320327
],
321328
"Resource": [
322329
"arn:aws:s3:::` + bucketName + `/*"

‎controllers/objectstorage/controllers/objectstoragebucket_controller_test.go‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
package controllers
2+
3+
import (
4+
"encoding/json"
5+
"testing"
6+
)
7+
8+
type policyDocument struct {
9+
Statement []policyStatement `json:"Statement"`
10+
}
11+
12+
type policyStatement struct {
13+
Action []string `json:"Action"`
14+
Resource []string `json:"Resource"`
15+
}
16+
17+
func TestBuildPolicyPublicReadDoesNotAllowAnonymousList(t *testing.T) {
18+
t.Parallel()
19+
20+
policy := mustParsePolicy(t, buildPolicy(PublicReadBucketPolicy, "test-bucket"))
21+
actions := collectActions(policy)
22+
23+
assertHasAction(t, actions, "s3:GetObject")
24+
assertNoAction(t, actions, "s3:ListBucket")
25+
assertNoAction(t, actions, "s3:DeleteObject")
26+
}
27+
28+
func TestBuildPolicyPublicReadwriteAllowsAnonymousListAndObjectWrites(t *testing.T) {
29+
t.Parallel()
30+
31+
policy := mustParsePolicy(t, buildPolicy(PublicReadwriteBucketPolicy, "test-bucket"))
32+
actions := collectActions(policy)
33+
34+
assertHasAction(t, actions, "s3:GetBucketLocation")
35+
assertHasAction(t, actions, "s3:ListBucket")
36+
assertHasAction(t, actions, "s3:ListBucketMultipartUploads")
37+
assertHasAction(t, actions, "s3:GetObject")
38+
assertHasAction(t, actions, "s3:PutObject")
39+
assertHasAction(t, actions, "s3:DeleteObject")
40+
assertHasAction(t, actions, "s3:AbortMultipartUpload")
41+
assertHasAction(t, actions, "s3:ListMultipartUploadParts")
42+
}
43+
44+
func TestBuildPolicyBucketServiceAccountSeparatesBucketAndObjectResources(t *testing.T) {
45+
t.Parallel()
46+
47+
const bucketName = "test-bucket"
48+
49+
policy := mustParsePolicy(t, buildPolicy(BucketServiceAccountPolicy, bucketName))
50+
if len(policy.Statement) != 2 {
51+
t.Fatalf("expected 2 statements, got %d", len(policy.Statement))
52+
}
53+
54+
bucketStatement := policy.Statement[0]
55+
assertResources(t, bucketStatement.Resource, "arn:aws:s3:::"+bucketName)
56+
assertHasAction(t, actionSet(bucketStatement.Action), "s3:ListBucket")
57+
assertHasAction(t, actionSet(bucketStatement.Action), "s3:GetBucketLocation")
58+
assertNoAction(t, actionSet(bucketStatement.Action), "s3:GetObject")
59+
60+
objectStatement := policy.Statement[1]
61+
assertResources(t, objectStatement.Resource, "arn:aws:s3:::"+bucketName+"/*")
62+
assertHasAction(t, actionSet(objectStatement.Action), "s3:GetObject")
63+
assertHasAction(t, actionSet(objectStatement.Action), "s3:PutObject")
64+
assertHasAction(t, actionSet(objectStatement.Action), "s3:DeleteObject")
65+
assertNoAction(t, actionSet(objectStatement.Action), "s3:ListBucket")
66+
}
67+
68+
func mustParsePolicy(t *testing.T, raw string) policyDocument {
69+
t.Helper()
70+
71+
var policy policyDocument
72+
if err := json.Unmarshal([]byte(raw), &policy); err != nil {
73+
t.Fatalf("policy is invalid JSON: %v", err)
74+
}
75+
return policy
76+
}
77+
78+
func collectActions(policy policyDocument) map[string]struct{} {
79+
actions := map[string]struct{}{}
80+
for _, statement := range policy.Statement {
81+
for _, action := range statement.Action {
82+
actions[action] = struct{}{}
83+
}
84+
}
85+
return actions
86+
}
87+
88+
func actionSet(actions []string) map[string]struct{} {
89+
actionMap := map[string]struct{}{}
90+
for _, action := range actions {
91+
actionMap[action] = struct{}{}
92+
}
93+
return actionMap
94+
}
95+
96+
func assertHasAction(t *testing.T, actions map[string]struct{}, action string) {
97+
t.Helper()
98+
99+
if _, ok := actions[action]; !ok {
100+
t.Fatalf("expected action %q", action)
101+
}
102+
}
103+
104+
func assertNoAction(t *testing.T, actions map[string]struct{}, action string) {
105+
t.Helper()
106+
107+
if _, ok := actions[action]; ok {
108+
t.Fatalf("did not expect action %q", action)
109+
}
110+
}
111+
112+
func assertResources(t *testing.T, resources []string, want string) {
113+
t.Helper()
114+
115+
if len(resources) != 1 || resources[0] != want {
116+
t.Fatalf("expected resources [%q], got %v", want, resources)
117+
}
118+
}

‎controllers/objectstorage/main.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ func main() {
5252
var enableLeaderElection bool
5353
var probeAddr string
5454
flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.")
55+
flag.String("monitor-bind-address", ":9090", "The address the monitor endpoint binds to.")
5556
flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
5657
flag.BoolVar(&enableLeaderElection, "leader-elect", false,
5758
"Enable leader election for controller manager. "+

0 commit comments

Comments
 (0)