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

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

Closed
wants to merge 1 commit into from

Conversation

f4tq
Copy link

@f4tq f4tq commented Dec 16, 2020

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
Copy link
Owner

will commented Dec 16, 2020

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 #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
Copy link
Contributor

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
Copy link
Author

f4tq commented Dec 16, 2020

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
Copy link
Contributor

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
Copy link
Owner

will commented Dec 16, 2020

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
Copy link
Author

f4tq commented Dec 16, 2020

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
Copy link
Owner

will commented Dec 16, 2020

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
Copy link
Author

f4tq commented Dec 16, 2020

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
Copy link
Owner

will commented Dec 16, 2020

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

  • some sort of setting that people opt into
  • only on fully verified ssl connections where the client can be sure that there is no MITM

anything else is irresponsible.

NOTE: Only allow cleartext when using an ssl connection though
@f4tq
Copy link
Author

f4tq commented Dec 16, 2020

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
Copy link
Author

f4tq commented Dec 18, 2020

@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:[email protected]/postgres") do |db|
  puts db.scalar("SELECT 42")
end
 
$ crystal main.cr
42
<https://teams.microsoft.com/l/message/19:[email protected]/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
Copy link
Contributor

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

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.

3 participants