fix(bucket): Convert bools to string in policy

AWS converts bool values in policy conditions to strings when getting them from the API. Add a new policy parser based on structs that can handle AWS policies correctly, including single items instead of lists. Signed-off-by: Maximilian Blatt (external expert on behalf of DB Netz) <maximilian.blatt-extern@deutschebahn.com>
crossplane-contrib · Jun 13, 2023 · 4d6eac4 · 4d6eac4
1 parent f2c6a7c
commit 4d6eac4
Show file tree

Hide file tree

Showing 9 changed files with 599 additions and 33 deletions.
diff --git a/go.mod b/go.mod
@@ -44,6 +44,7 @@ require (
 	k8s.io/api v0.26.1
 	k8s.io/apimachinery v0.26.1
 	k8s.io/client-go v0.26.1
+	k8s.io/utils v0.0.0-20221128185143-99ec85e7a448
 	sigs.k8s.io/controller-runtime v0.14.1
 	sigs.k8s.io/controller-tools v0.11.1
 	sigs.k8s.io/yaml v1.3.0
@@ -153,7 +154,6 @@ require (
 	k8s.io/component-base v0.26.1 // indirect
 	k8s.io/klog/v2 v2.80.1 // indirect
 	k8s.io/kube-openapi v0.0.0-20221012153701-172d655c2280 // indirect
-	k8s.io/utils v0.0.0-20221128185143-99ec85e7a448 // indirect
 	sigs.k8s.io/json v0.0.0-20220713155537-f223a00ba0e2 // indirect
 	sigs.k8s.io/structured-merge-diff/v4 v4.2.3 // indirect
 )
diff --git a/pkg/controller/s3/bucket/policy.go b/pkg/controller/s3/bucket/policy.go
@@ -27,16 +27,18 @@ import (
 	"github.com/crossplane/crossplane-runtime/pkg/meta"
 
 	"github.com/crossplane-contrib/provider-aws/apis/s3/v1beta1"
-	aws "github.com/crossplane-contrib/provider-aws/pkg/clients"
 	awsclient "github.com/crossplane-contrib/provider-aws/pkg/clients"
 	"github.com/crossplane-contrib/provider-aws/pkg/clients/s3"
+	policyutils "github.com/crossplane-contrib/provider-aws/pkg/utils/policy"
 )
 
 const (
-	policyGetFailed    = "cannot get bucket policy"
-	policyFormatFailed = "cannot format bucket policy"
-	policyPutFailed    = "cannot put bucket policy"
-	policyDeleteFailed = "cannot delete bucket policy"
+	policyGetFailed     = "cannot get bucket policy"
+	policyFormatFailed  = "cannot format bucket policy"
+	policyParseSpec     = "cannot parse spec policy"
+	policyPutFailed     = "cannot put bucket policy"
+	policyDeleteFailed  = "cannot delete bucket policy"
+	policyParseExternal = "cannot parse external policy"
 )
 
 // PolicyClient is the client for API methods and reconciling the PublicAccessBlock
@@ -63,45 +65,35 @@ func (e *PolicyClient) Observe(ctx context.Context, cr *v1beta1.Bucket) (Resourc
 		}
 		return NeedsUpdate, errors.Wrap(err, policyGetFailed)
 	}
-	policy, err := e.formatBucketPolicy(cr)
-	if err != nil {
-		return NeedsUpdate, errors.Wrap(err, policyFormatFailed)
-	}
 
 	// To ensure backwards compatbility with the previous behaviour
 	// (Bucket + BucketPolicy).
 	// Only delete the policy on AWS if the user has specified to do so.
-	if policy == nil && resp.Policy != nil && getBucketPolicyDeletionPolicy(cr) == v1beta1.BucketPolicyDeletionPolicyIfNull {
-		return NeedsDeletion, nil
-	}
-
-	if EqualsJSON(aws.StringValue(policy), aws.StringValue(resp.Policy)) {
+	if cr.Spec.ForProvider.Policy == nil {
+		if resp.Policy != nil && getBucketPolicyDeletionPolicy(cr) == v1beta1.BucketPolicyDeletionPolicyIfNull {
+			return NeedsDeletion, nil
+		}
 		return Updated, nil
 	}
 
-	return NeedsUpdate, nil
-}
-
-// JSONNormalize bring JsonStrings to an []byte
-func JSONNormalize(jStr string) *string {
-	var iface any
-	err := json.Unmarshal([]byte(jStr), &iface)
+	specPolicyRaw, err := e.formatBucketPolicy(cr)
 	if err != nil {
-		return &jStr
+		return NeedsUpdate, errors.Wrap(err, policyFormatFailed)
 	}
-
-	jRaw, err := json.Marshal(iface)
+	specPolicy, err := policyutils.ParsePolicyString(awsclient.StringValue(specPolicyRaw))
 	if err != nil {
-		return &jStr
+		return NeedsUpdate, errors.Wrap(err, policyParseSpec)
+	}
+	curPolicy, err := policyutils.ParsePolicyString(awsclient.StringValue(resp.Policy))
+	if err != nil {
+		return NeedsUpdate, errors.Wrap(err, policyParseExternal)
 	}
-	return aws.String(string(jRaw))
-}
 
