libgearman patch to support SSL connections in Gearman PHP library #63

esabol · 2016-12-07T21:50:07Z

There's a patch to add SSL support to the the Gearman PHP library that's been languishing for over 2 years over at http://bugs.php.net/bug.php?id=67623. The reason for that is that it relies on some (relatively straightforward?) changes to libgearman. The libgearman patch can be found at http://bugs.launchpad.net/gearmand/+bug/1338861. The patch (to gearmand 1.1.12) needs to be rebased, obviously, and it looks like it could use a little more polish. I'm willing to take a crack at that. If anyone has any comments on the patch or changes you feel need to be made to the patch in order to get it accepted, I'd like to hear them. It would be really nice to get this committed here, so that some future release of the Gearman PHP library could support SSL connections.

SpamapS · 2016-12-07T22:05:14Z

Thanks for surfacing that Ed. It would be great if you could submit it as a PR. Alexey and myself will look ASAP.

p-alik · 2016-12-07T22:08:37Z

I just tried to patch and build patched code. I'll do some tests tomorrow.

p-alik · 2016-12-08T10:28:41Z

It doesn't look very promising. No changes with regard to issue #28. I run only sequential test. So I can't say anything to issue #29 yet.
Some SSL tests passed, some failed. gearmand logs on failer:

  DEBUG 2016-12-08 10:17:06.962672 [     3 ] gearmand_sockfd_close() called with an invalid socket, this was probably ok -> libgearman-server/io.cc:1055
  DEBUG 2016-12-08 10:17:06.962720 [     3 ] Received RUN wakeup event -> libgearman-server/gearmand_thread.cc:624
  ERROR 2016-12-08 10:17:06.962730 [     3 ] con 140715208214752 is already cleaned-up. returning -> libgearman-server/connection.cc:225
  DEBUG 2016-12-08 10:17:06.964922 [  main ] accept() fd:33 -> libgearman-server/gearmand.cc:874
   INFO 2016-12-08 10:17:06.964940 [  main ] Accepted connection from ::3539:3538:3600:0:59588
  DEBUG 2016-12-08 10:17:06.964963 [     2 ] Received CON wakeup event -> libgearman-server/gearmand_thread.cc:619
  DEBUG 2016-12-08 10:17:06.964982 [     2 ] ::3539:3538:3600:0:59588 Watching  POLLIN  -> libgearman-server/gearmand_thread.cc:151
  ERROR 2016-12-08 10:17:06.966579 [     2 ] error:00000001:lib(0):func(0):reason(1)(1) _gear_con_add(LOST_CONNECTION) -> libgearman-server/plugins/protocol/gear/protocol.cc:398
  ERROR 2016-12-08 10:17:06.966733 [     2 ] ::3539:3538:3600:0:59588 _con_add() has failed, please report any crashes that occur immediately after this. gearmand_con_check_queue(LOST_CONNECTION) -> libgearman-server/gearmand_con.cc:774

Client/Worker show on failer:

SSL connect attempt failed because of handshake problems error:140943FC:SSL routines:ssl3_read_bytes:sslv3 alert bad record mac

esabol · 2016-12-08T11:09:09Z

I wouldn't expect any changes with regard to issues #28 or #29. The purpose of the patch is to extend the API, specifically the addition of the gearman_client_set_ssl() and gearman_worker_set_ssl() functions.

There are at least four versions of the patch file. Which one did you use?

Do you have a fork I can checkout?

p-alik · 2016-12-08T11:26:09Z

There are at least four versions of the patch file. Which one did you use?

I started by the first patch.

Do you have a fork I can checkout?

ssl branch

esabol · 2016-12-08T13:18:03Z

I think we want the more recent patch uploaded in comment 4 at https://bugs.launchpad.net/gearmand/+bug/1338861/comments/4. In comment 6, the author says there's an even newer version, but that patch includes a lot of unrelated changes.

The change to enable_ssl() in libgearman/client.hpp makes me nervous. I wonder if we can do without that?

Do you think the changes to the libgearman-server files are unnecessary?

p-alik · 2016-12-08T14:10:08Z

I think we want the more recent patch uploaded in comment 4

done but I doesn't apply any changes to libgearman/client.hpp, libgearman-server/plugins/queue/mysql/queue.cc

Do you think the changes to the libgearman-server files are unnecessary?

I applied the patch to

libgearman-server/io.cc change only logging purpose
libgearman-server/plugins/protocol/gear/protocol.cc at least first part seems to be useful.

@@ -393,6 +393,10 @@ static gearmand_error_t _gear_con_add(gearman_server_con_st *connection)
         case SSL_ERROR_SSL:
         case SSL_ERROR_ZERO_RETURN:
         default:
