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

feat: support TLS server #565

Open
wants to merge 27 commits into
base: dev
Choose a base branch
from
Open

feat: support TLS server #565

wants to merge 27 commits into from

Conversation

leslie-fei
Copy link
Contributor

功能

gnet添加TLS Server支持, TLS库从golang官方库修改非阻塞版本, 改动golang tls源码部分并不算太多, tls库主要改动握手部分代码和编解码tls包

魔改TLS库部分

原来tls库等待client ack包是阻塞等待, 这里支持非阻塞需要拆分握手步骤
image
image
image

tls读取包之前需要先预读是否是一个完成的tls包, 如果不足就抛出ErrNotEnough, 等待下次包够了再去处理, 要么读完整包, 要么不读, 这样可以保证后面去conn中读取不会有半包情况
image

添加tlsConn实现gnet.Conn接口和tlsEventHandler

实现gnet的tls server 主要通过将原始gnet.Conn和EventHandler进行代理包装成tlsConn和tlsEventHandler, Engine Options添加WithTLSConfig
image
image

用户代码块

用户只要Run的时候WithTLSConfig 就可以实现TLS Server
image

@leslie-fei
Copy link
Contributor Author

还有些代码优化还没做, 有空看看实现思路和代码可不可行

@gh-translator
Copy link
Collaborator

🤖 Non-English text detected, translating...


There are still some code optimizations that haven't been done yet. I'll see if the implementation ideas and codes are feasible when I have time.

@leslie-fei
Copy link
Contributor Author

gnet_test添加了tls的单元测试

@gh-translator
Copy link
Collaborator

🤖 Non-English text detected, translating...


gnet_test adds unit testing for tls

@leslie-fei
Copy link
Contributor Author

PING

@panjf2000
Copy link
Owner

There seems to be a lot of CI failures, please fix that.

@leslie-fei
Copy link
Contributor Author

There seems to be a lot of CI failures, please fix that.

由于tls包是引用了golang1.22的tls包进行修改,他的代码风格不符合CI的代码风格验证,能把这个包的代码风格验证跳过吗,我在gnet_test加入了gnet tls的server用例,用gnet 启动TLS server,client用的golang的TLS client,能够跑过测试完成包的收发握手和编解码

@panjf2000
Copy link
Owner

Thanks for the efforts, I'll find some time next week to look into this PR.

@panjf2000
Copy link
Owner

Sorry for the delay, I'm still trying to spare some time for this, but got no luck, I expect to come back in a few days. Keep up the good work!

@leslie-fei
Copy link
Contributor Author

Sorry for the delay, I'm still trying to spare some time for this, but got no luck, I expect to come back in a few days. Keep up the good work!

OK

@leslie-fei
Copy link
Contributor Author

This PR is quite large and contains a lot of code. I could use your help. My idea is to minimize changes to the original code structure and wrap the original connection implementation to provide TLS functionality within TLS support.

@kolinfluence
Copy link

@leslie-fei can i ask some questions about this tls? can you put up an issue here?
https://github.com/leslie-fei/gnet/tree/dev

would like to use it now and help with testing

@leslie-fei
Copy link
Contributor Author

leslie-fei commented Apr 21, 2024

@kolinfluence Sure, I'm glad to have you help test the TLS-related module functions. I have enabled the issue tracker for you.

@kolinfluence
Copy link

kolinfluence commented Apr 21, 2024

@leslie-fei asked. would really like to test it on my end. been waiting for tls for gnet for 4 years. pls check the question on the issue tracker. thx for the work

not sure if u have seen this repo
https://github.com/luyu6056/tls
https://github.com/luyu6056/gnet

even the http2 is working but i cant seem to get so-reuseport to work. no idea why even when i set it to true.

can you see if there's anything useful from there too? thx

@leslie-fei
Copy link
Contributor Author

leslie-fei commented Apr 23, 2024

@panjf2000 有时间的话可以看看 leslie-fei#1 用tls功能实现了一个简单的HTTPs, 并且WRK测试过, 也跑过gnet_test用例

@gh-translator
Copy link
Collaborator

🤖 Non-English text detected, translating...


@panjf2000 If you have time, you can take a look at leslie-fei#1 A simple HTTPs is implemented using the tls function, and has been tested by WRK

@kolinfluence
Copy link

kolinfluence commented Apr 23, 2024

there are some issue with wrk if you try a few times with GOMAXPROCS=1.

it's not tested complete. there are issues. pls dont pull yet.

@panjf2000
Copy link
Owner

panjf2000 commented Apr 23, 2024

Thanks all for the efforts here!

I've been examining this TLS support lately, and I wonder if we can implement this feature outside the core gnet project and integrate it as a module, putting it in @gnet-io. We may need to make some code changes for gnet if necessary. Coping the whole tls library from go std to gnet doesn't look like a good idea on second thought, because we're coupling the std tls and gnet, and what's worse, std tls is a tremendous codebase. If anything goes wrong with the gnet with tls, not to mention that we have to debug the huge codebase and fix bugs, it may also affect those users who use gnet without tls.

@kolinfluence
Copy link

kolinfluence commented Apr 23, 2024

@panjf2000 sounds like a "good" idea for the moment but eventually hope it can be part of gnet. or gnet's own version of tls for use with gnet like ktls etc.

how about supporting ktls then? :D

i'm ok with it actually. and for the moment i think you are right. until u think it's ready then maybe can merge but still, u have a very good point for now.

however i dont think adding ktls will be a very huge overhead / more work than porting std crypto tls to this one. maybe someone can look into ktls then. there's a half done https://github.com/0-haha/gnet-tls-go1-20 here. not sure why it's not continued further.

