(rules): RDS Cluster Instances fail RDS Instance Rules

[mattvanstone]

General Issue

RDS Instances that are part of a cluster will always fail DB_INSTANCE_BACKUP_ENABLED, RDS_SNAPSHOT_ENCRYPTED, and RDS_STORAGE_ENCRYPTED rules.

The Question

When there are RDS clusters, the cluster resource has the StorageEncrypted and BackupRetentionPeriod properties instead of the individual instances, which results in DB_INSTANCE_BACKUP_ENABLED, RDS_SNAPSHOT_ENCRYPTED, and RDS_STORAGE_ENCRYPTED rules failing.

https://github.com/aws-cloudformation/aws-guard-rules-registry/blob/828f505c78f7fb3b7cce2779f5632749927fcae3/rules/aws/amazon_rds/rds_storage_encrypted.guard#L31-L34

I think the resource selection on these rules should exclude AWS::RDS::DBInstance resources when the DBClusterIdentifier property exists. Then we would need separate rules for clusters.

 let aws_rds_instances_storage_encrypted = Resources.*[ Type == 'AWS::RDS::DBInstance' 
   Properties.DBClusterIdentifier !exists
   Metadata.guard.SuppressedRules not exists or 
   Metadata.guard.SuppressedRules.* != "RDS_STORAGE_ENCRYPTED" 
 ] 

Either that, or we should include AWS::RDS::DBCluster in these rules and check for the properties on those resources as well.

Thoughts? I'm happy to implement the changes and submit a PR after your feedback.

CloudFormation Guard Version

2.1.0

OS

Ubuntu

OS Version

No response

Other information

Here is some sample CloudFormation

"cdxauroradb91C3606F": {
    "Type": "AWS::RDS::DBCluster",
    "Properties": {
     "Engine": "aurora-postgresql",
     "BackupRetentionPeriod": 30,
     "CopyTagsToSnapshot": true,
     "DatabaseName": "abcd",
     "DBClusterParameterGroupName": "default.aurora-postgresql11",
     "DBSubnetGroupName": {
      "Ref": "cdxauroradbSubnets9798BDEF"
     },
     "EngineVersion": "11.13",
     "Port": 3306,
     "PreferredBackupWindow": "05:00-05:45",
     "PreferredMaintenanceWindow": "Sun:06:00-Sun:06:30",
     "StorageEncrypted": true,
    },
    "UpdateReplacePolicy": "Delete",
    "DeletionPolicy": "Delete",
   },
   "cdxauroradbInstance17E53CD7C": {
    "Type": "AWS::RDS::DBInstance",
    "Properties": {
     "DBInstanceClass": "db.t3.medium",
     "DBClusterIdentifier": {
      "Ref": "cdxauroradb91C3606F"
     },
     "DBSubnetGroupName": {
      "Ref": "cdxauroradbSubnets9798BDEF"
     },
     "Engine": "aurora-postgresql",
     "EngineVersion": "11.13",
     "PubliclyAccessible": false,
    },
    "UpdateReplacePolicy": "Delete",
    "DeletionPolicy": "Delete",
   },
   "cdxauroradbInstance2AE60BE5C": {
    "Type": "AWS::RDS::DBInstance",
    "Properties": {
     "DBInstanceClass": "db.t3.medium",
     "DBClusterIdentifier": {
      "Ref": "cdxauroradb91C3606F"
     },
     "DBSubnetGroupName": {
      "Ref": "cdxauroradbSubnets9798BDEF"
     },
     "Engine": "aurora-postgresql",
     "EngineVersion": "11.13",
     "PubliclyAccessible": false,
    },
    "UpdateReplacePolicy": "Delete",
    "DeletionPolicy": "Delete",
   }

[mattvanstone]

Minor update, DB_INSTANCE_BACKUP_ENABLED is not applicable for clusters because the BackupRetentionPeriod property on a cluster must be a value from 1 to 35.

I think RDS_SNAPSHOT_ENCRYPTED is redundant because it checks for the same property as RDS_STORAGE_ENCRYPTED.

For RDS_STORAGE_ENCRYPTED, here is the change I made to rds_storage_encrypted.guard to make it applicable to clusters as well as instances.

let aws_rds_instances_storage_encrypted_types = [
  'AWS::RDS::DBInstance',
  'AWS::RDS::DBCluster'
]

