Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Major cosockets patch #290

Closed
wants to merge 30 commits into from
Closed

Conversation

aviramc
Copy link

@aviramc aviramc commented Oct 7, 2013

This includes the following changes:

  • Added support for SSL sockets (includes the verification of the remote end's certificate). Related to issue SSL/TLS support for cosocket API? #178
  • Setting different timeouts for receive and send.
  • Now read and write can be done from different Lua threads. Relates to issue Can't read/write to the same cosocket from different threads #241.
  • Added fake_close so that we won't be able to read again from the client socket after it is closed.
  • receive API - added the option 'bsd_read', which enables to receive the maximum number of bytes given (like the BSD API, not LuaSocket API).

The is a part of pull request #286

Aviram and others added 15 commits April 24, 2013 10:36
…header that determines whether or not to replace underscores with hyphens.

Previously, underscores were replaced unconditionally.
Currently each of the functions has another boolean argument. If it's false, underscores would not be touched. If it's true, they would.
The default value of the argument is true.
…ptions), in which the only possible option currently is replace_underscores.
 - Now receive and send operations can be done simultaneous from several threads.
 - A different timeout can be set for receive and send.
 - Added fake_close for the client socket (ngx.req.socket), so that a thread that receives on this socket can be notified that we don't want to read from it anymore.
…s to receive just like BSD's recv call, meaning the maximum number of bytes given.

This will only work only when a number is given as the first parameter for receive.
Note that this differs from the original LuaSocket API.
@bungle
Copy link
Member

bungle commented Oct 14, 2013

@agentzh I have not yet tested this patch (I will try it out soon). I suppose that you are reviewing the changes. Do you have an estimate when these changes are going to be pulled to OpenResty bundle release? I understand that changes are quite big, and that they warrant further investigation as this separation of read-write state affects all the libs, e.g. lua resty mysql. lua resty redis, and lua resty websocket. I may test this soon myself, and will report to this thread my findings.

@agentzh
Copy link
Member

agentzh commented Oct 14, 2013

@bungle I'm currently working on the pool-based connection number limiting feature in ngx_lua. I'll look into the patch for the full duplex support for cosocket after that.

aviram added 2 commits October 17, 2013 18:31
…cket is finalized, so that if `ngx_http_lua_wev_handler` runs after the raw downstream socket is closed, it won't try to call its read/write handler.
…or handling functions, as if both read and write are done simultaenously, they may override each others preapre_retvals function.
@subnetmarco
Copy link

👍

aviramc added 3 commits December 23, 2013 16:18
Also closing the connection if any error has happened during the handshake.
…is an optional argument in the options table of tcpsock:connect()
@blablacio
Copy link

Any updates on this?

I'm pretty eager to try it out as I already have a few use cases, but would rather prefer not to patch openresty manually on each update.

@agentzh
Copy link
Member

agentzh commented Apr 14, 2014

@blablacio No progress here yet. I've been busy with other things. Sorry.

@DorianGray
Copy link

FYI:: This can be worked around by setting up an http->https proxy for a custom location in nginx proper. While not optimal, it's quite fast...we had to do this because one of our services absolutely required https, even for internal calls.

@blablacio
Copy link

@DorianGray Not sure I follow, care to explain what you mean?

@DorianGray
Copy link

a very minimal example for http...

server {
listen port;
location / {
proxy_pass https://host:port;
}
}

then you would send http requests to this server and it would reverse proxy to the backend service that requires https.
ngx.socket.tcp().connect("localhost", port) or whatever
The major feature of this patch is the ability to use ssl. This is how you connect to ssl services without the patch.

@blablacio
Copy link

Okay, I see what you mean. The patch actually contains other bits that I'm using, so I wasn't sure.

Thanks for the clarification, it's greatly appreciated.

agentzh added a commit that referenced this pull request Jun 26, 2014
… writer "light thread" can operate on the same cosocket simultaneously. thanks shun zhang and aviramc for the original patches in #367 and #290, respectively.
@agentzh
Copy link
Member

agentzh commented Jun 26, 2014

@aviramc and others: I've come up with a patch for the full duplex cosocket feature mostly based on @ashun's patch in #367 and committed to the "duplex-socket" branch on GitHub (as commit 934e33e). It'll be great if you can review the patch and try it out on your side. Many thanks! And sorry for the long delay on my side.

@agentzh
Copy link
Member

agentzh commented Jul 1, 2014

FYI, the full-duplex stream-typed cosocket feature has already been included in the v0.9.9 release of ngx_lua.

