Thrift
  1. Thrift
  2. THRIFT-151

TSSLServerSocket and TSSLSocket implementation

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.7
    • Component/s: C++ - Library
    • Labels:
      None

      Description

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

      It would be good if there was a C++ implementation of TSocket and TServerSocket classes.

      This is similar to the Java issue Thrift 106.

      1. thrift-0.7.0-cpp-ssl-mac.patch
        42 kB
        Rowan Kerr
      2. thrift-0.6.0-cpp-ssl.patch
        50 kB
        Kevin Worth
      3. TSSLSocket.cpp.diff
        10 kB
        Vlad Galu
      4. TSSLSocket.h.diff
        4 kB
        Vlad Galu
      5. Thrift.zip
        10 kB
        Vlad Galu
      6. ssl-redesigned.patch
        40 kB
        Ping Li
      7. ssl-test-pingli.patch
        10 kB
        Ping Li
      8. ssl-pingli.patch
        48 kB
        Ping Li
      9. ssl.patch
        40 kB
        Ian Pye

        Issue Links

          Activity

          Hide
          David Reiss added a comment -

          I think the right thing to do in that case is to just use a c-style cast. It's semantics are a bit complicated, but I'm pretty sure it does the right thing.

          Show
          David Reiss added a comment - I think the right thing to do in that case is to just use a c-style cast. It's semantics are a bit complicated, but I'm pretty sure it does the right thing.
          Hide
          Rowan Kerr added a comment -

          Seems we need a platform specific wrapper for the cast in callbackThreadID. reinterpret for Mac, static for Linux...

          Show
          Rowan Kerr added a comment - Seems we need a platform specific wrapper for the cast in callbackThreadID. reinterpret for Mac, static for Linux...
          Hide
          Bryan Duxbury added a comment -

          New version works for me, so I committed it. Thanks all who contributed!

          Show
          Bryan Duxbury added a comment - New version works for me, so I committed it. Thanks all who contributed!
          Hide
          Rowan Kerr added a comment -

          Rerolled Kevin's 0.6 updated patch against trunk (pre-0.7) with a couple fixes for Mac compiling.

          Show
          Rowan Kerr added a comment - Rerolled Kevin's 0.6 updated patch against trunk (pre-0.7) with a couple fixes for Mac compiling.
          Hide
          Rowan Kerr added a comment -

          No it's fine besides that. I applied it and am re-adding the bits that sorted out building on Mac for me. Will re-up in a minute.

          Show
          Rowan Kerr added a comment - No it's fine besides that. I applied it and am re-adding the bits that sorted out building on Mac for me. Will re-up in a minute.
          Hide
          Kevin Worth added a comment -

          You can just apply with patch -p1 to ignore the top-level directories and perform the patch in-place. Is there something besides that that is "funky" ?

          Show
          Kevin Worth added a comment - You can just apply with patch -p1 to ignore the top-level directories and perform the patch in-place. Is there something besides that that is "funky" ?
          Hide
          Rowan Kerr added a comment -

          Can you re-roll the patch against trunk? Got some funky paths in it.

          Show
          Rowan Kerr added a comment - Can you re-roll the patch against trunk? Got some funky paths in it.
          Hide
          Rowan Kerr added a comment -

          unsigned long worked for me when I updated the client socket to build on Mac (based on the older "redesigned" patch). Also, had to change the static_cast from callbackThreadID to reinterpret_cast.

          Show
          Rowan Kerr added a comment - unsigned long worked for me when I updated the client socket to build on Mac (based on the older "redesigned" patch). Also, had to change the static_cast from callbackThreadID to reinterpret_cast.
          Hide
          Kevin Worth added a comment -

          Sounds like the ulong typedef isn't present on Mac. Suspect replacing instances of "ulong" with "unsigned long" would be safer or else could "typedef unsigned long ulong" above the offending line to see if that resolves it.

          Show
          Kevin Worth added a comment - Sounds like the ulong typedef isn't present on Mac. Suspect replacing instances of "ulong" with "unsigned long" would be safer or else could "typedef unsigned long ulong" above the offending line to see if that resolves it.
          Hide
          Bryan Duxbury added a comment -

          I applied this patch and tried to build it, but got the following:

          /bin/sh ../../libtool  --tag=CXX   --mode=compile g++ -DHAVE_CONFIG_H -I. -I../..  -I/opt/local/include -I./src  -Wall -g -O2 -MT TSSLSocket.lo -MD -MP -MF .deps/TSSLSocket.Tpo -c -o TSSLSocket.lo `test -f 'src/transport/TSSLSocket.cpp' || echo './'`src/transport/TSSLSocket.cpp
          libtool: compile:  g++ -DHAVE_CONFIG_H -I. -I../.. -I/opt/local/include -I./src -Wall -g -O2 -MT TSSLSocket.lo -MD -MP -MF .deps/TSSLSocket.Tpo -c src/transport/TSSLSocket.cpp  -fno-common -DPIC -o .libs/TSSLSocket.o
          src/transport/TSSLSocket.cpp:480: error: ‘ulong’ does not name a type
          src/transport/TSSLSocket.cpp: In static member function ‘static void apache::thrift::transport::TSSLSocketFactory::initializeOpenSSL()’:
          src/transport/TSSLSocket.cpp:520: error: ‘callbackThreadID’ was not declared in this scope
          src/transport/TSSLSocket.cpp: In function ‘void apache::thrift::transport::buildErrors(std::string&, int)’:
          src/transport/TSSLSocket.cpp:550: error: ‘ulong’ was not declared in this scope
          src/transport/TSSLSocket.cpp:550: error: expected `;' before ‘errorCode’
          src/transport/TSSLSocket.cpp:554: error: ‘errorCode’ was not declared in this scope
          make[1]: *** [TSSLSocket.lo] Error 1
          make: *** [all-recursive] Error 1
          

          I'm on a Mac. Anything special I should need to do?

          Show
          Bryan Duxbury added a comment - I applied this patch and tried to build it, but got the following: /bin/sh ../../libtool --tag=CXX --mode=compile g++ -DHAVE_CONFIG_H -I. -I../.. -I/opt/local/include -I./src -Wall -g -O2 -MT TSSLSocket.lo -MD -MP -MF .deps/TSSLSocket.Tpo -c -o TSSLSocket.lo `test -f 'src/transport/TSSLSocket.cpp' || echo './'`src/transport/TSSLSocket.cpp libtool: compile: g++ -DHAVE_CONFIG_H -I. -I../.. -I/opt/local/include -I./src -Wall -g -O2 -MT TSSLSocket.lo -MD -MP -MF .deps/TSSLSocket.Tpo -c src/transport/TSSLSocket.cpp -fno-common -DPIC -o .libs/TSSLSocket.o src/transport/TSSLSocket.cpp:480: error: ‘ulong’ does not name a type src/transport/TSSLSocket.cpp: In static member function ‘ static void apache::thrift::transport::TSSLSocketFactory::initializeOpenSSL()’: src/transport/TSSLSocket.cpp:520: error: ‘callbackThreadID’ was not declared in this scope src/transport/TSSLSocket.cpp: In function ‘void apache::thrift::transport::buildErrors(std::string&, int )’: src/transport/TSSLSocket.cpp:550: error: ‘ulong’ was not declared in this scope src/transport/TSSLSocket.cpp:550: error: expected `;' before ‘errorCode’ src/transport/TSSLSocket.cpp:554: error: ‘errorCode’ was not declared in this scope make[1]: *** [TSSLSocket.lo] Error 1 make: *** [all-recursive] Error 1 I'm on a Mac. Anything special I should need to do?
          Hide
          Kevin Worth added a comment -

          Uploading Ping Li's ssl-redesigned.patch updated as thrift-0.6.0-cpp-ssl.patch so that it patches and compiles (includes Makefile stuff) against the thrift-0.6.0 release and tests properly against a Python SSL client (using patch in THRIFT-1068). Is there anything holding this improvement up from being committed? This issue has been around a looong time.

          Show
          Kevin Worth added a comment - Uploading Ping Li's ssl-redesigned.patch updated as thrift-0.6.0-cpp-ssl.patch so that it patches and compiles (includes Makefile stuff) against the thrift-0.6.0 release and tests properly against a Python SSL client (using patch in THRIFT-1068 ). Is there anything holding this improvement up from being committed? This issue has been around a looong time.
          Hide
          Rowan Kerr added a comment -

          Had to make open, close, read, write in TSocket.h virtuals to get Ping's patch to work for me. (Otherwise the _virt methods in TVirtualTransport were looking up the wrong implementations).

          Show
          Rowan Kerr added a comment - Had to make open, close, read, write in TSocket.h virtuals to get Ping's patch to work for me. (Otherwise the _virt methods in TVirtualTransport were looking up the wrong implementations).
          Hide
          Alexey Savartsov added a comment - - edited

          Hi Ping,

          I had a problem with that patch only when I used it to connect to Evernote. Test server/client based on this patches only was ok.
          The problem was reproducible, but I had no time to explore it so I just deleted second call to SSL_shutdown (just thinking that first SSL_shutdown notify other side that we're closing connection so the socket may be closed as far as connection will not be used again)

          void TSSLSocket::close() {
            if (ssl_ != NULL) {
              int rc = SSL_shutdown(ssl_);
              if (rc == 0) {
                rc = SSL_shutdown(ssl_); // <-- when I closed a transport my app hung here 
              }
              if (rc < 0) {
          

          But now I can't reproduce it. Maybe Evernote guys fixed something... or whatever, it works good as for now.

          Show
          Alexey Savartsov added a comment - - edited Hi Ping, I had a problem with that patch only when I used it to connect to Evernote. Test server/client based on this patches only was ok. The problem was reproducible, but I had no time to explore it so I just deleted second call to SSL_shutdown (just thinking that first SSL_shutdown notify other side that we're closing connection so the socket may be closed as far as connection will not be used again) – void TSSLSocket::close() { if (ssl_ != NULL) { int rc = SSL_shutdown(ssl_); if (rc == 0) { rc = SSL_shutdown(ssl_); // <-- when I closed a transport my app hung here } if (rc < 0) { – But now I can't reproduce it. Maybe Evernote guys fixed something... or whatever, it works good as for now.
          Hide
          Rowan Kerr added a comment -

          Looks like these patches require client certs?

          Show
          Rowan Kerr added a comment - Looks like these patches require client certs?
          Hide
          Vlad Galu added a comment -

          AFAIR the TSSLSocketFactory changes were just a hack for easier integration with our own code. You can safely disregard them. It's the TSSLSocket changes that mattered to us.

          Show
          Vlad Galu added a comment - AFAIR the TSSLSocketFactory changes were just a hack for easier integration with our own code. You can safely disregard them. It's the TSSLSocket changes that mattered to us.
          Hide
          Ping Li added a comment -

          i browsed very quickly. one thing i notice is you exposed the open ssl api and data structures to the client, that's what i tried to avoid.

          Show
          Ping Li added a comment - i browsed very quickly. one thing i notice is you exposed the open ssl api and data structures to the client, that's what i tried to avoid.
          Hide
          Vlad Galu added a comment -

          OK, I've attached the diffs between the old and the new TSSLSocket.h and TSSLSocket.cpp files.

          Looking at the diffs brings back some memories:

          • I split the SSLContext in two subclasses (client/server), to use SSLv3_client_method()/SSLv3_server_method())
          • I moved the SSL ctx cleanup in ~TSSLSocket()
          • moved checkHandshake() after TSocket::Open(), in TSSLSocket::open()
          • I'm not sure why I commented out the verification code in TSSLSocket::authorize(), though. Might be a leftover from when I was debugging
          • other API changes, which are quite irrelevant for the general use case, but helped keep our own code cleaner

          HTH,
          Vlad

          Show
          Vlad Galu added a comment - OK, I've attached the diffs between the old and the new TSSLSocket.h and TSSLSocket.cpp files. Looking at the diffs brings back some memories: I split the SSLContext in two subclasses (client/server), to use SSLv3_client_method()/SSLv3_server_method()) I moved the SSL ctx cleanup in ~TSSLSocket() moved checkHandshake() after TSocket::Open(), in TSSLSocket::open() I'm not sure why I commented out the verification code in TSSLSocket::authorize(), though. Might be a leftover from when I was debugging other API changes, which are quite irrelevant for the general use case, but helped keep our own code cleaner HTH, Vlad
          Hide
          Ping Li added a comment -

          Vlad,

          can you attach a diff? i'd like to see what you changed. btw, i removed access manager.

          Show
          Ping Li added a comment - Vlad, can you attach a diff? i'd like to see what you changed. btw, i removed access manager.
          Hide
          Vlad Galu added a comment -

          Hello Ping,

          Please review the changes I made to your redesigned patch. About a a year ago I tried using it for a project, but encountered some I/O issues. Unfortunately I don't quite remember what they were, but I do remember that my changes fixed them. In the attached archive there's also a Ruby wrapper for the TSSLSocket class.

          Show
          Vlad Galu added a comment - Hello Ping, Please review the changes I made to your redesigned patch. About a a year ago I tried using it for a project, but encountered some I/O issues. Unfortunately I don't quite remember what they were, but I do remember that my changes fixed them. In the attached archive there's also a Ruby wrapper for the TSSLSocket class.
          Hide
          Ping Li added a comment -

          Hi, Alex,

          Are you having the problem with the testing code in this patch or your own code? I didn't have this issue with the testing code. It'll be helpful if it can be reproduced.

          This patch was uploaded about a year ago. It was before the apache name space change. It was based on Facebook's internal version, not the public version (I had problems to build the public version, and gave up the conversion.)

          I'm not on the core Thrift team. This patch is for review/comment.

          Show
          Ping Li added a comment - Hi, Alex, Are you having the problem with the testing code in this patch or your own code? I didn't have this issue with the testing code. It'll be helpful if it can be reproduced. This patch was uploaded about a year ago. It was before the apache name space change. It was based on Facebook's internal version, not the public version (I had problems to build the public version, and gave up the conversion.) I'm not on the core Thrift team. This patch is for review/comment.
          Hide
          Alexey Savartsov added a comment -

          I've applied your 'redisigned' patch to thrift-0.2.0 and tried communicate with Evernote service today.
          I had to make some little changes to get the code compile successfully (changed facebook namespace to apache, local includes to system ("TSocket.h" to <transport/TSocket.h> and so on)), and it (I've used only client class) seems to work pretty good, except closing connection. TSSLSocket::close() hangs on second call to SSL_shutdown. I've just removed second call, but I have a question. As far as I know, according to TSL standard it is acceptable for an application to only send its shutdown alert and then close the underlying connection without waiting for the peer's response. Is it safe here? Or client or server side should be fixed?

          Show
          Alexey Savartsov added a comment - I've applied your 'redisigned' patch to thrift-0.2.0 and tried communicate with Evernote service today. I had to make some little changes to get the code compile successfully (changed facebook namespace to apache, local includes to system ("TSocket.h" to <transport/TSocket.h> and so on)), and it (I've used only client class) seems to work pretty good, except closing connection. TSSLSocket::close() hangs on second call to SSL_shutdown. I've just removed second call, but I have a question. As far as I know, according to TSL standard it is acceptable for an application to only send its shutdown alert and then close the underlying connection without waiting for the peer's response. Is it safe here? Or client or server side should be fixed?
          Hide
          Ping Li added a comment -

          I created a diff for the redesigned SSL. Again, I'm not familiar with the make file structure. This diff doesn't include required changes in the Makefile. It's loaded only for review.

          One issue I have is the error queue clean up issue in OpenSSL. If OpenSSL API is called in a thread, it is important to call ERR_remove_state(0) at the end of that thread. This is difficult to do in Thrift. In this diff, ERR_remove_state(0) is called whenever an SSL socket is closed. As long as only the Thrift application server is being used, this is not a problem. However, if the SSL socket is passed around threads, the error queue of involved threads may not be cleaned up correctly.

          I tried a few approaches, but either didn't work or had other issues. Other than the possible memory leak and unnecessary calls to ERR_remove_state(0), this design is very clean.

          I added tutorial/README.SSL. For working example, take a look at test/cpp/src/TestClient.cpp and TestServer.cpp. OpenSSL initialization and cleanup are taken care of inside TSSLSocketFactory, so you don't have to.

          Show
          Ping Li added a comment - I created a diff for the redesigned SSL. Again, I'm not familiar with the make file structure. This diff doesn't include required changes in the Makefile. It's loaded only for review. One issue I have is the error queue clean up issue in OpenSSL. If OpenSSL API is called in a thread, it is important to call ERR_remove_state(0) at the end of that thread. This is difficult to do in Thrift. In this diff, ERR_remove_state(0) is called whenever an SSL socket is closed. As long as only the Thrift application server is being used, this is not a problem. However, if the SSL socket is passed around threads, the error queue of involved threads may not be cleaned up correctly. I tried a few approaches, but either didn't work or had other issues. Other than the possible memory leak and unnecessary calls to ERR_remove_state(0), this design is very clean. I added tutorial/README.SSL. For working example, take a look at test/cpp/src/TestClient.cpp and TestServer.cpp. OpenSSL initialization and cleanup are taken care of inside TSSLSocketFactory, so you don't have to.
          Hide
          Ping Li added a comment -

          Sorry, I was working on a different project since the beginning of this year. I'll work on this as early as tomorrow. I'll post a diff as soon as it's ready.

          The code I posted is still not ready. The issues are,

          1. Doesn't support dNS/subjectAltName.
          2. Doesn't support PKCS12.
          3. Doesn't have API to set encryption algorithm suite.
          4. Doesn't verify the private keys (don't remember details).

          On top of that, my concern is the design can be further improved. I'm thinking to introduce SocketFactory/SSLSocketFactory, ServerSocketFactory/SSLServerSocketFactory, KeyLoader (to support different X509 formats like PKCS12).

          SocketFactory =<derives>=> SSLSocketFactory =<uses>=> KeyLoader.
          ServerSocketFactory =<derives>=> SSLServerSocketFactory =<uses>=> KeyLoader.

          <ThriftServer> =[uses]=> ServerSocketFactory.

          This is just a thought. I haven't tested it in code yet. I'll also add some demo/tutorial code.

          Show
          Ping Li added a comment - Sorry, I was working on a different project since the beginning of this year. I'll work on this as early as tomorrow. I'll post a diff as soon as it's ready. The code I posted is still not ready. The issues are, 1. Doesn't support dNS/subjectAltName. 2. Doesn't support PKCS12. 3. Doesn't have API to set encryption algorithm suite. 4. Doesn't verify the private keys (don't remember details). On top of that, my concern is the design can be further improved. I'm thinking to introduce SocketFactory/SSLSocketFactory, ServerSocketFactory/SSLServerSocketFactory, KeyLoader (to support different X509 formats like PKCS12). SocketFactory =<derives>=> SSLSocketFactory =<uses>=> KeyLoader. ServerSocketFactory =<derives>=> SSLServerSocketFactory =<uses>=> KeyLoader. <ThriftServer> = [uses] => ServerSocketFactory. This is just a thought. I haven't tested it in code yet. I'll also add some demo/tutorial code.
          Hide
          Bryan Duxbury added a comment -

          What's the status of this issue? It looks like it was more or less ready to commit.

          Show
          Bryan Duxbury added a comment - What's the status of this issue? It looks like it was more or less ready to commit.
          Hide
          Ian Pye added a comment -

          Your patch looks good to me, and I don't have any other comments. It would be good to get a 3rd option though. Anyone?

          It seems like we should have a plan for committing something however. Is there a plan? Do you want to take a stab at creating a more polished version?

          2. Windows – if Windows support isn't needed, I'm more than happy leaving it out. Looking here (http://wiki.apache.org/thrift/ThriftInstallationWin32) it looks like the windows build runs via Cygwin, so I don't think we have a problem.

          For the rest, I agree that the "nice to have" SSL functions can be added in later, or in the application layer.

          Show
          Ian Pye added a comment - Your patch looks good to me, and I don't have any other comments. It would be good to get a 3rd option though. Anyone? It seems like we should have a plan for committing something however. Is there a plan? Do you want to take a stab at creating a more polished version? 2. Windows – if Windows support isn't needed, I'm more than happy leaving it out. Looking here ( http://wiki.apache.org/thrift/ThriftInstallationWin32 ) it looks like the windows build runs via Cygwin, so I don't think we have a problem. For the rest, I agree that the "nice to have" SSL functions can be added in later, or in the application layer.
          Hide
          Ping Li added a comment -

          I do receive the notification of my own comment. As to your concern about usage example, I'm uploading the TestClient.cpp and TestServer.cpp I modified to test it. It doesn't look very nice though.

          Show
          Ping Li added a comment - I do receive the notification of my own comment. As to your concern about usage example, I'm uploading the TestClient.cpp and TestServer.cpp I modified to test it. It doesn't look very nice though.
          Hide
          Ping Li added a comment -

          Hi, Ian,

          Sorry I'm late again. I just figured out how to "watch" this thread. I hope I can receive notifications from now on.

          This "patch" is to show you the idea. Don't compile it. It's created under FB's system, where it compiles and runs without problem. I just don't have time to convert it. If it's not accepted, the conversion will be a waste of time.

          1. SSL vs BIO

          There's no particular reason I choose to use SSL rather than BIO. SSL APIs are enough.

          2. build on Windows

          I'm not sure about this. I remember I read something says Thrift's cpp library can only be built on Unix-like systems. Plus, the concurrency code is based on Unix too.

          2. Client and Server certificates
          3. Handling encrypted keys

          These are OpenSSL related features. I added callback functions for encrypted keys. Then I removed it. OpenSSL is very powerful. I only want to enable SSL for Thrift and make it possible to add any OpenSSL functions.

          I only added two SSL related classes OpenSSL and TSSLContext. The idea is not to include any convenient features. I'd like to leave it to developers.

          4. SocketServer and SocketClient are confusing

          Yes, I agree. I added some notes at the beginning of TSSLSocket.h. I hope that may help a little bit.

          5. dyn_create_function...

          At this time, they're not required by OpenSSL. So I skipped. As to performance, I believe session cache is more important. Ideally, I should implement a built-in session cache, and allow developers to override it. But given the time I have, I decided to leave it to application developers.

          The initial version is rudimentary. But it can always be added later.

          Show
          Ping Li added a comment - Hi, Ian, Sorry I'm late again. I just figured out how to "watch" this thread. I hope I can receive notifications from now on. This "patch" is to show you the idea. Don't compile it. It's created under FB's system, where it compiles and runs without problem. I just don't have time to convert it. If it's not accepted, the conversion will be a waste of time. 1. SSL vs BIO There's no particular reason I choose to use SSL rather than BIO. SSL APIs are enough. 2. build on Windows I'm not sure about this. I remember I read something says Thrift's cpp library can only be built on Unix-like systems. Plus, the concurrency code is based on Unix too. 2. Client and Server certificates 3. Handling encrypted keys These are OpenSSL related features. I added callback functions for encrypted keys. Then I removed it. OpenSSL is very powerful. I only want to enable SSL for Thrift and make it possible to add any OpenSSL functions. I only added two SSL related classes OpenSSL and TSSLContext. The idea is not to include any convenient features. I'd like to leave it to developers. 4. SocketServer and SocketClient are confusing Yes, I agree. I added some notes at the beginning of TSSLSocket.h. I hope that may help a little bit. 5. dyn_create_function... At this time, they're not required by OpenSSL. So I skipped. As to performance, I believe session cache is more important. Ideally, I should implement a built-in session cache, and allow developers to override it. But given the time I have, I decided to leave it to application developers. The initial version is rudimentary. But it can always be added later.
          Hide
          Ian Pye added a comment -

          Ping,

          Happy new year back at ya!

          Here are a few notes after playing with your patch for a bit.

          • Code almost compiles fine for me on a Macintosh.

          To compile, I had to adjust the header prefix paths, removing the
          /thrift/lib/cpp/src prefix.

          Also, the type ulong is u_long on a Mac, which caused some problems.

          • There also a few warnings which seem easily fixed and I think are again Mac specific:
            src/transport/TFileTransport.cpp: In member function 'bool facebook::thrift::transport::TFileTransport::isEventCorrupted()':
            src/transport/TFileTransport.cpp:620: warning: format '%ld' expects type 'long int', but argument 7 has type 'long long int'
            src/transport/TFileTransport.cpp: In member function 'void facebook::thrift::transport::TFileTransport::performRecovery()':
            src/transport/TFileTransport.cpp:661: warning: format '%lu' expects type 'long unsigned int', but argument 3 has type 'long long int'
          • TSSLServer.cpp: static ulong callbackThreadID()

          The implementation of this method seems very pthread specific and I don't think will work for Windows. How is multi-threading

          This method seems very pthread specific and I don't think will work for Windows or other non-POSIX places. Is it the case that thrift requires pthreads though anyway?

          • lib/cpp/Makefile.am

          For the TSSLSocket.o file to be generated, TSSLSocket.cpp and
          TSSLSocket.h should be added to the list of source files.

          Also, a reference to -lssl might need to be added to /Makefile.am so
          that the build system checks for its presence.

          • SSL v BIO

          Is there a reason you are using the SSL_ functions, as opposed to the
          BIO_ ones?

          • When I try running the code hacked into the cpp tutorial, it fails with the following error:

          terminate called after throwing an instance of 'facebook::thrift::transport::TTransportException'
          what(): SSL_CTX_new() returns NULL

          I'm too lazy now to dig into this, but if I have some free-time, I'll try to see if this is my fault or the libs.

          In general, the ssl implementation seems complicated, and a small usage example would be very helpful.

          • Client and Server certificates:

          I might be missing this, but it looks like the default in TSSLSocket is for
          only the server to send a cert. to the client, and not for the client
          to identify itself to the server. I argue that in Thrift the
          default should be for both ends to identify themselves.

          • Architecturally, I like the way that TSSLAcceptSocket inherits from
            TServerSocket.
          • Keeping both the SocketServer and SocketClient classes in the same file is a bit confusing for me.
          • Handling encrypted keys is a good feature to have.
          • It looks like the basic MT synchronization functions are present. What about added the additional ones, dyn_create_function, dyn_lock_function and dyn_destroy_function for better performance?

          Ian

          Show
          Ian Pye added a comment - Ping, Happy new year back at ya! Here are a few notes after playing with your patch for a bit. Code almost compiles fine for me on a Macintosh. To compile, I had to adjust the header prefix paths, removing the /thrift/lib/cpp/src prefix. Also, the type ulong is u_long on a Mac, which caused some problems. There also a few warnings which seem easily fixed and I think are again Mac specific: src/transport/TFileTransport.cpp: In member function 'bool facebook::thrift::transport::TFileTransport::isEventCorrupted()': src/transport/TFileTransport.cpp:620: warning: format '%ld' expects type 'long int', but argument 7 has type 'long long int' src/transport/TFileTransport.cpp: In member function 'void facebook::thrift::transport::TFileTransport::performRecovery()': src/transport/TFileTransport.cpp:661: warning: format '%lu' expects type 'long unsigned int', but argument 3 has type 'long long int' TSSLServer.cpp: static ulong callbackThreadID() The implementation of this method seems very pthread specific and I don't think will work for Windows. How is multi-threading This method seems very pthread specific and I don't think will work for Windows or other non-POSIX places. Is it the case that thrift requires pthreads though anyway? lib/cpp/Makefile.am For the TSSLSocket.o file to be generated, TSSLSocket.cpp and TSSLSocket.h should be added to the list of source files. Also, a reference to -lssl might need to be added to /Makefile.am so that the build system checks for its presence. SSL v BIO Is there a reason you are using the SSL_ functions, as opposed to the BIO_ ones? When I try running the code hacked into the cpp tutorial, it fails with the following error: terminate called after throwing an instance of 'facebook::thrift::transport::TTransportException' what(): SSL_CTX_new() returns NULL I'm too lazy now to dig into this, but if I have some free-time, I'll try to see if this is my fault or the libs. In general, the ssl implementation seems complicated, and a small usage example would be very helpful. Client and Server certificates: I might be missing this, but it looks like the default in TSSLSocket is for only the server to send a cert. to the client, and not for the client to identify itself to the server. I argue that in Thrift the default should be for both ends to identify themselves. Architecturally, I like the way that TSSLAcceptSocket inherits from TServerSocket. Keeping both the SocketServer and SocketClient classes in the same file is a bit confusing for me. Handling encrypted keys is a good feature to have. It looks like the basic MT synchronization functions are present. What about added the additional ones, dyn_create_function, dyn_lock_function and dyn_destroy_function for better performance? Ian
          Hide
          Ping Li added a comment -

          Ian,

          Sorry, I didn't receive notification of your comments. This is open source project. Anyone can contribute. I think it's more interesting to have multiple submissions.

          I just uploaded my code, so you can take a look. I didn't compile it under the open source project though. It's just for discussion purpose. I'll appreciate your feedback.

          This code is still being reviewed.

          Show
          Ping Li added a comment - Ian, Sorry, I didn't receive notification of your comments. This is open source project. Anyone can contribute. I think it's more interesting to have multiple submissions. I just uploaded my code, so you can take a look. I didn't compile it under the open source project though. It's just for discussion purpose. I'll appreciate your feedback. This code is still being reviewed.
          Hide
          Ping Li added a comment -

          This patch is for demo/feedback purpose only. I didn't even compile it.

          The patch was created with "svn diff".

          Happy new year,

          Show
          Ping Li added a comment - This patch is for demo/feedback purpose only. I didn't even compile it. The patch was created with "svn diff". Happy new year,
          Hide
          Ian Pye added a comment -

          Ping,

          If you are working on an internal version of thrift with ssl support, does it make sense for me to continue with this patch?

          Otherwise, regarding multithreading, I see your point here and agree that it needs to be enabled.

          Show
          Ian Pye added a comment - Ping, If you are working on an internal version of thrift with ssl support, does it make sense for me to continue with this patch? Otherwise, regarding multithreading, I see your point here and agree that it needs to be enabled.
          Hide
          Ping Li added a comment -

          Ian,

          I looked at your patch (11/23). My comments posted on 11/26 are still valid.

          Show
          Ping Li added a comment - Ian, I looked at your patch (11/23). My comments posted on 11/26 are still valid.
          Hide
          Ping Li added a comment -

          Hi, Ian,

          Sorry for being late. I've been quite busy lately. The reason I joined this thread is because we have an urgent need for Thrift SSL support. After I commented on your code last time, I didn't know when you'll make changes and how the changes would be. So I went ahead and created an implementation of SSL code for Thrift. The basic SSL functions are already implemented and briefly tested. I'm thinking to take out X509v3 subjectAltName verification and add session cache. It's not determined yet. I'm also thinking to take out both in the initial version, because I don't have time to test these functions.

          Here's what I'm thinking,

          1. MT support has two implications:
          (a) you have to enable MT for OpenSSL (see CRYPTO_num_locks, CRYPTO_set_id_callback, CRYPTO_set_locking_callback, and other dynamic locking functions)
          (b) TServerSocket::accept() returns a blocking client socket. I believe you initiate server side handshake immediately after accept() returns (same thread). This will block the server, regardless TThreadedServer or TThreadPoolServer.

          2. I changed my mind. I don't use multi-inheritance in my code. It's hard to explain without showing you my code.

          3. In the previous patch, you copied quite a lot code from TSocket and TServerSocket. I don't think this is the right approach. I don't copy any in my code.

          4. TNonblockingServer requires extra work. I don't support it in my code this time. I may add it later.

          I'll take look at your new updates and give you feedback.

          Show
          Ping Li added a comment - Hi, Ian, Sorry for being late. I've been quite busy lately. The reason I joined this thread is because we have an urgent need for Thrift SSL support. After I commented on your code last time, I didn't know when you'll make changes and how the changes would be. So I went ahead and created an implementation of SSL code for Thrift. The basic SSL functions are already implemented and briefly tested. I'm thinking to take out X509v3 subjectAltName verification and add session cache. It's not determined yet. I'm also thinking to take out both in the initial version, because I don't have time to test these functions. Here's what I'm thinking, 1. MT support has two implications: (a) you have to enable MT for OpenSSL (see CRYPTO_num_locks, CRYPTO_set_id_callback, CRYPTO_set_locking_callback, and other dynamic locking functions) (b) TServerSocket::accept() returns a blocking client socket. I believe you initiate server side handshake immediately after accept() returns (same thread). This will block the server, regardless TThreadedServer or TThreadPoolServer. 2. I changed my mind. I don't use multi-inheritance in my code. It's hard to explain without showing you my code. 3. In the previous patch, you copied quite a lot code from TSocket and TServerSocket. I don't think this is the right approach. I don't copy any in my code. 4. TNonblockingServer requires extra work. I don't support it in my code this time. I may add it later. I'll take look at your new updates and give you feedback.
          Hide
          Ian Pye added a comment - - edited

          New patch submitted reflecting Ping's comments.

          I created TSSLContext and TSSLTransport classes.

          TSSLContext is a wrapper around a SSL Context class.
          Most of the ssl resources are handled by this class. It can be passed as a boost::shared_ptr to the TSSLSocket and TSSLServerSocket classes.

          The TSSLTransport handles X509 cert verification, and is inherited from both TSSLSocket and TSSLServerSocket classes.

          Multithreading is left out, as I feel that it is already handled at the TServer level.

          By default, only SSLv3 is supported now, and both servers and clients supply certificates. Also, supplied certs are checked to make sure the subject name matches the host they are connecting to.

          Show
          Ian Pye added a comment - - edited New patch submitted reflecting Ping's comments. I created TSSLContext and TSSLTransport classes. TSSLContext is a wrapper around a SSL Context class. Most of the ssl resources are handled by this class. It can be passed as a boost::shared_ptr to the TSSLSocket and TSSLServerSocket classes. The TSSLTransport handles X509 cert verification, and is inherited from both TSSLSocket and TSSLServerSocket classes. Multithreading is left out, as I feel that it is already handled at the TServer level. By default, only SSLv3 is supported now, and both servers and clients supply certificates. Also, supplied certs are checked to make sure the subject name matches the host they are connecting to.
          Hide
          Ian Pye added a comment -

          Ping,

          Thanks for the feedback. Your ideas all sound good to me. I'll try to upload a new patch incorporating these soon.

          As for suggestion 8, I think it makes a lot of sense for both TSSLSocket and TSSLServerSocket to inherit from a common ancestor. This will mean multiple-inheritance (TSSLSocket inherits from TTransport and TSSLTransport, for example). But since the domains of each parent are quite difference, this doesn't seem too bad.

          For 9, (MT Support), I thought that Thrift was already taking care of this at a higher level (TThreadPoolServer, TThreadedServer). Since the (currently single threaded) socket classes are given to the multi-threaded server, would having multi-threaded server sockets provide more performance than a single threaded one? That said, having a mt ssl server socket could make writing new servers easier, so it still seems like a good feature to have.

          Show
          Ian Pye added a comment - Ping, Thanks for the feedback. Your ideas all sound good to me. I'll try to upload a new patch incorporating these soon. As for suggestion 8, I think it makes a lot of sense for both TSSLSocket and TSSLServerSocket to inherit from a common ancestor. This will mean multiple-inheritance (TSSLSocket inherits from TTransport and TSSLTransport, for example). But since the domains of each parent are quite difference, this doesn't seem too bad. For 9, (MT Support), I thought that Thrift was already taking care of this at a higher level (TThreadPoolServer, TThreadedServer). Since the (currently single threaded) socket classes are given to the multi-threaded server, would having multi-threaded server sockets provide more performance than a single threaded one? That said, having a mt ssl server socket could make writing new servers easier, so it still seems like a good feature to have.
          Hide
          Ping Li added a comment - - edited

          Hi, Ian,

          Thanks for the patch. Having SSL support is very important. Here's my comments/questions.

          [1] TSSLSocket.h/.cpp

          Q1 - Q6: TSSLSocket::open() and/or TSSLServerSocket::listen()

          Q1. SSL_library_init() and SSL_load_error_strings() are usually called once at the beginning of the application. Is there any reason you want to call them for each new SSL socket?

          Q2. SSL_library_init() should be called before SSL_load_error_strings(), right?

          Q3. I suspect library resources are not properly released.

          Q4. SSL_CTX is also an application level object. It can be shared by many SSL sockets across the application. Now it's created for each SSL socket.

          Q5. TSSLSocket uses SSLv23 compatible mode (SSLv23_client_method). I don't think supporting SSL v2 is a good idea. Can you remove SSL v2 from SSLv23 (using SSL_CTX_set_options with SSL_OP_NO_SSLv2)? Then the SSL_CTX supports both SSL v3 and TLS.

          Q6. When verifyCert() fails, it doesn't free the certificate before throw the exception.

          if (paranoid_ && !verifyCert(peerCertificate)){
              // free peerCertificate here
              throw TTransportException(TTransportException::UNKNOWN, 
          			      "Paranoid verification failed.");
            } 
          

          Q7. TSSLSocket::thriftSeedPRNG(egdPath.c_str());

          The 2nd parameter of this call defaults to "false", which makes the first EGD path useless. It always uses system time as random seed, which is not acceptable.

          Q8. TSSLSocket::verifyCert(); only checks X509v1 commonName. It's recommended to use subjectAltName.dNSName of X509.3, and fall back to commonName if it's not present.

          [2] TSSLServerSocket.h/.cpp

          Q9. There's no server side certificate verification. It may be required in certain applications.

          Q10. string TSSLServerSocket::int2str() is for internal use, but is public. And it's only used once. Rather than stringstream, I'd replace the call with the following lines,

          char port [64]; // 64 should be long enough for 128-bit integer type
          snprintf(port, sizeof(port), "%u", port_number);
          abio_ = BIO_new_accept(const_cast<char *>(port));
          

          Q11. There's no multi-thread support, right? In the tutorial SSLServer.cpp, it seems like the server can be implemented with multi-threads.

          Here's what I'm thinking,

          1. Create a TSSLContext class,

          2. Create an initialization function that performs the followings,

          • SSL_library_init(); // initialize only once
          • SSL_error_load_strings(); // load only once
          • seed PRNG; // move from TSSLSocket to TSSLContext, seed only once, should not use system time as seeds
          • Make seedPRNG a virtual function, so that it can be overriden for customized see functions.
            virtual int TSSLContext::seedRand() {   // replaces TSSLSocket::seedPRNG()
              if ( useEGD )
                if ( seed with egd system was successful )
                  return SEED_WITH_EGD;
             
              if ( "/dev/urandom/" exists )
                if ( seed with "/dev/urandom" was successful )
                  return SEED_WITH_URANDOM;
            
              return -1;
            }
            

          3. virtual int TSSLContext::addRand() to add randomness at run time. The default implementation can be empty and returns error.

          4. TSSLContext should release SSL library resources (e.g. error strings) when no longer used

          5. TSSLContext should support the followings,

          • SSL_CTX * getSSLCTX(); // for user to customize SSL_CTX, so you don't have to wrap all OpenSSL calls on SSL_CTX
          • add default implementation to support password protected private key files (SSL_CTX_set_default_passwd_cb, SSL_CTX_set_default_passwd_cb_userdata)

          6. TSSLContext objects can be attached to TSSLServerSocket and TSSLSocket.

          7. Both TSSLSocket and TSSLServerSocket should have paranoid default to "true".

          8. It may be necessary to create a virtual base class for TSSLSocket and TSSLServerSocket so that they can share some functions (e.g. verifyCertificate, paranoid etc.)

          9. Add MT support.

          Please let me know how you think about it.

          Show
          Ping Li added a comment - - edited Hi, Ian, Thanks for the patch. Having SSL support is very important. Here's my comments/questions. [1] TSSLSocket.h/.cpp Q1 - Q6: TSSLSocket::open() and/or TSSLServerSocket::listen() Q1. SSL_library_init() and SSL_load_error_strings() are usually called once at the beginning of the application. Is there any reason you want to call them for each new SSL socket? Q2. SSL_library_init() should be called before SSL_load_error_strings(), right? Q3. I suspect library resources are not properly released. Q4. SSL_CTX is also an application level object. It can be shared by many SSL sockets across the application. Now it's created for each SSL socket. Q5. TSSLSocket uses SSLv23 compatible mode (SSLv23_client_method). I don't think supporting SSL v2 is a good idea. Can you remove SSL v2 from SSLv23 (using SSL_CTX_set_options with SSL_OP_NO_SSLv2)? Then the SSL_CTX supports both SSL v3 and TLS. Q6. When verifyCert() fails, it doesn't free the certificate before throw the exception. if (paranoid_ && !verifyCert(peerCertificate)){ // free peerCertificate here throw TTransportException(TTransportException::UNKNOWN, "Paranoid verification failed." ); } Q7. TSSLSocket::thriftSeedPRNG(egdPath.c_str()); The 2nd parameter of this call defaults to "false", which makes the first EGD path useless. It always uses system time as random seed, which is not acceptable. Q8. TSSLSocket::verifyCert(); only checks X509v1 commonName. It's recommended to use subjectAltName.dNSName of X509.3, and fall back to commonName if it's not present. [2] TSSLServerSocket.h/.cpp Q9. There's no server side certificate verification. It may be required in certain applications. Q10. string TSSLServerSocket::int2str() is for internal use, but is public. And it's only used once. Rather than stringstream, I'd replace the call with the following lines, char port [64]; // 64 should be long enough for 128-bit integer type snprintf(port, sizeof(port), "%u" , port_number); abio_ = BIO_new_accept(const_cast< char *>(port)); Q11. There's no multi-thread support, right? In the tutorial SSLServer.cpp, it seems like the server can be implemented with multi-threads. Here's what I'm thinking, 1. Create a TSSLContext class, 2. Create an initialization function that performs the followings, SSL_library_init(); // initialize only once SSL_error_load_strings(); // load only once seed PRNG; // move from TSSLSocket to TSSLContext, seed only once, should not use system time as seeds Make seedPRNG a virtual function, so that it can be overriden for customized see functions. virtual int TSSLContext::seedRand() { // replaces TSSLSocket::seedPRNG() if ( useEGD ) if ( seed with egd system was successful ) return SEED_WITH_EGD; if ( "/dev/urandom/" exists ) if ( seed with "/dev/urandom" was successful ) return SEED_WITH_URANDOM; return -1; } 3. virtual int TSSLContext::addRand() to add randomness at run time. The default implementation can be empty and returns error. 4. TSSLContext should release SSL library resources (e.g. error strings) when no longer used 5. TSSLContext should support the followings, SSL_CTX * getSSLCTX(); // for user to customize SSL_CTX, so you don't have to wrap all OpenSSL calls on SSL_CTX add default implementation to support password protected private key files (SSL_CTX_set_default_passwd_cb, SSL_CTX_set_default_passwd_cb_userdata) 6. TSSLContext objects can be attached to TSSLServerSocket and TSSLSocket. 7. Both TSSLSocket and TSSLServerSocket should have paranoid default to "true". 8. It may be necessary to create a virtual base class for TSSLSocket and TSSLServerSocket so that they can share some functions (e.g. verifyCertificate, paranoid etc.) 9. Add MT support. Please let me know how you think about it.
          Hide
          Ian Pye added a comment -

          OpenSSL implementation of TSSLServerSocket and TSSLSocket

          Show
          Ian Pye added a comment - OpenSSL implementation of TSSLServerSocket and TSSLSocket
          Hide
          Ian Pye added a comment -

          106 is the Java version of this issue

          Show
          Ian Pye added a comment - 106 is the Java version of this issue
          Hide
          Ian Pye added a comment -

          Java and C++ versions

          Show
          Ian Pye added a comment - Java and C++ versions

            People

            • Assignee:
              Unassigned
              Reporter:
              Ian Pye
            • Votes:
              5 Vote for this issue
              Watchers:
              11 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