Issue #63: Add set_ssl API to libgearman for PHP extension and other potential uses

[esabol]

There seems to be renewed interest in SSL connections from PHP to gearmand (coming up twice in the past two weeks: today on Stack Overflow and recently over at wcgallego/pecl-gearman/issues/43), so I have rebased and tweaked the patch tracked at issue #63 in the hopes of landing this.

There were also some references to wolfssl and CyaTLSv1_client_method that I have corrected or made more generic. If you prefer that I split off those minor things into a separate PR, just let me know.

The original patch came from here:
http://bugs.launchpad.net/gearmand/+bug/1338861

But I have made a few tweaks.

[esabol]

Clang rightfully complained about ssl_error = ERR_peek_last_error(); since ERR_peek_last_error() returns an unsigned long and ssl_error is defined to be an int. That's becauseSSL_get_error() returns an int. Why does SSL_get_error() returns an int when ERR_peek_last_error() returns an unsigned long?! That makes no sense! Ugh...

Anyway, I'm not sure the following code

                if (ERR_peek_last_error())
                {
                  ssl_error = ERR_peek_last_error();
                }

is really doing much that's useful here anyway. Should I just remove that change?

Alternative 1: Change the code like so:

                char errorString[SSL_ERROR_SIZE]= { 0 };
                if (ERR_peek_last_error())
                {
                  unsigned long last_ssl_error = ERR_peek_last_error();
                  ERR_error_string_n(last_ssl_error, errorString, sizeof(errorString));
                }
                else
                {
                  ERR_error_string_n(ssl_error, errorString, sizeof(errorString));
                }

Alternative 2: Declare ssl_error to be unsigned int.

Any preference?

[p-alik]

Why does SSL_get_error() returns an int when ERR_peek_last_error() returns an unsigned long?

see https://stackoverflow.com/a/37984115/2789312

Any preference?

If I interpret the aforementioned answer correctly, we've to use SSL_get_error instead of ERR_peek_last_error

[esabol]

If I interpret the aforementioned answer correctly, we've to use SSL_get_error instead of ERR_peek_last_error

We already are. OK, I've removed the calls to ERR_peek_last_error(). They didn't seem relevant to this issue/PR or particularly useful.

What I think we might be missing are calls to ERR_clear_error(). I've read it should be called prior to any SSL I/O operation? But that's probably beyond the scope of this issue/PR as well.

[esabol]

In order to get a clean report on the latest build for this PR, please restart the following Travis CI jobs:

https://travis-ci.org/gearman/gearmand/jobs/619116884
https://travis-ci.org/gearman/gearmand/jobs/619116887

Thanks!

[esabol]

I should mention that I left out this part of the original patch:

--- libgearman-server/plugins/protocol/gear/protocol.cc	2014-02-12 08:05:28.000000000 +0800
+++ libgearman-server/plugins/protocol/gear/protocol.cc	2014-07-24 13:57:38.895352987 +0800
@@ -418,9 +422,9 @@
 Gear::Gear() :
   Plugin("Gear"),
   _port(GEARMAN_DEFAULT_TCP_PORT_STRING),
-  _ssl_ca_file(GEARMAND_CA_CERTIFICATE),
-  _ssl_certificate(GEARMAND_SERVER_PEM),
-  _ssl_key(GEARMAND_SERVER_KEY),
+  _ssl_ca_file(""),
+  _ssl_certificate(""),
+  _ssl_key(""),
   opt_ssl(false)
   {
     command_line_options().add_options()
@@ -477,19 +481,40 @@
 
   if (opt_ssl)
   {
-    if (getenv("GEARMAND_CA_CERTIFICATE"))
+    if (_ssl_ca_file.empty())
     {
-      _ssl_ca_file= getenv("GEARMAND_CA_CERTIFICATE");
+      if (getenv("GEARMAND_CA_CERTIFICATE"))
+      {
+        _ssl_ca_file= getenv("GEARMAND_CA_CERTIFICATE");
+      }
+      else
+      {
+        _ssl_ca_file= GEARMAND_CA_CERTIFICATE;
+      }
     }
 
-    if (getenv("GEARMAND_SERVER_PEM"))
+    if (_ssl_certificate.empty())
     {
-      _ssl_certificate= getenv("GEARMAND_SERVER_PEM");
+      if (getenv("GEARMAND_SERVER_PEM"))
+      {
+        _ssl_certificate= getenv("GEARMAND_SERVER_PEM");
+      }
+      else
+      {
+        _ssl_certificate= GEARMAND_SERVER_PEM;
+      }
     }
 
-    if (getenv("GEARMAND_SERVER_KEY"))
+    if (_ssl_key.empty())
     {
-      _ssl_key= getenv("GEARMAND_SERVER_KEY");
+      if (getenv("GEARMAND_SERVER_KEY"))
+      {
+        _ssl_key= getenv("GEARMAND_SERVER_KEY");
+      }
+      else
+      {
+        _ssl_key= GEARMAND_SERVER_KEY;
+      }
     }
 
     gearmand->init_ssl();

This just didn’t seem like a useful change to make. What do you think?

[esabol]

Any comments? If not, please merge.