@blablacio
Copy link

Excellent! What about SSL support?

I would like to test the new release, but I'm using the SSL patch too.

@agentzh
Copy link
Member

agentzh commented Jul 1, 2014

@blablacio The SSL cosocket support will probably land in the next release (0.9.10) or beyond. I am still thinking about the implementation details. I'm not happy with the current API in this pull request and I may probably introduce an explicit sslhandshake method for the cosocket object.

@agentzh
Copy link
Member

agentzh commented Jul 8, 2014

@aviramc I'm thinking about the following API for SSL cosockets:

    local ok, err = sock:connect(host, port)
    local session, err = sock:sslhandshake(reused_session, verify)

Basically, the sslhandshake function returns the SSL session (when successful) that can be later reused by feeding as the first (optional) argument. How to cache the session is left to the caller. Other configuration options can be later added as more function arguments or a dedicated option table for sslhandshake(), which may not appear in the first version of SSL cosockets. If the connection comes from connection pool in the connect() call and it has already been handshaked, then sslhandshake() returns immediately.

My plan is to include the first version of SSL cosockets in the 0.9.11 release of ngx_lua (I've already missed the 0.9.10 release's merge window).

What do you think?

@aviramc
Copy link
Author

aviramc commented Jul 9, 2014

@agentzh the API looks fine, but what are the additional arguments that can be passed? I think that one should be the name of the host that should be verified (I guess that the verify parameter is boolean).

@agentzh
Copy link
Member

agentzh commented Jul 9, 2014

@aviramc I'll keep the first implementation of SSL cosockets simple. Basically only the "session" and "verify_host" arguments (or just the first one) will be supported for now.

agentzh added a commit that referenced this pull request Jul 22, 2014
* added new method sslhandshake() to the stream-typed cosocket objects.
* added new configuration directives lua_ssl_trusted_certificate,
  lua_ssl_verify_depth, lua_ssl_crl, lua_ssl_protocols, and
  lua_ssl_ciphers.

Thanks aviramc for the original patch in #290.
@agentzh
Copy link
Member

agentzh commented Jul 22, 2014

@aviramc Okay, I've finally finished the first implementation of SSL cosockets based on your patch. It has been committed to the git branch ssl-cosocket as commit d10bcc7.

The sslhandshake method now has the following prototype:

local session, err = sock:sslhandshake(session, host, verify)

The first session argument as well as the return value are SSL sessions. They can be shared and recycled by Lua-land LRU caches like lua-resty-lrucache. Do not put the session objects into the ngx.shared.DICT because they're just userdata holding references to raw OpenSSL session objects.

The 2nd host argument both serves as the server name for SNI and as the name for checking the host in the server certificate (when the verify argument is true).

The 3rd verify argument takes a boolean value for checking the validity of the server certifcate, including whether is is being trusted and whether the host name matches (if the second host argument is specified).

All the 3 arguments are optional.

I haven't written formal documentation for this new feature yet. For now, the most comprehensive "documentation" is the new test cases: https://github.com/openresty/lua-nginx-module/blob/ssl-cosocket/t/129-ssl-socket.t

Also added new nginx config directives lua_ssl_trusted_certificate, lua_ssl_verify_depth, lua_ssl_crl, lua_ssl_protocols, and lua_ssl_ciphers, similar to their ngx_proxy module equivalents.

The ssl-cosocket branch of ngx_lua can still be compiled with nginx cores up to 0.8.54 but to enable all the features implemented on that branch, one needs to use at least nginx 1.7.0 (I used nginx 1.7.3 most of the time during the development).

@daurnimator Right now you need separate locations to have separate sets of trusted certificates. In the (near) future, we can add support for a 4th argument similar to luasec's "context" object (but not 100% compatible). Patches welcome :)

@lhmwzy The MySQL server allows SSL and non-SSL connections share the same server port by postponing the standard SSL handshake after its own (clear-text) handshake. Please check out MySQL's documentation on this trick: http://dev.mysql.com/doc/internals/en/ssl.html This also justifies the design of a separate sslhandshake method instead of bundling it with the connect method. You're very welcome to make use of the new sslhandshake method to implement MySQL SSL connections in lua-resty-mysql. I am not 100% sure that it will work and I'd love to hear about your findings.

Feedback welcome!

agentzh added a commit that referenced this pull request Jul 22, 2014
* added new method sslhandshake() to the stream-typed cosocket objects.
* added new configuration directives lua_ssl_trusted_certificate,
  lua_ssl_verify_depth, lua_ssl_crl, lua_ssl_protocols, and
  lua_ssl_ciphers.

