Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix false negatives in the S3 invalid ACL rule #341

Merged
merged 1 commit into from
Jun 27, 2019
Merged

Fix false negatives in the S3 invalid ACL rule #341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 0 additions & 92 deletions rules/awsrules/aws_s3_bucket_invalid_acl_test.go
View file
Open in desktop

This file was deleted.

39 changes: 21 additions & 18 deletions rules/awsrules/aws_s3_bucket_invalid_acl.go → ...rules/models/aws_s3_bucket_invalid_acl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,32 @@
package awsrules
// This file generated by `tools/model-rule-gen/main.go`. DO NOT EDIT

package models

import (
"fmt"
"log"

"github.com/hashicorp/hcl2/hcl"
"github.com/wata727/tflint/issue"
"github.com/wata727/tflint/tflint"
)

// AwsS3BucketInvalidACLRule checks whether "aws_s3_bucket" has invalid ACL setting.
// AwsS3BucketInvalidACLRule checks the pattern is valid
type AwsS3BucketInvalidACLRule struct {
resourceType string
attributeName string
aclTypes map[string]bool
enum []string
}

// NewAwsS3BucketInvalidACLRule returns new rule with default attributes
func NewAwsS3BucketInvalidACLRule() *AwsS3BucketInvalidACLRule {
return &AwsS3BucketInvalidACLRule{
resourceType: "aws_s3_bucket",
attributeName: "acl",
aclTypes: map[string]bool{
"private": true,
"public-read": true,
"public-read-write": true,
"aws-exec-read": true,
"authenticated-read": true,
"bucket-owner-read": true,
"bucket-owner-full-control": true,
"log-delivery-write": true,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this one removed though? It seems perfectly valid to me?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, all of the above are valid according to the same link you've referenced in the PR description (https://docs.aws.amazon.com/AmazonS3/latest/dev/acl-overview.html#canned-acl), so I don't quite understand the change?

Copy link
Member Author

@wata727 wata727 Jul 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Umm, what you are saying seems right. It's weird...
The list is generated from aws-sdk API definition, so It may be something wrong with how to use. I will investigate it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like that block is way out of date - hasn't been updated in 5 years...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were commits to add new Canned ACLs since then (aws/aws-sdk-go@0922c23 for example), but that particular block was never updated :(

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aws/aws-sdk-go#2683

enum: []string{
"private",
"public-read",
"public-read-write",
"authenticated-read",
},
}
}
Expand All @@ -54,19 +51,25 @@ func (r *AwsS3BucketInvalidACLRule) Link() string {
return ""
}

// Check checks whether "aws_s3_bucket" has invalid ACL type.
// Check checks the pattern is valid
func (r *AwsS3BucketInvalidACLRule) Check(runner *tflint.Runner) error {
log.Printf("[INFO] Check `%s` rule for `%s` runner", r.Name(), runner.TFConfigPath())

return runner.WalkResourceAttributes(r.resourceType, r.attributeName, func(attribute *hcl.Attribute) error {
var acl string
err := runner.EvaluateExpr(attribute.Expr, &acl)
var val string
err := runner.EvaluateExpr(attribute.Expr, &val)

return runner.EnsureNoError(err, func() error {
if !r.aclTypes[acl] {
found := false
for _, item := range r.enum {
if item == val {
found = true
}
}
if !found {
runner.EmitIssue(
r,
fmt.Sprintf("\"%s\" is invalid canned ACL type.", acl),
`acl is not a valid value`,
attribute.Expr.Range(),
)
}
Expand Down
2 changes: 1 addition & 1 deletion rules/awsrules/models/mappings/s3.hcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ mapping "aws_s3_account_public_access_block" {
mapping "aws_s3_bucket" {
bucket = BucketName
bucket_prefix = any
acl = any // TODO: BucketCannedACL
acl = BucketCannedACL
policy = Policy
tags = TagSet
force_destroy = any
Expand Down
1 change: 0 additions & 1 deletion rules/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ var manualRules = []Rule{
awsrules.NewAwsLaunchConfigurationInvalidTypeRule(),
awsrules.NewAwsRouteNotSpecifiedTargetRule(),
awsrules.NewAwsRouteSpecifiedMultipleTargetsRule(),
awsrules.NewAwsS3BucketInvalidACLRule(),
terraformrules.NewTerraformModulePinnedSourceRule(),
}

Expand Down
1 change: 1 addition & 0 deletions rules/provider_model.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,6 +517,7 @@ var modelRules = []Rule{
awsmodelrules.NewAwsRoute53ZoneInvalidDelegationSetIDRule(),
awsmodelrules.NewAwsRoute53ZoneInvalidNameRule(),
awsmodelrules.NewAwsS3BucketInvalidAccelerationStatusRule(),
awsmodelrules.NewAwsS3BucketInvalidACLRule(),
awsmodelrules.NewAwsS3BucketInvalidRegionRule(),
awsmodelrules.NewAwsS3BucketInvalidRequestPayerRule(),
awsmodelrules.NewAwsS3BucketInventoryInvalidIncludedObjectVersionsRule(),
Expand Down