You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/08/03 13:37:04 UTC
[GitHub] [apisix] membphis opened a new pull request #1979: bugfix: empty `allow_admin` means that any IP is allowed to access Ad…
membphis opened a new pull request #1979:
URL: https://github.com/apache/apisix/pull/1979
…min API.
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
fix #1939
### Pre-submission checklist:
* [ ] Did you explain what problem does this PR solve? Or what new features have been added?
* [ ] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [ ] Is this PR backward compatible?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #1979: bugfix: empty `allow_admin` means that any IP is allowed to access Admin API and Dashboard
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #1979:
URL: https://github.com/apache/apisix/pull/1979#discussion_r504425295
##########
File path: .travis/apisix_cli_test.sh
##########
@@ -211,8 +211,40 @@ fi
echo "passed: worker_shutdown_timeout in nginx.conf is ok"
+# empty allow_admin in conf/config.yaml
Review comment:
nice catch
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #1979: bugfix: empty `allow_admin` means that any IP is allowed to access Admin API and Dashboard
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #1979:
URL: https://github.com/apache/apisix/pull/1979#discussion_r504354046
##########
File path: .travis/apisix_cli_test.sh
##########
@@ -211,8 +211,40 @@ fi
echo "passed: worker_shutdown_timeout in nginx.conf is ok"
+# empty allow_admin in conf/config.yaml
Review comment:
I think the title is not a good title. Because the following example has a situation where ʻallow_admin` is not empty.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #1979: bugfix: empty `allow_admin` means that any IP is allowed to access Ad…
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #1979:
URL: https://github.com/apache/apisix/pull/1979#discussion_r502904442
##########
File path: .travis/apisix_cli_test.sh
##########
@@ -211,8 +211,40 @@ fi
echo "passed: worker_shutdown_timeout in nginx.conf is ok"
+# empty allow_admin in conf/config.yaml
+
+echo "
+apisix:
+ allow_admin:
+ - 127.0.0.9
+" > conf/config.yaml
+
+make init
+
+count=`grep -c "allow 127.0.0.9" conf/nginx.conf`
+if [ $count -eq 0 ]; then
+ echo "failed: not found 'allow 127.0.0.9;' in conf/nginx.conf"
+ exit 1
+fi
+
+echo "
+apisix:
+ allow_admin: ~
+" > conf/config.yaml
+
+make init
+
+count=`grep -c "allow all;" conf/nginx.conf || true`
Review comment:
Look like the `|| true` doesn't take effect?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #1979: bugfix: empty `allow_admin` means that any IP is allowed to access Ad…
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #1979:
URL: https://github.com/apache/apisix/pull/1979#discussion_r502899561
##########
File path: .travis/apisix_cli_test.sh
##########
@@ -211,6 +211,33 @@ fi
echo "passed: worker_shutdown_timeout in nginx.conf is ok"
+# empty allow_admin in conf/config.yaml
+
+git checkout conf/config.yaml
+
+sed -i 's/- 127.0.0.0\/24/- 127.0.0.9/' conf/config.yaml
+
+make init
+
+grep -E "allow 127.0.0.9;" conf/nginx.conf > /dev/null
+if [ ! $? -eq 0 ]; then
+ echo "failed: not found 'allow 127.0.0.9;' in conf/nginx.conf"
+ exit 1
+fi
+
+sed -i 's/- 127.0.0.9/\# - 127.0.0.9/' conf/config.yaml
+
+make init
+
+set +ex
+grep "allow 127.0.0.9;" conf/nginx.conf > /dev/null
+if [ ! $? -eq 1 ]; then
+ echo "failed: found allow 127.0.0.9; in conf/nginx.conf"
+ exit 1
Review comment:
this way is better, updated
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #1979: bugfix: empty `allow_admin` means that any IP is allowed to access Ad…
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #1979:
URL: https://github.com/apache/apisix/pull/1979#issuecomment-690913327
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #1979: bugfix: empty `allow_admin` means that any IP is allowed to access Ad…
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #1979:
URL: https://github.com/apache/apisix/pull/1979#issuecomment-706653728
The `.travis/apisix_cli_test.sh` is in conflicted now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Yiyiyimu commented on a change in pull request #1979: bugfix: empty `allow_admin` means that any IP is allowed to access Ad…
Posted by GitBox <gi...@apache.org>.
Yiyiyimu commented on a change in pull request #1979:
URL: https://github.com/apache/apisix/pull/1979#discussion_r502878260
##########
File path: .travis/apisix_cli_test.sh
##########
@@ -211,6 +211,33 @@ fi
echo "passed: worker_shutdown_timeout in nginx.conf is ok"
+# empty allow_admin in conf/config.yaml
+
+git checkout conf/config.yaml
+
+sed -i 's/- 127.0.0.0\/24/- 127.0.0.9/' conf/config.yaml
+
+make init
+
+grep -E "allow 127.0.0.9;" conf/nginx.conf > /dev/null
+if [ ! $? -eq 0 ]; then
+ echo "failed: not found 'allow 127.0.0.9;' in conf/nginx.conf"
+ exit 1
+fi
+
+sed -i 's/- 127.0.0.9/\# - 127.0.0.9/' conf/config.yaml
+
+make init
+
+set +ex
+grep "allow 127.0.0.9;" conf/nginx.conf > /dev/null
+if [ ! $? -eq 1 ]; then
+ echo "failed: found allow 127.0.0.9; in conf/nginx.conf"
+ exit 1
Review comment:
I think we should not check if 127.0.0.9 is in nginx.conf, since it's commented out, it would never be in nginx.conf, so current version would still pass this test.
I think we need to test if there is
```
location /apisix/admin {
deny all;
```
which cause this problem.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #1979: bugfix: empty `allow_admin` means that any IP is allowed to access Ad…
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #1979:
URL: https://github.com/apache/apisix/pull/1979#issuecomment-690913327
Need to update apisix_cli_test.sh before merge.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #1979: bugfix: empty `allow_admin` means that any IP is allowed to access Ad…
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #1979:
URL: https://github.com/apache/apisix/pull/1979#issuecomment-690913327
Need to update apisix_cli_test.sh before merge.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #1979: bugfix: empty `allow_admin` means that any IP is allowed to access Ad…
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #1979:
URL: https://github.com/apache/apisix/pull/1979#discussion_r503938891
##########
File path: .travis/apisix_cli_test.sh
##########
@@ -211,8 +211,40 @@ fi
echo "passed: worker_shutdown_timeout in nginx.conf is ok"
+# empty allow_admin in conf/config.yaml
+
+echo "
+apisix:
+ allow_admin:
+ - 127.0.0.9
+" > conf/config.yaml
+
+make init
+
+count=`grep -c "allow 127.0.0.9" conf/nginx.conf`
+if [ $count -eq 0 ]; then
+ echo "failed: not found 'allow 127.0.0.9;' in conf/nginx.conf"
+ exit 1
+fi
+
+echo "
+apisix:
+ allow_admin: ~
+" > conf/config.yaml
+
+make init
+
+count=`grep -c "allow all;" conf/nginx.conf || true`
Review comment:
nice, removed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis merged pull request #1979: bugfix: empty `allow_admin` means that any IP is allowed to access Admin API and Dashboard
Posted by GitBox <gi...@apache.org>.
membphis merged pull request #1979:
URL: https://github.com/apache/apisix/pull/1979
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on pull request #1979: bugfix: empty `allow_admin` means that any IP is allowed to access Ad…
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on pull request #1979:
URL: https://github.com/apache/apisix/pull/1979#issuecomment-708103358
I think the title is not a good title, I think it’s better like this: "empty `allow_admin` means that any IP is allowed to access Admin API and Dashboard. " . Because this `pr` does two things: allowed to access Admin API and Dashboard.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #1979: bugfix: empty `allow_admin` means that any IP is allowed to access Ad…
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #1979:
URL: https://github.com/apache/apisix/pull/1979#issuecomment-690913327
Need to update apisix_cli_test.sh before merge.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org