You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/06/09 06:37:35 UTC
[GitHub] [apisix] soulbird opened a new pull request, #7221: feat(ssl): support get upstream cert from ssl object
soulbird opened a new pull request, #7221:
URL: https://github.com/apache/apisix/pull/7221
### Description
Support get upstream cert from ssl object:
1、Add `ssl.type` field to indicate the certificate type in the ssl object.
2、Only when `ssl.type==1`, the SSL object can be referenced.
### 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] spacewander commented on a diff in pull request #7221: feat(ssl): support get upstream cert from ssl object
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7221:
URL: https://github.com/apache/apisix/pull/7221#discussion_r895402815
##########
docs/en/latest/admin-api.md:
##########
@@ -792,7 +792,7 @@ Currently, the response is returned from etcd.
| labels | False | Match Rules | Attributes of the resource specified as key-value pairs. | {"version":"v2","build":"16","env":"production"} |
| create_time | False | Auxiliary | Epoch timestamp (in seconds) of the created time. If missing, this field will be populated automatically. | 1602883670 |
| update_time | False | Auxiliary | Epoch timestamp (in seconds) of the updated time. If missing, this field will be populated automatically. | 1602883670 |
-| type | False | Certificate type | Identifies the type of certificate, default `server`. | `client` Indicates that the certificate is a client certificate, which is used when APISIX accesses the upstream; `server` Indicates that the certificate is a server-side certificate, which is used by APISIX when verifying client requests. |
+| type | False | Certificate position | Identifies the type of certificate, default `server`. | `client` Indicates that the certificate is a client certificate, which is used when APISIX accesses the upstream; `server` Indicates that the certificate is a server-side certificate, which is used by APISIX when verifying client requests. |
Review Comment:
If this field is controversial, what about using `Auxiliary`?
--
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] spacewander commented on a diff in pull request #7221: feat(ssl): support get upstream cert from ssl object
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7221:
URL: https://github.com/apache/apisix/pull/7221#discussion_r894125055
##########
apisix/upstream.lua:
##########
@@ -415,6 +425,29 @@ local function check_upstream_conf(in_dp, conf)
return false, "invalid configuration: " .. err
end
+ local ssl_id = conf.tls and conf.tls.client_cert_id or nil
Review Comment:
```suggestion
local ssl_id = conf.tls and conf.tls.client_cert_id
```
is enough
##########
t/node/upstream-mtls.t:
##########
@@ -545,3 +545,68 @@ GET /t
GET /hello_chunked
--- response_body
hello world
+
+
+
+=== TEST 13: get cert by tls.client_cert_id
Review Comment:
Let's add a case that there is no SSL object according to the client_cert_id
--
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] spacewander merged pull request #7221: feat(ssl): support get upstream cert from ssl object
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #7221:
URL: https://github.com/apache/apisix/pull/7221
--
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] soulbird commented on a diff in pull request #7221: feat(ssl): support get upstream cert from ssl object
Posted by GitBox <gi...@apache.org>.
soulbird commented on code in PR #7221:
URL: https://github.com/apache/apisix/pull/7221#discussion_r895363953
##########
docs/en/latest/admin-api.md:
##########
@@ -792,7 +792,7 @@ Currently, the response is returned from etcd.
| labels | False | Match Rules | Attributes of the resource specified as key-value pairs. | {"version":"v2","build":"16","env":"production"} |
| create_time | False | Auxiliary | Epoch timestamp (in seconds) of the created time. If missing, this field will be populated automatically. | 1602883670 |
| update_time | False | Auxiliary | Epoch timestamp (in seconds) of the updated time. If missing, this field will be populated automatically. | 1602883670 |
-| type | False | Certificate type | Identifies the type of certificate, default `server`. | `client` Indicates that the certificate is a client certificate, which is used when APISIX accesses the upstream; `server` Indicates that the certificate is a server-side certificate, which is used by APISIX when verifying client requests. |
+| type | False | Certificate position | Identifies the type of certificate, default `server`. | `client` Indicates that the certificate is a client certificate, which is used when APISIX accesses the upstream; `server` Indicates that the certificate is a server-side certificate, which is used by APISIX when verifying client requests. |
Review Comment:
In fact, I didn't find any official concept to describe client short certificate and server certificate, do you have any suggestion?
--
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] tzssangglass commented on a diff in pull request #7221: feat(ssl): support get upstream cert from ssl object
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7221:
URL: https://github.com/apache/apisix/pull/7221#discussion_r894234311
##########
docs/en/latest/admin-api.md:
##########
@@ -789,6 +792,7 @@ Currently, the response is returned from etcd.
| labels | False | Match Rules | Attributes of the resource specified as key-value pairs. | {"version":"v2","build":"16","env":"production"} |
| create_time | False | Auxiliary | Epoch timestamp (in seconds) of the created time. If missing, this field will be populated automatically. | 1602883670 |
| update_time | False | Auxiliary | Epoch timestamp (in seconds) of the updated time. If missing, this field will be populated automatically. | 1602883670 |
+| type | False | Certificate type | Identifies the type of certificate, default `server`. | `client` Indicates that the certificate is a client certificate, which is used when APISIX accesses the upstream; `server` Indicates that the certificate is a server-side certificate, which is used by APISIX when verifying client requests. |
Review Comment:
`Certificate type` there may be misunderstandings, what about `Certificate position`?
--
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] spacewander commented on a diff in pull request #7221: feat(ssl): support get upstream cert from ssl object
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7221:
URL: https://github.com/apache/apisix/pull/7221#discussion_r895355774
##########
docs/en/latest/admin-api.md:
##########
@@ -792,7 +792,7 @@ Currently, the response is returned from etcd.
| labels | False | Match Rules | Attributes of the resource specified as key-value pairs. | {"version":"v2","build":"16","env":"production"} |
| create_time | False | Auxiliary | Epoch timestamp (in seconds) of the created time. If missing, this field will be populated automatically. | 1602883670 |
| update_time | False | Auxiliary | Epoch timestamp (in seconds) of the updated time. If missing, this field will be populated automatically. | 1602883670 |
-| type | False | Certificate type | Identifies the type of certificate, default `server`. | `client` Indicates that the certificate is a client certificate, which is used when APISIX accesses the upstream; `server` Indicates that the certificate is a server-side certificate, which is used by APISIX when verifying client requests. |
+| type | False | Certificate position | Identifies the type of certificate, default `server`. | `client` Indicates that the certificate is a client certificate, which is used when APISIX accesses the upstream; `server` Indicates that the certificate is a server-side certificate, which is used by APISIX when verifying client requests. |
Review Comment:
What does `Certificate position` mean? Is there any source for this concept?
--
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] tokers commented on a diff in pull request #7221: feat(ssl): support get upstream cert from ssl object
Posted by GitBox <gi...@apache.org>.
tokers commented on code in PR #7221:
URL: https://github.com/apache/apisix/pull/7221#discussion_r895262112
##########
docs/en/latest/admin-api.md:
##########
@@ -570,6 +571,8 @@ You can set the `scheme` to `tls`, which means "TLS over TCP".
To use mTLS to communicate with Upstream, you can use the `tls.client_cert/key` in the same format as SSL's `cert` and `key` fields.
+Or you can reference SSL object by `tls.client_cert_id` to set SSL cert and key. The SSL object can be referenced only if the `type` field is `client`. Only `cert` and `key` will be used in the SSL object.
Review Comment:
@soulbird What's the result?
--
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] spacewander commented on a diff in pull request #7221: feat(ssl): support get upstream cert from ssl object
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7221:
URL: https://github.com/apache/apisix/pull/7221#discussion_r893307113
##########
apisix/schema_def.lua:
##########
@@ -722,6 +731,13 @@ _M.ssl = {
type = "object",
properties = {
id = id_schema,
+ type = {
+ description = "ssl certificate type, " ..
+ "0 to server certificate, " ..
+ "1 to client certificate for upstream",
+ type = "integer",
+ enum = {0, 1}
Review Comment:
Better to provide a default value, so we don't need to handle an unset situation.
--
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] membphis commented on a diff in pull request #7221: feat(ssl): support get upstream cert from ssl object
Posted by GitBox <gi...@apache.org>.
membphis commented on code in PR #7221:
URL: https://github.com/apache/apisix/pull/7221#discussion_r894087809
##########
docs/en/latest/admin-api.md:
##########
@@ -541,8 +541,9 @@ In addition to the equalization algorithm selections, Upstream also supports pas
| labels | optional | Attributes of the Upstream specified as key-value pairs. | {"version":"v2","build":"16","env":"production"} |
| create_time | optional | Epoch timestamp (in seconds) of the created time. If missing, this field will be populated automatically. | 1602883670 |
| update_time | optional | Epoch timestamp (in seconds) of the updated time. If missing, this field will be populated automatically. | 1602883670 |
-| tls.client_cert | optional | Sets the client certificate while connecting to a TLS Upstream. | |
-| tls.client_key | optional | Sets the client private key while connecting to a TLS Upstream. | |
+| tls.client_cert | optional, can't be used with `tls.client_cert_id` | Sets the client certificate while connecting to a TLS Upstream. | |
+| tls.client_key | optional, can't be used with `tls.client_cert_id` | Sets the client private key while connecting to a TLS Upstream. | |
+| tls.client_cert_id | optional, can't be used with `tls.client_cert` and `tls.client_key` | Set the referenced [SSL](#ssl) id. | |
Review Comment:
@tokers pls confirm this first
--
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] spacewander commented on a diff in pull request #7221: feat(ssl): support get upstream cert from ssl object
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7221:
URL: https://github.com/apache/apisix/pull/7221#discussion_r893303237
##########
apisix/schema_def.lua:
##########
@@ -722,6 +731,13 @@ _M.ssl = {
type = "object",
properties = {
id = id_schema,
+ type = {
+ description = "ssl certificate type, " ..
+ "0 to server certificate, " ..
+ "1 to client certificate for upstream",
+ type = "integer",
+ enum = {0, 1}
Review Comment:
Please use enum string like `server` & `client`, thanks!
##########
apisix/init.lua:
##########
@@ -512,6 +512,22 @@ function _M.http_access_phase()
return pubsub_kafka.access(api_ctx)
end
+ if api_ctx.matched_upstream and api_ctx.matched_upstream.tls_id then
Review Comment:
Could we move this just after the upstream is found:
https://github.com/apache/apisix/blob/975038ea70dc47e0613c42b3eabb8dcc730216a4/apisix/init.lua#L497
##########
apisix/admin/ssl.lua:
##########
@@ -46,7 +46,7 @@ local function check_conf(id, conf, need_id)
conf.id = id
core.log.info("schema: ", core.json.delay_encode(core.schema.ssl))
- core.log.info("conf : ", core.json.delay_encode(conf))
+ core.log.info("conf : ", core.json.delay_encode(conf))
Review Comment:
```suggestion
core.log.info("conf: ", core.json.delay_encode(conf))
```
--
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] tokers commented on a diff in pull request #7221: feat(ssl): support get upstream cert from ssl object
Posted by GitBox <gi...@apache.org>.
tokers commented on code in PR #7221:
URL: https://github.com/apache/apisix/pull/7221#discussion_r894089623
##########
apisix/init.lua:
##########
@@ -495,6 +495,24 @@ function _M.http_access_phase()
or route_val.upstream
end
+ if api_ctx.matched_upstream and api_ctx.matched_upstream.tls and
+ api_ctx.matched_upstream.tls.client_cert_id then
+
+ local cert_id = api_ctx.matched_upstream.tls.client_cert_id
+ local upstream_ssl = router.router_ssl.get_by_id(cert_id)
+ if not upstream_ssl or upstream_ssl.type ~= "client" then
Review Comment:
here dd a log will be better.
##########
docs/en/latest/admin-api.md:
##########
@@ -541,8 +541,9 @@ In addition to the equalization algorithm selections, Upstream also supports pas
| labels | optional | Attributes of the Upstream specified as key-value pairs. | {"version":"v2","build":"16","env":"production"} |
| create_time | optional | Epoch timestamp (in seconds) of the created time. If missing, this field will be populated automatically. | 1602883670 |
| update_time | optional | Epoch timestamp (in seconds) of the updated time. If missing, this field will be populated automatically. | 1602883670 |
-| tls.client_cert | optional | Sets the client certificate while connecting to a TLS Upstream. | |
-| tls.client_key | optional | Sets the client private key while connecting to a TLS Upstream. | |
+| tls.client_cert | optional, can't be used with `tls.client_cert_id` | Sets the client certificate while connecting to a TLS Upstream. | |
+| tls.client_key | optional, can't be used with `tls.client_cert_id` | Sets the client private key while connecting to a TLS Upstream. | |
+| tls.client_cert_id | optional, can't be used with `tls.client_cert` and `tls.client_key` | Set the referenced [SSL](#ssl) id. | |
Review Comment:
No problem but currently it looks strange as we use an` SSL` object while the name is `client_cert_id`.
##########
docs/en/latest/admin-api.md:
##########
@@ -570,6 +571,8 @@ You can set the `scheme` to `tls`, which means "TLS over TCP".
To use mTLS to communicate with Upstream, you can use the `tls.client_cert/key` in the same format as SSL's `cert` and `key` fields.
+Or you can reference SSL object by `tls.client_cert_id` to set SSL cert and key. The SSL object can be referenced only if the `type` field is `client`. Only `cert` and `key` will be used in the SSL object.
Review Comment:
**Otherwise the request will be rejected by APISIX**.
I think we should tell users the result.
--
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] soulbird commented on a diff in pull request #7221: feat(ssl): support get upstream cert from ssl object
Posted by GitBox <gi...@apache.org>.
soulbird commented on code in PR #7221:
URL: https://github.com/apache/apisix/pull/7221#discussion_r895403864
##########
docs/en/latest/admin-api.md:
##########
@@ -792,7 +792,7 @@ Currently, the response is returned from etcd.
| labels | False | Match Rules | Attributes of the resource specified as key-value pairs. | {"version":"v2","build":"16","env":"production"} |
| create_time | False | Auxiliary | Epoch timestamp (in seconds) of the created time. If missing, this field will be populated automatically. | 1602883670 |
| update_time | False | Auxiliary | Epoch timestamp (in seconds) of the updated time. If missing, this field will be populated automatically. | 1602883670 |
-| type | False | Certificate type | Identifies the type of certificate, default `server`. | `client` Indicates that the certificate is a client certificate, which is used when APISIX accesses the upstream; `server` Indicates that the certificate is a server-side certificate, which is used by APISIX when verifying client requests. |
+| type | False | Certificate position | Identifies the type of certificate, default `server`. | `client` Indicates that the certificate is a client certificate, which is used when APISIX accesses the upstream; `server` Indicates that the certificate is a server-side certificate, which is used by APISIX when verifying client requests. |
Review Comment:
That's great!
--
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