let aws_rds_instances_storage_encrypted = Resources.*[
  Type in %aws_rds_instances_storage_encrypted_types
  Properties.DBClusterIdentifier !exists
  Metadata.guard.SuppressedRules !exists or
  Metadata.guard.SuppressedRules.* != "RDS_STORAGE_ENCRYPTED"
]

rule RDS_STORAGE_ENCRYPTED when %aws_rds_instances_storage_encrypted !empty {
  %aws_rds_instances_storage_encrypted.Properties.StorageEncrypted exists
  %aws_rds_instances_storage_encrypted.Properties.StorageEncrypted == true
  <<
    Violation: All RDS instances must have encrypted storage.
    Fix: Set the StorageEncrypted parameter to true.
  >>
}

[grolston]

Thank you @mattvanstone
With any all-caps AWS Guard Rule we have done our best of effort to doing a one-to-one mapping for the AWS Config managed rules when they could be defined as a guard rule (preventative rule). Sometimes they seem redundant, but the goal is to try to satisfy the AWS Config rule and make it obvious about the mapping. That is to say that if you have your pipeline deliver the IaC, you should not be triggering the AWS Config rule that it does best of effort to represent.

[grolston]

Regarding the exclusion if the db cluster identifier is present, would it break anything if we had both the values present? What I am asking is: would the explicit setting of that value on the DB instance even though the DB Cluster has that set as well, break anything on the deployment if both were set to true? Many times it is better to be explicit than implicit and as your configurations serves as documentation for your environment, it is better to be explicit what what you are doing. Then there is no question about your intent or desired outcome.

[mattvanstone]

Regarding the exclusion if the db cluster identifier is present, would it break anything if we had both the values present? What I am asking is: would the explicit setting of that value on the DB instance even though the DB Cluster has that set as well, break anything on the deployment if both were set to true? Many times it is better to be explicit than implicit and as your configurations serves as documentation for your environment, it is better to be explicit what what you are doing. Then there is no question about your intent or desired outcome.

That's a good point. My CloudFormation is synthesized from CDK, and CDK does not put those properties on the instances. So I forgot to consider a scenario where someone creates a CloudFormation template manually. I guess in this scenario I will need to create my own rule.

[benbridts]

Regarding the exclusion if the db cluster identifier is present, would it break anything if we had both the values present?

Yes, the documentation says:

If you specify the SourceDBInstanceIdentifier property, don't specify this property. The value is inherited from the source DB instance, and if the DB instance is encrypted, the specified KmsKeyId property is used.

Many times it is better to be explicit than implicit and as your configurations serves as documentation for your environment, it is better to be explicit what what you are doing. Then there is no question about your intent or desired outcome. I think this doesn't apply for things that can't be overridden (ie if you enable encryption on the instance, but not on the cluster your data would not be encrypted and that would not be reflected in the code).

If there was a rule that checks the cluster, you could explicitly ignore it on the Instance, and mention that it is enabled on the cluster level.

With any all-caps AWS Guard Rule we have done our best of effort to doing a one-to-one mapping for the AWS Config managed rules when they could be defined as a guard rule (preventative rule). Sometimes they seem redundant, but the goal is to try to satisfy the AWS Config rule and make it obvious about the mapping

I'm not sure if I agree with the 1:1 mapping (as a long term goal); but I also think that isn't a problem here: Or better; doing the cluster-level check is probably a better mapping to what Config does, than only checking the DbInstances.

This is because I believe not setting it on the Instance; but setting it on the Cluster should not trigger the Config rule (as the actual deployed instance will be encrypted).

[grolston]

Given the issue with specifying it, we should do that check to be clear and not force someone to try to meet compliance when it is actually breaking something. We should put that in as a fix.

[grolston]

@mattvanstone The CDK generation of CloudFormation is something I can take a look into. Looking at this we hope to definitely support CDK and if that is not producing it, we can add those checks to this to support CDK in that way out of the box.

[mattvanstone]

@grolston that code I added to the end of my first post was generated by CDK (with some stuff redacted). If you want to see more I'd be happy to share it with you directly.

With CDK, every resource has an aws:cdk:path metadata property. The guard rule could check for the presence of this to determine if the rule should be skipped on an instance.

   "Metadata": {
    "aws:cdk:path": "redacted"
   }

I'm happy to help with this and contribute if there is anything I can do.