will / crystal-pg

a postgres driver for crystal
BSD 3-Clause "New" or "Revised" License
462 stars 77 forks source link

Support Azure's non-mutable use of cleartext passwords over TLS #218

Closed f4tq closed 3 years ago

f4tq commented 3 years ago

As discussed in crystal-db 141, Azure insists on asking for cleartext passwords.

While not ideal, it remains supported by postgresql and in most sane azure cases, sslmode=required.

will commented 3 years ago

I can see merging something like this for azure users. However, I don’t want to merge something that makes everyone else susceptible to downgrade attacks. Please see https://github.com/will/crystal-pg/issues/192 . If there was some build-time (or runtime, that's okay too maybe) flag that clearly made people aware that turning it on is a risk they're taking, then it'd be okay.

straight-shoota commented 3 years ago

So I've been looking at whot some of the other pg drivers do. Interestingley it seems that cleartext authentication is typically supported and cannot even be disabled. I haven't found a single one that doesn't have clear text authentication.

Maybe it's not even considered a big issue in the big world... 🤷

Still, it makes sense to not use cleartext by default (at least on non-TLS connections). I'd suggest to add a configuration option auth_methods which can be used to specify a list of allowed password authentication methods. The default should be safe, so probably just scram-sha-256 because md5 is also deprecated.

f4tq commented 3 years ago

As @straight-shoota notes in #192, it's perfectly fine if sslmode=true i.e. using an encrypted connection.

I added a commit to raise - as before - if NOT using an ssl connection the 2nd commit.

straight-shoota commented 3 years ago

If the password is transmitted over TLS, the connection is secure. But you may still not want to sent cleartext passwords. Especially considering that currently only OpenSSL::SSL::VerifyMode::NONE is implemented. And there are valid reasons to use cleartext password on a non-encrypted connection.

So, I'd still suggest to have a configuration setting. We could consider to enable cleartext by default for encrypted connections.

will commented 3 years ago

So I've been looking at whot some of the other pg drivers do. Interestingley it seems that cleartext authentication is typically supported and cannot even be disabled. I haven't found a single one that doesn't have clear text authentication.

Maybe it's not even considered a big issue in the big world... 🤷

Still, it makes sense to not use cleartext by default (at least on non-TLS connections). I'd suggest to add a configuration option auth_methods which can be used to specify a list of allowed password authentication methods. The default should be safe, so probably just scram-sha-256 because md5 is also deprecated.

It's a non-obvious problem, and the project itself is wrong for supporting this.

Also just TLS does not fix it. Only verify-full. If the attacker is in the middle, they can terminate TLS at both ends, like to the client that the server only supports clear-text when the server is doing a better auth mechanism, then get the password from the client, and do the proper auth to the server.

f4tq commented 3 years ago

Isn't this about supporting a very popular database?

All the way through postgresql 13, password is supported.

I'm just trying to use crystal on a new project. I obviously won't be able to do that if I can't use the only crystal database driver to connect to my pre-existing database from a gigantic provider.

None of us have any control over Azure or Postgresql.

will commented 3 years ago

Isn't this about supporting a very popular database?

Yes, and I'm willing to merge something that does not put all the other people not using Azure at risk. Just ssl is not enough, but something where people can turn it on in such a way that they understand that it's a security risk is ok. Or SSL if it's fully validating the remote server.

f4tq commented 3 years ago

There are thousands of user scenarios, all of which are controlled by the database administrator, not the client.

pg_admin.conf is a server side config file that dictates which access methods are allowed to a ip/server/database/auth method. The 'why' for such auth choice lies with the server side admin not the client.

You're over policing this. The client should provide any method the server decides it wants to use. That is why every other language provides a client with cleartext and md5.

FWIW, md5 is no better than cleartext on it own.

will commented 3 years ago

The point is when cleartext is an option, and even if the server doesn't support it, a man in the middle attacker can lie to the client that cleartext is the only supported method, and get the password. MD5 at least has a unique salt each time, and would require brute forcing.

My mind is made up on this, sorry. The only way cleartext makes it into this driver is either with

anything else is irresponsible.

f4tq commented 3 years ago

That's funny!

You're right, I'd didn't see that sslmode=(verify|verify-full) aren't implemented! alla curl -k...

And connection.cr just swallows sslmode=verify* and doesn't raise?

To be minimally consistent with this thread and truly responsible, the code should raise on any attempt to use the sslmode verify* verbs. And to be consistent with this thread, require as well so as to protect users from themselves. Right?

Until it's fixed there should be a prominent README.md statement for require and full rejection of sslmode=(verify-full|verify).

For my part, to be productive and consistent I am going to try to get Azure to surface more auth options - at least md5 as it is ultimately a server side change that I need.

And meanwhile, use my fork...

f4tq commented 3 years ago

@straight-shoota , @will and any other Azure Postgres Users...

I got a reply from Azure Support. I guess Postgres Single Server is the bad step-child.

Hello Chris,

I have this reply from our Engineering team. Please let me know if you have any questions.

< 
At this point we don't support MD5 or SCRAM-SHA-256 for Postgres Single Server. We recommend using sslmode=verify-full to secure the connection (https://docs.microsoft.com/en-us/azure/postgresql/concepts-ssl-connection-security#applications-that-require-certificate-verification-for-tls-connectivity). Looking at this in more detail, the customer has two options: (1) Use a patched version of the crystal Postgres driver that supports cleartext auth (and ideally patch the Crystal driver to support full verification), (2) Use the new Postgres Flexible Server offering, which supports MD5 hashing for the password (and will support SCRAM-SHA-256 in the future)
SSL/TLS - Azure Database for PostgreSQL - Single Server
Instructions and information on how to configure TLS connectivity for Azure Database for PostgreSQL - Single Server.
docs.microsoft.com
​

Also, I've confirmed that the Crystal Postgres driver can successfully connect to a Postgres Flexible Server, without requiring any changes to the driver.
​
Here is the example I ran successfully on Postgres Flexible Server, feel free to share with the customer: ```
$ cat shard.yml
name: connection-test
version: 0.1.0

dependencies:
  pg:
    github: will/crystal-pg

$ cat main.cr

require "db"
require "pg"

# Note this is a Postgres Flexible Server deployment
DB.connect("postgres://postgres:PASSWORD@SERVERNAME.postgres.database.azure.com/postgres") do |db|
  puts db.scalar("SELECT 42")
end

$ crystal main.cr
42
<https://teams.microsoft.com/l/message/19:f46368718fe143d98f015885a2b6b0ae@thread.skype/1608259256876?tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47&amp;groupId=1f71ba9d-ad7f-4c80-b2b4-8a82675d1f19&amp;parentMessageId=1608252777102&amp;teamName=Orcas Supportability&amp;channelName=Azure DB for PostgreSQL - Single server&amp;createdTime=1608259256876>

> 

Sincerely,
Azure SQL (PaaS) Rapid Response Support Engineer
Data and Business Intelligence | M-F 9:00am – 6:00pm ET
straight-shoota commented 3 years ago

Wow, that's a really great response. Even checked that the recomended alternative server works with this driver.