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.

Sign up for GitHub

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

Jump to bottom

cosocket: add function tcpsock:setclientcert, reimplemented tcpsock:sslhandshake with FFI. #1602

Closed
wants to merge 2 commits into from
Closed

cosocket: add function tcpsock:setclientcert, reimplemented tcpsock:sslhandshake with FFI. #1602

wants to merge 2 commits into from

Conversation

dndx
Copy link
Member

@dndx dndx commented Sep 18, 2019

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

Sister PR: openresty/lua-resty-core#278

This was referenced Sep 19, 2019
Closed
Closed
@dndx dndx force-pushed the feat/cosocket_tlshandshake branch 5 times, most recently from a6c4f3e to b407ffb Compare September 25, 2019 21:11
@dndx dndx mentioned this pull request Sep 25, 2019
Closed
@dndx dndx marked this pull request as ready for review September 25, 2019 21:11
@dndx dndx force-pushed the feat/cosocket_tlshandshake branch from b407ffb to 2d00cf6 Compare September 25, 2019 22:28
kikito referenced this pull request in openresty/lua-resty-core Oct 24, 2019
@dndx
feature: implement the tcpsock:tlshandshake and tcpsock:sslhandshake
d75e6f6 
functions using FFI.
@dndx dndx force-pushed the feat/cosocket_tlshandshake branch 2 times, most recently from ac60ef5 to 6d2a474 Compare October 24, 2019 22:17
@dndx dndx mentioned this pull request Oct 30, 2019
Merged
@dndx dndx force-pushed the feat/cosocket_tlshandshake branch from 6d2a474 to 9ccbe85 Compare December 6, 2019 00:02
thibaultcha
thibaultcha reviewed Jan 8, 2020
View reviewed changes
t/162-socket-tls-handshake.t
@@ -0,0 +1,439 @@
# vim:set ft= ts=4 sw=4 et fdm=marker:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if this new test suite belongs to lua-resty-core instead. After all, all FFI-based APIs are tested there, and not in lua-nginx-module.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I thought about this initially, but seems that this clear split is not being followed by existing tests like https://github.com/openresty/lua-nginx-module/blob/master/t/154-semaphore.t.

But, I don't have strong feeling about where to keep the cases, if moving is a better in your opinion then let's move it.

@bungle bungle mentioned this pull request Jan 9, 2020
Closed
@thibaultcha thibaultcha mentioned this pull request Apr 16, 2020
Closed
@mergify
Copy link

mergify bot commented Jun 26, 2020