p.s. : after serious consideration, you point is very valid and i totally agree with your suggestion. however, if you think tls codebase is too large. i do hope you can at least look at ktls and see if it's possible to integrate this. it shld run faster with ktls though with 2 alloc / op that's all.

@panjf2000
Copy link
Owner

panjf2000 commented Apr 23, 2024

@panjf2000 sounds like a "good" idea for the moment but eventually hope it can be part of gnet. or gnet's own version of tls for use with gnet like ktls etc.

how about supporting ktls then? :D

i'm ok with it actually. and for the moment i think you are right. until u think it's ready then maybe can merge but still, u have a very good point for now.

however i dont think adding ktls will be a very huge overhead / more work than porting std crypto tls to this one. maybe someone can look into ktls then. there's a half done 0-haha/gnet-tls-go1-20 here. not sure why it's not continued further.

I want to make more time for the TLS support of gnet in the near future, but again, I can't make any definite promise for that because I'm not a full-time open-source developer. Therefore, I'd again suggest you look up some alternatives for gnet with tls if you have urgent business requirements, it's also welcome to share the results if you find something, it could be helpful for us to develop our own TLS for gnet. I'm grateful for your (and all the others) interest and help with gnet , and I hope we can get this done eventually, to make gnet even better.

@kolinfluence

@leslie-fei
Copy link
Contributor Author

leslie-fei commented Apr 24, 2024

Thanks all for the efforts here!

I've been examining this TLS support lately, and I wonder if we can implement this feature outside the core gnet project and integrate it as a module, putting it in @gnet-io. We may need to make some code changes for gnet if necessary. Coping the whole tls library from go std to gnet doesn't look like a good idea on second thought, because we're coupling the std tls and gnet, and what's worse, std tls is a tremendous codebase. If anything goes wrong with the gnet with tls, not to mention that we have to debug the huge codebase and fix bugs, it may also affect those users who use gnet without tls.

@panjf2000 My modifications will not affect the original gnet code. I have not made any changes to gnet's external interfaces or internal code. Simply adding WithTLSConfig during Run enables TLS send and receive. In case of issues, it will only affect the TLS part and not impact existing users. I think it's also possible to put TLS support in gnet-io

@kolinfluence
Copy link

kolinfluence commented Apr 25, 2024

@panjf2000 i've tested the code program, pls look into the code and review. it's working fine. u shld be able to pull into gnet as part of it without needing to make it separate (at your pleasure) but do check out the code, i'm so amazed at the performance i think it's hard to believe. but it does match the overall expectation.

performance of non tls version is 165k req/s on my AMD Ryzen 5 7640HS w/ Radeon 760M Graphic,
and tls is around 105k req/s

(both results on a single core with GOMAXPROCS=1)

very good.

thx @leslie-fei

update : verified to work. @panjf2000 pls mention when an eta can be expected for this to be pulled into main repo. thx

@@ -127,6 +128,9 @@ type Options struct {
// Don't enable it unless you are 100% sure what you are doing.
// Note that this option is only available for stream-oriented protocol.
EdgeTriggeredIO bool

// TLSConfig support TLS
TLSConfig *tls.Config
Copy link
Owner

Choose a reason for hiding this comment

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

Make this an interface instead of a pointer of a specific struct, that way we can implement TLS outside the gnet and pass the interface to the gnet. Decoupling the TLS from gnet also enables the users to use other TLS implementations that implement the interface of gnet TLS.

@panjf2000
Copy link
Owner

@panjf2000 i've tested the code program, pls look into the code and review. it's working fine. u shld be able to pull into gnet as part of it without needing to make it separate (at your pleasure) but do check out the code, i'm so amazed at the performance i think it's hard to believe. but it does match the overall expectation.

performance of non tls version is 165k req/s on my AMD Ryzen 5 7640HS w/ Radeon 760M Graphic, and tls is around 105k req/s

(both results on a single core with GOMAXPROCS=1)

very good.

thx @leslie-fei

update : verified to work. @panjf2000 pls mention when an eta can be expected for this to be pulled into main repo. thx

As I said before, copying the whole Go std TLS into gnet is a bad idea. I'd like to find a way to implement the TLS outside gnet and integrate it when necessary.

@sprappcom
Copy link

@panjf2000 can u pull into gnet-io as u mentioned then? hope to use it as endorsed "officially" by you.

so how long more u need to assess the repo? i understand the importance of doing due diligence. no rush for quality. when can you get this done? pull into gnet-io

@panjf2000
Copy link
Owner

panjf2000 commented Apr 26, 2024

@panjf2000 can u pull into gnet-io as u mentioned then? hope to use it as endorsed "officially" by you.

so how long more u need to assess the repo? i understand the importance of doing due diligence. no rush for quality. when can you get this done? pull into gnet-io

I've created a new repository: tls where I expect to put the TLS implementation for gnet. We should move most of this PR there, meanwhile, I'm examining the source code of Go std TLS and kernel TLS.

@leslie-fei
Copy link
Contributor Author

@panjf2000 can u pull into gnet-io as u mentioned then? hope to use it as endorsed "officially" by you.
so how long more u need to assess the repo? i understand the importance of doing due diligence. no rush for quality. when can you get this done? pull into gnet-io

I've created a new repository: tls where I expect to put the TLS implementation for gnet. We should move most of this PR there, meanwhile, I'm examining the source code of Go std TLS and kernel TLS.

@panjf2000 gnet-io/tls#1 I have merged my relevant code into this TLS library, I hope you have the opportunity to review it.

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.

None yet

5 participants