-// EqualsJSON whether two JSON structs are equal
-func EqualsJSON(a, b string) bool {
-	pa := JSONNormalize(a)
-	pb := JSONNormalize(b)
-	return cmp.Equal(pa, pb)
+	diff := cmp.Diff(specPolicy, curPolicy)
+	if diff != "" {
+		return NeedsUpdate, nil
+	}
+	return Updated, nil
 }
 
 // formatBucketPolicy parses and formats the bucket.Spec.BucketPolicy struct

diff --git a/pkg/controller/s3/bucket/policy_test.go b/pkg/controller/s3/bucket/policy_test.go
@@ -25,6 +25,7 @@ import (
 	"github.com/aws/smithy-go"
 	"github.com/crossplane/crossplane-runtime/pkg/test"
 	"github.com/google/go-cmp/cmp"
+	"k8s.io/utils/pointer"
 
 	"github.com/crossplane-contrib/provider-aws/apis/s3/common"
 	"github.com/crossplane-contrib/provider-aws/apis/s3/v1beta1"
@@ -75,6 +76,127 @@ func TestPolicyObserve(t *testing.T) {
 		},
 	}
 
+	var testPolicyIssue1771 = &common.BucketPolicyBody{
+		Version: "2012-10-17",
+		Statements: []common.BucketPolicyStatement{
+			{
+				Action: []string{
+					"s3:PutObject",
+				},
+				Condition: []common.Condition{
+					{
+						OperatorKey: "StringNotEquals",
+						Conditions: []common.ConditionPair{
+							{
+								ConditionKey: "s3:x-amz-server-side-encryption",
+								ConditionListValue: []string{
+									"AES256",
+									"aws:kms",
+								},
+							},
+						},
+					},
+				},
+				Effect: "Deny",
+				Principal: &common.BucketPrincipal{
+					AllowAnon: true,
+				},
+				Resource: []string{
+					"arn:aws:s3:::test-bucket-xxxx/*",
+				},
+				SID: awsclient.String("DenyIncorrectEncryptionHeader"),
+			},
+			{
+				Action: []string{
+					"s3:PutObject",
+				},
+				Condition: []common.Condition{
+					{
+						OperatorKey: "Null",
+						Conditions: []common.ConditionPair{
+							{
+								ConditionKey:          "s3:x-amz-server-side-encryption",
+								ConditionBooleanValue: awsclient.Bool(true),
+							},
+						},
+					},
+				},
+				Effect: "Deny",
+				Principal: &common.BucketPrincipal{
+					AllowAnon: true,
+				},
+				Resource: []string{
+					"arn:aws:s3:::test-bucket-xxxx/*",
+				},
+				SID: awsclient.String("DenyUnEncryptedObjectUploads"),
+			},
+			{
+				Action: []string{
+					"s3:GetBucketLocation",
+					"s3:GetBucketVersioning",
+					"s3:GetLifecycleConfiguration",
+					"s3:GetObject",
+					"s3:GetObjectAcl",
+					"s3:GetObjectVersion",
+					"s3:GetObjectTagging",
+					"s3:GetObjectRetention",
+					"s3:PutObject",
+					"s3:PutObjectAcl",
+					"s3:DeleteObject",
+					"s3:ListBucket",
+					"s3:ListBucketVersions",
+				},
+				Condition: []common.Condition{
+					{
+						OperatorKey: "StringEquals",
+						Conditions: []common.ConditionPair{
+							{
+								ConditionKey:         "aws:PrincipalAccount",
+								ConditionStringValue: awsclient.String("123456789012"),
+							},
+						},
+					},
+				},
+				Effect: "Allow",
+				Principal: &common.BucketPrincipal{
+					AllowAnon: true,
+				},
+				Resource: []string{
+					"arn:aws:s3:::test-bucket-xxxx",
+					"arn:aws:s3:::test-bucket-xxxx/*",
+				},
+				SID: awsclient.String("AllowTenantReadWrite"),
+			},
+			{
+				Action: []string{
+					"s3:*",
+				},
+				Condition: []common.Condition{
+					{
+						OperatorKey: "Bool",
+						Conditions: []common.ConditionPair{
+							{
+								ConditionKey:          "aws:SecureTransport",
+								ConditionBooleanValue: pointer.Bool(false),
+							},
+						},
+					},
+				},
+				Effect: "Deny",
+				Principal: &common.BucketPrincipal{
+					AllowAnon: true,
+				},
+				Resource: []string{
+					"arn:aws:s3:::test-bucket-xxxx",
+					"arn:aws:s3:::test-bucket-xxxx/*",
+				},
+				SID: awsclient.String("AllowSSLRequestsOnly"),
+			},
+		},
+	}
+
+	testPolicyIssue1771External := "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Sid\":\"DenyIncorrectEncryptionHeader\",\"Effect\":\"Deny\",\"Principal\":\"*\",\"Action\":\"s3:PutObject\",\"Resource\":\"arn:aws:s3:::test-bucket-xxxx/*\",\"Condition\":{\"StringNotEquals\":{\"s3:x-amz-server-side-encryption\":[\"AES256\",\"aws:kms\"]}}},{\"Sid\":\"DenyUnEncryptedObjectUploads\",\"Effect\":\"Deny\",\"Principal\":\"*\",\"Action\":\"s3:PutObject\",\"Resource\":\"arn:aws:s3:::test-bucket-xxxx/*\",\"Condition\":{\"Null\":{\"s3:x-amz-server-side-encryption\":\"true\"}}},{\"Sid\":\"AllowTenantReadWrite\",\"Effect\":\"Allow\",\"Principal\":\"*\",\"Action\":[\"s3:GetBucketLocation\",\"s3:GetBucketVersioning\",\"s3:GetLifecycleConfiguration\",\"s3:GetObject\",\"s3:GetObjectAcl\",\"s3:GetObjectVersion\",\"s3:GetObjectTagging\",\"s3:GetObjectRetention\",\"s3:PutObject\",\"s3:PutObjectAcl\",\"s3:DeleteObject\",\"s3:ListBucket\",\"s3:ListBucketVersions\"],\"Resource\":[\"arn:aws:s3:::test-bucket-xxxx\",\"arn:aws:s3:::test-bucket-xxxx/*\"],\"Condition\":{\"StringEquals\":{\"aws:PrincipalAccount\":\"123456789012\"}}},{\"Sid\":\"AllowSSLRequestsOnly\",\"Effect\":\"Deny\",\"Principal\":\"*\",\"Action\":\"s3:*\",\"Resource\":[\"arn:aws:s3:::test-bucket-xxxx\",\"arn:aws:s3:::test-bucket-xxxx/*\"],\"Condition\":{\"Bool\":{\"aws:SecureTransport\":\"false\"}}}]}"
+
 	testPolicyRawShuffled := "{\"Statement\":[{\"Effect\":\"Allow\",\"Action\":\"s3:ListBucket\",\"Principal\":\"*\",\"Resource\":\"arn:aws:s3:::test.s3.crossplane.com\"}],\"Version\":\"2012-10-17\"}"
 	testPolicyRaw := makeRawPolicy(testPolicy)
 	testPolicyOtherRaw := makeRawPolicy(testPolicyOther)
