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.

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

Added IP-based statement in bucket policy #216

Merged
merged 1 commit into from
Mar 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,7 @@ Available targets:
| <a name="input_s3_replication_permissions_boundary_arn"></a> [s3\_replication\_permissions\_boundary\_arn](#input\_s3\_replication\_permissions\_boundary\_arn) | Permissions boundary ARN for the created IAM replication role. | `string` | `null` | no |
| <a name="input_s3_replication_rules"></a> [s3\_replication\_rules](#input\_s3\_replication\_rules) | Specifies the replication rules for S3 bucket replication if enabled. You must also set s3\_replication\_enabled to true. | <pre>list(object({<br> id = optional(string)<br> priority = optional(number)<br> prefix = optional(string)<br> status = optional(string, "Enabled")<br> # delete_marker_replication { status } had been flattened for convenience<br> delete_marker_replication_status = optional(string, "Disabled")<br> # Add the configuration as it appears in the resource, for consistency<br> # this nested version takes precedence if both are provided.<br> delete_marker_replication = optional(object({<br> status = string<br> }))<br><br> # destination_bucket is specified here rather than inside the destination object because before optional<br> # attributes, it made it easier to work with the Terraform type system and create a list of consistent type.<br> # It is preserved for backward compatibility, but the nested version takes priority if both are provided.<br> destination_bucket = optional(string) # destination bucket ARN, overrides s3_replica_bucket_arn<br><br> destination = object({<br> bucket = optional(string) # destination bucket ARN, overrides s3_replica_bucket_arn<br> storage_class = optional(string, "STANDARD")<br> # replica_kms_key_id at this level is for backward compatibility, and is overridden by the one in `encryption_configuration`<br> replica_kms_key_id = optional(string, "")<br> encryption_configuration = optional(object({<br> replica_kms_key_id = string<br> }))<br> access_control_translation = optional(object({<br> owner = string<br> }))<br> # account_id is for backward compatibility, overridden by account<br> account_id = optional(string)<br> account = optional(string)<br> # For convenience, specifying either metrics or replication_time enables both<br> metrics = optional(object({<br> event_threshold = optional(object({<br> minutes = optional(number, 15) # Currently 15 is the only valid number<br> }), { minutes = 15 })<br> status = optional(string, "Enabled")<br> }), { status = "Disabled" })<br> # To preserve backward compatibility, Replication Time Control (RTC) is automatically enabled<br> # when metrics are enabled. To enable metrics without RTC, you must explicitly configure<br> # replication_time.status = "Disabled".<br> replication_time = optional(object({<br> time = optional(object({<br> minutes = optional(number, 15) # Currently 15 is the only valid number<br> }), { minutes = 15 })<br> status = optional(string)<br> }))<br> })<br><br> source_selection_criteria = optional(object({<br> replica_modifications = optional(object({<br> status = string # Either Enabled or Disabled<br> }))<br> sse_kms_encrypted_objects = optional(object({<br> status = optional(string)<br> }))<br> }))<br> # filter.prefix overrides top level prefix<br> filter = optional(object({<br> prefix = optional(string)<br> tags = optional(map(string), {})<br> }))<br> }))</pre> | `null` | no |
| <a name="input_s3_replication_source_roles"></a> [s3\_replication\_source\_roles](#input\_s3\_replication\_source\_roles) | Cross-account IAM Role ARNs that will be allowed to perform S3 replication to this bucket (for replication within the same AWS account, it's not necessary to adjust the bucket policy). | `list(string)` | `[]` | no |
| <a name="input_source_ip_allow_list"></a> [source\_ip\_allow\_list](#input\_source\_ip\_allow\_list) | List of IP addresses to allow to perform all actions to the bucket | `list(string)` | `[]` | no |
| <a name="input_source_policy_documents"></a> [source\_policy\_documents](#input\_source\_policy\_documents) | List of IAM policy documents (in JSON) that are merged together into the exported document.<br>Statements defined in source\_policy\_documents must have unique SIDs.<br>Statement having SIDs that match policy SIDs generated by this module will override them. | `list(string)` | `[]` | no |
| <a name="input_sse_algorithm"></a> [sse\_algorithm](#input\_sse\_algorithm) | The server-side encryption algorithm to use. Valid values are `AES256` and `aws:kms` | `string` | `"AES256"` | no |
| <a name="input_ssm_base_path"></a> [ssm\_base\_path](#input\_ssm\_base\_path) | The base path for SSM parameters where created IAM user's access key is stored | `string` | `"/s3_user/"` | no |
Expand Down
1 change: 1 addition & 0 deletions docs/terraform.md
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@
| <a name="input_s3_replication_permissions_boundary_arn"></a> [s3\_replication\_permissions\_boundary\_arn](#input\_s3\_replication\_permissions\_boundary\_arn) | Permissions boundary ARN for the created IAM replication role. | `string` | `null` | no |
| <a name="input_s3_replication_rules"></a> [s3\_replication\_rules](#input\_s3\_replication\_rules) | Specifies the replication rules for S3 bucket replication if enabled. You must also set s3\_replication\_enabled to true. | <pre>list(object({<br> id = optional(string)<br> priority = optional(number)<br> prefix = optional(string)<br> status = optional(string, "Enabled")<br> # delete_marker_replication { status } had been flattened for convenience<br> delete_marker_replication_status = optional(string, "Disabled")<br> # Add the configuration as it appears in the resource, for consistency<br> # this nested version takes precedence if both are provided.<br> delete_marker_replication = optional(object({<br> status = string<br> }))<br><br> # destination_bucket is specified here rather than inside the destination object because before optional<br> # attributes, it made it easier to work with the Terraform type system and create a list of consistent type.<br> # It is preserved for backward compatibility, but the nested version takes priority if both are provided.<br> destination_bucket = optional(string) # destination bucket ARN, overrides s3_replica_bucket_arn<br><br> destination = object({<br> bucket = optional(string) # destination bucket ARN, overrides s3_replica_bucket_arn<br> storage_class = optional(string, "STANDARD")<br> # replica_kms_key_id at this level is for backward compatibility, and is overridden by the one in `encryption_configuration`<br> replica_kms_key_id = optional(string, "")<br> encryption_configuration = optional(object({<br> replica_kms_key_id = string<br> }))<br> access_control_translation = optional(object({<br> owner = string<br> }))<br> # account_id is for backward compatibility, overridden by account<br> account_id = optional(string)<br> account = optional(string)<br> # For convenience, specifying either metrics or replication_time enables both<br> metrics = optional(object({<br> event_threshold = optional(object({<br> minutes = optional(number, 15) # Currently 15 is the only valid number<br> }), { minutes = 15 })<br> status = optional(string, "Enabled")<br> }), { status = "Disabled" })<br> # To preserve backward compatibility, Replication Time Control (RTC) is automatically enabled<br> # when metrics are enabled. To enable metrics without RTC, you must explicitly configure<br> # replication_time.status = "Disabled".<br> replication_time = optional(object({<br> time = optional(object({<br> minutes = optional(number, 15) # Currently 15 is the only valid number<br> }), { minutes = 15 })<br> status = optional(string)<br> }))<br> })<br><br> source_selection_criteria = optional(object({<br> replica_modifications = optional(object({<br> status = string # Either Enabled or Disabled<br> }))<br> sse_kms_encrypted_objects = optional(object({<br> status = optional(string)<br> }))<br> }))<br> # filter.prefix overrides top level prefix<br> filter = optional(object({<br> prefix = optional(string)<br> tags = optional(map(string), {})<br> }))<br> }))</pre> | `null` | no |
| <a name="input_s3_replication_source_roles"></a> [s3\_replication\_source\_roles](#input\_s3\_replication\_source\_roles) | Cross-account IAM Role ARNs that will be allowed to perform S3 replication to this bucket (for replication within the same AWS account, it's not necessary to adjust the bucket policy). | `list(string)` | `[]` | no |
| <a name="input_source_ip_allow_list"></a> [source\_ip\_allow\_list](#input\_source\_ip\_allow\_list) | List of IP addresses to allow to perform all actions to the bucket | `list(string)` | `[]` | no |
| <a name="input_source_policy_documents"></a> [source\_policy\_documents](#input\_source\_policy\_documents) | List of IAM policy documents (in JSON) that are merged together into the exported document.<br>Statements defined in source\_policy\_documents must have unique SIDs.<br>Statement having SIDs that match policy SIDs generated by this module will override them. | `list(string)` | `[]` | no |
| <a name="input_sse_algorithm"></a> [sse\_algorithm](#input\_sse\_algorithm) | The server-side encryption algorithm to use. Valid values are `AES256` and `aws:kms` | `string` | `"AES256"` | no |
| <a name="input_ssm_base_path"></a> [ssm\_base\_path](#input\_ssm\_base\_path) | The base path for SSM parameters where created IAM user's access key is stored | `string` | `"/s3_user/"` | no |
Expand Down
22 changes: 22 additions & 0 deletions main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,28 @@ data "aws_iam_policy_document" "bucket_policy" {
}
}
}

dynamic "statement" {
for_each = length(var.source_ip_allow_list) > 0 ? [1] : []

content {
sid = "AllowIPPrincipals"
effect = "Deny"
actions = ["s3:*"]
resources = [local.bucket_arn, "${local.bucket_arn}/*"]
principals {
Copy link
Member

Choose a reason for hiding this comment

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

Is this section required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for not writing the description...
Yes, it is required to achieve what I wrote in the description now.
Could you please review my PR again?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry what I meant was is the principals element required?

Copy link
Contributor Author

@soya-miyoshi soya-miyoshi Feb 27, 2024

Choose a reason for hiding this comment

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

Yes, it is required to be a valid policy. If we omit this we will get MalformedPolicyDocument error.

The reason for the wildcard is that I would like to block any principal outside of the specified IP.

identifiers = ["*"]
type = "*"
}
condition {
test = "NotIpAddress"
variable = "aws:SourceIp"
values = var.source_ip_allow_list
}
}

}

}

data "aws_iam_policy_document" "aggregated_policy" {
Expand Down
7 changes: 7 additions & 0 deletions variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,13 @@ variable "privileged_principal_actions" {
nullable = false
}

variable "source_ip_allow_list" {
type = list(string)
default = []
description = "List of IP addresses to allow to perform all actions to the bucket"
nullable = false
}

variable "transfer_acceleration_enabled" {
type = bool
default = false
Expand Down