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

Bugfix/Fix sslmode with custom sqlite path and on Heroku #189

Merged

Conversation

@c-w
Copy link
Member

c-w commented May 8, 2019

There is an issue in dj-database-url which always sets the sslmode even if the underlying engine doesn't support SSL. See also the conversation with @deltatree on #170 for more information.

There also is an issue in django-heroku which seems to always override sslmode=True. See also the conversation with @dveselov on #170 for more information.

This change implements a work-around for both behaviors.

@c-w c-w changed the title Bugfix/Fix sslmode with custom SQLite DATABASE_URL Bugfix/Fix sslmode with custom sqlite path and on Heroku May 8, 2019
@c-w c-w force-pushed the CatalystCode:bugfix/ssl-connection-with-sqlite branch from a3eb0e9 to 65f2420 May 8, 2019
@icoxfog417

This comment has been minimized.

Copy link
Contributor

icoxfog417 commented May 8, 2019

Let me arrange the issue (it'll be a little bothering, but please forgive me because it is security matter).

Problem

User can not use non-SSL connection at the development environment.

Cause

Solution

We can overwrite database setting after django-heroku (example from).

import django_heroku
import dj_database_url
django_heroku.settings(locals())
# override DATABASE_URL set by django_heroku because it forces SSL mode locally
ssl_require = os.environ['ENV'] != 'development'
locals()['DATABASES']['default'] = dj_database_url.config(
    conn_max_age=django_heroku.MAX_CONN_AGE, ssl_require=ssl_require)

To use environment variable IS_HEROKU will force the users should set IS_HEROKU=True when deploying doccano to Heroku. If user forget this, the configuration does not set appropriately.

Can we use DEBUG environment variable instead? (It will be set in the development environment but
not in the production environment). note: this setting assumes users will not use sqlite in production.

@c-w

This comment has been minimized.

Copy link
Member Author

c-w commented May 9, 2019

@icoxfog417 Thanks for the review. Calling django_herok.settings(...) earlier is a great idea! I've moved the call in 89372b6 which also lets us remove the IS_HEROKU flag.

As for using the DEBUG flag to disable SSL, why not trust the user's input? If the user does not set ?sslmode=... in DATABASE_URL, we will default to having SSL required as before. However, if the user knows what they're doing and explicitly passes sslmode=... we should probably respect that explicit setting instead of requiring another environment variable to be set (e.g. see principle of least astonishment). Curious to hear your thoughts!

@icoxfog417

This comment has been minimized.

Copy link
Contributor

icoxfog417 commented May 10, 2019

@c-w Thank you for your hard work!

To use DEBUG is an alternative idea for IS_HEROKU. But we can overcome the problem just only change the position of django_herok.settings. It's great and environment variable is no more need.

Finally, let me confirm the code.

ssl_require='sslmode' not in furl(env('DATABASE_URL', '')).args
  • True: ssl_mode does not exist in the DATABASE_URL
  • False: ssl_mode exsits in the DATABASE_URL
    • If ssl_mode is require

So please add test code for sslmode=require case.

@c-w

This comment has been minimized.

Copy link
Member Author

c-w commented May 10, 2019

@icoxfog417 You're correct. if ssl_require=True is passed into the dj_database_url.config function, SSL will always be set, no matter what is the value DATABASE_URL. If ssl_require=False, whatever value is present in DATABASE_URL will be used, or SSL will be disabled if the URL does not set anything. This is why we do the check: if DATABASE_URL does not set an explicit sslmode, we default to enabling SSL for secure defaults and to be backwards compatible with earlier behavior. However, if DATABASE_URL contains an explicit sslmode, we set ssl_require=False such that the sslmode provided by the user will not be overwritten.

I've added the new test case in 756fff8.

Correct values can be found here:
https://www.postgresql.org/docs/9.1/libpq-ssl.html
@icoxfog417

This comment has been minimized.

Copy link
Contributor

icoxfog417 commented May 11, 2019

Thank you for your fix!
@Hironsan Please check & merge if it ok.

@dveselov

This comment has been minimized.

Copy link

dveselov commented May 14, 2019

Are there any obstacles to merge it, guys?

@Hironsan Hironsan merged commit eba04ab into doccano:master May 15, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@icoxfog417

This comment has been minimized.

Copy link
Contributor

icoxfog417 commented May 15, 2019

Sorry for the late reply. We were confirming Heroku deploy 🏃.

@c-w c-w deleted the CatalystCode:bugfix/ssl-connection-with-sqlite branch May 15, 2019
@dveselov

This comment has been minimized.

Copy link

dveselov commented May 15, 2019

Everything works good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.