Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.6
    • Component/s: Java - Library
    • Labels:
      None
    • Environment:

      n/a

      Description

      SSL Connection w/ autogenerated self signed x509 certs seems to be the state of the art for rpc layers.

      if thrift had one ...that would be very good.
      http://java.sun.com/javase/6/docs/technotes/guides/security/jsse/JSSERefGuide.html

      if someone does this pls ping/email me, I will do some testing and write a simple key mgmt utility.

      1. TSSLTrasportFactory.patch
        28 kB
        Nirmal Ranganathan
      2. Sample-keystore-for-tests-and-tutorial.patch
        3 kB
        Nirmal Ranganathan
      3. java-ssl.patch
        2 kB
        David Reiss
      4. ssl.patch
        32 kB
        Ian Pye

        Issue Links

          Activity

          Hide
          Eric Faccer added a comment - - edited

          Note to Bryan Duxbury: This code actually works fine. ...The issue was getting keys mixed up. Will cleanup/re-up it

          Show
          Eric Faccer added a comment - - edited Note to Bryan Duxbury: This code actually works fine. ...The issue was getting keys mixed up. Will cleanup/re-up it
          Hide
          David Reiss added a comment -

          Am I the only one who can't see the attachments in JIRA?

          Show
          David Reiss added a comment - Am I the only one who can't see the attachments in JIRA?
          Hide
          Bryan Duxbury added a comment -

          They were deleted.

          Show
          Bryan Duxbury added a comment - They were deleted.
          Hide
          Ian Pye added a comment -

          Here's a patch I've been working on which implements TSSLSocket and TSSLServerSocket classes in c++ – I wasn't quite sure whether to send it here or start a new issue in the C++ category.

          All of the IO is done via OpenSSL's BIO abstraction.

          Currently both classes expect to be given .pem files which contain the X509 certs to use. TSSLServerSocket does not handle encrypted private keys, but it would be fairly easy to add this feature in.

          Show
          Ian Pye added a comment - Here's a patch I've been working on which implements TSSLSocket and TSSLServerSocket classes in c++ – I wasn't quite sure whether to send it here or start a new issue in the C++ category. All of the IO is done via OpenSSL's BIO abstraction. Currently both classes expect to be given .pem files which contain the X509 certs to use. TSSLServerSocket does not handle encrypted private keys, but it would be fairly easy to add this feature in.
          Hide
          Ian Pye added a comment -

          New version of this patch which fixes SSL initialization errors. It also takes some socket settings from TServerSocket.cpp.

          Show
          Ian Pye added a comment - New version of this patch which fixes SSL initialization errors. It also takes some socket settings from TServerSocket.cpp.
          Hide
          David Reiss added a comment -

          How does this compare to the gnutls-based patch that you submitted to the old Thrift list?

          Show
          David Reiss added a comment - How does this compare to the gnutls-based patch that you submitted to the old Thrift list?
          Hide
          Ian Pye added a comment -

          Same functionality, totally different implementation.

          The new patch uses OpenSSL entirely and does not use gnutls at all.

          Also, Instead of subclassing the T*Socket classes, this is built directly up from the T*Transport interfaces.

          Show
          Ian Pye added a comment - Same functionality, totally different implementation. The new patch uses OpenSSL entirely and does not use gnutls at all. Also, Instead of subclassing the T*Socket classes, this is built directly up from the T*Transport interfaces.
          Hide
          David Reiss added a comment -

          Would you suggest that we abandon the gnutls implementation in favor of this one then? Are they wire-compatible (or can they be made so)? Are any of these implementations compatible with wrapping a normal Thrift server in stunnel?

          Show
          David Reiss added a comment - Would you suggest that we abandon the gnutls implementation in favor of this one then? Are they wire-compatible (or can they be made so)? Are any of these implementations compatible with wrapping a normal Thrift server in stunnel?
          Hide
          Ian Pye added a comment -

          1) I switched to working with openssl because I was having a hard time getting the memory allocation working 100% reliably with gnutls and eventually got fed up with having hard to track down warnings about double free()s and such coming up all the time. I find openssl's BIO abstraction much easier to work with (This may say more about me than about openssl though). Also, just on the basis of my very ad-hoc tests, openssl seems to be faster at setting up a secure connection than gnutls. Other reasons I switched include the popularity of openssl, and the fact this openssl's license is a bit more lax, just requiring citing the use of openssl in the linking source code (I believe).

          So, I don't know where you are at with the gnutls implementation and how stable it is, but my general opinion is that openssl is a more mature project which is a lot more fun to code against.

          2) My understanding is that gnutls can emulate openssl, but not vice versa. With this emulation going, the two libraries are wire-compatible. One limitation of openssl is that it doesn't support OpenPGP authentication.

          3) stunnel compiles against both SSLeay and OpenSSL. So a openssl enable thrift client could talk with a stunnel'd thrift server, and vice versa. But since gnutls can emulate openssl, it should also be able to interoperate with stunnel.

          Show
          Ian Pye added a comment - 1) I switched to working with openssl because I was having a hard time getting the memory allocation working 100% reliably with gnutls and eventually got fed up with having hard to track down warnings about double free()s and such coming up all the time. I find openssl's BIO abstraction much easier to work with (This may say more about me than about openssl though). Also, just on the basis of my very ad-hoc tests, openssl seems to be faster at setting up a secure connection than gnutls. Other reasons I switched include the popularity of openssl, and the fact this openssl's license is a bit more lax, just requiring citing the use of openssl in the linking source code (I believe). So, I don't know where you are at with the gnutls implementation and how stable it is, but my general opinion is that openssl is a more mature project which is a lot more fun to code against. 2) My understanding is that gnutls can emulate openssl, but not vice versa. With this emulation going, the two libraries are wire-compatible. One limitation of openssl is that it doesn't support OpenPGP authentication. 3) stunnel compiles against both SSLeay and OpenSSL. So a openssl enable thrift client could talk with a stunnel'd thrift server, and vice versa. But since gnutls can emulate openssl, it should also be able to interoperate with stunnel.
          Hide
          David Reiss added a comment -

          Thanks for the additional info. The big negative of OpenSSL in my mind is the so-called "obnoxious advertising clause", which requires that products acknowledge OpenSSL that in any advertising material that mentions features provided by OpenSSL. However, I think ease of coding trumps that. Would you mind creating a separate JIRA issue for the C++ implementation?

          Show
          David Reiss added a comment - Thanks for the additional info. The big negative of OpenSSL in my mind is the so-called "obnoxious advertising clause", which requires that products acknowledge OpenSSL that in any advertising material that mentions features provided by OpenSSL. However, I think ease of coding trumps that. Would you mind creating a separate JIRA issue for the C++ implementation?
          Hide
          Jeremy Hanna added a comment -

          what is the status of this ticket - is there something that needs updating before the patch can be used or is there something wrong with the approach or ???

          Show
          Jeremy Hanna added a comment - what is the status of this ticket - is there something that needs updating before the patch can be used or is there something wrong with the approach or ???
          Hide
          Bryan Duxbury added a comment -

          The patch contains nothing but C++ code, so I don't know why it's tagged for java.

          Show
          Bryan Duxbury added a comment - The patch contains nothing but C++ code, so I don't know why it's tagged for java.
          Hide
          Jeremy Hanna added a comment -

          Ah right - sorry. I should have looked at the patch itself. So for java, there is no progress, it has to be done from the ground up.

          Show
          Jeremy Hanna added a comment - Ah right - sorry. I should have looked at the patch itself. So for java, there is no progress, it has to be done from the ground up.
          Hide
          David Reiss added a comment -

          I think we have an experimental Java implementation here at Facebook. I can try to get it out if people are interested, but I haven't tried yet because it was never extensively tested and there are no plans to use it in production.

          Show
          David Reiss added a comment - I think we have an experimental Java implementation here at Facebook. I can try to get it out if people are interested, but I haven't tried yet because it was never extensively tested and there are no plans to use it in production.
          Hide
          Jeremy Hanna added a comment -

          David - that would great if you wouldn't mind posting it. Thank you.

          Show
          Jeremy Hanna added a comment - David - that would great if you wouldn't mind posting it. Thank you.
          Hide
          Jeremy Hanna added a comment -

          David - are you able to post the experimental Java implementation - we're looking to start looking at implementing it here. Would be great to have a starting point.

          Show
          Jeremy Hanna added a comment - David - are you able to post the experimental Java implementation - we're looking to start looking at implementing it here. Would be great to have a starting point.
          Hide
          James E. King, III added a comment -

          The patch for Endpoints in THRIFT-66 is being ported to Java. This will provide SSL with self-signed certificate authentication. In fact each service channel can be declared as accessible to unauthenticated or authenticated clients. I know this doesn't help you immediately, but I am working on getting this moved over for discussion and adoption by the devs.

          Show
          James E. King, III added a comment - The patch for Endpoints in THRIFT-66 is being ported to Java. This will provide SSL with self-signed certificate authentication. In fact each service channel can be declared as accessible to unauthenticated or authenticated clients. I know this doesn't help you immediately, but I am working on getting this moved over for discussion and adoption by the devs.
          Hide
          David Reiss added a comment -

          Here it is. It looks like there was never a client implementation. This underwent basic testing, but was never used in production.

          Show
          David Reiss added a comment - Here it is. It looks like there was never a client implementation. This underwent basic testing, but was never used in production.
          Hide
          Jeremy Hanna added a comment -

          David and James - Thanks!

          Show
          Jeremy Hanna added a comment - David and James - Thanks!
          Hide
          Nirmal Ranganathan added a comment -

          Here's a TSSLTransportFactory that wraps TServerSocket and TSocket with SSL/TLS enabled underlying sockets and server sockets. I've also attached sample self-signed certificates and keys.
          This is only a blocking version and doesn't work for TNonBlockingTransport.
          The tutorial/java is also updated to show a working example.

          Show
          Nirmal Ranganathan added a comment - Here's a TSSLTransportFactory that wraps TServerSocket and TSocket with SSL/TLS enabled underlying sockets and server sockets. I've also attached sample self-signed certificates and keys. This is only a blocking version and doesn't work for TNonBlockingTransport. The tutorial/java is also updated to show a working example.
          Hide
          Jeremy Hanna added a comment -

          Bryan: The code looks good and I applied against thrift trunk and ran the new tutorial - JavaClient secure - works great. So looks good to me.

          Show
          Jeremy Hanna added a comment - Bryan: The code looks good and I applied against thrift trunk and ran the new tutorial - JavaClient secure - works great. So looks good to me.
          Hide
          Bryan Duxbury added a comment -

          A few comments:

          • The spacing is off. We use 2-space indentation.
          • The addition of getServerSocket to TServerSocket seems unnecessary, since it's not used anywhere.
          • Why override TSocket.open() at all in TSSLSocket? TSocket throws an exception if it's already open, but TSSLSocket will silently continue. Is this a necessary/intended semantic change? If we don't need that change, do we need a TSSLSocket at all?
          • TSSLServerSocket also seems unnecessary.
          • Would it be possible to get a separate unit test for TSSLSocketFactory? Maybe just set up a simple server-client pair and push some bytes around? The tutorial stuff is great, but it's not going to be part of our standard test suite, so we could break it by accident.
          • Hate to sounds pedantic, but do you mind using ifs with curly braces, even though the then clause is single-line? Just for readability/consistency.
          Show
          Bryan Duxbury added a comment - A few comments: The spacing is off. We use 2-space indentation. The addition of getServerSocket to TServerSocket seems unnecessary, since it's not used anywhere. Why override TSocket.open() at all in TSSLSocket? TSocket throws an exception if it's already open, but TSSLSocket will silently continue. Is this a necessary/intended semantic change? If we don't need that change, do we need a TSSLSocket at all? TSSLServerSocket also seems unnecessary. Would it be possible to get a separate unit test for TSSLSocketFactory? Maybe just set up a simple server-client pair and push some bytes around? The tutorial stuff is great, but it's not going to be part of our standard test suite, so we could break it by accident. Hate to sounds pedantic, but do you mind using ifs with curly braces, even though the then clause is single-line? Just for readability/consistency.
          Hide
          Nirmal Ranganathan added a comment -

          The spacing is off. We use 2-space indentation.

          I'll clean that up.

          The addition of getServerSocket to TServerSocket seems unnecessary, since it's not used anywhere.

          I added it for 2 reasons, one is similarity to TSocket.getSocket and the other if anyone want's to override TServerSocket, it would be nice to have the underlying socket. Case in point we would require something like that for Cassandra.

          Why override TSocket.open() at all in TSSLSocket? TSocket throws an exception if it's already open, but TSSLSocket will silently continue. Is this a necessary/intended semantic change? If we don't need that change, do we need a TSSLSocket at all?

          TSSLServerSocket also seems unnecessary.

          That was just an idiomatic addition. Since everyone is used to Transport.open(). The override ignores the call if it's already open. I can remove it, if you feel that's unnecessary.

          Would it be possible to get a separate unit test for TSSLSocketFactory? Maybe just set up a simple server-client pair and push some bytes around? The tutorial stuff is great, but it's not going to be part of our standard test suite, so we could break it by accident.

          Do we require something more than the current TestTSSLSocketFactory that I added? It does use the underlying ServerTestBase for the tests, but uses a TSSLSocket and TSSLServerSocket from the TSSLSocketFactory as a simple client-server pair. Let me know if I misunderstood something there.

          Hate to sounds pedantic, but do you mind using ifs with curly braces, even though the then clause is single-line? Just for readability/consistency.

          Sure, I'll update it

          Show
          Nirmal Ranganathan added a comment - The spacing is off. We use 2-space indentation. I'll clean that up. The addition of getServerSocket to TServerSocket seems unnecessary, since it's not used anywhere. I added it for 2 reasons, one is similarity to TSocket.getSocket and the other if anyone want's to override TServerSocket, it would be nice to have the underlying socket. Case in point we would require something like that for Cassandra. Why override TSocket.open() at all in TSSLSocket? TSocket throws an exception if it's already open, but TSSLSocket will silently continue. Is this a necessary/intended semantic change? If we don't need that change, do we need a TSSLSocket at all? TSSLServerSocket also seems unnecessary. That was just an idiomatic addition. Since everyone is used to Transport.open(). The override ignores the call if it's already open. I can remove it, if you feel that's unnecessary. Would it be possible to get a separate unit test for TSSLSocketFactory? Maybe just set up a simple server-client pair and push some bytes around? The tutorial stuff is great, but it's not going to be part of our standard test suite, so we could break it by accident. Do we require something more than the current TestTSSLSocketFactory that I added? It does use the underlying ServerTestBase for the tests, but uses a TSSLSocket and TSSLServerSocket from the TSSLSocketFactory as a simple client-server pair. Let me know if I misunderstood something there. Hate to sounds pedantic, but do you mind using ifs with curly braces, even though the then clause is single-line? Just for readability/consistency. Sure, I'll update it
          Hide
          Nirmal Ranganathan added a comment -

          My bad! I thought I'd attached the test case. Here it is now.

          Show
          Nirmal Ranganathan added a comment - My bad! I thought I'd attached the test case. Here it is now.
          Hide
          Bryan Duxbury added a comment -

          The addition of getServerSocket to TServerSocket seems unnecessary, since it's not used anywhere.

          I added it for 2 reasons, one is similarity to TSocket.getSocket and the other if anyone want's to override TServerSocket, it would be nice to have the underlying socket. Case in point we would require something like that for Cassandra.

          OK, that's fine by me.

          I think I'd prefer not to have those weak wrapper classes since they offer so little functionality. It doesn't seem particularly useful to change the semantics of isOpen(), so the rest is just an unnecessary class.

          Glad to see you wrote a test

          Just as a side note, it would be slightly more convenient for me if you attached a combined patch instead of several in the future.

          Show
          Bryan Duxbury added a comment - The addition of getServerSocket to TServerSocket seems unnecessary, since it's not used anywhere. I added it for 2 reasons, one is similarity to TSocket.getSocket and the other if anyone want's to override TServerSocket, it would be nice to have the underlying socket. Case in point we would require something like that for Cassandra. OK, that's fine by me. I think I'd prefer not to have those weak wrapper classes since they offer so little functionality. It doesn't seem particularly useful to change the semantics of isOpen(), so the rest is just an unnecessary class. Glad to see you wrote a test Just as a side note, it would be slightly more convenient for me if you attached a combined patch instead of several in the future.
          Hide
          Nirmal Ranganathan added a comment -

          That sounds good. I'll wrap up with the suggested changes (removing TSSLSocket and TSSLServerSocket) and consolidate everything into one patch file. I'll just keep the keystores as a separate patch though.

          Show
          Nirmal Ranganathan added a comment - That sounds good. I'll wrap up with the suggested changes (removing TSSLSocket and TSSLServerSocket) and consolidate everything into one patch file. I'll just keep the keystores as a separate patch though.
          Hide
          Nirmal Ranganathan added a comment -

          Consolidated to one patch file and updated based on comments.

          Show
          Nirmal Ranganathan added a comment - Consolidated to one patch file and updated based on comments.
          Hide
          Bryan Duxbury added a comment -

          I saw a note in TSSLTransportFactory that TCompactProtocol isn't supported. Is that true? If so, why?

          Show
          Bryan Duxbury added a comment - I saw a note in TSSLTransportFactory that TCompactProtocol isn't supported. Is that true? If so, why?
          Hide
          Bryan Duxbury added a comment -

          I just committed this patch. Thanks for all the hard work, Nirmal!

          Show
          Bryan Duxbury added a comment - I just committed this patch. Thanks for all the hard work, Nirmal!

            People

            • Assignee:
              Nirmal Ranganathan
              Reporter:
              rico sec
            • Votes:
              2 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 6h
                6h
                Remaining:
                Remaining Estimate - 6h
                6h
                Logged:
                Time Spent - Not Specified
                Not Specified

                  Development