[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1185 I cross my fingers so it passes the tests. I want to continue coding but I don't want to do it until we have this issue merged. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1185 My guess is that the python tests are no longer correct, or the method of making python use SSLv23 in the code we merged is wrong. What I would suggest is backing out the last patch db9ac77f1e9c01ae57a9c6df48cb8670e9045ebb which changed the python stuff. I have cherry picked that into THRIFT-4084 which I am working on so you can drop it. I will send you another PR shortly which eliminates this non-c_glib stuff from your effort and passes both "make check" and "make cross" locally. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1185 It seems it still not fine... Do I have to see something? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1185 Hi again, As the code is pure c we should be able to support all configurations. It makes me a little surprised to mix mechanism in handshake and negotiation but as far as you know what you are requesting is ok. I'm working passing some certifications PCI, VISA and Master and none of them allows SSL3 to run. Even for handshake. This is the reason why we stick to TLS_1.2. I will check the code a little bit for this way of doing when I have a time slot today. Is there a predefined server that does this way. I suppose that Java is already doing and suppose it's already working since some tests are passing. Can you confirm, please? Thank you a lot for your effort again. Best regards, --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1185 @nsuke something like this seems to work in the ubuntu docker image: ``` has_secure_ssl_handshake = 0 try: from OpenSSL.SSL import OP_NO_SSLv3 has_secure_ssl_handshake = 1 except ImportError: has_secure_ssl_handshake = 0 then later... # SSL 2.0 and 3.0 are disabled via ssl.OP_NO_SSLv2 and ssl.OP_NO_SSLv3 if (has_secure_ssl_handshake): _default_protocol = ssl.PROTOCOL_SSLv23 else: _default_protocol = ssl.PROTOCOL_TLSv1 ``` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1185 I opened THRIFT-4084 as a mechanism to verify all the SSL server implementations are secure "enough" at their default settings. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1185 @gadLinux The current standard for thrift is to use the SSLv23 handshake, but to negotiate at TLSv1.0 or later. See Apache Jira https://issues.apache.org/jira/browse/THRIFT-3165 for that. Code to be checked in today for SSL support should use "SSLTLS" and disable SSL v2 and v3 negotiation which is what I did. So no, we cannot change it to anything else than what I put in my 3rd PR to you. As a consuming application one should be able to set the accepted version. I know you can do this in C++ and in Java because the constructors to TSSLSocket take an enumeration. In your implementation here how can we change it so that when a thrift_ssl_socket class is initialized, the consuming application can decide the SSL versions to support? I'm not sure this implementation allows for that control as-is. @nsuke I think I found a way to make the code conditional on OP_NO_SSLv3 support instead of a Python version. I'll post once I get the syntax right. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1185 Seems it's still failing. Do you think I can modify the back the protocol and use SSL2-3 for the tests? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1185 Hi, I must say that TLS1.2 is the recommended version for SSL because SSL3 is so buggy and insecure that everyone is filtering it. For testing can be ok. I would not change the default one. I would change just for the test. I suppose I have to write a brief documentation to let people know how to do it. i will do in my blog but I can send the patches for the apache web. One question? Do you sleep? Cause I have responses at all hours! You are so great. Thank you so much. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user nsuke commented on the issue: https://github.com/apache/thrift/pull/1185 @jeking3 in that case, Ubuntu might be using a patched version of Python. According to the [doc](https://docs.python.org/2/library/ssl.html#ssl.OP_NO_SSLv2), it's added in 2.7.9. > ssl.OP_NO_SSLv2 > Prevents an SSLv2 connection. This option is only applicable in conjunction with PROTOCOL_SSLv23. It prevents the peers from choosing SSLv2 as the protocol version. > New in version 2.7.9. We may want to directly check the existence of `OP_NO_SSLLv*` instead of current blanket version check. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1185 @gadLinux sent you a pull request and changed to use the proper validation cert @nsuke pointed out as part of it --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1185 @nsuke I am using the ubuntu docker image with python 2.7.6 in it and I changed the ssl server code in python to use SSLv23 unconditionally as a test and ran make cross, and everything passes: ``` diff --git a/lib/py/src/transport/TSSLSocket.py b/lib/py/src/transport/TSSLSocket.py index 0f7d45e9..86968081 100644 --- a/lib/py/src/transport/TSSLSocket.py +++ b/lib/py/src/transport/TSSLSocket.py @@ -40,13 +40,8 @@ class TSSLBase(object): # ciphers argument is not available for Python < 2.7.0 _has_ciphers = sys.hexversion >= 0x020700F0 -# For pythoon >= 2.7.9, use latest TLS that both client and server -# supports. -# SSL 2.0 and 3.0 are disabled via ssl.OP_NO_SSLv2 and ssl.OP_NO_SSLv3. -# For pythoon < 2.7.9, use TLS 1.0 since TLSv1_X nor OP_NO_SSLvX is -# unavailable. -_default_protocol = ssl.PROTOCOL_SSLv23 if _has_ssl_context else \ -ssl.PROTOCOL_TLSv1 +# SSL 2.0 and 3.0 are disabled via ssl.OP_NO_SSLv2 and ssl.OP_NO_SSLv3 +_default_protocol = ssl.PROTOCOL_SSLv23 def _init_context(self, ssl_version): if self._has_ssl_context: ``` By specifying PROTOCOL_TLSv1 it breaks compatibility with the universal SSLv3 hello handshake from which SSL determines the best protocol. This is the reason why the csharp and d servers are not compatible with all the other clients right now. PROTOCOL_SSLv23 is the best choice. The python doesn't break saying the OP_NO_SSLv* are not defined. I looked at the .NET documentation for the csharp SslProtocol enumeration. I don't see one that allows for the SSLv23 handshake AND disables negotiation of SSLv2 or SSLv3. There is a story in the backlog I originated to push every SSL implementation by default to negotiate only at TLSv1_2 by default, and the consuming application can then decide if it wants to relax that. This would make consumption of thrift safer. When we do that story, this SSLv23 hello handshake issue will no longer be an issue for the CI build / make cross. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user nsuke commented on the issue: https://github.com/apache/thrift/pull/1185 @jeking3 I can confirm that our C# and Python2 doesn't work for TLS 1.1/1.2. For Python2, it requires Python >= 2.7.9 which includes a lot of security API updates. Only Debian image has this, among our CI images. For C#, Mono version itself was actually sufficiently new, if I remember correctly. We would only need to drop .NET 3.5 compatibility. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1185 I've also found that the csharp and the d interop for ssl have been almost completely disabled by the known_test_failures file. In looking at the csharp implementation it does not accept SSLv3 at all, which means it won't accept the more universal SSLv23 handshake after which it bumps up to TLSv1_2. This is probably why all of them are disabled; I suspect the "d" server is affected the same way. I'm okay adding the c_glib failing tests to these blocks for now. I'll send you another PR that has make cross passing. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1185 One issue I found was that TLSv1_2 was being specified. We use SSLv23 with SSLv2 and SSLv3 disabled; this allows OpenSSL to negotiate at the highest possible level including TLSv1.2. So I changed the c_glib ssl client socket code to use SSLTLS as the default protocol (which is SSLv23 as described above). At that point I expected it to interop with the python ssl server in make cross. The docker image has python 2.7.6 and there was some conditional code in the python server class that seems to be restricting it to TLSv1_0 specifically. I set it to use the same logic (SSLv23 with v2 and v3 disabled). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1185 This could be issues in the other implementations. While cpp and java passed ssl with c_glib, others like d and csharp did not. I have a docker instance running here, so I will run the full suite and try to figure out what happened. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1185 Oh, it failed again. How can this be? Should I rebase again? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1185 jeking3 Thank you a lot for your patches. They seem to fix the problem you found. But I don't really know why so I will investigate it a little bit after. I want to run my own tests anyway because I'm not sure if pinning will work well like this. About nsuke concerns, the code can load the CA certificate as well. I will try later to see if it helps. For now it seems lot better. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1185 I sent you a pull request that fixes the c_glib library and allows the ssl make cross tests to pass with a c_glib ip-sssl client against cpp and java servers. Didn't test the others, we'll let CI do that. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1185 Given we have a failing cross test, merging it doesn't make sense because the evidence suggests it doesn't work. I'll see if I can resolve it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1185 Let's fix it latter, please. I'm tired of getting this issue around. I will open a bug report if you want to track it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1185 Interesting failure now. The non-ssl testing against java and cpp work properly but the ssl variant seems to be skewed / off by one. When testVoid() is sent, testString() is returned. Everything is off by one. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1185 I forgot to say that also the code was rebased. A curious thing is that everytime I do a rebase the system says that my code diverged... I don't know why since always rebasing the upstream master branch. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1185 Okay. I revamped a little bit the system to allow it to run the cross tests. Since you are not using specific functions provided by the API it was causing to not initialize the instance properly. I added sane default values for SSL. It will use TLSv12 by default with dissallowed selfsigned certificates and basic pining function. So I also changed the test unit to allow selfsigned certificates when the client is created. Now the tests run but fails. But there I don't what's the reason. It's because Java, because the test is wrong, or the code in SSL branch is wrong. The latter seems difficult since it works in our environment. So maybe you can help to fix the tests. The important thing is that now it seems to be properly configured even if no API provided is used. Also a couple of straightforward errors on test were corrected. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1185 @gadLinux I submitted a pull request against your branch (https://github.com/gadLinux/thrift/pull/1) to put the cross testing code in place; it is seg faulting so hopefully you can figure out why in short order. With that in place, c_glib ssl will be tested against every server that supports ssl as part of the build. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user Jens-G commented on the issue: https://github.com/apache/thrift/pull/1185 We are in middle of the transition process from automake to cmake. Until we completed this task, both are relevant. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1185 autoconf is deprecated and there is an epic in the Jira backlog to remove it in favor of cmake. I agree maintaining two build systems is a bit painful. I believe it is possible to have "make cross" do a client-only transport. I'm going to take your pull request and see if I can enable cross test on the ip-ssl transport and run it in the docker container like the build would. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1185 Just a question. Why all CMake stuff. Having two compilation environments seems to me somewhat overloaded... Does it worth it? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1185 Hi Jeking3, Yes there's only the client at this time. Cause I had no time to implement server. The tests can be run against Java or any other server. I will add after this commit is approved. The tests I added are rather limited because the lack of c_glib server. So yes I will add them at latter step. About cross checks. It's the first time I see about it. In fact all tests I did where against Java so they are naturally cross language. Again this should be added at a latter step. I need the client to be "in" the next release cause lot of effort has been set securing it. So the patch is big enough to split it in different issues. We can discuss later about a common library between C++ and c_glib, more testing, server and many other improvements. But I think it's a good idea to have this in now that's running. In our case is in production. Tested daily against hundreds of clients. And it's working fine. Can you merge it as is, please? And prepare after in the maillist the next features. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1185 Thank you for the effort --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1185: Thrift 3369 - Third try to merge SSL c_glib
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1185 Thanks, I'll wait for the CI build results before I comment. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
