feat: add SSL to new DB parameters #14673
Conversation
| == "postgresql+psycopg2://username:password@localhost:5432/dbname?foo=bar" | ||
| assert sqlalchemy_uri == ( | ||
| "postgresql+psycopg2://username:password@localhost:5432/dbname?" | ||
| "foo=bar&sslmode=verify-ca" |
There was a problem hiding this comment.
Could go with an enum approach to make sure the user is entering one of the 5 properly
There was a problem hiding this comment.
@hughhhh the user is not choosing one, with the new form they only choose between SSL on or off. If they choose "on" we set it to verify-ca, since it's the least restrictive one that still ensures an encrypted connection (note that require is identical to verify-ca).
If the user wants to use a different value (eg, verify-full) they need to edit the SQLAlchemy URI directly.
| @@ -1339,8 +1343,16 @@ class BasicParametersMixin: | |||
| "drivername://user:password@host:port/dbname[?key=value&key=value...]" | |||
| ) | |||
|
|
|||
| # query parameter to enable encryption in the database connection | |||
| # for Postgres this would be `{"sslmode": "verify-ca"}`, eg. | |||
| encryption_parameters: Dict[str, str] = {} | |||
There was a problem hiding this comment.
do you feel that we will need to check/enforce that this is defined on the engine specs that use this mixin?
There was a problem hiding this comment.
Oh, good idea... maybe if the users passes encryption=True when configuring a database we should raise an exception if this is empty.
Codecov Report
@@ Coverage Diff @@
## master #14673 +/- ##
=======================================
Coverage 77.54% 77.55%
=======================================
Files 958 958
Lines 48541 48559 +18
Branches 5702 5702
=======================================
+ Hits 37640 37658 +18
Misses 10700 10700
Partials 201 201
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
* feat: add SSL to new DB parameters * Fix test * Raise if cls.encryption_parameters is empty
* feat: add SSL to new DB parameters * Fix test * Raise if cls.encryption_parameters is empty
* feat: add SSL to new DB parameters * Fix test * Raise if cls.encryption_parameters is empty
mistercrunch
added
🏷️ bot
SUMMARY
Expose an option to configure an encrypted connection when adding a new database with the new parameters flow.
In the new flow, users can just specify that they want SSL on, and we'll build a SQLAlchemy URI that enforces encryption. For Postgres, eg, this is done by adding
sslmode=verify-cato the query parameters.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TEST PLAN
Updated unit tests.
ADDITIONAL INFORMATION