Thanks aviramc for the original patch in #290.
@agentzh
Copy link
Member

agentzh commented Jul 22, 2014

Regarding my previous comment, I've just fixed a bug in the commit for SSL cosockets that the sslhandshake method did not handle timeout errors at all. I've re-committed a new version of the patch that adds a timer to the ssl-cosocket git branch as commit 0a82152. Please try out the latest ssl-cosocket branch instead. (The standard ngx_proxy module also has a similar issue which I've just submitted a patch to fix it too: http://mailman.nginx.org/pipermail/nginx-devel/2014-July/005627.html )

Thanks!

agentzh added a commit that referenced this pull request Jul 23, 2014
* added new method sslhandshake() to the stream-typed cosocket objects.
* added new configuration directives lua_ssl_trusted_certificate,
  lua_ssl_verify_depth, lua_ssl_crl, lua_ssl_protocols, and
  lua_ssl_ciphers.

Thanks aviramc for the original patch in #290.
@agentzh
Copy link
Member

agentzh commented Jul 23, 2014

Okay, I've just merged the ssl-cosocket branch back into master. This will get included in the next ngx_lua release, v0.9.11.

Also, I've fixed a memory leak regression (when connection pool is used and lua_code_cache is off) in my cosocket patch in the new commit a6a0ed5.

@agentzh
Copy link
Member

agentzh commented Aug 4, 2014

@lhmwzy I've prepared the following patch for lua-resty-mysql that can connect to MySQL via SSL with the latest git master HEAD of ngx_lua:

diff --git a/lib/resty/mysql.lua b/lib/resty/mysql.lua
index a629daa..2e310de 100644
--- a/lib/resty/mysql.lua
+++ b/lib/resty/mysql.lua
@@ -38,6 +38,7 @@ local STATE_CONNECTED = 1
 local STATE_COMMAND_SENT = 2

 local COM_QUERY = 0x03
+local CLIENT_SSL = 0x0800

 local SERVER_MORE_RESULTS_EXISTS = 8

@@ -136,7 +137,7 @@ local function _dump(data)
     local len = #data
     local bytes = new_tab(len, 0)
     for i = 1, len do
-        bytes[i] = strbyte(data, i)
+        bytes[i] = string.format("%x", strbyte(data, i))
     end
     return concat(bytes, " ")
 end
@@ -175,11 +176,13 @@ local function _send_packet(self, req, size)

     self.packet_no = self.packet_no + 1

-    --print("packet no: ", self.packet_no)
+    -- print("packet no: ", self.packet_no)

     local packet = _set_byte3(size) .. strchar(self.packet_no) .. req

-    --print("sending packet...")
+    -- print("sending packet: ", _dump(packet))
+
+    -- print("sending packet... of size " .. #packet)

     return sock:send(packet)
 end
@@ -562,9 +565,10 @@ function _M.connect(self, opts)
     pos = pos + 9 -- skip filler

     -- two lower bytes
-    self._server_capabilities, pos = _get_byte2(packet, pos)
+    local server_capabilities
+    server_capabilities, pos = _get_byte2(packet, pos)

-    --print("server capabilities: ", self._server_capabilities)
+    -- print(string.format("server capabilities: %#x", server_capabilities))

     self._server_lang = strbyte(packet, pos)
     pos = pos + 1
@@ -578,8 +582,8 @@ function _M.connect(self, opts)
     local more_capabilities
     more_capabilities, pos = _get_byte2(packet, pos)

-    self._server_capabilities = bor(self._server_capabilities,
-                                    lshift(more_capabilities, 16))
+    server_capabilities = bor(server_capabilities,
+                              lshift(more_capabilities, 16))

     --print("server capabilities: ", self._server_capabilities)

@@ -598,13 +602,36 @@ function _M.connect(self, opts)
     scramble = scramble .. scramble_part2
     --print("scramble: ", _dump(scramble))

+    -- local client_flags = self._server_capabilities
+    local client_flags = 0x3f7cf;
+
+    if opts.ssl then
+        if band(server_capabilities, CLIENT_SSL) == 0 then
+            return nil, "ssl disabled on server"
+        end
+
+        -- send a SSL Request Packet
+        local req = _set_byte4(bor(client_flags, CLIENT_SSL))
+                    .. _set_byte4(self._max_packet_size)
+                    .. "\0" -- TODO: add support for charset encoding
+                    .. strrep("\0", 23)
+
+        local packet_len = 4 + 4 + 1 + 23
+        local bytes, err = _send_packet(self, req, packet_len)
+        if not bytes then
+            return nil, "failed to send client authentication packet: " .. err
+        end
+
+        local ok, err = sock:sslhandshake(false)
+        if not ok then
+            return nil, "failed to do ssl handshake: " .. (err or "")
+        end
+    end
+
     local password = opts.password or ""

     local token = _compute_token(password, scramble)

-    -- local client_flags = self._server_capabilities
-    local client_flags = 260047;
-
     --print("token: ", _dump(token))

     local req = _set_byte4(client_flags)
diff --git a/t/ssl.t b/t/ssl.t
new file mode 100644
index 0000000..1f7f998
--- /dev/null
+++ b/t/ssl.t
@@ -0,0 +1,89 @@
+# vim:set ft= ts=4 sw=4 et:
+
+use Test::Nginx::Socket::Lua;
+use Cwd qw(cwd);
+
+repeat_each(2);
+
+plan tests => repeat_each() * (3 * blocks() + 4);
+
+my $pwd = cwd();
+
+our $HttpConfig = qq{
+    resolver \$TEST_NGINX_RESOLVER;
+    lua_package_path "$pwd/lib/?.lua;$pwd/t/lib/?.lua;;";
+    lua_package_cpath "/usr/local/openresty-debug/lualib/?.so;/usr/local/openresty/lualib/?.so;;";
+};
+
+$ENV{TEST_NGINX_RESOLVER} = '8.8.8.8';
+$ENV{TEST_NGINX_MYSQL_PORT} ||= 3306;
+$ENV{TEST_NGINX_MYSQL_HOST} ||= '127.0.0.1';
+$ENV{TEST_NGINX_MYSQL_PATH} ||= '/var/run/mysql/mysql.sock';
+
+#log_level 'warn';
+
+no_long_string();
+no_shuffle();
+check_accum_error_log();
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: send query w/o result set
+--- http_config eval: $::HttpConfig
+--- config
+    location /t {
+        content_by_lua '
+            local mysql = require "resty.mysql"
+            local db = mysql:new()
+
+            db:set_timeout(1000) -- 1 sec
+
+            local ok, err, errno, sqlstate = db:connect({
+                host = "$TEST_NGINX_MYSQL_HOST",
+                port = $TEST_NGINX_MYSQL_PORT,
+                database = "ngx_test",
+                user = "ngx_test",
+                password = "ngx_test",
+                ssl = true,
+            })
+
+            if not ok then
+                ngx.say("failed to connect: ", err, ": ", errno, " ", sqlstate)
+                return
+            end
+
+            ngx.say("connected to mysql ", db:server_ver(), ".")
+
+            local bytes, err = db:send_query("drop table if exists cats")
+            if not bytes then
+                ngx.say("failed to send query: ", err)
+            end
+
+            ngx.say("sent ", bytes, " bytes.")
+
+            local res, err, errno, sqlstate = db:read_result()
+            if not res then
+                ngx.say("bad result: ", err, ": ", errno, ": ", sqlstate, ".")
+            end
+
+            local ljson = require "ljson"
+            ngx.say("result: ", ljson.encode(res))
+
+            local ok, err = db:close()
+            if not ok then
+                ngx.say("failed to close: ", err)
+                return
+            end
+        ';
+    }
+--- request
+GET /t
+--- response_body_like chop
+^connected to mysql \d\.\S+\.
+sent 30 bytes\.
+result: \{"affected_rows":0,"insert_id":0,"server_status":2,"warning_count":[01]\}$
+--- no_error_log
+[error]
+

SSL verification support has not yet been added nor tested yet, but should work easily as well :)

@agentzh
Copy link
Member

agentzh commented Aug 5, 2014

@lhmwzy Okay, I've added both "ssl" and "ssl_verify" boolean-value options to lua-resty-mysql's connect method and committed the patch to the ssl git branch of the lua-resty-mysql repos:

openresty/lua-resty-mysql@ecc50848

Please try it out on your side :)

@agentzh
Copy link
Member

agentzh commented Aug 5, 2014

Hey guys! I've just documented the SSL cosocket feature here:

2b40b44

Please review the current behavior and API semantics before it becomes too late to change ;)

Thank you for your time!

@aviramc
Copy link
Author

aviramc commented Apr 1, 2015

Will reopen new pull requests for each of the required features

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants