asyncpg icon indicating copy to clipboard operation
asyncpg copied to clipboard

asyncpg does not work with "sslmode" query param when called from SQLAlchemy

Open igortg opened this issue 3 years ago • 16 comments

  • asyncpg version: 0.22
  • PostgreSQL version: 12.3
  • Do you use a PostgreSQL SaaS? If so, which? Can you reproduce the issue with a local PostgreSQL install?: I use DigitalOcean and was able to also reproduce it locally
  • Python version: 3.8.9
  • Platform: Linux
  • Do you use pgbouncer?: No
  • Did you install asyncpg with pip?: Yes
  • If you built asyncpg locally, which version of Cython did you use?: -
  • Can the issue be reproduced under both asyncio and uvloop?: N/D

First, congratulation to the people of MagicStack for this great library.

When using DigitalOcean default environment DATABASE URL to set the database string on my app I got the following error:

  File "/usr/local/lib/python3.8/site-packages/sqlalchemy/pool/base.py", line 599, in __connect
    connection = pool._invoke_creator(self)
  File "/usr/local/lib/python3.8/site-packages/sqlalchemy/engine/create.py", line 578, in connect
    return dialect.connect(*cargs, **cparams)
  File "/usr/local/lib/python3.8/site-packages/sqlalchemy/engine/default.py", line 558, in connect
    return self.dbapi.connect(*cargs, **cparams)
  File "/usr/local/lib/python3.8/site-packages/sqlalchemy/dialects/postgresql/asyncpg.py", line 747, in connect
    await_only(self.asyncpg.connect(*arg, **kw)),
TypeError: connect() got an unexpected keyword argument 'sslmode'

In my app, since I use psycopg2 to do some "management tasks", I also need to do something like that:

if ssl_enabled and driver == "asyncpg":
    base_url += "?ssl=require"
elif ssl_enabled and driver == "psycopg2":
    base_url += "?sslmode=require"

Shouldn't asyncpg use the more standard sslmode= query param instead of the ssl=?

igortg avatar Apr 08 '21 20:04 igortg

I think we do use sslmode in the DSN URL?

https://github.com/MagicStack/asyncpg/blob/075114c195e9eb4e81c8365d81540beefb46065c/asyncpg/connect_utils.py#L308-L311

Is it the ssl parameter in the Python method signature of asyncpg.connect() (not the DSN URL string) you were running into?

fantix avatar Apr 08 '21 21:04 fantix

This is a bug in SQLAlchemy's create_connect_args implementation for asyncpg.

elprans avatar Apr 08 '21 22:04 elprans

I wouldn't say it's a bug. Seems more an oversight from SQLAlchemy that this param had to be translated for asyncpg since SQLAlchemy parses the URL by itself and calls connect with query params as kwargs. Since asyncpg uses ssl as the param name on connect method, the sslmode query param would not work.

https://github.com/MagicStack/asyncpg/blob/075114c195e9eb4e81c8365d81540beefb46065c/asyncpg/connection.py#L1747

I'll open an issue on SQLAlchemy to see if they have something to say about that.

igortg avatar Apr 14 '21 21:04 igortg

Just find out that there was a similar issue with pg8000 DBAPI: https://github.com/sqlalchemy/sqlalchemy/issues/4146#issuecomment-441939790 Seems that SQLAlchemy folks expect DBAPIs to have the standard param names for connect.

Is there any chance asyncpg.connect would change the ssl param to sslmode (or add an extra alias param)?

igortg avatar Apr 14 '21 21:04 igortg

Seems that SQLAlchemy folks expect DBAPIs to have the standard param names for connect.

It is weird for SQLAlchemy to require that, given that it's supposed to be an abstraction over varying driver implementations. The create_connect_args hook I linked above seems to be designed exactly for the purpose of massaging SQLAlchemy's connection arguments to the format accepted by the driver. It simply doesn't have code to remap sslmode to ssl.

elprans avatar Apr 14 '21 21:04 elprans

Oh... Now I got it. Seems a simple fix on the SQLAlchemy side.

