-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add TLS/mTLS options in database and cache settings for PostgreSQL and Redis #46
Add TLS/mTLS options in database and cache settings for PostgreSQL and Redis #46
Conversation
@mariobehling, @marcoag could you please review the PR |
if USE_DATABASE_TLS or USE_DATABASE_MTLS: | ||
tls_config = {} | ||
if not USE_DATABASE_MTLS: | ||
if 'postgresql' in db_backend: |
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.
db_backend
is a string, like 'postgresql'
or 'mysql'
.
The way you are checking if 'postgresql' in db_backend
is weird.
Please do just normal, common way with ==
.
I think we should not enable TLS if we are connecting PostgreSQL, Redis on the same machine. It means that we should check for If connecting database on the same machine, the connection is already safe. Adding TLS just add overhead without benefit. What do you think, Mario, Marco? |
By the way, I think that the routine of building TLS config can be extracted to some file and imported to settings.py, because the settings.py file is already too long, and has many nested blocks. |
Why is this PR almost the same as #45 ? Have you copied from the same source? |
You copied the code from upstream https://github.com/pretix/pretix/blob/3b98d87a26d02f3dd72a28452421ce42b6bc6620/src/pretix/settings.py#L126-L143, which is illegal. You may not understand the scope of the work. |
hey @hongquan, I was not aware about that so I just tried to follow the PR mentioned in the issue... |
This PR fixes: #28
Current behavior:-
With the current settings we can use PostgreSQL as database and Redis as cache, we only have to specify it in the configurations.
database: backend:
config topostgresql
redis: location:
configBy giving these configs our application will start running with postgresql and redis as DB and cache respectively.
However, the current
settings.py
does not specify any TLS/mTLS options for DB and cache.New Behavior:-
So to support TLS connections with the database and cache I have added new options only for postgresql database and redis caches.
The new keys added are:-
These values should be set in the configurations before running the server if we need to run in TLS/mTLS mode.
I have tested the code manually by running postgresql locally on a different port.