This pull request is now in conflict :(

@mergify mergify bot added the conflict label Jun 26, 2020
@shtaif
Copy link

shtaif commented Jul 1, 2020

@dndx @kikito @thibaultcha thank you for the great work! ✌️ ✌️
May I kindly ask, would this feature make it possible to dynamically determine the TLS client certificate to use when proxying requests over to an upstream (on a per-request basis)?

@thibaultcha thibaultcha force-pushed the feat/cosocket_tlshandshake branch from d618c47 to f1d1b63 Compare July 6, 2020 02:11
@mergify mergify bot removed the conflict label Jul 6, 2020
@thibaultcha
Copy link
Member

thibaultcha commented Jul 6, 2020 •
edited
Loading

I just rebased this PR (and sibling PR) on top of the latest master branch and fixed the conflicts now that OpenResty 1.17.8.1 has been released (for potential users of the patch).

@dndx
Copy link
Member Author

dndx commented Jul 6, 2020

@shtaif This PR is for cosocket connections only. For setting client certs to use in upstream connections, https://github.com/Kong/lua-kong-nginx-module#restykongtlsset_upstream_cert_and_key can do it. You just need to apply the required patches from https://github.com/Kong/kong-build-tools/blob/master/openresty-patches/patches/1.15.8.3/nginx-1.15.8_01-upstream_client_certificate_and_ssl_verify.patch.

@shtaif
Copy link

shtaif commented Jul 21, 2020

@shtaif This PR is for cosocket connections only. For setting client certs to use in upstream connections, https://github.com/Kong/lua-kong-nginx-module#restykongtlsset_upstream_cert_and_key can do it. You just need to apply the required patches from https://github.com/Kong/kong-build-tools/blob/master/openresty-patches/patches/1.15.8.3/nginx-1.15.8_01-upstream_client_certificate_and_ssl_verify.patch.

@dndx this was indeed the right way to go, thanks!

@mergify
Copy link

mergify bot commented Jul 23, 2020

This pull request is now in conflict :(

@mergify mergify bot added conflict and removed conflict labels Jul 23, 2020
@dndx dndx changed the title cosocket: add function tcpsock:tlshandshake, retired the Lua C API based tcpsock:sslhandshake implementation. cosocket: add function tcpsock:setclientcert, reimplemented tcpsock:sslhandshake with FFI function. Mar 5, 2022
@dndx dndx changed the title cosocket: add function tcpsock:setclientcert, reimplemented tcpsock:sslhandshake with FFI function. cosocket: add function tcpsock:setclientcert, reimplemented tcpsock:sslhandshake with FFI. Mar 5, 2022
@dndx
Copy link
Member Author

dndx commented Mar 5, 2022 •
edited
Loading

@zhuizhuhaomeng the title has been changed. We can squash the change if you prefer.

@chronolaw chronolaw force-pushed the feat/cosocket_tlshandshake branch 4 times, most recently from 004e4c2 to 53c0999 Compare March 7, 2022 02:49
@dndx dndx force-pushed the feat/cosocket_tlshandshake branch 2 times, most recently from 87d4259 to 220d9ee Compare March 7, 2022 03:33
dndx added a commit to dndx/lua-resty-core that referenced this pull request Mar 7, 2022
@dndx @chronolaw
cosocket: add function tcpsock:setclientcert, reimplemented `tcpsock:…
cdb745d 
…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 dndx force-pushed the feat/cosocket_tlshandshake branch 2 times, most recently from 51e897f to d677c00 Compare March 7, 2022 05:00
dndx added a commit to dndx/lua-resty-core that referenced this pull request Mar 7, 2022
@dndx @chronolaw
cosocket: add function tcpsock:setclientcert, reimplemented `tcpsock:…
68bf4bf 
…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>
@zhuizhuhaomeng
Copy link
Contributor

You can squash the changes.

@zhuizhuhaomeng
Copy link
Contributor

zhuizhuhaomeng commented Mar 9, 2022 •
edited
Loading

@dndx would you please add the script to generate the mtls certificates to t/cert.
when the certs expired, we can regenerate them.

@dndx @chronolaw
cosocket: add function tcpsock:setclientcert, reimplemented `tcpsoc…
fe1a1fe 
…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>
@dndx dndx force-pushed the feat/cosocket_tlshandshake branch from d677c00 to fe1a1fe Compare March 13, 2022 15:16
@dndx
tests: add script to automatically regenerate mtls related certificates
9a53f92 
Currently the script generates certs that are valid for 20 years, and
should be enough for a while.
@dndx
Copy link
Member Author

dndx commented Mar 13, 2022

@zhuizhuhaomeng The script has been added. One of the Travis run failed because of "negative credit balance" for Travis. However, considering the other build did passed, I think the new script generated certs should be good:

https://app.travis-ci.com/github/openresty/lua-nginx-module/builds/247813268

@zhuizhuhaomeng
Copy link
Contributor

merged with the following patch

--- a/.travis.yml
+++ b/.travis.yml
@@ -88,7 +88,7 @@ install:
   - git clone https://github.com/openresty/rds-json-nginx-module.git ../rds-json-nginx-module
   - git clone https://github.com/openresty/srcache-nginx-module.git ../srcache-nginx-module
   - git clone https://github.com/openresty/redis2-nginx-module.git ../redis2-nginx-module
-  - git clone -b feat/cosocket_tlshandshake https://github.com/dndx/lua-resty-core.git ../lua-resty-core
+  - git clone https://github.com/dndx/lua-resty-core.git ../lua-resty-core
   - git clone https://github.com/openresty/lua-resty-lrucache.git ../lua-resty-lrucache
   - git clone https://github.com/openresty/lua-resty-mysql.git ../lua-resty-mysql
   - git clone https://github.com/openresty/lua-resty-string.git ../lua-resty-string

@mergify
Copy link

mergify bot commented Mar 15, 2022

This pull request is now in conflict :(

@mergify mergify bot added the conflict label Mar 15, 2022
@Lekensteyn Lekensteyn mentioned this pull request Aug 18, 2022
Open
@Bec-k
Copy link

Bec-k commented Sep 6, 2022

Why this wasn't merged?

tkan145 added a commit to tkan145/APIcast that referenced this pull request Apr 30, 2024
@tkan145
[test] fix failed test in apicast-policy-tls.t
a8d3a6c 
In 1.21.4 openresty sock:handshake return cdata type instead of
userdata

Reference: openresty/lua-nginx-module#1602
tkan145 added a commit to tkan145/APIcast that referenced this pull request May 1, 2024
@tkan145
[t] fix failed test in apicast-policy-tls.t
37dbe50 
In 1.21.4 openresty sock:handshake return cdata type instead of
userdata

Reference: openresty/lua-nginx-module#1602
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kikito kikito kikito left review comments

@thibaultcha thibaultcha thibaultcha left review comments

@chronolaw chronolaw chronolaw left review comments

@pintsized pintsized pintsized approved these changes

@doujiang24 doujiang24 doujiang24 approved these changes

@zhuizhuhaomeng zhuizhuhaomeng zhuizhuhaomeng approved these changes

@spacewander spacewander spacewander approved these changes

@rainingmaster rainingmaster rainingmaster approved these changes

Assignees
No one assigned
Labels
conflict could-be-merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.