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

fixed bug that client cannot receive right packages with ssl_async enabled #1774

Merged
merged 1 commit into from
May 28, 2023

Conversation

oyaya
Copy link

@oyaya oyaya commented May 5, 2023

issue: There is an error when tengine is running with TLSv1.3, "ssl_early_data on" and "ssl_async on;“. Client can't receive right packages and get the right answer.
nginx.conf:
image
echo -e "GET / HTTP/1.1\r\nHost: www.test-async.com\r\nConnection: close\r\n\r\n" > request.txt
echo|openssl s_client -connect 127.0.0.1:443 -sess_out session.pem -servername www.test-async.com

  • before bugfix:

openssl client error:
client_error1
client_error2
nginx error_log:
error_log1

  • after bugfix:

openssl client log:
client_right1
client_right2
nginx error_log:
right_log1
right_log2
nginx access_log:
right_access_log1

@oyaya
Copy link
Author

oyaya commented May 5, 2023

@chobits

@oyaya
Copy link
Author

oyaya commented May 23, 2023

@wangfakang

@oyaya
Copy link
Author

oyaya commented May 23, 2023

@yaoweibin

@chobits
Copy link
Member

chobits commented May 25, 2023

For ssl_async feature, the original PR can be found at #993.

@chobits
Copy link
Member

chobits commented May 25, 2023

All tengine test cases have passed, with the exception of the resolve_file.t test case, which may have failed due to the GitHub workflow environment blocking port 53 listening.

ACK

@chobits
Copy link
Member

chobits commented May 25, 2023

We strongly recommend adding your manual testing case to the tengine tests suite at tests/nginx-tests/tengine-tests/http_ssl_asynchronous_mode.t. The test suite can just verify that the ssl response is received.

@oyaya
Copy link
Author

oyaya commented May 26, 2023

哦,我可能没说清楚
条件为:当TLS协议为TLSv1.3 打开early_early_data on;并且本机(非keyless的方式)用硬件或者intel的cryptoNI等加解密加速设备时;
配置参考:
image

这时候代码流程:
ngx_ssl_handshake 通过c->ssl->try_early_data判断->ngx_ssl_try_early_data
->SSL_read_early_data(c->ssl->connection, &buf, 1, &readbytes) 如果返回的不是SSL_READ_EARLY_DATA_ERROR;
sslerr = SSL_get_error(c->ssl->connection, n); 有可能取到SSL_ERROR_WANT_ASYNC 也就是error.log里面的9, 这个时候需要有个地方设置回调并且return NGX_AGAIN;
image
image

tests/nginx-tests/tengine-tests/http_ssl_asynchronous_mode.t 中early_data是默认关闭的,没有写开启的指令,所以测试不到这个逻辑

@oyaya
Copy link
Author

oyaya commented May 26, 2023

@chobits 再看看

@oyaya
Copy link
Author

oyaya commented May 26, 2023

@wa5i 看看

@chobits chobits merged commit db0777a into alibaba:master May 28, 2023
@chobits
Copy link
Member

chobits commented May 28, 2023

tests/nginx-tests/tengine-tests/http_ssl_asynchronous_mode.t 中early_data是默认关闭的,没有写开启的指令,所以测试不到这个逻辑

完全同意。但有2个方法可以绕过。

  1. 同一个t里面可以反复启动nginx实例,不同的实例可以使用不同的配置,写起来稍微复杂
  2. 简单的方法是,再开一个类似http_ssl_asynchronous_mode_bugfix_<xxxx>.t 给early_data开启

加入测试的好处是,现在tengine项目已经接入github workflow,所有同目录下的*.t文件都会被自动测试,这样防止后续出现异常回滚


关于自动化测试:https://github.com/alibaba/tengine/actions/runs/4890602870/jobs/8730897619

image


逻辑review过,先合并了

@chobits chobits changed the title bugfix async ssl_early_data fixed bug that client cannot receive right packages with ssl_async enabled Jun 8, 2023
@chobits chobits added this to the 2.4.1 milestone Jun 9, 2023
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.

3 participants