@@ -217,6 +339,23 @@ func TestPolicyObserve(t *testing.T) {
 				err:    nil,
 			},
 		},
+		"TestIssue1771Updated": {
+			args: args{
+				b: s3testing.Bucket(s3testing.WithPolicy(testPolicyIssue1771)),
+				cl: NewPolicyClient(fake.MockBucketClient{
+					MockBucketPolicyClient: fake.MockBucketPolicyClient{
+						MockGetBucketPolicy: func(ctx context.Context, input *s3.GetBucketPolicyInput, opts []func(*s3.Options)) (*s3.GetBucketPolicyOutput, error) {
+							return &s3.GetBucketPolicyOutput{
+								Policy: &testPolicyIssue1771External,
+							}, nil
+						},
+					},
+				}),
+			},
+			want: want{
+				status: Updated,
+			},
+		},
 	}
 
 	for name, tc := range cases {

diff --git a/pkg/utils/policy/compare.go b/pkg/utils/policy/compare.go
@@ -0,0 +1,12 @@
+package policy
+
+import (
+	"github.com/google/go-cmp/cmp"
+)
+
+// ArePoliciesEqal determines if the two Policy objects can be considered
+// equal.
+func ArePoliciesEqal(a, b *Policy) (equal bool, diff string) {
+	diff = cmp.Diff(a, b)
+	return diff == "", diff
+}
diff --git a/pkg/utils/policy/parse.go b/pkg/utils/policy/parse.go
@@ -0,0 +1,25 @@
+package policy
+
+import "encoding/json"
+
+// ParsePolicyBytes from a byte array representing a raw JSOn string.
+func ParsePolicyBytes(raw []byte) (Policy, error) {
+	policy := Policy{}
+	err := json.Unmarshal(raw, &policy)
+	return policy, err
+}
+
+// ParsePolicyString from a raw JSON string.
+func ParsePolicyString(raw string) (Policy, error) {
+	return ParsePolicyBytes([]byte(raw))
+}
+
+// ParsePolicyObject parses a policy from an object (i.e. an API struct) which
+// can be marshalled into JSON.
+func ParsePolicyObject(obj any) (Policy, error) {
+	input, err := json.Marshal(obj)
+	if err != nil {
+		return Policy{}, err
+	}
+	return ParsePolicyBytes(input)
+}