+          if (ERR_peek_last_error())
+          {
+            cyassl_error = ERR_peek_last_error();
+          }

esabol · 2016-12-12T23:56:46Z

Thanks, @p-alik. I'll try to compile your branch soon and test it.

Sure, the libgearman-server changes seem useful. Just thought it might be beyond the scope of this issue.

I have no idea how efficient ERR_peek_last_error() is. Would it make more sense to implement it in such a way that it isn't called twice?

Also, does ERR_peek_last_error() work with CyaSSL?? Based on my googling, it appears to be an OpenSSL function.... Are CyaSSL and OpenSSL fully API compatible?

Probably most of those cyassl_* variables in libgearman-server should be renamed to ssl_* (as they are in libgearman). It appears as though CyaSSL support was added first and then someone added support for OpenSSL afterward.

SpamapS · 2016-12-13T00:40:59Z

CyaSSL (now called wolfSSL btw) has an OpenSSL compatibility layer. I actually help maintain it in Debian, and it's a well cared for library with a clear license vs. the OpenSSL license which is of questionable origin and does not play nice with GPL. It's intention is to be lightweight and fast, so I recommend we actually try setting up Travis jobs to test with it. Excerpts from Ed Sabol's message of 2016-12-12 15:56:47 -0800:

…

Thanks, Andrei. I'll try to compile your branch soon and test it. Sure, the libgearman-server changes seem useful. Just thought it might be beyond the scope of this issue. I have no idea how efficient `ERR_peek_last_error()` is. Would it make more sense to implement it in such a way that it isn't called twice? Also, does `ERR_peek_last_error()` work with CyaSSL?? Based on my googling, it appears to be an OpenSSL function.... Are CyaSSL and OpenSSL fully API compatible? Probably most of those `cyassl_*` variables in libgearman-server should be renamed to `ssl_*` (as they are in libgearman). It appears as though CyaSSL support was added first and then someone added support for OpenSSL afterward.

esabol · 2017-03-03T19:25:58Z

While in the process of applying this patch to gearmand 1.1.15, I noticed that line 476 of libgearman/universal.cc has a change that I don't think we want.

gearmand 1.1.15 has:

    if ((_ctx_ssl= SSL_CTX_new(TLSv1_client_method())) == NULL)

This patched version has

    if ((_ctx_ssl= SSL_CTX_new(SSLv23_client_method())) == NULL)

BrianAker · 2017-03-04T03:38:26Z

SSLv23_client_method() is deprecated. TLS_client_method() is the currently favored. What I do not recall is if TLSv1_client_method() will force the client into 1.0 and not let it degrade to SSL 3, which would clearly be bad. I cannot recall off the top of my head of the UDP code was ever merged, but if it was then that section of code should flow through a different set of functions. — Brian

On Mar 3, 2017, at 11:25, Ed Sabol ***@***.***> wrote: While in the process of applying this patch to gearmand 1.1.15, I noticed that line 476 of libgearman/universal.cc has a change that I don't think we want. gearmand 1.1.15 has: if ((_ctx_ssl= SSL_CTX_new(TLSv1_client_method())) == NULL) This patched version has if ((_ctx_ssl= SSL_CTX_new(SSLv23_client_method())) == NULL) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

乌

p-alik · 2017-03-16T17:05:20Z

Also, does ERR_peek_last_error() work with CyaSSL??

wolfSSL provides ERR_peek_last_error

p-alik · 2017-03-16T17:10:00Z

compilation of patched code

with --enable-ssl OK
with --disable-ssl NOK. Compilation exception:

In file included from ./libgearman/connection.hpp:43:0,
                 from ./libgearman/common.h:53,
                 from libgearman/pipe.cc:41:
./libgearman/interface/universal.hpp: In member function ‘void gearman_universal_st::ssl_ca_file(const char*)’:
./libgearman/interface/universal.hpp:234:65: error: ‘strlen’ was not declared in this scope
     if (ssl_ca_file_ && (ssl_ca_file_size_ = strlen(ssl_ca_file_)))
                                                                 ^
./libgearman/interface/universal.hpp: In member function ‘void gearman_universal_st::ssl_certificate(const char*)’:
./libgearman/interface/universal.hpp:263:77: error: ‘strlen’ was not declared in this scope
     if (ssl_certificate_ && (ssl_certificate_size_ = strlen(ssl_certificate_)))
                                                                             ^
./libgearman/interface/universal.hpp: In member function ‘void gearman_universal_st::ssl_key(const char*)’:
./libgearman/interface/universal.hpp:292:53: error: ‘strlen’ was not declared in this scope
     if (ssl_key_ && (ssl_key_size_ = strlen(ssl_key_)))
                                                     ^
