[jira] Commented: (THRIFT-151) TSSLServerSocket and TSSLSocket implementation

["Ping Li (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/THRIFT-151?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12646048#action_12646048 ] 

Ping Li commented on THRIFT-151:
--------------------------------

Hi, Ian,

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

[1] TSSLSocket.h/.cpp

- TSSLSocket::open(), 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)){
    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
- release SSL library resources (e.g. error strings)  when no longer used

int TSSLContext::seedPRNG(const char *egd_path = NULL) {
  if ( egd_path exists )
    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. 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) 

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

5. paranoid should default to "true".

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

Please let me know how you think about it.

> TSSLServerSocket and TSSLSocket implementation
> ----------------------------------------------
>
>                 Key: THRIFT-151
>                 URL: https://issues.apache.org/jira/browse/THRIFT-151
>             Project: Thrift
>          Issue Type: Improvement
>          Components: Library (C++)
>            Reporter: Ian Pye
>         Attachments: ssl.patch
>
>   Original Estimate: 6h
>  Remaining Estimate: 6h
>
> 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.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.