I opened an issue there: https://github.com/sqlalchemy/sqlalchemy/issues/6275

igortg avatar Apr 15 '21 01:04 igortg

@igortg have you been able to make it work? I read linked issue but I can't make engine creator working with async connect function and also an option with make_url still throws the same error about sslmode parameter.

url = make_url(...)
url = url.update_query_dict({'sslmode': 'require'})

I can't get it

vashchukmaksim avatar Jun 13 '21 16:06 vashchukmaksim

On asyncpg the query string is ssl instead of sslmode

igortg avatar Jun 14 '21 21:06 igortg

Hm, with this setup I receive the following error:

url = make_url(...)
url = url.update_query_dict({'sslmode': 'require'})

connect() got an unexpected keyword argument 'sslmode'

If I change sslmode to ssl:

url = make_url(...)
url = url.update_query_dict({'ssl': 'require'})

I receive another one:

Connection reset by peer

So I'm not sure that ssl parameter was passed ok to DigitalOcean eventually (but maybe it is). Also I have pool_pre_ping set to True that helped me in the similar scenario with DO and psycopg2. Probably ssl parameter is passing correctly in the last case and the reason is different. In case you have anything in mind to make it work, since as I understood you also set up asyncpg with DO as well, it will be really appreciated 🙏

vashchukmaksim avatar Jun 15 '21 18:06 vashchukmaksim

I am not sure why it still does not work after this https://github.com/MagicStack/asyncpg/pull/768 ?

svajipay avatar Aug 19 '21 09:08 svajipay

Hello, after running into this problem as well, I think it is a conflict of documentation versus how Postgresql annotates SSL Support (https://www.postgresql.org/docs/9.1/libpq-ssl.html)

The code accepts a ssl parameter: https://magicstack.github.io/asyncpg/current/api/index.html?highlight=ssl

But the problem I think is the assumption that the keyword sslmode will work from the dsn and that the ssl attribute will be optional or one will overwrite the other. This was not the case for me.

It is still not clear to me when to choose between sslmode in the dsn versus the ssl. But, something that explains these different scenarios might help mitigate people even using sslmode in the query params, since it is obviously not working for other libraries. Maybe even taking reference to sslmode in the dsn out of the documentation or just not supporting it at all.

mmarti8895 avatar Aug 31 '21 13:08 mmarti8895

After using connection_string.replace('sslmode','ssl') I am getting another error:

connect() got an unexpected keyword argument 'sslrootcert'

Do you know any solution so I can still use sslrootcert?

karolzlot avatar Oct 02 '21 03:10 karolzlot

Here's how I finally got it to work (https://github.com/sqlalchemy/sqlalchemy/discussions/5975):

from ssl import create_default_context, Purpose as SSLPurpose

ssl_context = create_default_context(
    SSLPurpose.CLIENT_AUTH,
    cafile="docker-compose.d/cockroach-certs/ca.crt"
)
ssl_context.load_cert_chain(
    "docker-compose.d/cockroach-certs/client.root.crt",
    keyfile="docker-compose.d/cockroach-certs/client.root.key"
)
ssl_context.check_hostname = True

_engine = create_async_engine(
    "cockroachdb+asyncpg://root@localhost:26257/mydb",
    echo=True,
    connect_args={"ssl": ssl_context}
)

Equivalent to sslmode=verify-full&sslcert=..&sslkey=..&sslrootcert=.. per https://magicstack.github.io/asyncpg/current/api/index.html

evindunn avatar Feb 26 '22 07:02 evindunn

Another solution is here: https://github.com/cossacklabs/acra/blob/e8ca8203dfe92566762cb1638667a62c204e4263/tests/test.py#L228

karolzlot avatar Feb 27 '22 01:02 karolzlot

On asyncpg the query string is ssl instead of sslmode

this worked for me, thanks!

Abhi011999 avatar Apr 25 '23 16:04 Abhi011999

@evindunn What if my sslrootcert is a string and not a file?

SnoozeFreddo avatar Feb 15 '24 16:02 SnoozeFreddo