feature: add function `tcpsock:setclientcert`, reimplemented `tcpsock:sslhandshake` with FFI. by dndx · Pull Request #278 · openresty/lua-resty-core

dndx · 2019-09-18T23:44:40Z

functions using FFI.

I hereby granted the copyright of the changes in this pull request
to the authors of this lua-resty-core project.

Sister PR: openresty/lua-nginx-module#1602

Note that stream support is intentionally omitted in this PR to keep the size down. I will help porting this over to stream after it is merged.

lib/resty/core/socket_tcp.lua

thibaultcha

A couple of comments to get your thoughts @dndx; I have a local branch in which I already made edits that I would like to suggest in this PR, maybe as a new commit that we can squash at merge time, if that sounds good to you.

lib/resty/core/socket_tcp.lua

dndx · 2020-01-07T22:33:21Z

@thibaultcha Squashing at merge sounds good to me :)

README.markdown

spacewander · 2020-11-23T02:55:17Z

@dndx
Do you have time to solve the conflict? Thanks!

dndx · 2020-11-23T07:56:58Z

@spacewander not a problem, I will resolve it today!

doujiang24 · 2020-12-15T16:10:36Z

lib/resty/core/socket/tcp.lua

+        end
+
+        sock.tlshandshake = tlshandshake
+        sock.sslhandshake = sslhandshake


@dndx I think it's better to move this module into this file? we can save two set field operations then.
https://github.com/openresty/lua-resty-core/blob/master/lib/resty/core/socket.lua

agentzh · 2020-12-15T20:02:19Z

It's confusing to have both tlshandshake and sslhandshake.

zhuizhuhaomeng · 2022-03-04T13:42:49Z

@chronolaw would you please rebase to the master ?

zhuizhuhaomeng · 2022-03-04T13:44:21Z

lib/resty/core/socket.lua

+local openssl_error_code = ffi_new("int[1]")
+
+
+local function setclientcert(cosocket, cert, pkey)


Suggested change

local function setclientcert(cosocket, cert, pkey)

local function set_client_cert(cosocket, cert, pkey)

This is to keep the style consistent with existing cosocket API function names. e.g. tcpsock:getreusedtimes

lib/resty/core/socket.lua

zhuizhuhaomeng · 2022-03-04T14:28:54Z

lib/resty/core/socket.lua

+    local n = select("#", ...)
+    if not cosocket or n > 1 then
+        error("ngx.socket sslhandshake: expecting 1 ~ 5 arguments " ..
+              "(including the object), but seen " .. (cosocket and 5 + n or 0))
+    end


Suggested change

local n = select("#", ...)

if not cosocket or n > 1 then

error("ngx.socket sslhandshake: expecting 1 ~ 5 arguments " ..

"(including the object), but seen " .. (cosocket and 5 + n or 0))

end

I don't think we need this test.
And if you want to test the number, it is
if not cosocket or n > 0

@chronolaw I think @zhuizhuhaomeng is correct here. Since the Lua implementation of sslhandshake already takes 5 arguments, it will be incorrect to check not cosocket or n > 1 as that indicates there are 7 arguments passed. It should be not cosocket or n > 0 instead.

zhuizhuhaomeng · 2022-03-04T14:47:29Z

lib/resty/core/socket.lua

+        end
+
+        if session_ptr[0] == nil then
+            return nil


should return the error at the same time

See my reviews, I think we can remove this if session_ptr[0] == nil then branch altogether and keep only the assert below.

Actually, I had a closer look at this logic compared to https://github.com/openresty/lua-nginx-module/blob/master/src/ngx_http_lua_socket_tcp.c#L1935-L1956. The issue is that:

In the original C implementation, when the server did not turn on session reuse, the sslhandshake function returned a lightuserdata that is NULL and did not set the __gc meta method on it. This is different than returning a Lua nil value which indicates an error.

