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

fix: fix deny exception policy #272

Merged
merged 10 commits into from
Aug 28, 2024
+81 −18
Merged

fix: fix deny exception policy #272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b001f57
fix: fix deny exception policy
JianLi-Expedia Aug 27, 2024
2ba83fb
fix: change variable type
JianLi-Expedia Aug 27, 2024
c9680cf
fix: change deny exception policy
JianLi-Expedia Aug 27, 2024
e0c6ac0
fix: fix condition
JianLi-Expedia Aug 27, 2024
372c5d5
fix: fix policy
JianLi-Expedia Aug 27, 2024
608d49e
fix: fix deny exception roles
JianLi-Expedia Aug 27, 2024
d363838
fix: fix typo
JianLi-Expedia Aug 27, 2024
9af15d6
fix: fix list
JianLi-Expedia Aug 27, 2024
52cd8d4
fix: update change log
JianLi-Expedia Aug 27, 2024
b761e3b
fix: update varaible name
JianLi-Expedia Aug 27, 2024
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
7 changes: 7 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,13 @@ All notable changes to this project will be documented in this file.

The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

## [7.3.2] - 2024-08-27
### Fixed
- Fixed schema deny exception policy.
- Added `apiary_governance_iamroles` into deny exception policy.
- Added new variable `apiary_managed_service_iamroles` to handle tagging service IAM roles in deny exception policy.
- Added new variable `system_schema_producer_iamroles` to support system schema producer IAM roles.

## [7.3.1] - 2024-08-26
### Fixed
- Fixed incorrect `s3-inventory` service account secret binding.
Expand Down
34 changes: 31 additions & 3 deletions s3-other.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,16 +240,44 @@ resource "aws_s3_bucket" "apiary_system" {
"arn:aws:s3:::${local.apiary_system_bucket}/*"
]
},
%{endif}
%{if length(var.system_schema_producer_iamroles) > 0}
{
"Sid": "system schema customer account permissions",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this section specifically for system bucket?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the producer iam role does not apply to system buckets and s3 access log, as well as s3 inventory buckets

"Effect": "Allow",
"Principal": {
"AWS": [ "${join("\",\"", var.system_schema_producer_iamroles)}" ]
},
"Action": [
"s3:GetBucketLocation",
"s3:GetObject",
"s3:GetObjectAcl",
"s3:GetBucketAcl",
"s3:ListBucket",
"s3:PutObject",
"s3:PutObjectAcl",
"s3:DeleteObject",
"s3:GetBucketVersioning",
"s3:PutBucketVersioning",
"s3:ReplicateObject",
"s3:ReplicateDelete",
"s3:ObjectOwnerOverrideToBucketOwner"
],
"Resource": [
"arn:aws:s3:::${local.apiary_system_bucket}",
"arn:aws:s3:::${local.apiary_system_bucket}/*"
]
},
%{endif}
{
"Sid": "DenyUnSecureCommunications",
"Effect": "Deny",
"Principal": {"AWS": "*"},
"Action": "s3:*",
"Resource": [
"arn:aws:s3:::${local.apiary_system_bucket}",
"arn:aws:s3:::${local.apiary_system_bucket}/*"
],
"arn:aws:s3:::${local.apiary_system_bucket}",
"arn:aws:s3:::${local.apiary_system_bucket}/*"
],
"Condition": {
"Bool": {
"aws:SecureTransport": "false"
Expand Down
3 changes: 1 addition & 2 deletions s3.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,7 @@ locals {
governance_iamroles = join("\",\"", var.apiary_governance_iamroles)
consumer_prefix_roles = lookup(var.apiary_consumer_prefix_iamroles, schema["schema_name"], {})
common_producer_iamroles = join("\",\"", var.apiary_common_producer_iamroles)
deny_global_write_access = lookup(schema, "deny_global_write_access", var.deny_global_write_access)
producer_roles = lookup(schema, "producer_roles", var.producer_roles)
deny_exception_iamroles = lookup(schema, "deny_exception_iamroles", "") == "" ? "" : join("\",\"", compact(concat(split(",", schema["deny_exception_iamroles"]), var.apiary_managed_service_iamroles, var.apiary_governance_iamroles)))
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure about this anymore.

If you add apiary_governance_iamroles and apiary_managed_service_iamroles by default, this means that all buckets will restrict writing from now on. Because in many clauses deny_exception_iamroles != 0

Before, with my last update, only 1 bucket was affected, and the rest of buckets were with same functionality, but now with these changes, you are forcing all buckets to be restricted.

Is that correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, there is a condition, add it to default only when deny_exception_iamroles is not null, which mean you have deny exception defined. once you have it then add governace and egdl managed service roles as default.

})
}
}
Expand Down
39 changes: 34 additions & 5 deletions templates/apiary-bucket-policy.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,36 @@
%{endif}
%{endfor ~}
%{endif}
%{if deny_global_write_access == "true" && producer_roles != "" }
%{if deny_exception_iamroles != "" }
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe roles have access by default if it is from the same account, right?

