You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "An-DJ (via GitHub)" <gi...@apache.org> on 2023/03/22 13:10:18 UTC
[GitHub] [apisix] An-DJ opened a new pull request, #9147: feat(cli): support APISIX_ALLOW_NONE_AUTHENTICATION
An-DJ opened a new pull request, #9147:
URL: https://github.com/apache/apisix/pull/9147
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
APISIX now does not allow the requests with empty adminKey to access Admin API except 127.0.0.0/24.
The user can allow this by set env variable `APISIX_ALLOW_NONE_AUTHENTICATION=true`.
Fixes #9071
### 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
- [X] 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
[GitHub] [apisix] An-DJ commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1156948538
##########
conf/config-default.yaml:
##########
@@ -588,6 +588,10 @@ deployment:
role_traditional:
config_provider: etcd
admin:
+ # admin_key required or not. Default value is true.
+ # Bypass the Admin API authentication by modifying this value to false if needed.
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] An-DJ commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152957781
##########
t/cli/test_admin.sh:
##########
@@ -189,6 +189,62 @@ fi
echo "pass: missing admin key and only allow 127.0.0.0/24 to access admin api"
+# allow any IP to access admin api with empty admin_key, when admin_key_required=true
+
+git checkout conf/config.yaml
+
+echo '
+deployment:
+ admin:
+ admin_key_required: true
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if ! grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: false
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should not show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+if ! grep -E "Warning! AdminKey is bypassed" output.log > /dev/null; then
+ echo "failed: should show 'Warning! AdminKey is bypassed'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: invalid-value
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "path[deployment->admin->admin_key_required] expect: boolean, but got: string" output.log > /dev/null; then
+ echo "failed: should show 'expect: boolean, but got: string'"
Review Comment:
I mean that this msg is automatically created and outputed by the schema check step.
Total output:
It seems that we cannot easily custom info...
I think the grep text now has enough to help to know "what happened".
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152851627
##########
conf/config-default.yaml:
##########
@@ -588,6 +588,10 @@ deployment:
role_traditional:
config_provider: etcd
admin:
+ # If admin_key is required, default value is true.
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] monkeyDluffy6017 commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152939378
##########
t/cli/test_admin.sh:
##########
@@ -189,6 +189,62 @@ fi
echo "pass: missing admin key and only allow 127.0.0.0/24 to access admin api"
+# allow any IP to access admin api with empty admin_key, when admin_key_required=true
+
+git checkout conf/config.yaml
+
+echo '
+deployment:
+ admin:
+ admin_key_required: true
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if ! grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: false
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should not show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+if ! grep -E "Warning! AdminKey is bypassed" output.log > /dev/null; then
+ echo "failed: should show 'Warning! AdminKey is bypassed'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: invalid-value
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "path[deployment->admin->admin_key_required] expect: boolean, but got: string" output.log > /dev/null; then
+ echo "failed: should show 'expect: boolean, but got: string'"
Review Comment:
The warning message prefix is needed: `check admin_key_required value failed`, or you will not know what happened only with `failed: should show 'expect: boolean, but got: string`
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152983596
##########
apisix/cli/ops.lua:
##########
@@ -189,6 +189,13 @@ local function init(env)
and #allow_admin == 1 and allow_admin[1] == "127.0.0.0/24" then
checked_admin_key = true
end
+ -- check if admin_key is required
Review Comment:
We talk about this in another PR
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1151579331
##########
apisix/cli/ops.lua:
##########
@@ -190,6 +190,15 @@ local function init(env)
checked_admin_key = true
end
+ -- check if APISIX_BYPASS_ADMIN_API_AUTH=true
+ local allow_none_auth = getenv("APISIX_BYPASS_ADMIN_API_AUTH")
+ if allow_none_auth == "true" then
+ checked_admin_key = true
+ print("Warning! AdminKey is bypassed because of APISIX_BYPASS_ADMIN_API_AUTH=true.",
+ "If you are deploying APISIX in a production environment,",
+ "please disable it and set a secure password for the adminKey!")
+ end
+
Review Comment:
Do you mean that hint appears in both stdout and log?
1. `opt.lua` checks if allow_admin in config.yaml is correct before APISIX starts. We need to bypass the adminKey check here.
2. `init.lua` checks the adminKey when accessing the Admin API. We need to bypass check_token, too.
That's why I have implemented bypass-logic in two different stage.(before and after the start).
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support APISIX_ALLOW_NONE_AUTHENTICATION
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1145603321
##########
apisix/cli/ngx_tpl.lua:
##########
@@ -51,6 +51,8 @@ worker_shutdown_timeout {* worker_shutdown_timeout *};
env APISIX_PROFILE;
env PATH; # for searching external plugin runner's binary
+env APISIX_ALLOW_NONE_AUTHENTICATION; # allow any IP with empty admin_key
Review Comment:
IMO we just bypass the configuration check here.
That is, if the user set `APISIX_ALLOW_NONE_AUTHENTICATION=true` but still configure the adminKey, the key also works.
So this env var just `allow` none auth, not `bypass`.
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1151627161
##########
apisix/cli/ngx_tpl.lua:
##########
@@ -51,6 +51,8 @@ worker_shutdown_timeout {* worker_shutdown_timeout *};
env APISIX_PROFILE;
env PATH; # for searching external plugin runner's binary
+env APISIX_BYPASS_ADMIN_API_AUTH; # bypass admin api auth
Review Comment:
Sounds weird...
I prefer the variable names in bitnami/etcd that bypass authentication:
`ALLOW_NONE_AUTHENTICATION` looks more natural.
If we use `ADMIN_API_TOKEN_REQUIRED`, users should bypass authentication by specifying `false`.
IMHO, `false` is not a good value to make users feel positive.
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152973340
##########
apisix/cli/ops.lua:
##########
@@ -189,6 +189,13 @@ local function init(env)
and #allow_admin == 1 and allow_admin[1] == "127.0.0.0/24" then
checked_admin_key = true
end
+ -- check if admin_key is required
Review Comment:
I checked this, after set `127.0.0.0/24` and remove the admin key:
```
curl http://127.0.0.1:9180/apisix/admin/upstreams/1 -X PUT -d '
{
"type": "chash",
"key": "remote_addr",
"nodes": {
"127.0.0.1:80": 1,
"foo.com:80": 2
}
}'
{"error_msg":"failed to check token"}
```
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152900614
##########
apisix/admin/init.lua:
##########
@@ -395,6 +401,13 @@ function _M.init_worker()
events.register(reload_plugins, reload_event, "PUT")
if ngx_worker_id() == 0 then
+ -- check if admin_key is required
+ if not local_conf.deployment.admin.admin_key_required then
+ core.log.warn("AdminKey is bypassed.",
+ "If you are deploying APISIX in a production environment,",
+ "please disable it and set a secure password for the adminKey!")
Review Comment:
Is this better ?
```
core.log.warn("Admin key is bypassed! ",
"If you are deploying APISIX in a production environment, ",
"please disable it and set a secure password for the admin Key!")
```
##########
apisix/cli/ops.lua:
##########
@@ -189,6 +189,13 @@ local function init(env)
and #allow_admin == 1 and allow_admin[1] == "127.0.0.0/24" then
checked_admin_key = true
end
+ -- check if admin_key is required
+ if not yaml_conf.deployment.admin.admin_key_required then
+ checked_admin_key = true
+ print("Warning! AdminKey is bypassed. "
Review Comment:
ditto
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152927825
##########
apisix/cli/ops.lua:
##########
@@ -189,6 +189,13 @@ local function init(env)
and #allow_admin == 1 and allow_admin[1] == "127.0.0.0/24" then
checked_admin_key = true
end
+ -- check if admin_key is required
Review Comment:
OK, it's just for starting, righting? if set `127.0.0.0/24`, the user cann't access the APISIX? I don't find the code
--
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] moonming commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1154263684
##########
t/admin/token.t:
##########
@@ -177,3 +177,21 @@ PUT /apisix/admin/plugins/reload?api_key=4054f7cf07e344346cd3f287985e76a2
--- request
GET /apisix/admin/routes??api_key=4054f7cf07e344346cd3f287985e76a2
--- error_code: 401
+
+
+
+=== TEST 10: access without api key
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
+
+
+
+=== TEST 11: access without api key, but APISIX_BYPASS_ADMIN_API_AUTH=true
+--- main_config
+env APISIX_BYPASS_ADMIN_API_AUTH=true;
Review Comment:
How to set this configure in environment variable?
--
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] moonming commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1156954006
##########
apisix/cli/ops.lua:
##########
@@ -189,6 +189,13 @@ local function init(env)
and #allow_admin == 1 and allow_admin[1] == "127.0.0.0/24" then
checked_admin_key = true
end
+ -- check if admin_key is required
+ if yaml_conf.deployment.admin.admin_key_required == false then
+ checked_admin_key = true
+ print("Warning! Admin key is bypassed! "
+ .. "If you are deploying APISIX in a production environment, "
+ .. "please disable it and set a secure password for the admin Key!")
+ end
Review Comment:
Please add an issue for the duplicate code and fix it later.
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152844472
##########
conf/config-default.yaml:
##########
@@ -588,6 +588,10 @@ deployment:
role_traditional:
config_provider: etcd
admin:
+ # If admin_key is required, default value is true.
Review Comment:
Is `admin_key required or not` 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 commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152912586
##########
t/cli/test_admin.sh:
##########
@@ -189,6 +189,62 @@ fi
echo "pass: missing admin key and only allow 127.0.0.0/24 to access admin api"
+# allow any IP to access admin api with empty admin_key, when admin_key_required=true
+
+git checkout conf/config.yaml
+
+echo '
+deployment:
+ admin:
+ admin_key_required: true
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if ! grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: false
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should not show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+if ! grep -E "Warning! AdminKey is bypassed" output.log > /dev/null; then
+ echo "failed: should show 'Warning! AdminKey is bypassed'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: invalid-value
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "path[deployment->admin->admin_key_required] expect: boolean, but got: string" output.log > /dev/null; then
+ echo "failed: should show 'expect: boolean, but got: string'"
Review Comment:
echo "check admin_key_required value failed: should show 'expect: boolean, but got: string'"
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152934932
##########
t/cli/test_admin.sh:
##########
@@ -189,6 +189,62 @@ fi
echo "pass: missing admin key and only allow 127.0.0.0/24 to access admin api"
+# allow any IP to access admin api with empty admin_key, when admin_key_required=true
+
+git checkout conf/config.yaml
+
+echo '
+deployment:
+ admin:
+ admin_key_required: true
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if ! grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: false
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should not show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+if ! grep -E "Warning! AdminKey is bypassed" output.log > /dev/null; then
+ echo "failed: should show 'Warning! AdminKey is bypassed'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: invalid-value
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "path[deployment->admin->admin_key_required] expect: boolean, but got: string" output.log > /dev/null; then
+ echo "failed: should show 'expect: boolean, but got: string'"
Review Comment:
This is the schema check error msg automatically created... It seems that it cannot be modified by custom.
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1159510999
##########
t/admin/api.t:
##########
@@ -156,3 +156,51 @@ X-API-VERSION: v2
GET /t
--- response_body
passed
+
+
+
+=== TEST 10: Access with api key, and admin_key_required=true
+--- yaml_config
+deployment:
+ admin:
+ admin_key_required: true
+--- more_headers
+X-API-KEY: edd1c9f034335f136f87ad84b625c8f1
+--- request
+GET /apisix/admin/routes
+--- error_code: 200
+
+
+
+=== TEST 11: Access without api key, but admin_key_required=true
+--- yaml_config
+deployment:
+ admin:
+ admin_key_required: true
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
+
+
+
+=== TEST 12: Access without api key, but admin_key_required=true
+--- yaml_config
+deployment:
+ admin:
+ admin_key_required: true
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
Review Comment:
Fixed
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1156948151
##########
t/admin/token.t:
##########
@@ -177,3 +177,21 @@ PUT /apisix/admin/plugins/reload?api_key=4054f7cf07e344346cd3f287985e76a2
--- request
GET /apisix/admin/routes??api_key=4054f7cf07e344346cd3f287985e76a2
--- error_code: 401
+
+
+
+=== TEST 10: access without api key
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
+
+
+
+=== TEST 11: access without api key, but APISIX_BYPASS_ADMIN_API_AUTH=true
+--- main_config
+env APISIX_BYPASS_ADMIN_API_AUTH=true;
Review Comment:
We can export an env variable and use it in `config.yaml`:
https://github.com/apache/apisix/blob/master/conf/config.yaml#L28
For example:
```
export admin_key_required=false
echo '
deployment:
admin:
admin_key_required: ${{admin_key_required}}
' > conf/config.yaml
```
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1151353676
##########
apisix/cli/ngx_tpl.lua:
##########
@@ -51,6 +51,8 @@ worker_shutdown_timeout {* worker_shutdown_timeout *};
env APISIX_PROFILE;
env PATH; # for searching external plugin runner's binary
+env APISIX_ALLOW_NONE_AUTHENTICATION; # allow any IP with empty admin_key
Review Comment:
Replaced by APISIX_BYPASS_ADMIN_API_AUTH.
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1151627838
##########
apisix/cli/ngx_tpl.lua:
##########
@@ -51,6 +51,8 @@ worker_shutdown_timeout {* worker_shutdown_timeout *};
env APISIX_PROFILE;
env PATH; # for searching external plugin runner's binary
+env APISIX_BYPASS_ADMIN_API_AUTH; # bypass admin api auth
Review Comment:
Ref: https://hub.docker.com/r/bitnami/etcd/
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1151591775
##########
apisix/cli/ops.lua:
##########
@@ -190,6 +190,15 @@ local function init(env)
checked_admin_key = true
end
+ -- check if APISIX_BYPASS_ADMIN_API_AUTH=true
+ local allow_none_auth = getenv("APISIX_BYPASS_ADMIN_API_AUTH")
+ if allow_none_auth == "true" then
+ checked_admin_key = true
+ print("Warning! AdminKey is bypassed because of APISIX_BYPASS_ADMIN_API_AUTH=true.",
+ "If you are deploying APISIX in a production environment,",
+ "please disable it and set a secure password for the adminKey!")
+ end
+
Review Comment:
In other words, opt.lua does a static check on the configuration file (if it is not 127.0.0.0/24 and the adminKey is empty, APISIX will not start).
We also have to bypass the checks here.
##########
apisix/cli/ops.lua:
##########
@@ -190,6 +190,15 @@ local function init(env)
checked_admin_key = true
end
+ -- check if APISIX_BYPASS_ADMIN_API_AUTH=true
+ local allow_none_auth = getenv("APISIX_BYPASS_ADMIN_API_AUTH")
+ if allow_none_auth == "true" then
+ checked_admin_key = true
+ print("Warning! AdminKey is bypassed because of APISIX_BYPASS_ADMIN_API_AUTH=true.",
+ "If you are deploying APISIX in a production environment,",
+ "please disable it and set a secure password for the adminKey!")
+ end
+
Review Comment:
In other words, `opt.lua` does a static check on the configuration file (if it is not 127.0.0.0/24 and the adminKey is empty, APISIX will not start).
We also have to bypass the checks here.
--
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] moonming commented on a diff in pull request #9147: feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1151594155
##########
apisix/cli/ops.lua:
##########
@@ -190,6 +190,15 @@ local function init(env)
checked_admin_key = true
end
+ -- check if APISIX_BYPASS_ADMIN_API_AUTH=true
+ local allow_none_auth = getenv("APISIX_BYPASS_ADMIN_API_AUTH")
+ if allow_none_auth == "true" then
+ checked_admin_key = true
+ print("Warning! AdminKey is bypassed because of APISIX_BYPASS_ADMIN_API_AUTH=true.",
+ "If you are deploying APISIX in a production environment,",
+ "please disable it and set a secure password for the adminKey!")
+ end
+
Review Comment:
If there is repeated code, then it needs to be abstracted into a function
--
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] moonming commented on a diff in pull request #9147: feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1151592445
##########
t/admin/token.t:
##########
@@ -177,3 +177,21 @@ PUT /apisix/admin/plugins/reload?api_key=4054f7cf07e344346cd3f287985e76a2
--- request
GET /apisix/admin/routes??api_key=4054f7cf07e344346cd3f287985e76a2
--- error_code: 401
+
+
+
+=== TEST 10: access without api key
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
+
+
+
+=== TEST 11: access without api key, but APISIX_BYPASS_ADMIN_API_AUTH=true
+--- main_config
+env APISIX_BYPASS_ADMIN_API_AUTH=true;
Review Comment:
it's too hacky. We need a configure checker(if apisix not has) to check schema and values.
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152920733
##########
t/cli/test_admin.sh:
##########
@@ -189,6 +189,62 @@ fi
echo "pass: missing admin key and only allow 127.0.0.0/24 to access admin api"
+# allow any IP to access admin api with empty admin_key, when admin_key_required=true
+
+git checkout conf/config.yaml
+
+echo '
+deployment:
+ admin:
+ admin_key_required: true
Review Comment:
what happened if `admin_key_required` is missing?
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152961476
##########
t/cli/test_admin.sh:
##########
@@ -189,6 +189,62 @@ fi
echo "pass: missing admin key and only allow 127.0.0.0/24 to access admin api"
+# allow any IP to access admin api with empty admin_key, when admin_key_required=true
+
+git checkout conf/config.yaml
+
+echo '
+deployment:
+ admin:
+ admin_key_required: true
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if ! grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: false
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should not show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+if ! grep -E "Warning! AdminKey is bypassed" output.log > /dev/null; then
+ echo "failed: should show 'Warning! AdminKey is bypassed'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: invalid-value
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "path[deployment->admin->admin_key_required] expect: boolean, but got: string" output.log > /dev/null; then
+ echo "failed: should show 'expect: boolean, but got: string'"
Review Comment:
OK, this is for the output file, i mean what you `echo` to the screen.
`echo "failed: should show 'expect: boolean, but got: string'"`
You don't know what's going on from this alert message, and need to `cat ooutput.log`
--
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] moonming commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1154250154
##########
apisix/admin/init.lua:
##########
@@ -395,6 +401,13 @@ function _M.init_worker()
events.register(reload_plugins, reload_event, "PUT")
if ngx_worker_id() == 0 then
+ -- check if admin_key is required
+ if local_conf.deployment.admin.admin_key_required == false then
+ core.log.warn("Admin key is bypassed! ",
+ "If you are deploying APISIX in a production environment, ",
+ "please disable it and set a secure password for the admin Key!")
Review Comment:
admin key is not password
##########
conf/config-default.yaml:
##########
@@ -588,6 +588,10 @@ deployment:
role_traditional:
config_provider: etcd
admin:
+ # admin_key required or not. Default value is true.
+ # Bypass the Admin API authentication by modifying this value to false if needed.
Review Comment:
```suggestion
# admin API authentication is enabled by default.
# Set it false in the production environment will cause a serious security issue.
```
##########
apisix/cli/ops.lua:
##########
@@ -189,6 +189,13 @@ local function init(env)
and #allow_admin == 1 and allow_admin[1] == "127.0.0.0/24" then
checked_admin_key = true
end
+ -- check if admin_key is required
+ if yaml_conf.deployment.admin.admin_key_required == false then
+ checked_admin_key = true
+ print("Warning! Admin key is bypassed! "
+ .. "If you are deploying APISIX in a production environment, "
+ .. "please disable it and set a secure password for the admin Key!")
+ end
Review Comment:
duplicate code
##########
apisix/admin/init.lua:
##########
@@ -395,6 +401,13 @@ function _M.init_worker()
events.register(reload_plugins, reload_event, "PUT")
if ngx_worker_id() == 0 then
+ -- check if admin_key is required
+ if local_conf.deployment.admin.admin_key_required == false then
+ core.log.warn("Admin key is bypassed! ",
+ "If you are deploying APISIX in a production environment, ",
+ "please disable it and set a secure password for the admin Key!")
Review Comment:
Please write clearly what configuration to disable
##########
t/admin/api.t:
##########
@@ -156,3 +156,40 @@ X-API-VERSION: v2
GET /t
--- response_body
passed
+
+
+
+=== TEST 10: Access with api key, and admin_key_required=true
+--- yaml_config
+deployment:
+ admin:
+ admin_key_required: true
+--- more_headers
+X-API-KEY: edd1c9f034335f136f87ad84b625c8f1
+--- request
+GET /apisix/admin/routes
+--- error_code: 200
+
+
+
+=== TEST 11: Access without api key, but admin_key_required=true
+--- yaml_config
+deployment:
+ admin:
+ admin_key_required: true
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
+
+
+
+=== TEST 12: Access without api key, but admin_key_required=false
Review Comment:
no test case for `Access with api key, but admin_key_required=false`
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1156948916
##########
apisix/admin/init.lua:
##########
@@ -395,6 +401,13 @@ function _M.init_worker()
events.register(reload_plugins, reload_event, "PUT")
if ngx_worker_id() == 0 then
+ -- check if admin_key is required
+ if local_conf.deployment.admin.admin_key_required == false then
+ core.log.warn("Admin key is bypassed! ",
+ "If you are deploying APISIX in a production environment, ",
+ "please disable it and set a secure password for the admin Key!")
Review Comment:
Done.
##########
t/admin/api.t:
##########
@@ -156,3 +156,40 @@ X-API-VERSION: v2
GET /t
--- response_body
passed
+
+
+
+=== TEST 10: Access with api key, and admin_key_required=true
+--- yaml_config
+deployment:
+ admin:
+ admin_key_required: true
+--- more_headers
+X-API-KEY: edd1c9f034335f136f87ad84b625c8f1
+--- request
+GET /apisix/admin/routes
+--- error_code: 200
+
+
+
+=== TEST 11: Access without api key, but admin_key_required=true
+--- yaml_config
+deployment:
+ admin:
+ admin_key_required: true
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
+
+
+
+=== TEST 12: Access without api key, but admin_key_required=false
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] An-DJ commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1156967875
##########
apisix/admin/init.lua:
##########
@@ -395,6 +401,13 @@ function _M.init_worker()
events.register(reload_plugins, reload_event, "PUT")
if ngx_worker_id() == 0 then
+ -- check if admin_key is required
+ if local_conf.deployment.admin.admin_key_required == false then
+ core.log.warn("Admin key is bypassed! ",
+ "If you are deploying APISIX in a production environment, ",
+ "please disable it and set a secure password for the admin Key!")
Review Comment:
Corrected.
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1151596688
##########
t/admin/token.t:
##########
@@ -177,3 +177,21 @@ PUT /apisix/admin/plugins/reload?api_key=4054f7cf07e344346cd3f287985e76a2
--- request
GET /apisix/admin/routes??api_key=4054f7cf07e344346cd3f287985e76a2
--- error_code: 401
+
+
+
+=== TEST 10: access without api key
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
+
+
+
+=== TEST 11: access without api key, but APISIX_BYPASS_ADMIN_API_AUTH=true
+--- main_config
+env APISIX_BYPASS_ADMIN_API_AUTH=true;
Review Comment:
> need to add test cases for `APISIX_BYPASS_ADMIN_API_AUTH` is not `true`
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] An-DJ commented on pull request #9147: feat(cli): support APISIX_ALLOW_NONE_AUTHENTICATION
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on PR #9147:
URL: https://github.com/apache/apisix/pull/9147#issuecomment-1486374804
CC @monkeyDluffy6017 Key hints are added.
Besides, I have changed the implementation details according to @moonming 's advice.
If `APISIX_ALLOW_NONE_AUTHENTICATION=true`, all the requests can access the Admin API without adminKey, no matter whether the user config it.
--
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 pull request #9147: feat(cli): support APISIX_ALLOW_NONE_AUTHENTICATION
Posted by "leslie-tsang (via GitHub)" <gi...@apache.org>.
leslie-tsang commented on PR #9147:
URL: https://github.com/apache/apisix/pull/9147#issuecomment-1486131752
> If this option is turned on, can we provide some hints, otherwise there may be security risks Do we need to add this inspection to the inspection script? @leslie-tsang
Yes for sure, need to log it in error log 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
[GitHub] [apisix] moonming commented on a diff in pull request #9147: feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1151642560
##########
apisix/cli/ngx_tpl.lua:
##########
@@ -51,6 +51,8 @@ worker_shutdown_timeout {* worker_shutdown_timeout *};
env APISIX_PROFILE;
env PATH; # for searching external plugin runner's binary
+env APISIX_BYPASS_ADMIN_API_AUTH; # bypass admin api auth
Review Comment:
I don't think etcd has a good name for this configuration.
Straightforward, without explanation, is a good name.
--
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] moonming commented on a diff in pull request #9147: feat(cli): support APISIX_ALLOW_NONE_AUTHENTICATION
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1151309966
##########
apisix/cli/ngx_tpl.lua:
##########
@@ -51,6 +51,8 @@ worker_shutdown_timeout {* worker_shutdown_timeout *};
env APISIX_PROFILE;
env PATH; # for searching external plugin runner's binary
+env APISIX_ALLOW_NONE_AUTHENTICATION; # allow any IP with empty admin_key
Review Comment:
need a straightforward, self-explanatory name
--
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] moonming commented on a diff in pull request #9147: feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1151553333
##########
apisix/admin/init.lua:
##########
@@ -66,6 +67,12 @@ local router
local function check_token(ctx)
+ -- check if APISIX_BYPASS_ADMIN_API_AUTH=true
+ local none_auth = getenv("APISIX_BYPASS_ADMIN_API_AUTH")
Review Comment:
```suggestion
local bypass_admin_api_auth = getenv("APISIX_BYPASS_ADMIN_API_AUTH")
```
##########
t/admin/token.t:
##########
@@ -177,3 +177,21 @@ PUT /apisix/admin/plugins/reload?api_key=4054f7cf07e344346cd3f287985e76a2
--- request
GET /apisix/admin/routes??api_key=4054f7cf07e344346cd3f287985e76a2
--- error_code: 401
+
+
+
+=== TEST 10: access without api key
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
+
+
+
+=== TEST 11: access without api key, but APISIX_BYPASS_ADMIN_API_AUTH=true
+--- main_config
+env APISIX_BYPASS_ADMIN_API_AUTH=true;
Review Comment:
need to add test cases for `APISIX_BYPASS_ADMIN_API_AUTH` is not `true`
##########
t/admin/token.t:
##########
@@ -177,3 +177,21 @@ PUT /apisix/admin/plugins/reload?api_key=4054f7cf07e344346cd3f287985e76a2
--- request
GET /apisix/admin/routes??api_key=4054f7cf07e344346cd3f287985e76a2
--- error_code: 401
+
+
+
+=== TEST 10: access without api key
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
+
+
+
+=== TEST 11: access without api key, but APISIX_BYPASS_ADMIN_API_AUTH=true
+--- main_config
+env APISIX_BYPASS_ADMIN_API_AUTH=true;
Review Comment:
and if we set `APISIX_BYPASS_ADMIN_API_AUTH` to `abc`, what will happen?
##########
apisix/cli/ops.lua:
##########
@@ -190,6 +190,15 @@ local function init(env)
checked_admin_key = true
end
+ -- check if APISIX_BYPASS_ADMIN_API_AUTH=true
+ local allow_none_auth = getenv("APISIX_BYPASS_ADMIN_API_AUTH")
+ if allow_none_auth == "true" then
+ checked_admin_key = true
+ print("Warning! AdminKey is bypassed because of APISIX_BYPASS_ADMIN_API_AUTH=true.",
+ "If you are deploying APISIX in a production environment,",
+ "please disable it and set a secure password for the adminKey!")
+ end
+
Review Comment:
Why are there duplicate codes?
##########
t/admin/token.t:
##########
@@ -177,3 +177,21 @@ PUT /apisix/admin/plugins/reload?api_key=4054f7cf07e344346cd3f287985e76a2
--- request
GET /apisix/admin/routes??api_key=4054f7cf07e344346cd3f287985e76a2
--- error_code: 401
+
+
+
+=== TEST 10: access without api key
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
+
+
+
+=== TEST 11: access without api key, but APISIX_BYPASS_ADMIN_API_AUTH=true
+--- main_config
+env APISIX_BYPASS_ADMIN_API_AUTH=true;
Review Comment:
do we have a schema check for these configures?
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152920733
##########
t/cli/test_admin.sh:
##########
@@ -189,6 +189,62 @@ fi
echo "pass: missing admin key and only allow 127.0.0.0/24 to access admin api"
+# allow any IP to access admin api with empty admin_key, when admin_key_required=true
+
+git checkout conf/config.yaml
+
+echo '
+deployment:
+ admin:
+ admin_key_required: true
Review Comment:
what happened if admin_key_required is missing
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152950522
##########
apisix/cli/ops.lua:
##########
@@ -189,6 +189,13 @@ local function init(env)
and #allow_admin == 1 and allow_admin[1] == "127.0.0.0/24" then
checked_admin_key = true
end
+ -- check if admin_key is required
Review Comment:
> OK, it's just for starting, righting?
Yes. It just a check before starting.
> if set `127.0.0.0/24`, the user cann't access the APISIX? I don't find the code
I mean if set `127.0.0.0/24`, the user **CAN** access the APISIX with empty admin key configuraion. Otherwise the APISIX will start failed (such as `0.0.0.0/0` in allow_admin but empty admin_key)..
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152961476
##########
t/cli/test_admin.sh:
##########
@@ -189,6 +189,62 @@ fi
echo "pass: missing admin key and only allow 127.0.0.0/24 to access admin api"
+# allow any IP to access admin api with empty admin_key, when admin_key_required=true
+
+git checkout conf/config.yaml
+
+echo '
+deployment:
+ admin:
+ admin_key_required: true
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if ! grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: false
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should not show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+if ! grep -E "Warning! AdminKey is bypassed" output.log > /dev/null; then
+ echo "failed: should show 'Warning! AdminKey is bypassed'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: invalid-value
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "path[deployment->admin->admin_key_required] expect: boolean, but got: string" output.log > /dev/null; then
+ echo "failed: should show 'expect: boolean, but got: string'"
Review Comment:
OK, this is for the output file, i mean what you `echo` to the screen.
`echo "failed: should show 'expect: boolean, but got: string'"`
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1156967360
##########
apisix/cli/ops.lua:
##########
@@ -189,6 +189,13 @@ local function init(env)
and #allow_admin == 1 and allow_admin[1] == "127.0.0.0/24" then
checked_admin_key = true
end
+ -- check if admin_key is required
+ if yaml_conf.deployment.admin.admin_key_required == false then
+ checked_admin_key = true
+ print("Warning! Admin key is bypassed! "
+ .. "If you are deploying APISIX in a production environment, "
+ .. "please disable it and set a secure password for the admin Key!")
+ end
Review Comment:
It seems that the only code common to both sides is:
```
if conf.deployment.admin.admin_key_required == false then
print / log
end
```
Actually the output approach is different (before APISIX starting is `print`, after starting is `log`).
So IMHO, merge this two part to one function is unnecessary, because the common part is too simple (only with common part `conf.deployment.admin.admin_key_required == false` )
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1156971898
##########
conf/config-default.yaml:
##########
@@ -588,6 +588,10 @@ deployment:
role_traditional:
config_provider: etcd
admin:
+ # admin_key required or not. Default value is true.
+ # Bypass the Admin API authentication by modifying this value to false if needed.
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] moonming commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1156950870
##########
t/admin/token.t:
##########
@@ -177,3 +177,21 @@ PUT /apisix/admin/plugins/reload?api_key=4054f7cf07e344346cd3f287985e76a2
--- request
GET /apisix/admin/routes??api_key=4054f7cf07e344346cd3f287985e76a2
--- error_code: 401
+
+
+
+=== TEST 10: access without api key
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
+
+
+
+=== TEST 11: access without api key, but APISIX_BYPASS_ADMIN_API_AUTH=true
+--- main_config
+env APISIX_BYPASS_ADMIN_API_AUTH=true;
Review Comment:
Good. we can write this to docs.
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1151627161
##########
apisix/cli/ngx_tpl.lua:
##########
@@ -51,6 +51,8 @@ worker_shutdown_timeout {* worker_shutdown_timeout *};
env APISIX_PROFILE;
env PATH; # for searching external plugin runner's binary
+env APISIX_BYPASS_ADMIN_API_AUTH; # bypass admin api auth
Review Comment:
Sounds weird...
I prefer the variable name in bitnami/etcd that bypass authentication:
`ALLOW_NONE_AUTHENTICATION` looks more natural.
If we use `ADMIN_API_TOKEN_REQUIRED`, users should bypass authentication by specifying `false`.
IMHO, `false` is not a good value to make users feel positive.
--
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 pull request #9147: feat(cli): support APISIX_ALLOW_NONE_AUTHENTICATION
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #9147:
URL: https://github.com/apache/apisix/pull/9147#issuecomment-1487846598
Please make the CI pass
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152908246
##########
t/cli/test_admin.sh:
##########
@@ -189,6 +189,62 @@ fi
echo "pass: missing admin key and only allow 127.0.0.0/24 to access admin api"
+# allow any IP to access admin api with empty admin_key, when admin_key_required=true
+
+git checkout conf/config.yaml
+
+echo '
+deployment:
+ admin:
+ admin_key_required: true
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if ! grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: false
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should not show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+if ! grep -E "Warning! AdminKey is bypassed" output.log > /dev/null; then
Review Comment:
`Admin key` is 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] An-DJ commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152942801
##########
t/cli/test_admin.sh:
##########
@@ -189,6 +189,62 @@ fi
echo "pass: missing admin key and only allow 127.0.0.0/24 to access admin api"
+# allow any IP to access admin api with empty admin_key, when admin_key_required=true
+
+git checkout conf/config.yaml
+
+echo '
+deployment:
+ admin:
+ admin_key_required: true
Review Comment:
True is assigned to admin_key_required by default in `config-default.yaml`.
IMHO we do not need to add more test cases to test the missing case.
All the test cases now under the `t/` can validate it, which not configuring the `admin_key_required`.
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152963796
##########
t/cli/test_admin.sh:
##########
@@ -189,6 +189,62 @@ fi
echo "pass: missing admin key and only allow 127.0.0.0/24 to access admin api"
+# allow any IP to access admin api with empty admin_key, when admin_key_required=true
+
+git checkout conf/config.yaml
+
+echo '
+deployment:
+ admin:
+ admin_key_required: true
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if ! grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: false
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should not show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+if ! grep -E "Warning! AdminKey is bypassed" output.log > /dev/null; then
+ echo "failed: should show 'Warning! AdminKey is bypassed'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: invalid-value
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "path[deployment->admin->admin_key_required] expect: boolean, but got: string" output.log > /dev/null; then
+ echo "failed: should show 'expect: boolean, but got: string'"
Review Comment:
Got it.
--
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] moonming commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1156956148
##########
conf/config-default.yaml:
##########
@@ -588,6 +588,10 @@ deployment:
role_traditional:
config_provider: etcd
admin:
+ # admin_key required or not. Default value is true.
+ # Bypass the Admin API authentication by modifying this value to false if needed.
Review Comment:
Not fixed yet.
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1159508278
##########
t/admin/api.t:
##########
@@ -156,3 +156,51 @@ X-API-VERSION: v2
GET /t
--- response_body
passed
+
+
+
+=== TEST 10: Access with api key, and admin_key_required=true
+--- yaml_config
+deployment:
+ admin:
+ admin_key_required: true
+--- more_headers
+X-API-KEY: edd1c9f034335f136f87ad84b625c8f1
+--- request
+GET /apisix/admin/routes
+--- error_code: 200
+
+
+
+=== TEST 11: Access without api key, but admin_key_required=true
+--- yaml_config
+deployment:
+ admin:
+ admin_key_required: true
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
+
+
+
+=== TEST 12: Access without api key, but admin_key_required=true
+--- yaml_config
+deployment:
+ admin:
+ admin_key_required: true
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
Review Comment:
It should be `Access with api key, but admin_key_required=false`
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1151583690
##########
t/admin/token.t:
##########
@@ -177,3 +177,21 @@ PUT /apisix/admin/plugins/reload?api_key=4054f7cf07e344346cd3f287985e76a2
--- request
GET /apisix/admin/routes??api_key=4054f7cf07e344346cd3f287985e76a2
--- error_code: 401
+
+
+
+=== TEST 10: access without api key
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
+
+
+
+=== TEST 11: access without api key, but APISIX_BYPASS_ADMIN_API_AUTH=true
+--- main_config
+env APISIX_BYPASS_ADMIN_API_AUTH=true;
Review Comment:
> do we have a schema check for these configures?
This variable is currently a reserved environment variable. It is not a configuration item in the configuration file (config.yaml).
It seems that currently only the configuration items in the configuration file are checked by schema.
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1151580464
##########
t/admin/token.t:
##########
@@ -177,3 +177,21 @@ PUT /apisix/admin/plugins/reload?api_key=4054f7cf07e344346cd3f287985e76a2
--- request
GET /apisix/admin/routes??api_key=4054f7cf07e344346cd3f287985e76a2
--- error_code: 401
+
+
+
+=== TEST 10: access without api key
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
+
+
+
+=== TEST 11: access without api key, but APISIX_BYPASS_ADMIN_API_AUTH=true
+--- main_config
+env APISIX_BYPASS_ADMIN_API_AUTH=true;
Review Comment:
> and if we set `APISIX_BYPASS_ADMIN_API_AUTH` to `abc`, what will happen?
All values that are not true will be considered false.
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1151617265
##########
t/admin/token.t:
##########
@@ -177,3 +177,21 @@ PUT /apisix/admin/plugins/reload?api_key=4054f7cf07e344346cd3f287985e76a2
--- request
GET /apisix/admin/routes??api_key=4054f7cf07e344346cd3f287985e76a2
--- error_code: 401
+
+
+
+=== TEST 10: access without api key
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
+
+
+
+=== TEST 11: access without api key, but APISIX_BYPASS_ADMIN_API_AUTH=true
+--- main_config
+env APISIX_BYPASS_ADMIN_API_AUTH=true;
Review Comment:
Do you mean we bypass the admin-api-auth by configuring in `config.yaml`, instead of setting reserved environment variable?
--
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] moonming commented on a diff in pull request #9147: feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1151607855
##########
t/admin/token.t:
##########
@@ -177,3 +177,21 @@ PUT /apisix/admin/plugins/reload?api_key=4054f7cf07e344346cd3f287985e76a2
--- request
GET /apisix/admin/routes??api_key=4054f7cf07e344346cd3f287985e76a2
--- error_code: 401
+
+
+
+=== TEST 10: access without api key
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
+
+
+
+=== TEST 11: access without api key, but APISIX_BYPASS_ADMIN_API_AUTH=true
+--- main_config
+env APISIX_BYPASS_ADMIN_API_AUTH=true;
Review Comment:
> This variable is currently a reserved environment variable. It is not a configuration item in the configuration file (config.yaml).
can we move it to config.yaml? near by the token?
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152900403
##########
apisix/cli/ops.lua:
##########
@@ -189,6 +189,13 @@ local function init(env)
and #allow_admin == 1 and allow_admin[1] == "127.0.0.0/24" then
checked_admin_key = true
end
+ -- check if admin_key is required
Review Comment:
It seems that this logic is not necessary, It's just for start APISIX, but you can not access apisix without admin key, right?
```
if yaml_conf.apisix.enable_admin and allow_admin
and #allow_admin == 1 and allow_admin[1] == "127.0.0.0/24" then
checked_admin_key = true
end
```
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152927825
##########
apisix/cli/ops.lua:
##########
@@ -189,6 +189,13 @@ local function init(env)
and #allow_admin == 1 and allow_admin[1] == "127.0.0.0/24" then
checked_admin_key = true
end
+ -- check if admin_key is required
Review Comment:
OK, it's just for start, right? if set `127.0.0.0/24`, the user cann't access the APISIX? I don't find the code
##########
apisix/cli/ops.lua:
##########
@@ -189,6 +189,13 @@ local function init(env)
and #allow_admin == 1 and allow_admin[1] == "127.0.0.0/24" then
checked_admin_key = true
end
+ -- check if admin_key is required
Review Comment:
OK, it's just for start, righting? if set `127.0.0.0/24`, the user cann't access the APISIX? I don't find the code
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152924875
##########
apisix/cli/ops.lua:
##########
@@ -189,6 +189,13 @@ local function init(env)
and #allow_admin == 1 and allow_admin[1] == "127.0.0.0/24" then
checked_admin_key = true
end
+ -- check if admin_key is required
Review Comment:
IMHO These codes does not conflict with the current feature.
This logic is to allow `127.0.0.0/24` authentication-free access to the Admin API by default.
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152939378
##########
t/cli/test_admin.sh:
##########
@@ -189,6 +189,62 @@ fi
echo "pass: missing admin key and only allow 127.0.0.0/24 to access admin api"
+# allow any IP to access admin api with empty admin_key, when admin_key_required=true
+
+git checkout conf/config.yaml
+
+echo '
+deployment:
+ admin:
+ admin_key_required: true
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if ! grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: false
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should not show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+if ! grep -E "Warning! AdminKey is bypassed" output.log > /dev/null; then
+ echo "failed: should show 'Warning! AdminKey is bypassed'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: invalid-value
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "path[deployment->admin->admin_key_required] expect: boolean, but got: string" output.log > /dev/null; then
+ echo "failed: should show 'expect: boolean, but got: string'"
Review Comment:
The warning message prefix is needed: `check admin_key_required value failed`, or you will not what happened only with `failed: should show 'expect: boolean, but got: string`
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 merged PR #9147:
URL: https://github.com/apache/apisix/pull/9147
--
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 pull request #9147: feat(cli): support APISIX_ALLOW_NONE_AUTHENTICATION
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #9147:
URL: https://github.com/apache/apisix/pull/9147#issuecomment-1482208683
If this option is turned on, can we provide some hints, otherwise there may be security risks
--
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] moonming commented on a diff in pull request #9147: feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1151643678
##########
apisix/cli/ngx_tpl.lua:
##########
@@ -51,6 +51,8 @@ worker_shutdown_timeout {* worker_shutdown_timeout *};
env APISIX_PROFILE;
env PATH; # for searching external plugin runner's binary
+env APISIX_BYPASS_ADMIN_API_AUTH; # bypass admin api auth
Review Comment:
> IMHO, `false` is not a good value to make users feel positive.
you are right. So we don't suggest the user set it to `false` in prod.
--
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] moonming commented on a diff in pull request #9147: feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1151644583
##########
t/admin/token.t:
##########
@@ -177,3 +177,21 @@ PUT /apisix/admin/plugins/reload?api_key=4054f7cf07e344346cd3f287985e76a2
--- request
GET /apisix/admin/routes??api_key=4054f7cf07e344346cd3f287985e76a2
--- error_code: 401
+
+
+
+=== TEST 10: access without api key
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
+
+
+
+=== TEST 11: access without api key, but APISIX_BYPASS_ADMIN_API_AUTH=true
+--- main_config
+env APISIX_BYPASS_ADMIN_API_AUTH=true;
Review Comment:
can we set it on both sides?
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1151579331
##########
apisix/cli/ops.lua:
##########
@@ -190,6 +190,15 @@ local function init(env)
checked_admin_key = true
end
+ -- check if APISIX_BYPASS_ADMIN_API_AUTH=true
+ local allow_none_auth = getenv("APISIX_BYPASS_ADMIN_API_AUTH")
+ if allow_none_auth == "true" then
+ checked_admin_key = true
+ print("Warning! AdminKey is bypassed because of APISIX_BYPASS_ADMIN_API_AUTH=true.",
+ "If you are deploying APISIX in a production environment,",
+ "please disable it and set a secure password for the adminKey!")
+ end
+
Review Comment:
Do you mean that hint appears in both stdout and log?
1. `opt.lua` checks if allow_admin in config.yaml is correct before APISIX starts. We need to bypass the adminKey check here.
2. `init.lua` checks the adminKey when accessing the Admin API (after APISIX starts). We need to bypass check_token, too.
That's why I have implemented bypass-logic in two different stage.(before and after the start).
--
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] moonming commented on a diff in pull request #9147: feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1151605575
##########
apisix/cli/ngx_tpl.lua:
##########
@@ -51,6 +51,8 @@ worker_shutdown_timeout {* worker_shutdown_timeout *};
env APISIX_PROFILE;
env PATH; # for searching external plugin runner's binary
+env APISIX_BYPASS_ADMIN_API_AUTH; # bypass admin api auth
Review Comment:
This configuration is more straightforward
##########
apisix/cli/ngx_tpl.lua:
##########
@@ -51,6 +51,8 @@ worker_shutdown_timeout {* worker_shutdown_timeout *};
env APISIX_PROFILE;
env PATH; # for searching external plugin runner's binary
+env APISIX_BYPASS_ADMIN_API_AUTH; # bypass admin api auth
Review Comment:
```suggestion
env ADMIN_API_TOKEN_REQUIRED = true; # token is required to access the admin API
```
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152920733
##########
t/cli/test_admin.sh:
##########
@@ -189,6 +189,62 @@ fi
echo "pass: missing admin key and only allow 127.0.0.0/24 to access admin api"
+# allow any IP to access admin api with empty admin_key, when admin_key_required=true
+
+git checkout conf/config.yaml
+
+echo '
+deployment:
+ admin:
+ admin_key_required: true
Review Comment:
what happened if `admin_key_required` is missing
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152950522
##########
apisix/cli/ops.lua:
##########
@@ -189,6 +189,13 @@ local function init(env)
and #allow_admin == 1 and allow_admin[1] == "127.0.0.0/24" then
checked_admin_key = true
end
+ -- check if admin_key is required
Review Comment:
> OK, it's just for starting, righting?
Yes. It just a check before starting.
> if set `127.0.0.0/24`, the user cann't access the APISIX? I don't find the code
I mean if set `127.0.0.0/24`, the user **CAN** access the APISIX with empty admin key configuraion. Otherwise the APISIX will start failed (such as `0.0.0.0/0` in allow_admin but empty admin_key)..
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152951544
##########
apisix/cli/ops.lua:
##########
@@ -189,6 +189,13 @@ local function init(env)
and #allow_admin == 1 and allow_admin[1] == "127.0.0.0/24" then
checked_admin_key = true
end
+ -- check if admin_key is required
Review Comment:
It is a default behavior that allow the user access Admin API from local.
--
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] An-DJ commented on pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on PR #9147:
URL: https://github.com/apache/apisix/pull/9147#issuecomment-1498770251
Test cases are designed as below:
CC @moonming
--
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] moonming commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1159469248
##########
t/admin/api.t:
##########
@@ -156,3 +156,51 @@ X-API-VERSION: v2
GET /t
--- response_body
passed
+
+
+
+=== TEST 10: Access with api key, and admin_key_required=true
+--- yaml_config
+deployment:
+ admin:
+ admin_key_required: true
+--- more_headers
+X-API-KEY: edd1c9f034335f136f87ad84b625c8f1
+--- request
+GET /apisix/admin/routes
+--- error_code: 200
+
+
+
+=== TEST 11: Access without api key, but admin_key_required=true
+--- yaml_config
+deployment:
+ admin:
+ admin_key_required: true
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
+
+
+
+=== TEST 12: Access without api key, but admin_key_required=true
+--- yaml_config
+deployment:
+ admin:
+ admin_key_required: true
+--- request
+GET /apisix/admin/routes
+--- error_code: 401
Review Comment:
Why are these two test cases the same?
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152909790
##########
t/cli/test_admin.sh:
##########
@@ -189,6 +189,62 @@ fi
echo "pass: missing admin key and only allow 127.0.0.0/24 to access admin api"
+# allow any IP to access admin api with empty admin_key, when admin_key_required=true
+
+git checkout conf/config.yaml
+
+echo '
+deployment:
+ admin:
+ admin_key_required: true
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if ! grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: false
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should not show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+if ! grep -E "Warning! AdminKey is bypassed" output.log > /dev/null; then
+ echo "failed: should show 'Warning! AdminKey is bypassed'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: invalid-value
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "path[deployment->admin->admin_key_required] expect: boolean, but got: string" output.log > /dev/null; then
+ echo "failed: should show 'expect: boolean, but got: string'"
+ exit 1
+fi
+
+echo "pass: allow empty admin_key, when admin_key_required=true"
Review Comment:
echo "pass: allow empty admin_key, when admin_key_required=false"
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152967930
##########
t/cli/test_admin.sh:
##########
@@ -189,6 +189,62 @@ fi
echo "pass: missing admin key and only allow 127.0.0.0/24 to access admin api"
+# allow any IP to access admin api with empty admin_key, when admin_key_required=true
+
+git checkout conf/config.yaml
+
+echo '
+deployment:
+ admin:
+ admin_key_required: true
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if ! grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: false
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should not show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+if ! grep -E "Warning! AdminKey is bypassed" output.log > /dev/null; then
+ echo "failed: should show 'Warning! AdminKey is bypassed'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: invalid-value
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "path[deployment->admin->admin_key_required] expect: boolean, but got: string" output.log > /dev/null; then
+ echo "failed: should show 'expect: boolean, but got: string'"
Review Comment:
Done.
##########
t/cli/test_admin.sh:
##########
@@ -189,6 +189,62 @@ fi
echo "pass: missing admin key and only allow 127.0.0.0/24 to access admin api"
+# allow any IP to access admin api with empty admin_key, when admin_key_required=true
+
+git checkout conf/config.yaml
+
+echo '
+deployment:
+ admin:
+ admin_key_required: true
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] An-DJ commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152849021
##########
conf/config-default.yaml:
##########
@@ -588,6 +588,10 @@ deployment:
role_traditional:
config_provider: etcd
admin:
+ # If admin_key is required, default value is true.
Review Comment:
Good point.
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152961476
##########
t/cli/test_admin.sh:
##########
@@ -189,6 +189,62 @@ fi
echo "pass: missing admin key and only allow 127.0.0.0/24 to access admin api"
+# allow any IP to access admin api with empty admin_key, when admin_key_required=true
+
+git checkout conf/config.yaml
+
+echo '
+deployment:
+ admin:
+ admin_key_required: true
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if ! grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: false
+ admin_key: ~
+ allow_admin:
+ - 0.0.0.0/0
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "ERROR: missing valid Admin API token." output.log > /dev/null; then
+ echo "failed: should not show 'ERROR: missing valid Admin API token.'"
+ exit 1
+fi
+
+if ! grep -E "Warning! AdminKey is bypassed" output.log > /dev/null; then
+ echo "failed: should show 'Warning! AdminKey is bypassed'"
+ exit 1
+fi
+
+echo '
+deployment:
+ admin:
+ admin_key_required: invalid-value
+' > conf/config.yaml
+
+make init > output.log 2>&1 | true
+
+if grep -E "path[deployment->admin->admin_key_required] expect: boolean, but got: string" output.log > /dev/null; then
+ echo "failed: should show 'expect: boolean, but got: string'"
Review Comment:
OK, this is for the output file, i mean what you `echo` to the screen
--
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] An-DJ commented on a diff in pull request #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "An-DJ (via GitHub)" <gi...@apache.org>.
An-DJ commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152960054
##########
t/cli/test_admin.sh:
##########
@@ -189,6 +189,62 @@ fi
echo "pass: missing admin key and only allow 127.0.0.0/24 to access admin api"
+# allow any IP to access admin api with empty admin_key, when admin_key_required=true
+
+git checkout conf/config.yaml
+
+echo '
+deployment:
+ admin:
+ admin_key_required: true
Review Comment:
LGTM 👍
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152949866
##########
t/cli/test_admin.sh:
##########
@@ -189,6 +189,62 @@ fi
echo "pass: missing admin key and only allow 127.0.0.0/24 to access admin api"
+# allow any IP to access admin api with empty admin_key, when admin_key_required=true
+
+git checkout conf/config.yaml
+
+echo '
+deployment:
+ admin:
+ admin_key_required: true
Review Comment:
What i think is that, the logic is like this:
```
-- check if admin_key is required
if not local_conf.deployment.admin.admin_key_required then
return true
end
```
If the `admin_key_required` is missing, the `admin_key_required` will be `nil`?
Is this better?
```
-- check if admin_key is required
if local_conf.deployment.admin.admin_key_required == false then
return true
end
``
##########
t/cli/test_admin.sh:
##########
@@ -189,6 +189,62 @@ fi
echo "pass: missing admin key and only allow 127.0.0.0/24 to access admin api"
+# allow any IP to access admin api with empty admin_key, when admin_key_required=true
+
+git checkout conf/config.yaml
+
+echo '
+deployment:
+ admin:
+ admin_key_required: true
Review Comment:
What i'm thinking is that, the logic is like this:
```
-- check if admin_key is required
if not local_conf.deployment.admin.admin_key_required then
return true
end
```
If the `admin_key_required` is missing, the `admin_key_required` will be `nil`?
Is this better?
```
-- check if admin_key is required
if local_conf.deployment.admin.admin_key_required == false then
return true
end
``
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152900614
##########
apisix/admin/init.lua:
##########
@@ -395,6 +401,13 @@ function _M.init_worker()
events.register(reload_plugins, reload_event, "PUT")
if ngx_worker_id() == 0 then
+ -- check if admin_key is required
+ if not local_conf.deployment.admin.admin_key_required then
+ core.log.warn("AdminKey is bypassed.",
+ "If you are deploying APISIX in a production environment,",
+ "please disable it and set a secure password for the adminKey!")
Review Comment:
Is this better ?
```
core.log.warn("Admin key is bypassed! ",
"If you are deploying APISIX in a production environment, ",
"please disable it and set a secure password for the admin Key!")
```
--
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 #9147: feat(cli): support bypassing Admin API Auth by configuration
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1152942890
##########
apisix/cli/ops.lua:
##########
@@ -189,6 +189,13 @@ local function init(env)
and #allow_admin == 1 and allow_admin[1] == "127.0.0.0/24" then
checked_admin_key = true
end
+ -- check if admin_key is required
Review Comment:
If this code is useless, would you mind creating a PR to remove it?
--
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] moonming commented on a diff in pull request #9147: feat(cli): support APISIX_ALLOW_NONE_AUTHENTICATION
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on code in PR #9147:
URL: https://github.com/apache/apisix/pull/9147#discussion_r1144927904
##########
apisix/cli/ngx_tpl.lua:
##########
@@ -51,6 +51,8 @@ worker_shutdown_timeout {* worker_shutdown_timeout *};
env APISIX_PROFILE;
env PATH; # for searching external plugin runner's binary
+env APISIX_ALLOW_NONE_AUTHENTICATION; # allow any IP with empty admin_key
Review Comment:
bypass_admin_api_auth is 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