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

feat(rds): Add AWS RDS cluster transport encryption check #4004

Conversation

madereddy
Copy link
Contributor

Context

Add additional RDS cluster transport level encryption logic for supported RDS versions:

For PostgreSQL and Aurora PostgreSQL clusters, if the rds.force_ssl parameter value is set to 0, the Transport Encryption feature is not enabled. For MySQL, Aurora MySQL and MariaDB clusters, if the require_secure_transport parameter value is set to OFF, the Transport Encryption feature is not enabled.

Description

Added checks for MySQL, MariaDB, PostgreSQL, Aurora PostgreSQL, and Aurora MySQL DB clusters.

Had to modify rds_instance_deletion_protection check and test as well to deal the modification to the db_clusters which allows the parameters to be read.

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@madereddy madereddy requested review from a team as code owners May 15, 2024 01:42
@github-actions github-actions bot added the provider/aws Issues/PRs related with the AWS provider label May 15, 2024
@madereddy
Copy link
Contributor Author

Competing with #4002 and #4003

Recommended to merge this after #4002 as that is a more important check. Cert: rds-ca-2019 will be expiring August 22nd 2024.

@@ -14,7 +14,7 @@ def __init__(self, provider):
# Call AWSService's __init__
super().__init__(__class__.__name__, provider)
self.db_instances = []
self.db_clusters = {}
self.db_clusters = []
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be kept as a dict using the cluster ARN as key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to a list so that it would match self.db_instances and could use similar parameter group code. I can switch this back to a dict if required or if it would cause breaking changes.

Copy link
Member

Choose a reason for hiding this comment

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

We must use maps to store objects using the ARN as key for faster retrievals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh I understand. Im going to close this PR and redo it entirely. Thank you @jfagoagas for the guidance.

)
# We must use a unique value as the dict key to have unique keys
db_cluster = []
Copy link
Member

Choose a reason for hiding this comment

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

Why this?

multi_az=cluster["MultiAZ"],
region=regional_client.region,
tags=cluster.get("TagList", []),
self.db_clusters.append(
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be kept as it was.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to more closely match db_instances. Can switch it back and rework the PR if this is required.

@madereddy madereddy closed this May 17, 2024
@jfagoagas
Copy link
Member

@madereddy are you planning to re-do the PR?

@madereddy
Copy link
Contributor Author

Yes I will redo it after the other certificate PR has been merged.

@sergargar
Copy link
Member

@madereddy I have merged the other PR!

@madereddy
Copy link
Contributor Author

I will start working on the update commit now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
provider/aws Issues/PRs related with the AWS provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants