Skip to content

Conversation

@bzp2010
Copy link
Contributor

@bzp2010 bzp2010 commented Oct 23, 2025

This PR provides additional ALPN extension options for tcpsock:sslhandshake, enabling cosocket to establish TLS handshakes and subsequent communication with servers that have specific ALPN negotiation requirements.

Specifically, HTTP/2 requires TLS ALPN negotiation before establishing an HTTP/2 connection and sending requests via the PRI prologue. If the client cannot handshake with ALPN h2, HTTP/2 over TLS cannot be implemented. In addition, there are other protocols that have this requirement.

https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids

The PR includes modifications to the FFI C functions and associated tests, which assert against debug logs in Nginx.

https://github.com/nginx/nginx/blob/78d1ab5a2c00839a36ff6bac661d9785fce3c1a4/src/http/modules/ngx_http_ssl_module.c#L455

As far as I know, I might also need to update the Lua API in lua-resty-core to accommodate these FFI modifications.
However, I'm not certain which should happen first. Strictly speaking, without modifying that Lua API, we'll never pass the tests. But modifying the Lua API would make it depend on an FFI modification that doesn't yet exist. I'm not sure how to proceed, but I'll submit a PR there as well.

The proposed API change is as follows: it remains the old designs, with a new parameter alpn added at the end.

function tcpsock:sslhandshake(reused_session, server_name, ssl_verify, send_status_req, alpn) end

It accepts such parameters and converts them to ngx_str_t[1] before the Lua FFI call.

local alpn_input = { "h2" , "http/1.1" }

It will be constructed as an ngx_str_t to be passed to the FFI C function.

if options.alpn then
    local alpn = {}
    for _, proto_str in ipairs(options.alpn)  do
        alpn[#alpn + 1] = string.len(proto_str)
        for _, proto_byte in ipairs({ string.byte(proto_str, 1, #proto_str) }) do
            alpn[#alpn + 1] = proto_byte
        end
    end
    alpn_str[0].data = ffi.new("unsigned char[?]", #alpn, alpn)
    alpn_str[0].len = #alpn
else
    alpn_str[0].data = nil
    alpn_str[0].len = 0
end

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

@bzp2010 bzp2010 marked this pull request as draft October 23, 2025 05:30
@bzp2010 bzp2010 marked this pull request as ready for review October 23, 2025 05:37
@bzp2010
Copy link
Contributor Author

bzp2010 commented Oct 23, 2025

Modifications to the Lua API: openresty/lua-resty-core#513

@bzp2010
Copy link
Contributor Author

bzp2010 commented Oct 23, 2025

The current issues here and openresty/lua-resty-core#513 's testing errors caused by code dependencies. I'm unsure how to proceed. cc @zhuizhuhaomeng

Regarding the API modification, it is backward compatible. The new optional arugment is added at the end of tcpsock:sslhandshake. When omitted, it will skip sending the ALPN extension.
Additionally, the documentation updates in the README are now ready.

@zhuizhuhaomeng
Copy link
Contributor

you can change the git url in the .travis.yml

@bzp2010
Copy link
Contributor Author

bzp2010 commented Oct 24, 2025

After modifying the CI to use my lua-resty-core dev branch and fixing some errors there, I have passed the tests.

https://github.com/openresty/lua-nginx-module/pull/2460/checks?check_run_id=53599698217

image

The test suite 4 shows identical results to the main branch commit. Some session cb yield-related tests failed in the 143 test file, which suggests this issue was not introduced by the current PR.


I believe this PR is ready for substantive review. I'm unsure whether I or you should ultimately update the repository address in .travis.yaml back to the correct one during merging.
If you need me to handle that, I'll do it. However, I think the PR (openresty/lua-resty-core#513) in the lua-resty-core repository likely needs to be merged first. That PR has passed testing.

I believe this may not have caused any additional side effects, so I'd like to know if it can be accepted and ultimately released in the 1.29 release cycle. If any points are identified that require modification, I will address them immediately.

Thanks for your help @zhuizhuhaomeng

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.

2 participants