You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "AlinsRan (via GitHub)" <gi...@apache.org> on 2023/07/26 03:49:40 UTC
[GitHub] [apisix] AlinsRan opened a new pull request, #9903: feat: support host level dynamic setting of tls protocol version
AlinsRan opened a new pull request, #9903:
URL: https://github.com/apache/apisix/pull/9903
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Fixes
### Checklist
- [ ] I have explained the need for this PR and the problem it solves
- [ ] I have explained the changes or the new features added to this PR
- [ ] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [ ] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] leslie-tsang commented on a diff in pull request #9903: feat: support host level dynamic setting of tls protocol version
Posted by "leslie-tsang (via GitHub)" <gi...@apache.org>.
leslie-tsang commented on code in PR #9903:
URL: https://github.com/apache/apisix/pull/9903#discussion_r1280175604
##########
apisix/init.lua:
##########
@@ -178,11 +178,30 @@ end
function _M.http_ssl_phase()
+ local ok, err = router.router_ssl.set(ngx.ctx.matched_ssl)
+ if not ok then
+ if err then
+ core.log.error("failed to fetch ssl config: ", err)
+ end
+ ngx_exit(-1)
+ end
+end
+
Review Comment:
```suggestion
```
Need two blank line between func.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] leslie-tsang commented on a diff in pull request #9903: feat: support host level dynamic setting of tls protocol version
Posted by "leslie-tsang (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 merged pull request #9903: feat: support host level dynamic setting of tls protocol version
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 merged PR #9903:
URL: https://github.com/apache/apisix/pull/9903
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] AlinsRan commented on a diff in pull request #9903: feat: support host level dynamic setting of tls protocol version
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #9903:
URL: https://github.com/apache/apisix/pull/9903#discussion_r1281471181
##########
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, "failed to match ssl certificate"
+ end
+ local ok, err
Review Comment:
Line 242 has.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] leslie-tsang commented on a diff in pull request #9903: feat: support host level dynamic setting of tls protocol version
Posted by "leslie-tsang (via GitHub)" <gi...@apache.org>.
leslie-tsang commented on code in PR #9903:
URL: https://github.com/apache/apisix/pull/9903#discussion_r1281466682
##########
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, "failed to match ssl certificate"
+ end
+ local ok, err
Review Comment:
unused var `ok` here.
##########
apisix/schema_def.lua:
##########
@@ -790,6 +790,15 @@ _M.ssl = {
enum = {1, 0},
default = 1
},
+ ssl_protocols = {
+ description = "set ssl protocols",
+ type = "array",
+ maxItems = 3,
+ uniqueItems = true,
+ items = {
+ enum = {"TLSv1.1","TLSv1.2", "TLSv1.3"}
Review Comment:
```suggestion
enum = {"TLSv1.1", "TLSv1.2", "TLSv1.3"}
```
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] AlinsRan commented on a diff in pull request #9903: feat: support host level dynamic setting of tls protocol version
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #9903:
URL: https://github.com/apache/apisix/pull/9903#discussion_r1281472050
##########
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, "failed to match ssl certificate"
+ end
+ local ok, err
Review Comment:
` ok, err = _M.set_cert_and_key(sni, new_ssl_value)`
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kayx23 commented on a diff in pull request #9903: feat: support host level dynamic setting of tls protocol version
Posted by "kayx23 (via GitHub)" <gi...@apache.org>.
kayx23 commented on code in PR #9903:
URL: https://github.com/apache/apisix/pull/9903#discussion_r1280170935
##########
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](https://en.wikipedia.org/wiki/Server_Name_Indication).
+
+**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`.**
+
+## ssl_protocols Configuration
+
+### Static Configuration
+
+The `ssl_protocols` parameter in the static configuration `config.yaml` applies to the entire APISIX, but cannot be dynamically modified. It only takes effect when the matching SSL resource does not set `ssl_protocols`.
+
+```yaml
+apisix:
+ ssl:
+ ssl_protocols: TLSv1.2 TLSv1.3 # default TLSv1.2 TLSv1.3
+```
+
+### Dynamic Configuration
+
+Use the `ssl_protocols` field in the `ssl` resource to dynamically specify different TLS protocol versions for each SNI.
+
+* Specify the `test.com` domain uses the TLSv1.2 and TLSv1.3.
+
+```bash
+{
+ "cert": "$cert",
+ "key": "$key",
+ "snis": ["test.com"],
+ "ssl_protocols": [
+ "TLSv1.2",
+ "TLSv1.3"
+ ]
+}
+```
+
+### Notes
+
+- Dynamic configuration has a higher priority than static configuration. When the ssl_protocols configuration item in the ssl resource is not empty, the static configuration will be overridden.
+- The static configuration applies to the entire APISIX and requires a restart of APISIX to take effect.
+- Dynamic configuration can control the TLS protocol version of each SNI in a fine-grained manner and can be dynamically modified, which is more flexible than static configuration.
+
+## Examples
+
+### How to specify the TLSv1.1 protocol
+
+There are some old clients that still use the lower-level TLSv1.1 protocol version, while new products use higher security-level TLS protocol versions. Enabling TLSv1.1 for new products may bring some security risks. To ensure the security of the API, we need to flexibly switch between protocol versions.
+For example: test.com is the domain name used by old clients and needs to be configured for TLSv1.1, while test2.com belongs to new products and supports TLSv1.2 and TLSv1.3 protocols.
+
+1. config.yaml configuration.
+
+```yaml
+apisix:
+ ssl:
+ ssl_protocols: TLSv1.3
+ # ssl_ciphers is for reference only
+ ssl_ciphers: ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-SHA:ECDHE-ECDSA-AES256-SHA:DHE-RSA-AES256-SHA:DHE-DSS-AES256-SHA
+```
+
+2. Specify the TLSv1.1 protocol version for the test.com domain.
+
+```bash
+curl http://127.0.0.1:9180/apisix/admin/ssls/1 \
+-H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+ "cert" : "'"$(cat server.crt)"'",
+ "key": "'"$(cat server.key)"'",
+ "snis": ["test.com"],
+ "ssl_protocols": [
+ "TLSv1.1"
+ ]
+}'
+```
+
+3. Create an SSL object for test.com without specifying the TLS protocol version, which will use the static configuration by default.
+
+```bash
+curl http://127.0.0.1:9180/apisix/admin/ssls/1 \
+-H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+{
+ "cert" : "'"$(cat server2.crt)"'",
+ "key": "'"$(cat server2.key)"'",
+ "snis": ["test2.com"]
+}'
+```
+
+3. Access Verification
Review Comment:
```
3. Access Verification
```
There's already a number 3 above: `3. Create an SSL object for test.com without specifying the TLS protocol version, which will use the static ...`
Same for the Chinese doc.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] AlinsRan commented on a diff in pull request #9903: feat: support host level dynamic setting of tls protocol version
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #9903:
URL: https://github.com/apache/apisix/pull/9903#discussion_r1283293265
##########
apisix/init.lua:
##########
@@ -178,11 +178,31 @@ end
function _M.http_ssl_phase()
+ local ok, err = router.router_ssl.set(ngx.ctx.matched_ssl)
+ if not ok then
+ if err then
+ core.log.error("failed to fetch ssl config: ", err)
+ end
+ ngx_exit(-1)
+ end
+end
+
+
+function _M.http_ssl_protocols_phase()
Review Comment:
Good.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9903: feat: support host level dynamic setting of tls protocol version
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9903:
URL: https://github.com/apache/apisix/pull/9903#discussion_r1281813886
##########
apisix/init.lua:
##########
@@ -178,11 +178,31 @@ end
function _M.http_ssl_phase()
+ local ok, err = router.router_ssl.set(ngx.ctx.matched_ssl)
+ if not ok then
+ if err then
+ core.log.error("failed to fetch ssl config: ", err)
+ end
+ ngx_exit(-1)
+ end
+end
+
+
+function _M.http_ssl_protocols_phase()
Review Comment:
is `http_ssl_client_hello_phase` 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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kayx23 commented on a diff in pull request #9903: feat: support host level dynamic setting of tls protocol version
Posted by "kayx23 (via GitHub)" <gi...@apache.org>.
kayx23 commented on code in PR #9903:
URL: https://github.com/apache/apisix/pull/9903#discussion_r1280180126
##########
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](https://en.wikipedia.org/wiki/Server_Name_Indication).
+
+**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`.**
+
+## ssl_protocols Configuration
+
+### Static Configuration
+
+The `ssl_protocols` parameter in the static configuration `config.yaml` applies to the entire APISIX, but cannot be dynamically modified. It only takes effect when the matching SSL resource does not set `ssl_protocols`.
+
+```yaml
+apisix:
+ ssl:
+ ssl_protocols: TLSv1.2 TLSv1.3 # default TLSv1.2 TLSv1.3
+```
+
+### Dynamic Configuration
+
+Use the `ssl_protocols` field in the `ssl` resource to dynamically specify different TLS protocol versions for each SNI.
+
+* Specify the `test.com` domain uses the TLSv1.2 and TLSv1.3.
Review Comment:
```suggestion
Specify the `test.com` domain uses the TLSv1.2 and TLSv1.3:
```
A lot of these don't need bullet points. Same for below.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9903: feat: support host level dynamic setting of tls protocol version
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9903:
URL: https://github.com/apache/apisix/pull/9903#discussion_r1281758681
##########
apisix/init.lua:
##########
@@ -192,10 +212,16 @@ function _M.http_ssl_phase()
if err then
core.log.error("failed to fetch ssl config: ", err)
end
+ core.log.error("failed to match 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)
+ if not ok then
+ core.log.error("failed to set ssl protocols: ", err)
+ ngx_exit(-1)
+ end
+end
Review Comment:
need a blank below
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kayx23 commented on a diff in pull request #9903: feat: support host level dynamic setting of tls protocol version
Posted by "kayx23 (via GitHub)" <gi...@apache.org>.
kayx23 commented on code in PR #9903:
URL: https://github.com/apache/apisix/pull/9903#discussion_r1280160656
##########
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](https://en.wikipedia.org/wiki/Server_Name_Indication).
+
+**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`.**
+
+## ssl_protocols Configuration
+
+### Static Configuration
+
+The `ssl_protocols` parameter in the static configuration `config.yaml` applies to the entire APISIX, but cannot be dynamically modified. It only takes effect when the matching SSL resource does not set `ssl_protocols`.
+
+```yaml
+apisix:
+ ssl:
+ ssl_protocols: TLSv1.2 TLSv1.3 # default TLSv1.2 TLSv1.3
+```
+
+### Dynamic Configuration
+
+Use the `ssl_protocols` field in the `ssl` resource to dynamically specify different TLS protocol versions for each SNI.
+
+* Specify the `test.com` domain uses the TLSv1.2 and TLSv1.3.
+
+```bash
+{
+ "cert": "$cert",
+ "key": "$key",
+ "snis": ["test.com"],
+ "ssl_protocols": [
+ "TLSv1.2",
+ "TLSv1.3"
+ ]
+}
+```
+
+### Notes
+
+- Dynamic configuration has a higher priority than static configuration. When the ssl_protocols configuration item in the ssl resource is not empty, the static configuration will be overridden.
Review Comment:
```suggestion
- Dynamic configuration has a higher priority than static configuration. When the `ssl_protocols` configuration item in the ssl resource is not empty, the static configuration will be overridden.
```
##########
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](https://en.wikipedia.org/wiki/Server_Name_Indication).
+
+**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`.**
+
+## ssl_protocols Configuration
+
+### Static Configuration
+
+The `ssl_protocols` parameter in the static configuration `config.yaml` applies to the entire APISIX, but cannot be dynamically modified. It only takes effect when the matching SSL resource does not set `ssl_protocols`.
+
+```yaml
+apisix:
+ ssl:
+ ssl_protocols: TLSv1.2 TLSv1.3 # default TLSv1.2 TLSv1.3
+```
+
+### Dynamic Configuration
+
+Use the `ssl_protocols` field in the `ssl` resource to dynamically specify different TLS protocol versions for each SNI.
+
+* Specify the `test.com` domain uses the TLSv1.2 and TLSv1.3.
+
+```bash
+{
+ "cert": "$cert",
+ "key": "$key",
+ "snis": ["test.com"],
+ "ssl_protocols": [
+ "TLSv1.2",
+ "TLSv1.3"
+ ]
+}
+```
+
+### Notes
+
+- Dynamic configuration has a higher priority than static configuration. When the ssl_protocols configuration item in the ssl resource is not empty, the static configuration will be overridden.
+- The static configuration applies to the entire APISIX and requires a restart of APISIX to take effect.
+- Dynamic configuration can control the TLS protocol version of each SNI in a fine-grained manner and can be dynamically modified, which is more flexible than static configuration.
+
+## Examples
+
+### How to specify the TLSv1.1 protocol
+
+There are some old clients that still use the lower-level TLSv1.1 protocol version, while new products use higher security-level TLS protocol versions. Enabling TLSv1.1 for new products may bring some security risks. To ensure the security of the API, we need to flexibly switch between protocol versions.
+For example: test.com is the domain name used by old clients and needs to be configured for TLSv1.1, while test2.com belongs to new products and supports TLSv1.2 and TLSv1.3 protocols.
+
+1. config.yaml configuration.
Review Comment:
```suggestion
1. `config.yaml` configuration.
```
##########
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](https://en.wikipedia.org/wiki/Server_Name_Indication).
+
+**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`.**
+
+## ssl_protocols Configuration
+
+### Static Configuration
+
+The `ssl_protocols` parameter in the static configuration `config.yaml` applies to the entire APISIX, but cannot be dynamically modified. It only takes effect when the matching SSL resource does not set `ssl_protocols`.
+
+```yaml
+apisix:
+ ssl:
+ ssl_protocols: TLSv1.2 TLSv1.3 # default TLSv1.2 TLSv1.3
+```
+
+### Dynamic Configuration
+
+Use the `ssl_protocols` field in the `ssl` resource to dynamically specify different TLS protocol versions for each SNI.
+
+* Specify the `test.com` domain uses the TLSv1.2 and TLSv1.3.
+
+```bash
+{
+ "cert": "$cert",
+ "key": "$key",
+ "snis": ["test.com"],
+ "ssl_protocols": [
+ "TLSv1.2",
+ "TLSv1.3"
+ ]
+}
+```
+
+### Notes
+
+- Dynamic configuration has a higher priority than static configuration. When the ssl_protocols configuration item in the ssl resource is not empty, the static configuration will be overridden.
+- The static configuration applies to the entire APISIX and requires a restart of APISIX to take effect.
+- Dynamic configuration can control the TLS protocol version of each SNI in a fine-grained manner and can be dynamically modified, which is more flexible than static configuration.
+
+## Examples
+
+### How to specify the TLSv1.1 protocol
+
+There are some old clients that still use the lower-level TLSv1.1 protocol version, while new products use higher security-level TLS protocol versions. Enabling TLSv1.1 for new products may bring some security risks. To ensure the security of the API, we need to flexibly switch between protocol versions.
+For example: test.com is the domain name used by old clients and needs to be configured for TLSv1.1, while test2.com belongs to new products and supports TLSv1.2 and TLSv1.3 protocols.
Review Comment:
```suggestion
For example, consider two domain names: `test.com`, utilized by legacy clients requiring TLSv1.1 configuration, and `test2.com`, associated with new products that support TLSv1.2 and TLSv1.3 protocols.
```
##########
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](https://en.wikipedia.org/wiki/Server_Name_Indication).
+
+**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`.**
+
+## ssl_protocols Configuration
+
+### Static Configuration
+
+The `ssl_protocols` parameter in the static configuration `config.yaml` applies to the entire APISIX, but cannot be dynamically modified. It only takes effect when the matching SSL resource does not set `ssl_protocols`.
+
+```yaml
+apisix:
+ ssl:
+ ssl_protocols: TLSv1.2 TLSv1.3 # default TLSv1.2 TLSv1.3
+```
+
+### Dynamic Configuration
+
+Use the `ssl_protocols` field in the `ssl` resource to dynamically specify different TLS protocol versions for each SNI.
+
+* Specify the `test.com` domain uses the TLSv1.2 and TLSv1.3.
+
+```bash
+{
+ "cert": "$cert",
+ "key": "$key",
+ "snis": ["test.com"],
+ "ssl_protocols": [
+ "TLSv1.2",
+ "TLSv1.3"
+ ]
+}
+```
+
+### Notes
+
+- Dynamic configuration has a higher priority than static configuration. When the ssl_protocols configuration item in the ssl resource is not empty, the static configuration will be overridden.
+- The static configuration applies to the entire APISIX and requires a restart of APISIX to take effect.
Review Comment:
```suggestion
- The static configuration applies to the entire APISIX and requires a reload of APISIX to take effect.
```
##########
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](https://en.wikipedia.org/wiki/Server_Name_Indication).
+
+**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`.**
+
+## ssl_protocols Configuration
+
+### Static Configuration
+
+The `ssl_protocols` parameter in the static configuration `config.yaml` applies to the entire APISIX, but cannot be dynamically modified. It only takes effect when the matching SSL resource does not set `ssl_protocols`.
+
+```yaml
+apisix:
+ ssl:
+ ssl_protocols: TLSv1.2 TLSv1.3 # default TLSv1.2 TLSv1.3
+```
+
+### Dynamic Configuration
+
+Use the `ssl_protocols` field in the `ssl` resource to dynamically specify different TLS protocol versions for each SNI.
+
+* Specify the `test.com` domain uses the TLSv1.2 and TLSv1.3.
+
+```bash
+{
+ "cert": "$cert",
+ "key": "$key",
+ "snis": ["test.com"],
+ "ssl_protocols": [
+ "TLSv1.2",
+ "TLSv1.3"
+ ]
+}
+```
+
+### Notes
+
+- Dynamic configuration has a higher priority than static configuration. When the ssl_protocols configuration item in the ssl resource is not empty, the static configuration will be overridden.
+- The static configuration applies to the entire APISIX and requires a restart of APISIX to take effect.
+- Dynamic configuration can control the TLS protocol version of each SNI in a fine-grained manner and can be dynamically modified, which is more flexible than static configuration.
+
+## Examples
+
+### How to specify the TLSv1.1 protocol
+
+There are some old clients that still use the lower-level TLSv1.1 protocol version, while new products use higher security-level TLS protocol versions. Enabling TLSv1.1 for new products may bring some security risks. To ensure the security of the API, we need to flexibly switch between protocol versions.
Review Comment:
```suggestion
While newer products utilize higher security-level TLS protocol versions, there are still legacy clients that rely on the lower-level TLSv1.1 protocol. However, enabling TLSv1.1 for new products presents potential security risks. In order to maintain the security of the API, it is crucial to have the ability to seamlessly switch between different protocol versions based on specific requirements and circumstances.
```
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] AlinsRan commented on a diff in pull request #9903: feat: support host level dynamic setting of tls protocol version
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #9903:
URL: https://github.com/apache/apisix/pull/9903#discussion_r1283293620
##########
apisix/init.lua:
##########
@@ -192,10 +212,16 @@ function _M.http_ssl_phase()
if err then
core.log.error("failed to fetch ssl config: ", err)
end
+ core.log.error("failed to match 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)
+ if not ok then
+ core.log.error("failed to set ssl protocols: ", err)
+ ngx_exit(-1)
+ end
+end
Review Comment:
Done.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] AlinsRan commented on pull request #9903: feat: support host level dynamic setting of tls protocol version
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on PR #9903:
URL: https://github.com/apache/apisix/pull/9903#issuecomment-1659602960
@kayx23 Please help review the doc.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] AlinsRan commented on a diff in pull request #9903: feat: support host level dynamic setting of tls protocol version
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #9903:
URL: https://github.com/apache/apisix/pull/9903#discussion_r1280130292
##########
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:
`api_ctx` has been released and should not be used.
Can only be selected in `ngx_ctx` and `local variables`.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org