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

update deployment to ensure RDS SSL conction and CA verification #256

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tonytheleg
Copy link
Contributor

@tonytheleg tonytheleg commented Nov 21, 2024

PR Template:

Describe your changes

  • updates config package to
    • add logic to check if there is an RDS CA cert value in cdapp config -- if so, it sets ssl mode to verify and leverages the RdsCa method from app-common to dump the cert to a temp file for consumption
    • add error handling for above changes
  • updates test for changes to ensure the RDS CA value is properly set and CA contents match app config value

Validation:
Can't really validate this in ephemeral easily without an RDS db provided by App Interface due to how ClowdApps work.

The tests confirm the logic works if the data is there, but we would need to fully validate this functionality in stage since we can't really mock App interface or test in ephemeral with it.

Ticket reference (if applicable)

Fixes #

Checklist

  • Are the agreed upon acceptance criteria fulfilled?

  • Was the 4-eye-principle applied? (async PR review, pairing, ensembling)

  • Do your changes have passing automated tests and sufficient observability?

  • Are the work steps you introduced repeatable by others, either through automation or documentation?

    • If automation is possible but not done due to other constraints, a ticket to the tech debt sprint is added
    • An SOP (Standard Operating Procedure) was created
  • The Changes were automatically built, tested, and - if needed, behind a feature flag - deployed to our production environment. (Please check this when the new deployment is done and you could verify it.)

  • Are the agreed upon coding/architectural practices applied?

  • Are security needs fullfilled? (e.g. no internal URL)

  • Is the corresponding Ticket in the right state? (should be on "review" now, put to done when this change made it to production)

  • For changes to the public API / code dependencies: Was the whole team (or a sufficient amount of ppl) able to review?

@tonytheleg tonytheleg marked this pull request as draft November 22, 2024 14:31
@tonytheleg
Copy link
Contributor Author

Found out that the app-common pkg we're already using has a helper for setting up an RDS CA file for consumption

The clowder library also comes with several other helpers
clowder.LoadedConfig.RdsCa() - creates a temporary file with the RDSCa and returns the filename.

Going to look into that option as it may not require any changes in deployment, just in config logic

@tonytheleg tonytheleg force-pushed the enforce-ssl-with-rds branch 2 times, most recently from b540297 to 49956b8 Compare November 22, 2024 17:46
@tonytheleg tonytheleg marked this pull request as ready for review November 22, 2024 18:18
@tylercreller
Copy link
Contributor

tylercreller commented Nov 22, 2024

LGTM - no comments. If you haven't, consider also testing things work fine with persistence disabled for good measure (it will probably have no impact but at least will give some additional confidence). I added some test examples with disabled persistence previously albeit down in lower layers of application code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants