-
Notifications
You must be signed in to change notification settings - Fork 3
VED-500: Shield Advanced Alerts for CSOC #769
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
Conversation
|
This branch is working on a ticket in the NHS England VED JIRA Project. Here's a handy link to the ticket: VED-500 |
|
infra/shield_protection.tf
Outdated
| data "aws_region" "current" {} | ||
| data "aws_caller_identity" "current" {} | ||
|
|
||
| provider "aws" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take it this didn't work with the default provider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it mentioned in the conference page that some global resources would not work with eu-west-2 such as Route53, cloudfront etc, Hence the reason why I am using us-east-1. Number two in this confluence link -> shield advance docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like it's needed for global resources, but maybe we could put the alternative alias provider in main: https://developer.hashicorp.com/terraform/language/block/provider
infra/shield_protection.tf
Outdated
| # Create all resources to Protect | ||
| resource "aws_shield_protection" "nat_eip" { | ||
| name = "shield_nat_eip" | ||
| resource_arn = "arn:aws:ec2:${data.aws_region.current.region}:${data.aws_caller_identity.current.account_id}:eip-allocation/${aws_eip.example.id}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aws_eip.example? Should this be aws_eip.nat?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| @@ -0,0 +1,34 @@ | |||
| resource "aws_iam_role" "eventbridge_forwarder_role" { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used? This is the only change in the terraform folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it isn't, it was a stale file, I forgot to remove, as I initially thought the code would go into terraform service level. Removing now
infra/csoc_role_creation.tf
Outdated
| @@ -0,0 +1,34 @@ | |||
| resource "aws_iam_role" "eventbridge_forwarder_role" { | |||
| name = "${local.short_prefix}-eventbridge-forwarder-role" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no short_prefix in the infra project. We could use imms-${var.environment} instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you Matt, done
infra/csoc_role_creation.tf
Outdated
| Action = "sts:AssumeRole", | ||
| Condition = { | ||
| StringEquals = { | ||
| "aws:SourceAccount" = var.immunisation_account_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be imms_account_id
infra/shield_protection.tf
Outdated
| resource_arn = "arn:aws:ec2:${data.aws_region.current.region}:${data.aws_caller_identity.current.account_id}:eip-allocation/${aws_eip.example.id}" | ||
|
|
||
| tags = { | ||
| Environment = "imms-${var.environment}-fhir-api-eip-shield" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it should go in the name attribute - same for the other aws_shield_protection resources
infra/shield_protection.tf
Outdated
| } | ||
| } | ||
|
|
||
| locals { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can combine these outer locals blocks
dlzhry2nhs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Let's do a plan or even apply to dev before we merge it.
Also will need to note that this will need releasing to higher environments from the /infra directory at some point in future.
|



PR Description
• Created IAM role and policy for EventBridge forwarding to CSOC, and added a csoc_account_id variable per environment in infra.
• Added Shield Advanced subscription in shield_subscription.tf.
• Created Shield protections for:
• NAT Gateway EIP
• Route53 Parent zone
• Route53 Child zone
• Added CloudWatch metric alarms (DDoSDetected) for the above resources.
• Created EventBridge rules and targets to forward alarm state changes to the CSOC Event Bus.
There may be additional resources (e.g. ALB, CloudFront) that could also require protection and alarms, not yet included.
Reviews Required
Review Checklist
ℹ️ This section is to be filled in by the reviewer.