Skip to content

Enhancement/Enable non-SSL connections to the database#170

Merged
Hironsan merged 2 commits into
doccano:masterfrom
CatalystCode:enhancement/allow-non-ssl-database-connections
May 7, 2019
Merged

Enhancement/Enable non-SSL connections to the database#170
Hironsan merged 2 commits into
doccano:masterfrom
CatalystCode:enhancement/allow-non-ssl-database-connections

Conversation

@c-w
Copy link
Copy Markdown
Member

@c-w c-w commented Apr 22, 2019

Currently, whenever the database is changed from the default SQLite via the DATABASE_URL environment variable, the connection to the database is forced to be SSL. This is a great default for production use-cases. However, in some scenarios, users may not want to use SSL, e.g. when testing against a local MySQL or PostgreSQL database.

This change enables users to explicitly request a non-SSL connection to the database by passing the ?sslmode=disable (or similar) query argument in the DATABASE_URL environment variable.

For backwards compatibility, if the sslmode is not set explicitly in the connection URL, the code still defaults to requiring SSL on the connection.

Currently, whenever the database is changed from the default SQLite via
the DATABASE_URL environment variable, the connection to the database is
forced to be SSL. This is a great default for production use-cases.
However, in some scenarios, users may not want to use SSL, e.g. when
testing against a local MySQL or PostgreSQL database.

This change enables users to explicitly request a non-SSL connection to
the database by passing the `?sslmode=disable` (or similar) query
argument in the DATABASE_URL environment variable.

For backwards compatibility, if the `sslmode` is not set explicitly in
the connection URL, the code still defaults to requiring SSL on the
connection.
@dveselov
Copy link
Copy Markdown

dveselov commented May 4, 2019

Can this be merged, please?

@c-w
Copy link
Copy Markdown
Member Author

c-w commented May 4, 2019

Tagging @Hironsan for visibility.

@Hironsan Hironsan merged commit 6ee23ec into doccano:master May 7, 2019
@Hironsan
Copy link
Copy Markdown
Member

Hironsan commented May 7, 2019

Merged. Thanks!

@deltatree
Copy link
Copy Markdown

New merge leads to an error:
File "/usr/local/lib/python3.6/site-packages/django/db/backends/sqlite3/base.py", line 159, in get_new_connection

conn = Database.connect(**conn_params)

TypeError: 'sslmode' is an invalid keyword argument for this function

@dveselov
Copy link
Copy Markdown

dveselov commented May 7, 2019

@c-w looks like dj-database-url doesn't react to sslmode argument at all :(
I've got following DATABASES content with that URL - postgresql://postgres:mysecretpassword@database/postgres?sslmode=disabled:

{
    'default': {
        'NAME': 'postgres',
        'USER': 'postgres',
        'PASSWORD': 'mysecretpassword',
        'HOST': 'database',
        'PORT': '',
        'CONN_MAX_AGE': 600,
        'OPTIONS': {
            'sslmode': 'require'
        },
        'ENGINE': 'django.db.backends.postgresql_psycopg2'
    }
}

@c-w
Copy link
Copy Markdown
Member Author

c-w commented May 8, 2019

@dveselov Could you please confirm that everything is set up correctly? See screenshot below for the database entry that I see when running with commit 6ee23ec.

Screenshot showing DATABASE_URL settings

@c-w c-w deleted the enhancement/allow-non-ssl-database-connections branch May 8, 2019 14:33
@c-w
Copy link
Copy Markdown
Member Author

c-w commented May 8, 2019

@deltatree Could you please let me know what environment settings you're using? With the default SQLite mode, I'm seeing correct behavior any errors and this was confirmed in the CI too.

Screenshot showing default SQLite database connection

I'm assuming you're using DATABASE_URL to set a custom SQLite path?

@c-w
Copy link
Copy Markdown
Member Author

c-w commented May 8, 2019

@deltatree I confirmed that this is an issue in dj-database-url which always sets the sslmode even if the underlying engine doesn't support SSL. I've implemented a work-around for this in #189.

I also tested the behavior with an older version of the code before the merge of this pull request and it seems to exhibit the same behavior so I believe this is not a regression but a newly discovered bug in its own right. Just to confirm, could you please share your environment settings so that I can properly reproduce? Thanks!

@dveselov
Copy link
Copy Markdown

dveselov commented May 8, 2019

@c-w can be reproduced via

$ docker run -ti --rm -e "DATABASE_URL=postgresql://postgres:passwd@database/postgres?sslmode=disabled" chakkiworks/doccano:latest python app/manage.py shell

Python 3.6.8 (default, Mar 27 2019, 08:53:45) 
[GCC 6.3.0 20170516] on linux
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> from django.conf import settings
>>> settings.DATABASES['default']
{'sslmode': 'require'}

@c-w
Copy link
Copy Markdown
Member Author

c-w commented May 8, 2019

@dveselov Thanks for providing the repro steps. I can confirm the behavior.

Looks like this is an issue in django-heroku which seems to always override sslmode=True.

I've pushed a work-around for this issue to #189 in commit e5c935a.

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.

4 participants