fix(mtls) update to use newer mTLS api #89
Conversation
The code path requires unmerged commits to both lua-nginx-module and lua-resty-core to work. Previously those commits introduced an alternative handshake method, `tlshandshake`. They have since been revised, in the hope that the feature will reach an OpenResty mainline release soon.
|
|
|
This can wait until the mTLS API arrives in mainline OpenResty. |
| else | ||
| session, err = sock:sslhandshake(nil, https_sni, | ||
| self.checks.active.https_verify_certificate) | ||
| ok, err = sock:setclientcert(self.ssl_cert, self.ssl_key) |
There was a problem hiding this comment.
Idea: what if we used feature detection (some combo of if type(sock.setclientcert) == "function" and if type(sock.tlshandshake) == "function") instead of a hard-coded change? In my mind then we would have the flexibility to merge+release this now rather than having one more transitive dependency to carefully update in lockstep with Kong. Replacing the feature detection with the hard-coded behavior could be left as a TODO once OpenResty+Kong are all on the same page, socket feature-wise.
|
Note: we'll also need to cherry-pick this commit to backport the change to the 1.x tree since Kong is still on 1.x. |
|
Hi @pintsized ! Thanks again for bringing this up 🏆 |
This revises the mTLS code to use the newer version of that feature (i.e.
setclientcertinstead oftlshandshake).The relevant PRs are here and here.
However, the tests in this repo do not touch this code path, which is a little alarming, but also perhaps understandable since the feature isn't available in OpenResty mainline.
Merging this will be harmless to anyone not using mTLS, but will break for anyone using the old patch set. The reason for this PR is to try to unravel this tech debt as we push towards proper cosocket mTLS support in OpenResty.