leslie-tsang commented on code in PR #9903:
URL: https://github.com/apache/apisix/pull/9903#discussion_r1280048243
##########
apisix/init.lua:
##########
@@ -192,10 +211,16 @@ function _M.http_ssl_phase()
if err then
core.log.error("failed to fetch ssl config: ", err)
end
+ core.log.error("failed to find any SSL certificate by SNI: ", sni)
ngx_exit(-1)
end
-end
+ ok ,err =
apisix_ssl.set_protocols_by_clienthello(ngx_ctx.matched_ssl.value.ssl_protocols)
Review Comment:
```suggestion
ok, err =
apisix_ssl.set_protocols_by_clienthello(ngx_ctx.matched_ssl.value.ssl_protocols)
```
##########
apisix/ssl.lua:
##########
@@ -38,8 +39,14 @@ local pkey_cache = core.lrucache.new {
local _M = {}
-function _M.server_name()
- local sni, err = ngx_ssl.server_name()
+function _M.server_name(clienthello)
+ local sni, err
+ if clienthello then
+ sni, err = ngx_ssl_client.get_client_hello_server_name()
+ else
+ sni, err = ngx_ssl.server_name()
+
Review Comment:
```suggestion
```
Plz remove the blank line.
##########
apisix/ssl.lua:
##########
@@ -56,6 +63,12 @@ function _M.server_name()
return sni
end
+function _M.set_protocols_by_clienthello(ssl_protocols)
+ if ssl_protocols then
+ return ngx_ssl_client.set_protocols(ssl_protocols)
+ end
+ return true
+end
Review Comment:
```suggestion
function _M.set_protocols_by_clienthello(ssl_protocols)
if ssl_protocols then
return ngx_ssl_client.set_protocols(ssl_protocols)
end
return true
end
```
Please follow the [code
style](https://github.com/apache/apisix/blob/master/CODE_STYLE.md)
##########
docs/zh/latest/ssl-protocol.md:
##########
@@ -0,0 +1,343 @@
+---
+title: SSL 协议
+---
+
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+`APISIX` 支持，还支持动态的为每一个 SNI 指定不同的 TLS 协议版本。
+
+**为了安全考虑，APISIX 默认使用的加密套件不支持 TLSv1.1 以及更低的版本。**
+**如果你需要启用 TLS 1.1 协议，请在 config.yaml 的配置项 apisix.ssl.ssl_ciphers 增加 TLSv1.1
协议所支持的加密套件。**
Review Comment:
```suggestion
**为了安全考虑，APISIX 默认使用的加密套件不支持 TLSv1.1 以及更低的版本。**
**如果你需要启用 TLSv1.1 协议，请在 config.yaml 的配置项 apisix.ssl.ssl_ciphers 增加 TLSv1.1
协议所支持的加密套件。**
```
##########
apisix/ssl/router/radixtree_sni.lua:
##########
@@ -206,13 +206,35 @@ function _M.match_and_set(api_ctx, match_only, alt_sni)
end
end
- local matched_ssl = api_ctx.matched_ssl
- core.log.info("debug - matched: ", core.json.delay_encode(matched_ssl,
true))
+ core.log.info("debug - matched: ",
core.json.delay_encode(api_ctx.matched_ssl, true))
if match_only then
return true
end
+ ok, err = _M.set(api_ctx.matched_ssl, sni)
+ if not ok then
+ return false, err
+ end
+
+ return true
+end
+
+
+function _M.set(matched_ssl, sni)
+ if not matched_ssl then
+ return false, "not found any matched ssl certificate"
+ end
+ local err,ok
Review Comment:
```suggestion
local ok, err
```
##########
docs/zh/latest/ssl-protocol.md:
##########
@@ -0,0 +1,343 @@
+---
+title: SSL 协议
+---
+
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+`APISIX` 支持，还支持动态的为每一个 SNI 指定不同的 TLS 协议版本。
+
+**为了安全考虑，APISIX 默认使用的加密套件不支持 TLSv1.1 以及更低的版本。**
+**如果你需要启用 TLS 1.1 协议，请在 config.yaml 的配置项 apisix.ssl.ssl_ciphers 增加 TLSv1.1
协议所支持的加密套件。**
Review Comment:
```suggestion
**为了安全考虑，APISIX 默认使用的加密套件不支持 TLSv1.1 以及更低的版本。**
**如果你需要启用 TLSv1.1 协议，请在 config.yaml 的配置项 apisix.ssl.ssl_ciphers 增加 TLSv1.1
协议所支持的加密套件。**
```
##########
apisix/init.lua:
##########
@@ -192,10 +211,16 @@ function _M.http_ssl_phase()
if err then
core.log.error("failed to fetch ssl config: ", err)
end
+ core.log.error("failed to find any SSL certificate by SNI: ", sni)
ngx_exit(-1)
end
-end
+ ok ,err =
apisix_ssl.set_protocols_by_clienthello(ngx_ctx.matched_ssl.value.ssl_protocols)
Review Comment:
BTW, I'm not sure if I should use `ngx_ctx` or `api_ctx` as a params.
##########
docs/en/latest/ssl-protocol.md:
##########
@@ -0,0 +1,343 @@
+---
+title: SSL Protocol
+---
+
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+`APISIX` supports set TLS protocol and also supports dynamically specifying
different TLS protocol versions for each SNI.
Review Comment:
```suggestion
`APISIX` supports set TLS protocol and also supports dynamically specifying
different TLS protocol versions for each
[SNI](https://en.wikipedia.org/wiki/Server_Name_Indication).
```
Would be better ?
##########
apisix/ssl.lua:
##########
@@ -14,9 +14,10 @@
-- See the License for the specific language governing permissions and
-- limitations under the License.
--
-local core = require("apisix.core")
-local ngx_ssl = require("ngx.ssl")
-local secret = require("apisix.secret")
+local core = require("apisix.core")
+local ngx_ssl = require("ngx.ssl")
+local secret = require("apisix.secret")
+local ngx_ssl_client = require("ngx.ssl.clienthello")
Review Comment:
```suggestion
local core = require("apisix.core")
local secret = require("apisix.secret")
local ngx_ssl = require("ngx.ssl")
local ngx_ssl_client = require("ngx.ssl.clienthello")
```
##########
docs/en/latest/ssl-protocol.md:
##########
@@ -0,0 +1,343 @@
+---
+title: SSL Protocol
+---
+
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+-->
+
+`APISIX` supports set TLS protocol and also supports dynamically specifying
different TLS protocol versions for each SNI.
+
+**For security reasons, the encryption suite used by default in `APISIX` does
not support TLSv1.1 and lower versions.**
+**If you need to enable the TLS 1.1 protocol, please add the encryption suite
supported by the TLSv1.1 protocol to the configuration item
`apisix.ssl.ssl_ciphers` in `config.yaml`.**
Review Comment:
```suggestion
**For security reasons, the encryption suite used by default in `APISIX`
does not support TLSv1.1 and lower versions.**
**If you need to enable the TLSv1.1 protocol, please add the encryption
suite supported by the TLSv1.1 protocol to the configuration item
`apisix.ssl.ssl_ciphers` in `config.yaml`.**
```
##########
apisix/init.lua:
##########
@@ -192,10 +211,16 @@ function _M.http_ssl_phase()
if err then
core.log.error("failed to fetch ssl config: ", err)
end
+ core.log.error("failed to find any SSL certificate by SNI: ", sni)
Review Comment:
```suggestion
core.log.error("failed to match any SSL certificate by SNI: ", sni)
```
##########
apisix/ssl/router/radixtree_sni.lua:
##########
@@ -206,13 +206,35 @@ function _M.match_and_set(api_ctx, match_only, alt_sni)
end
end
- local matched_ssl = api_ctx.matched_ssl
- core.log.info("debug - matched: ", core.json.delay_encode(matched_ssl,
true))
+ core.log.info("debug - matched: ",
core.json.delay_encode(api_ctx.matched_ssl, true))
if match_only then
return true
end
+ ok, err = _M.set(api_ctx.matched_ssl, sni)
+ if not ok then
+ return false, err
+ end
+
+ return true
+end
+
+
+function _M.set(matched_ssl, sni)
+ if not matched_ssl then
+ return false, "not found any matched ssl certificate"
Review Comment:
```suggestion
return false, "failed to match ssl certificate"
```
Would be better ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
