Adding SSL support with official OpenResty SSL cosockets implementation #7
Conversation
|
@blablacio Thank you for the patch! Will you add a test case for it to the existing test suite? Also, I've noted a use of uninitialized Lua global variable "session" on the following line in your patch: local session, err = sock:sslhandshake(session, host, ssl_verify)Because you're not reusing SSL sessions here anyway, better feed the UPDATE Sorry, I actually meant passing |
|
Thanks for the input, you're right! I'll tweak code to your suggestion and add some tests. |
- Adding certificates from test suite for use with new tests - Fixing a minor issue in client.lua
|
Any chance to get this into the next release? |
| @@ -113,6 +114,10 @@ function _M.connect(self, uri, opts) | |||
| if pool then | |||
| sock_opts = { pool = pool } | |||
| end | |||
|
|
|||
| if opts.ssl_verify then | |||
| ssl_verify = opts.ssl_verify | |||
There was a problem hiding this comment.
Better assign true directly to the ssl_verify variable here?
|
@blablacio Sorry for my delay on my side. Made some more comments on your latest patch :) Thanks for your time! |
|
Thanks for the input, I'll fix the issues tomorrow and get back to you with updated patch. |
|
@blablacio I'm not saying I'm seeing failures. I mean for testing |
|
@agentzh Ah, sure, will add that test too. Good point. |
|
@blablacio Hopefully these can get included in the next |
|
@agentzh Sounds fantastic! I'll push changes in the next 24 hours. Hopefully you can get the set-misc-nginx-module changes into the next release too :) |
|
@blablacio No need to be too hurry; we still have time :) Yes, sure, the ngx_set_misc too :) |
- ssl_verify is now set to true directly - passing false to sslhandshake method as first parameter as we're not reusing sessions - adding some checks to ensure SSL sockets are supported in ngx_lua or throw an error otherwise when using the wss scheme - adding a new test with SSL verification and failed handshake
|
@agentzh Just added the improvements we discussed, let me know if I missed something when you have a moment to review :) |
| if not ssl_support then | ||
| return nil, "ngx_lua 0.9.11+ required for SSL sockets" | ||
| end | ||
| local session, err = sock:sslhandshake(false, host, ssl_verify) |
There was a problem hiding this comment.
Better rename the return value session to ok since now sslhandshake never returns the session because we have disabled it by specifying the false argument.
|
@agentzh Sorry for the delay, just got around to fixing the issues you pointed out. |
|
@blablacio Thank you very much! Looking good to me now :) I'll try merging this very soon :) |
…s (i.e., the "wss://" scheme). thanks Vladislav Manchev for the patch in #7.
|
@blablacio Just applied a slightly modified version of your patch to git master. Thank you for your contribution! |
Adding support for SSL: