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 2024/06/07 02:46:57 UTC
[PR] feat(secret): support store ssl.keys ssl.certs in secrets mamager [apisix]
AlinsRan opened a new pull request, #11339:
URL: https://github.com/apache/apisix/pull/11339
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
As part of #8319
Support for storing certificates and private_keys on ssl through Secret Manager.
It is known that both `cert` and `key` support management through the Secret Manager, and `keys` and `certs` should also support it.
### 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
Re: [PR] feat(secret): support store ssl.keys ssl.certs in secrets mamager [apisix]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #11339:
URL: https://github.com/apache/apisix/pull/11339#discussion_r1630706017
##########
apisix/ssl.lua:
##########
@@ -278,8 +278,8 @@ function _M.check_ssl_conf(in_dp, conf)
end
for i = 1, numcerts do
- if not secret.check_secret_uri(conf.cert[i]) and
- not secret.check_secret_uri(conf.key[i]) then
Review Comment:
```lua
local function check_secret_uri(secret_uri)
-- ...
if not string.has_prefix(secret_uri, PREFIX) and
not string.has_prefix(upper(secret_uri), core.env.PREFIX) then
return false, "error secret_uri prefix: " .. secret_uri
end
-- ...
end
```
It will check the `prefix`, and to trigger an error, the following conditions must be met simultaneously:
1. Use env or secret, which means having the conditions to match the prefix
2. Certs and keys configured
There is also an out of bounds exception, which requires the number of certificates to be greater than the number of cert characters to trigger. This scenario is rare.
So this PR covers this point, but the master does not.
--
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
Re: [PR] feat(secret): support store ssl.keys ssl.certs in secrets mamager [apisix]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #11339:
URL: https://github.com/apache/apisix/pull/11339#discussion_r1630676318
##########
apisix/ssl.lua:
##########
@@ -278,8 +278,8 @@ function _M.check_ssl_conf(in_dp, conf)
end
for i = 1, numcerts do
- if not secret.check_secret_uri(conf.cert[i]) and
- not secret.check_secret_uri(conf.key[i]) then
Review Comment:
yes
--
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
Re: [PR] feat(secret): support store ssl.keys ssl.certs in secrets mamager [apisix]
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on code in PR #11339:
URL: https://github.com/apache/apisix/pull/11339#discussion_r1630672954
##########
apisix/ssl.lua:
##########
@@ -278,8 +278,8 @@ function _M.check_ssl_conf(in_dp, conf)
end
for i = 1, numcerts do
- if not secret.check_secret_uri(conf.cert[i]) and
- not secret.check_secret_uri(conf.key[i]) then
Review Comment:
Is this a previously hidden bug?
--
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
Re: [PR] feat(secret): support store ssl.keys ssl.certs in secrets mamager [apisix]
Posted by "AlinsRan (via GitHub)" <gi...@apache.org>.
AlinsRan commented on code in PR #11339:
URL: https://github.com/apache/apisix/pull/11339#discussion_r1630706017
##########
apisix/ssl.lua:
##########
@@ -278,8 +278,8 @@ function _M.check_ssl_conf(in_dp, conf)
end
for i = 1, numcerts do
- if not secret.check_secret_uri(conf.cert[i]) and
- not secret.check_secret_uri(conf.key[i]) then
Review Comment:
```lua
local function check_secret_uri(secret_uri)
-- ...
if not string.has_prefix(secret_uri, PREFIX) and
not string.has_prefix(upper(secret_uri), core.env.PREFIX) then
return false, "error secret_uri prefix: " .. secret_uri
end
-- ...
end
```
It will check the `prefix`, and to trigger an error, the following conditions must be met simultaneously:
1. Use env or secret, which means having the conditions to match the prefix
2. Certs and keys configured
There is also an out of bounds exception, which requires the number of certificates to be greater than the number of key characters to trigger. This scenario is rare.
So this PR covers this point, but the master does not.
--
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
Re: [PR] feat(secret): support store ssl.keys ssl.certs in secrets mamager [apisix]
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on code in PR #11339:
URL: https://github.com/apache/apisix/pull/11339#discussion_r1630685104
##########
apisix/ssl.lua:
##########
@@ -278,8 +278,8 @@ function _M.check_ssl_conf(in_dp, conf)
end
for i = 1, numcerts do
- if not secret.check_secret_uri(conf.cert[i]) and
- not secret.check_secret_uri(conf.key[i]) then
Review Comment:
do we have test cases to cover this now?
--
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
Re: [PR] feat(secret): support store ssl.keys ssl.certs in secrets mamager [apisix]
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming merged PR #11339:
URL: https://github.com/apache/apisix/pull/11339
--
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