Therefore, we should mimic this behavior by returning the session_ptr[0] cdata directly and not call ffi.gc on it. From the user's perspective, the handshake is still a success.

chronolaw · 2022-03-06T09:21:11Z

It just follows the idiom of openresty, like settimeout sslhandshake, these APIs don't use '_'.
#278 (comment)

chronolaw · 2022-03-06T09:24:11Z

It makes test case #31 happy in 129-ssl-socket.t of lua-nginx-module.

#278 (comment)

chronolaw · 2022-03-06T09:26:57Z

It works like original C function ngx_http_lua_ssl_handshake_retval_handler in ngx_http_lua_socket_tcp.c, just for compatibility.

#278 (comment)

…k:sslhandshake` with FFI This adds support for setting client certificate/private key that will be used later for mutual TLS handshake with a server. Also, the `tcpsock:sslhandshake` implementation has been rewritten to use FFI C API to be more performant and easier to maintain. Also see: openresty/lua-resty-core#278 Co-authored-by: Chrono Law <chrono.law@konghq.com>

…sslhandshake` with FFI This adds support for setting client certificate/private key that will be used later for mutual TLS handshake with a server. Also, the `tcpsock:sslhandshake` implementation has been rewritten to use FFI C API to be more performant and easier to maintain. Also see: openresty/lua-nginx-module#1602 Co-authored-by: Chrono Law <chrono.law@konghq.com>

dndx · 2022-03-07T05:25:46Z

@zhuizhuhaomeng Could you please take another look at this PR? It has been rebased and squashed and now passed all CI tests with it's lua-nginx-module counterpart.