{
"Sid": "Allow write permissions to the exception roles",
"Effect": "Allow",
"Principal": "*",
"Action": [
"s3:GetBucketLocation",
"s3:GetObject",
"s3:GetObjectAcl",
"s3:GetBucketAcl",
"s3:ListBucket",
"s3:PutObject",
"s3:PutObjectAcl",
"s3:DeleteObject",
"s3:GetBucketVersioning",
"s3:PutBucketVersioning",
"s3:ReplicateObject",
"s3:ReplicateDelete",
"s3:ObjectOwnerOverrideToBucketOwner"
],
"Resource": [
"arn:aws:s3:::${bucket_name}",
"arn:aws:s3:::${bucket_name}/*"
],
"Condition": {
"StringLike": {
"aws:PrincipalArn": [ "${deny_exception_iamroles}" ]
}
}
},
{
"Sid": "Deny write permissions to everything except the specified roles",
"Effect": "Deny",
Expand All @@ -97,7 +126,7 @@
"Resource": "arn:aws:s3:::${bucket_name}/*",
"Condition": {
"StringNotLike": {
"aws:PrincipalArn": [ "${producer_roles}" ]
"aws:PrincipalArn": [ "${deny_exception_iamroles}" ]
}
}
},
Expand Down Expand Up @@ -139,7 +168,7 @@
}
},
%{endif}
%{if producer_iamroles != ""}
%{if deny_exception_iamroles == "" && producer_iamroles != ""}
{
"Sid": "Apiary producer iamrole permissions",
"Effect": "Allow",
Expand Down Expand Up @@ -167,7 +196,7 @@
]
},
%{endif}
%{if common_producer_iamroles != ""}
%{if deny_exception_iamroles == "" && common_producer_iamroles != ""}
{
"Sid": "General read-write iamrole permissions",
"Effect": "Allow",
Expand Down Expand Up @@ -198,7 +227,7 @@
}
},
%{endif}
%{if governance_iamroles != ""}
%{if deny_exception_iamroles == "" && governance_iamroles != ""}
{
"Sid": "Apiary governance iamrole permissions",
"Effect": "Allow",
Expand Down
16 changes: 8 additions & 8 deletions variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -812,14 +812,14 @@ variable "tcp_keepalive_probes" {
default = 2
}

variable "deny_global_write_access" {
description = "Deny all write permissions from the S3 bucket except producer_roles. See VARIABLES.md for more information."
type = bool
default = false
variable "system_schema_producer_iamroles" {
description = "AWS IAM roles allowed write access to APIARY system schema"
type = list(string)
default = []
}

variable "producer_roles" {
description = "Comma separated list of roles that are able to write into the bucket. See VARIABLES.md for more information."
type = string
default = ""
variable "apiary_managed_service_iamroles" {
description = "apiary managed service IAM Roles read-write access to managed Apiary S3 buckets."
type = list(string)
default = []
}
Loading