You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "LinkinStars (via GitHub)" <gi...@apache.org> on 2024/04/19 12:14:42 UTC
[PR] fix(env): support for uppercase reference to get ENV and SECRET [apisix]
LinkinStars opened a new pull request, #11172:
URL: https://github.com/apache/apisix/pull/11172
### Description
Fixes #11141
```
"cert": "$ENV://APISIX_ENV_CERT",
"key": "$ENV://APISIX_ENV_KEY",
```
Only lowercase is matched in the 'cert' and 'key' parameters. To maintain consistency, match the uppercase as well.
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] 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
- [x] 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] fix(env): support for uppercase reference to get ENV and SECRET [apisix]
Posted by "LinkinStars (via GitHub)" <gi...@apache.org>.
LinkinStars commented on code in PR #11172:
URL: https://github.com/apache/apisix/pull/11172#discussion_r1579292741
##########
t/router/radixtree-sni2.t:
##########
@@ -820,3 +820,32 @@ location /t {
}
--- response_body
ssl handshake: true
+
+
+
+=== TEST 22: set ssl conf with secret ref: ENV
+--- request
+GET /t
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ -- set ssl
+ local code, body = t('/apisix/admin/ssls/1',
+ ngx.HTTP_PUT,
+ [[{
+ "cert": "$ENV://TEST_ENV_SSL_CRT",
+ "key": "$ENV://TEST_ENV_SSL_KEY",
+ "sni": "test2.com"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ return ngx.say(body)
+ end
+
+ ngx.say("passed")
+ }
+ }
+--- response_body
+passed
Review Comment:
Ok. I will create another test and use the `sslhandshake` to test if the SSL in action.
--
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] fix(env): support for uppercase reference to get ENV and SECRET [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on PR #11172:
URL: https://github.com/apache/apisix/pull/11172#issuecomment-2078500177
@LinkinStars I think using lowercase "`$env://`" and uppercase "`$SECRET://`" might still not work as the code hardcodes the case for sanity checks:
Hardcoding for secret reference:
https://github.com/shreemaan-abhishek/apisix/blob/07b3cc3f63db8fc4ae3706baa324a76ee3049338/apisix/secret.lua#L34
https://github.com/shreemaan-abhishek/apisix/blob/07b3cc3f63db8fc4ae3706baa324a76ee3049338/apisix/secret.lua#L121
Hardcoding for env reference:
https://github.com/shreemaan-abhishek/apisix/blob/07b3cc3f63db8fc4ae3706baa324a76ee3049338/apisix/core/env.lua#L30
https://github.com/shreemaan-abhishek/apisix/blob/07b3cc3f63db8fc4ae3706baa324a76ee3049338/apisix/core/env.lua#L70
If you were to write test cases covering the above situation it would fail. What could be a good solution for this?
--
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] fix(env): support for uppercase reference to get ENV and SECRET [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on PR #11172:
URL: https://github.com/apache/apisix/pull/11172#issuecomment-2068006902
@LinkinStars please add test cases as well
--
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] fix(env): support for uppercase reference to get ENV and SECRET [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on PR #11172:
URL: https://github.com/apache/apisix/pull/11172#issuecomment-2267412489
@LinkinStars please resolve the conflicts.
--
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] fix(env): support for uppercase reference to get ENV and SECRET [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #11172:
URL: https://github.com/apache/apisix/pull/11172#discussion_r1577959393
##########
t/router/radixtree-sni2.t:
##########
@@ -820,3 +820,32 @@ location /t {
}
--- response_body
ssl handshake: true
+
+
+
+=== TEST 22: set ssl conf with secret ref: ENV
+--- request
+GET /t
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ -- set ssl
+ local code, body = t('/apisix/admin/ssls/1',
+ ngx.HTTP_PUT,
+ [[{
+ "cert": "$ENV://TEST_ENV_SSL_CRT",
+ "key": "$ENV://TEST_ENV_SSL_KEY",
+ "sni": "test2.com"
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ return ngx.say(body)
+ end
+
+ ngx.say("passed")
+ }
+ }
+--- response_body
+passed
Review Comment:
this test proves that using UPPERCASE works, now please add another test that shows this SSL resource in action (and that the env vars are being 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] fix(env): support for uppercase reference to get ENV and SECRET [apisix]
Posted by "LinkinStars (via GitHub)" <gi...@apache.org>.
LinkinStars commented on PR #11172:
URL: https://github.com/apache/apisix/pull/11172#issuecomment-2076926679
> also, please fix the code lint problem
I think it's probably not a code issue that's causing the lint to not pass.
<img width="1691" alt="image" src="https://github.com/apache/apisix/assets/19712692/6327d796-79c0-42d1-a15e-897be5255f66">
FYI: https://github.com/orgs/community/discussions/120966
--
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] fix(env): support for uppercase reference to get ENV and SECRET [apisix]
Posted by "LinkinStars (via GitHub)" <gi...@apache.org>.
LinkinStars commented on PR #11172:
URL: https://github.com/apache/apisix/pull/11172#issuecomment-2079225639
@shreemaan-abhishek
First, we discuss the issue of `env`.
After my testing, **both the uppercase and lowercase should be supported** after changes. There are two reasons for this
1. unit test 18 passed https://github.com/shreemaan-abhishek/apisix/blob/07b3cc3f63db8fc4ae3706baa324a76ee3049338/t/router/radixtree-sni2.t#L700-L725
2. user can use it after changing to lowercase https://github.com/apache/apisix/issues/11141#issuecomment-2051028533
After reading the code I found out why. For env, both checking and parsing converted the target to uppercase preferentially.
`if string.has_prefix(upper(uri), core.env.PREFIX) then`
Because when characters are cut, the character length is used. Both uppercase and lowercase lengths are the same, so there's no problem.
`local path = sub(env_uri, #ENV_PREFIX + 1)`
----
Secondly, let's discuss the `secret`.
Unfortunately, as you said, using the uppercase 'SECRET' is problematic.
**I tried adding unit tests and found that they could not pass.** The reason is quite simple: 'secret' is not converted to uppercase like 'env' before comparison and parsing.
Dig more. I find the git history. The 'secret' was previously modified by KMS, and there was uppercase conversion before.
However, `$SECRET` has never been used. It's not even mentioned in the documentation.
So, in my opinion, I would **not recommend supporting uppercase SECRET**. I think the author who wrote the code at that time must have also considered that.
Of course, these are just my personal thoughts, if there is anything incorrect, please point it out.
---
All in all, there are two options.
1. not support `$SECRET`
2. suport `$SECRET`, just like the `ENV`. such as`string.has_prefix(upper(uri), secret.PREFIX)`
--
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] fix(env): support for uppercase reference to get ENV and SECRET [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on PR #11172:
URL: https://github.com/apache/apisix/pull/11172#issuecomment-2077134353
@LinkinStars, okay. Please rebase with master for the fix for CI failures.
--
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] fix(env): support for uppercase reference to get ENV and SECRET [apisix]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11172:
URL: https://github.com/apache/apisix/pull/11172#issuecomment-2241552164
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.
--
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] fix(env): support for uppercase reference to get ENV and SECRET [apisix]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11172:
URL: https://github.com/apache/apisix/pull/11172#issuecomment-2451629159
This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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] fix(env): support for uppercase reference to get ENV and SECRET [apisix]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #11172: fix(env): support for uppercase reference to get ENV and SECRET
URL: https://github.com/apache/apisix/pull/11172
--
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] fix(env): support for uppercase reference to get ENV and SECRET [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on PR #11172:
URL: https://github.com/apache/apisix/pull/11172#issuecomment-2075044948
also, please fix the code lint problem
--
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] fix(env): support for uppercase reference to get ENV and SECRET [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on PR #11172:
URL: https://github.com/apache/apisix/pull/11172#issuecomment-2082039714
let's only support uppercase for ENV and leave secret as it is. Really appreciate the detailed explanation 🙏🏼
--
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