…k:sslhandshake` with FFI This adds support for setting client certificate/private key that will be used later for mutual TLS handshake with a server. Also, the `tcpsock:sslhandshake` implementation has been rewritten to use FFI C API to be more performant and easier to maintain. Also see: openresty/lua-resty-core#278 Co-authored-by: Chrono Law <chrono.law@konghq.com>

zhuizhuhaomeng · 2022-03-15T14:21:29Z

merged with the following patch

--- a/.travis.yml
+++ b/.travis.yml
@@ -65,7 +65,7 @@ install:
   - git clone https://github.com/openresty/openresty.git ../openresty
   - git clone https://github.com/openresty/openresty-devel-utils.git
   - git clone https://github.com/simpl/ngx_devel_kit.git ../ndk-nginx-module
-  - git clone -b feat/cosocket_tlshandshake https://github.com/dndx/lua-nginx-module.git ../lua-nginx-module
+  - git clone https://github.com/dndx/lua-nginx-module.git ../lua-nginx-module
   - git clone https://github.com/openresty/no-pool-nginx.git ../no-pool-nginx
   - git clone https://github.com/openresty/echo-nginx-module.git ../echo-nginx-module
   - git clone https://github.com/openresty/lua-resty-lrucache.git

dndx mentioned this pull request Sep 18, 2019

cosocket: add function tcpsock:setclientcert, reimplemented tcpsock:sslhandshake with FFI. openresty/lua-nginx-module#1602

Closed

dndx force-pushed the feat/cosocket_tlshandshake branch from 68ca312 to 3b5e699 Compare September 25, 2019 20:22

dndx marked this pull request as ready for review September 25, 2019 21:11

dndx force-pushed the feat/cosocket_tlshandshake branch from da4f865 to 9e5a7d7 Compare September 25, 2019 21:23

kikito reviewed Oct 24, 2019

View reviewed changes

lib/resty/core/socket_tcp.lua Outdated Show resolved Hide resolved

lib/resty/core/socket_tcp.lua Outdated Show resolved Hide resolved

dndx force-pushed the feat/cosocket_tlshandshake branch from 9e5a7d7 to d27f2af Compare October 24, 2019 21:27

dndx mentioned this pull request Oct 30, 2019

feat(patches) add cosocket mTLS handshake patches Kong/openresty-patches#57

Merged

dndx force-pushed the feat/cosocket_tlshandshake branch 5 times, most recently from 8d4d81b to e3c7f85 Compare December 6, 2019 00:02

thibaultcha reviewed Jan 7, 2020

View reviewed changes

lib/resty/core/socket_tcp.lua Outdated Show resolved Hide resolved

lib/resty/core/socket_tcp.lua Outdated Show resolved Hide resolved

agentzh requested changes Jan 13, 2020

View reviewed changes

README.markdown Outdated Show resolved Hide resolved

thibaultcha mentioned this pull request Apr 16, 2020

Enable use of a Cassandra proxy using the resty cluster interface thibaultcha/lua-cassandra#138

Closed

thibaultcha force-pushed the feat/cosocket_tlshandshake branch from 414879b to 71d1257 Compare July 6, 2020 01:43

membphis mentioned this pull request Aug 14, 2020

feature: support talking to etcd via TLS. apache/apisix#158

Closed

spacewander approved these changes Nov 23, 2020

View reviewed changes

dndx force-pushed the feat/cosocket_tlshandshake branch from 71d1257 to 42710cb Compare November 24, 2020 09:37

doujiang24 reviewed Dec 15, 2020

View reviewed changes

dndx mentioned this pull request Dec 18, 2021

Will lua tcpsocksslhandshake be able to support mtls? openresty/lua-nginx-module#1781

Open

chronolaw mentioned this pull request Dec 22, 2021

feat mtls cosocket Kong/lua-kong-nginx-module#23

Closed

This was referenced Jan 13, 2022

feat(mtls) port mtls cosocket Kong/lua-kong-nginx-module#25

Closed

feat(mtls) port mtls cosocket Kong/lua-kong-nginx-module#26

Closed

dndx force-pushed the feat/cosocket_tlshandshake branch from 83c570e to a08907b Compare January 21, 2022 05:24

dndx force-pushed the feat/cosocket_tlshandshake branch from 0a4f02f to e9df5ed Compare January 28, 2022 10:19

chronolaw force-pushed the feat/cosocket_tlshandshake branch 11 times, most recently from a4acb9c to 00361a0 Compare March 4, 2022 08:39

zhuizhuhaomeng requested changes Mar 4, 2022

View reviewed changes

dndx changed the title ~~feature: implement the tcpsock:tlshandshake and tcpsock:sslhandshake~~ feature: add function tcpsock:setclientcert, reimplemented tcpsock:sslhandshake with FFI. Mar 5, 2022

dndx force-pushed the feat/cosocket_tlshandshake branch from f68dc90 to cdb745d Compare March 7, 2022 05:00

dndx force-pushed the feat/cosocket_tlshandshake branch from cdb745d to 68bf4bf Compare March 7, 2022 05:00

dndx requested review from agentzh, pintsized and zhuizhuhaomeng March 7, 2022 05:02

doujiang24 approved these changes Mar 12, 2022

View reviewed changes

zhuizhuhaomeng closed this Mar 15, 2022

		local openssl_error_code = ffi_new("int[1]")


		local function setclientcert(cosocket, cert, pkey)

	local function setclientcert(cosocket, cert, pkey)
	local function set_client_cert(cosocket, cert, pkey)

Conversation

dndx commented Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thibaultcha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dndx commented Jan 7, 2020

Uh oh!

Uh oh!

spacewander commented Nov 23, 2020

Uh oh!

dndx commented Nov 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agentzh commented Dec 15, 2020

Uh oh!

zhuizhuhaomeng commented Mar 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chronolaw commented Mar 6, 2022

Uh oh!

chronolaw commented Mar 6, 2022

Uh oh!

chronolaw commented Mar 6, 2022

Uh oh!

dndx commented Mar 7, 2022

Uh oh!

zhuizhuhaomeng commented Mar 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

dndx commented Sep 18, 2019 •

edited

Loading