Makefile:5696: recipe for target 'libgearman/libgearman_server_libgearman_server_la-pipe.lo' failed
make[2]: *** [libgearman/libgearman_server_libgearman_server_la-pipe.lo] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory '/home/palik/workspace/Gearman/gearmand'
Makefile:8369: recipe for target 'all-recursive' failed
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory '/home/palik/workspace/Gearman/gearmand'
Makefile:3088: recipe for target 'all' failed
make: *** [all] Error 2

esabol · 2017-03-16T23:26:48Z

with --disable-ssl NOK. Compilation exception:

I think that can fixed by adding:

#include <cstring>

in libgearman/interface/universal.hpp?

esabol · 2017-03-16T23:52:38Z

I recently applied a modified version of the patch to gearmand 1.1.15, and I will be testing it soon.

Besides the SSLv23_client_method, I'm also not too keen on the addition of the

  if (SSL_CTX_check_private_key(_ctx_ssl) != SSL_SUCCESS)

bits in libgearman/universal.cc and libgearman-server/plugins/protocol/gear/protocol.cc and util/instance.cc. I left those bits out of my version.

p-alik · 2017-03-17T06:05:34Z

What is disadvantage in using SSL_CTX_check_private_key?
Regarding ssl 1.0.2 doc it seems to be useful.

p-alik · 2017-03-17T06:35:48Z

