Several issues related to SSL/TLS #493
Comments
My only concern about this option would be the added dependency.
+1
We already have a partial patch for that: #453. I'll rebase it and add the runtime check for mono.
+1 That's the idea for release 3.0 |
|
Since this commit: postgres/postgres@272923a, Microsoft's SslStream also breaks on renegotiation. |
|
IMHO PostgreSQL should be aware that this breaks SslStream and possibly revert this change? |
|
I sent a mail to pgsql-hackers about that yesterday. Due to a bug in openssl, jdbc renegotiation doesn't work currently either. |
|
Hi guys, I created a branch at https://github.com/Emill/Npgsql/tree/tls-client-stream containing my implementation. It should fix all the problems mentioned above... It supports a quite comprehensive collection of cipher suites: https://github.com/Emill/Npgsql/blob/tls-client-stream/Npgsql/TlsClientStream/CipherSuiteInfo.cs, much more than I had thought of before I started this :) It uses Microsoft's crypto API as much as possible, but you can't imagine how old that is by almost only supporting old ciphers. It currently has no async support, but should be very easy to add using the AsyncGenerator. I know it's kind of strange to include a full-blown TLS client implementation in a database driver, but what the heck ;) Try it out and see if it works. |
|
That's awesome, @Emill ! You rock! I think you should definitely create a project to host this gem. I'm sure a lot of people out there would like to use your tls library. Another option would be to contribute it to the .net core so everybody in the ms.net and mono platforms can use it. ;) |
|
Wow @Emill, you really went ahead with it...! :) I'll try to take a look soon (am a bit busy lately with other things). @franciscojunior's suggestion makes sense to me, about creating it as a separate project. Unless I'm mistaken, there's no way to cover the different ciphers with tests, right? I mean, there's one test that connects with SSL, but actually testing different ciphers requires backend-side configuration? |
|
Correct. You must configure the server and set certificate. You can only have one certificate at a time so you have to restart the server to change cert. There can be both rsa, dsa and ecdsa certs. There is also the possibility for client certificate requests, which you can set a CA cert for in postgresql config. |
|
I think it's easier to test it against a separate raw SslStream in server mode where one can specify different certificates programmatically. |
|
Hi @Emill ! I just tried it here but I got a problem when connecting: I even tried to add a check for null in this accessor and return 0 and then 8. After doing this change, I got this error: I hope those stacktraces can be helpful. |
|
Hi. It seems you are using an old version of openssl that does not support TLS 1.2 for your postgresql server? |
|
I'm using openssl from osx. Indeed, from this log I get when I use an old Npgsql.dll library which connects to the server, it seems to be using tlsv1: This is the openssl version I'm using: |
|
According to this post: It seems the 0.9.8za is the official openssl version in osx which doesn't contain the heartbleed bug. |
|
Hmm. I have no idea why Apple bundle such an old version (0.9.8 was released in 2005) in their OS... |
Great! This will make it even better for osx users. :) |
|
I wonder however how many people there are running postgresql on their osx computer and wants to use encrypted connection. Probably those people run Npgsql over localhost anyway ... |
Good point! :) I think you can leave it as it is for now. I'll test it now connecting from my windows vm. |
|
This is what I get when running on windows: And this is what the server logs: I even recreated the server certificate in the windows server thinking this could be the problem cause. |
|
Hmm.. Could you add this: Console.Out.WriteLine(cipherSuite); to TlsClientStream.cs line 735 |
Note that when I use the old version of Npgsql.dll which connects to the host, it also says it is using tlsv1. I don't know if this is relevant though. I hope it helps. |
|
Ahh.. Found the problem. I thought I had double-checked the cipher suite definition list... |
|
Success!! :) I also can confirm that the ssl renegotiation error doesn't occur. You rock, @Emill !! :) |
|
Yeah, nice :) |
|
I remember when I was in my graduation course and EC were something theoretical and now, more than a decade and a half later, Npgsql is able to use it to cipher its communications... :) |
|
It seems to be a bit more complicated to add support for TLS 1.0 than I thought. TLS 1.0 uses the concatenation of an MD5-hash and a SHA1-hash when verifying or signing data with RSA, but the .NET RSACryptoServiceProvider does not support that. |
|
I've made some more tests with npgsql to check how the notifications problem #270 is going with the current code. I can confirm that current master npgsql code indeed fixes the problem. I also could test if it is working in when SSL is on and using @Emill branch, it worked perfectly. Which means support for checking if there is data in the SSL stream implemented by @Emill is working as intended. Note that current master branch has problems with SSL code. It doesn't connect :-( |
|
Hey guys, sorry but I was out of the loop for a while... @Emill, that's pretty amazing work. Here are some thoughts:
|
|
Hi, @roji !
The only issue I see with that would be regarding the maintenance of the code. If later someone points out the code has an error or something like that, what would we do if @Emill isn't around? :) On the other side, it would be very nice if sslstream could have something like the dataavailable feature. I don't know how easy this would be to implement in microsoft's sslstream and if they would accept a patch for this.
I'm still more inclined to include the code inside Npgsql for the only reason of removing the dependency of Mono.Security or any other library. In fact, Npgsql already contains md5 code for authentication we got from Mono project. If Mono.Security code was easily "importable", I think today we would have it inside Npgsql as well. :) I also have to admit that I don't know very well how is the current deploying scenario. I'm basing myself in the past where developers and administrators would have to install the libraries manually and also on the reports we received to remove the dependency on other libraries to facilitate Npgsql deployment.
I also think this is very critical. I think @Emill has a very good point as I think very few users would have osx as hosts for postgresql connecting with ssl. But I also think it wouldn't be nice to release a version lacking this support. :( |
|
Actually I set up a copy here: https://github.com/Emill/TlsClientStream
Ok, I'll add support for TLS 1.0 (OpenSSL implemented 1.1 and 1.2 at the same time). |
|
Ok, I have now implemented support for TLS 1.0 and 1.1 in my branch :) |
|
Hi, @Emill ! Just tested it here and it gives me this error: And this is what I get in the server logs: I hope this information is useful. |
|
Hmm. It worked on all servers using tls 1.0 at least that I tested...
|
|
Hi again. I downloaded both the EnterpriseDB installer version of |
|
I also tried to compile from source, and it indeed used the system version of openssl then (that only supports TLS 1.0). However, it worked perfectly... |
|
Hi, @Emill ! I compile from source downloading the source code from postgresql.org. I pass the option --with-ssl to ./configure when compiling. I'll try again when I get back home and let you know what I get. |
|
I found two other problems with how the ssl/tls stream is managed by Npgsql.
|
|
Hi, @Emill . I tested it here and it worked perfectly! |
Agreed.
I think there is no problem with this inversion. |
|
@Emill pretty much finished this. Keeping open since we want to move the setting of ssl_renegotiation_limit=0 to the startup packet, this way DISCARD ALL won't reset it to 512MB. This also has the performance advantage of eliminating an additional roundtrip on connection. The only issue is older databases which don't support this parameter (e.g. redshift), meaning we have no choice but to go back to a CompatibilityMode=redshift connection string parameter. |
|
Moved ssl_renegotiation_limit to the startup packet in de716be. |
|
Could all this be behind the troubles I'm having with CngKeyBlobFormat.cs (#962)? (Sorry for the dumb questions — you all understand this stuff far better than I do.) |
|
@JoeStrout nope, this is totally unrelated to your issue. |
This issue summarizes open questions on SSL, following conversations on gitter with @Emill and other existing issues.
One (extreme?) option to address the above, is to drop the built-in SslStream and move to another implementation.
Following the open sourcing of .NET, Mono seem to be actively working on replacing their SslStream implementation with Microsoft's. If and when that happens, problems 1 and 2 will go away.
Unless I've missed other problems/concerns, my vote would be to keep SslStream as is. We would need to find a solution for problem 3, but aside from that I see the 1 and 2 as outside of Npgsql's problem field (and probable to go away soon anyway).
If we do decide to keep SslStream, we need to do two things:
The text was updated successfully, but these errors were encountered: