Update test:cfn with check for ForceSSL statement IDs (#1437)

glerb · Aug 27, 2020 · ac23ab4 · ac23ab4
1 parent bfa7030
commit ac23ab4
Show file tree

Hide file tree

Showing 2 changed files with 60 additions and 41 deletions.
diff --git a/deployments/bootstrap.yml b/deployments/bootstrap.yml
@@ -139,7 +139,6 @@ Mappings:
 
 Conditions:
   ConfigureLogSubscriptions: !Not [!Equals [!Select [0, !Ref LogSubscriptionPrincipals], '']]
-  NotConfigureLogSubscriptions: !Not [Condition: ConfigureLogSubscriptions]
   EnableAccessLogs: !Equals [!Ref EnableS3AccessLogs, true]
   ExternalAccessLogs: !Not [!Equals [!Ref AccessLogsBucket, '']]
   ReplicateData: !Not [!Equals [!Ref DataReplicationBucket, '']]
@@ -466,46 +465,6 @@ Resources:
               Resource: !Sub ${DataReplicationBucket}/*
 
   ProcessedDataPolicy: # read access on processed data bucket for log subscriptions
-    Condition: ConfigureLogSubscriptions
-    Type: AWS::S3::BucketPolicy
-    Properties:
-      Bucket: !Ref ProcessedData
-      PolicyDocument:
-        Version: 2012-10-17
-        Statement:
-          - Sid: ReadBucket
-            Effect: Allow
-            Principal:
-              AWS: !Ref LogSubscriptionPrincipals
-            Action: s3:ListBucket
-            Resource: !GetAtt ProcessedData.Arn
-          - Sid: ReadLogData
-            Effect: Allow
-            Principal:
-              AWS: !Ref LogSubscriptionPrincipals
-            Action:
-              - s3:GetObject
-              - s3:GetObjectVersion
-            Resource: !Sub arn:${AWS::Partition}:s3:::${ProcessedData}/logs/*
-          - Sid: ReadRuleData
-            Effect: Allow
-            Principal:
-              AWS: !Ref LogSubscriptionPrincipals
-            Action:
-              - s3:GetObject
-              - s3:GetObjectVersion
-            Resource: !Sub arn:${AWS::Partition}:s3:::${ProcessedData}/rules/*
-          - Sid: ForceSSL
-            Effect: Deny
-            Principal: '*'
-            Action: s3:GetObject
-            Resource: !Sub arn:${AWS::Partition}:s3:::${ProcessedData}/*
-            Condition:
-              Bool:
-                aws:SecureTransport: false
-
-  ProcessedDataDefaultPolicy: # enforce secure access even if there aren't log subscriptions
-    Condition: NotConfigureLogSubscriptions
     Type: AWS::S3::BucketPolicy
     Properties:
       Bucket: !Ref ProcessedData
@@ -520,6 +479,28 @@ Resources:
             Condition:
               Bool:
                 aws:SecureTransport: false
+          - !If
+            - ConfigureLogSubscriptions
+            - Sid: ReadBucket
+              Effect: Allow
+              Principal:
+                AWS: !Ref LogSubscriptionPrincipals
+              Action: s3:ListBucket
+              Resource: !GetAtt ProcessedData.Arn
+            - !Ref AWS::NoValue
+          - !If
+            - ConfigureLogSubscriptions
+            - Sid: ReadLogAndRuleData
+              Effect: Allow
+              Principal:
+                AWS: !Ref LogSubscriptionPrincipals
+              Action:
+                - s3:GetObject
+                - s3:GetObjectVersion
+              Resource:
+                - !Sub arn:${AWS::Partition}:s3:::${ProcessedData}/logs/*
+                - !Sub arn:${AWS::Partition}:s3:::${ProcessedData}/rules/*
+            - !Ref AWS::NoValue
 
   AthenaResults: # athena query results
     Type: AWS::S3::Bucket

diff --git a/tools/mage/test_cfn.go b/tools/mage/test_cfn.go
@@ -116,6 +116,7 @@ func testCfnLint() error {
 			}
 		}
 
+		errs = append(errs, cfnValidateBuckets(template, body.Resources)...)
 		errs = append(errs, cfnValidateCustomResources(template, body.Resources)...)
 	}
 
@@ -127,6 +128,43 @@ func testCfnLint() error {
 	return nil
 }
 
+// Ensure all buckets block HTTP access. Returns a list of error messages.
+func cfnValidateBuckets(template string, resources map[string]cfnResource) []string {
+	// We don't evaluate the bucket policy logic, we just check that every bucket has an associated
+	// "ForceSSL" statement ID attached to its bucket policy.
+
+	// Map bucket resource logical ID to true if we found an associated ForceSSL policy.
+	buckets := make(map[string]bool)
+
+	for logicalID, resource := range resources {
+		switch resource.Type {
+		case "AWS::S3::Bucket":
+			if _, exists := buckets[logicalID]; !exists {
+				buckets[logicalID] = false
+			}
+
+		case "AWS::S3::BucketPolicy":
+			bucketID := resource.Properties["Bucket"].(map[string]interface{})["Ref"].(string)
+			policy := resource.Properties["PolicyDocument"].(map[string]interface{})
+			for _, stmt := range policy["Statement"].([]interface{}) {
+				if stmt.(map[string]interface{})["Sid"] == "ForceSSL" {
+					buckets[bucketID] = true
+					break
+				}
+			}
+		}
+	}
+
+	var errs []string
+	for bucketID, hasPolicy := range buckets {
+		if !hasPolicy {
+			errs = append(errs, fmt.Sprintf(
+				"%s: S3 bucket %s needs an associated ForceSSL policy statement ID", template, bucketID))
+		}
+	}
+	return errs
+}
+
 // Enforce custom resources in a CloudFormation template. Returns a list of error messages.
 func cfnValidateCustomResources(template string, resources map[string]cfnResource) []string {
 	if template == cfnstacks.BootstrapTemplate {