libgearman/worker.hpp is patched in 39c0269 in this way:

   void enable_ssl()
   { 
+    return;
     if (getenv("GEARMAND_CA_CERTIFICATE"))
     {
       gearman_worker_add_options(_worker, GEARMAN_WORKER_SSL);

Should be banished?

see: - https://www.openssl.org/docs/man1.0.2/ssl/SSLv23_client_method.html - gearman#63 (comment)

p-alik · 2017-03-17T08:31:05Z

On 17 March 2017 at 00:52 Ed Sabol notifications@github.com wrote:

I recently applied a modified version of the patch to gearmand 1.1.15, and I will be testing it soon.

@esabol, this branch is rebased against gearman master. Except removing of SSL_CTX_check_private_key it should be equivalent to your version of code. Is it feasible we collaborate on the branch?

esabol · 2017-03-17T20:47:18Z

libgearman/worker.hpp is patched in 39c0269 in this way:
[...]
Should be banished?

Yes. I didn't include this in my version.

esabol · 2017-03-17T20:56:08Z

What is disadvantage in using SSL_CTX_check_private_key?
Regarding ssl 1.0.2 doc it seems to be useful.

The change made me nervous since it's not in the existing code base. Also, I don't actually specify a private key in my gearmand SSL configuration, and I was worried that this change would require me to do so.

esabol · 2017-03-17T20:58:37Z

@esabol, this branch is rebased against gearman master. Except removing of SSL_CTX_check_private_key it should be equivalent to your version of code. Is it feasible we collaborate on the branch?

That's what I'm here to do. 👍 I just confirmed that our two versions are identical except for the SSL_CTX_check_private_key bits I have removed. I'm just waiting on my sys admin to install this and my patched PHP extension. Here's hoping it gets SSL working in PHP....

gearman#63 (comment)

see: - https://www.openssl.org/docs/man1.0.2/ssl/SSLv23_client_method.html - gearman#63 (comment)

gearman#63 (comment)

esabol · 2017-03-28T00:28:13Z

I have successfully tested this patched libgearman.so with hjr3's Gearman PHP extension + SSL patch. It works!

p-alik · 2017-03-29T18:52:30Z

@esabol, would you please create a PR from your code base?

esabol · 2017-03-30T19:17:15Z

@esabol, would you please create a PR from your code base?

I can do that. It's the same as yours except for the SSL_CTX_check_private_key bits. I really wish I understood better the impact of that change before finalizing it though. What do you think?

SpamapS · 2017-03-30T19:30:56Z

It's easier to discuss these things inline with code, rather than in an issue. Just focuses the discussion. Open that PR and we can all reason about the changes.

…man PHP library

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

Tyrael · 2020-12-02T23:00:08Z

now that #257 is in this should be closed, right?

esabol · 2020-12-03T16:47:09Z

now that #257 is in this should be closed, right?

Right. Closing....

SpamapS added the help wanted label Dec 7, 2016

esabol mentioned this issue Dec 8, 2016

Support SSL connections to gearmand wcgallego/pecl-gearman#43

Open

p-alik added a commit to p-alik/gearmand that referenced this issue Mar 17, 2017

revert using of deprecated SSLv23_client_method in SSL_CTX_new

e0c5b1c

see: - https://www.openssl.org/docs/man1.0.2/ssl/SSLv23_client_method.html - gearman#63 (comment)

p-alik added a commit to p-alik/gearmand that referenced this issue Mar 18, 2017

remove enable_ssl patch

20701a3

gearman#63 (comment)

p-alik added a commit to p-alik/gearmand that referenced this issue Mar 20, 2017

revert using of deprecated SSLv23_client_method in SSL_CTX_new

8ec6b0e

see: - https://www.openssl.org/docs/man1.0.2/ssl/SSLv23_client_method.html - gearman#63 (comment)

p-alik added a commit to p-alik/gearmand that referenced this issue Mar 20, 2017

remove enable_ssl patch

39a3d65

gearman#63 (comment)

bmeynell mentioned this issue Dec 17, 2018

The feature of persistence plugins #227

Open

esabol mentioned this issue Nov 19, 2019

Add support for SSL connections to gearmand (Issue #43) wcgallego/pecl-gearman#72

Open

esabol added a commit to esabol/gearmand that referenced this issue Nov 27, 2019

Issue gearman#63: libgearman patch to support SSL connections in Gear…

7c261f1

…man PHP library

esabol added a commit to esabol/gearmand that referenced this issue Nov 27, 2019

Issue gearman#63: libgearman patch to support SSL connections in Gear…

220412d

…man PHP library

esabol added a commit to esabol/gearmand that referenced this issue Nov 27, 2019

Issue gearman#63: libgearman patch to support SSL connections in Gear…

f150439

…man PHP library

esabol mentioned this issue Nov 27, 2019

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

Merged

SpamapS added a commit that referenced this issue Feb 10, 2020

Merge pull request #257 from esabol/issue-63-set-ssl-libgearman-patch…

a40856d

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

esabol removed the help wanted label Dec 3, 2020

esabol closed this as completed Dec 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libgearman patch to support SSL connections in Gearman PHP library #63

libgearman patch to support SSL connections in Gearman PHP library #63

esabol commented Dec 7, 2016 •

edited

Loading

SpamapS commented Dec 7, 2016

p-alik commented Dec 7, 2016

p-alik commented Dec 8, 2016

esabol commented Dec 8, 2016

p-alik commented Dec 8, 2016

esabol commented Dec 8, 2016 •

edited

Loading

p-alik commented Dec 8, 2016

esabol commented Dec 12, 2016 •

edited

Loading

SpamapS commented Dec 13, 2016 via email

esabol commented Mar 3, 2017

BrianAker commented Mar 4, 2017 via email

p-alik commented Mar 16, 2017

p-alik commented Mar 16, 2017

esabol commented Mar 16, 2017

esabol commented Mar 16, 2017 •

edited

Loading

p-alik commented Mar 17, 2017

p-alik commented Mar 17, 2017

p-alik commented Mar 17, 2017

esabol commented Mar 17, 2017

esabol commented Mar 17, 2017

esabol commented Mar 17, 2017

esabol commented Mar 28, 2017 •

edited

Loading

p-alik commented Mar 29, 2017

esabol commented Mar 30, 2017

SpamapS commented Mar 30, 2017

Tyrael commented Dec 2, 2020

esabol commented Dec 3, 2020

libgearman patch to support SSL connections in Gearman PHP library #63

libgearman patch to support SSL connections in Gearman PHP library #63

Comments

esabol commented Dec 7, 2016 • edited Loading

SpamapS commented Dec 7, 2016

p-alik commented Dec 7, 2016

p-alik commented Dec 8, 2016

esabol commented Dec 8, 2016

p-alik commented Dec 8, 2016

esabol commented Dec 8, 2016 • edited Loading

p-alik commented Dec 8, 2016

esabol commented Dec 12, 2016 • edited Loading

SpamapS commented Dec 13, 2016 via email

esabol commented Mar 3, 2017

BrianAker commented Mar 4, 2017 via email

p-alik commented Mar 16, 2017

p-alik commented Mar 16, 2017

esabol commented Mar 16, 2017

esabol commented Mar 16, 2017 • edited Loading

p-alik commented Mar 17, 2017

p-alik commented Mar 17, 2017

p-alik commented Mar 17, 2017

esabol commented Mar 17, 2017

esabol commented Mar 17, 2017

esabol commented Mar 17, 2017

esabol commented Mar 28, 2017 • edited Loading

p-alik commented Mar 29, 2017

esabol commented Mar 30, 2017

SpamapS commented Mar 30, 2017

Tyrael commented Dec 2, 2020

esabol commented Dec 3, 2020

esabol commented Dec 7, 2016 •

edited

Loading

esabol commented Dec 8, 2016 •

edited

Loading

esabol commented Dec 12, 2016 •

edited

Loading

esabol commented Mar 16, 2017 •

edited

Loading

esabol commented Mar 28, 2017 •

edited

Loading