You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/05/14 08:02:24 UTC

[GitHub] [apisix] kwanhur opened a new pull request, #7049: feat(ops): allow access admin loopback IP address

kwanhur opened a new pull request, #7049:
URL: https://github.com/apache/apisix/pull/7049

   
   ### Description
   Allow [IPv4 loopback address](https://datatracker.ietf.org/doc/html/rfc5735#section-3) 127.0.0.0/8 and [IPv6 loopback address](https://datatracker.ietf.org/doc/html/rfc4291#section-2.5.3) to access admin.
   
   Now, if not the address `127.0.0.0/24`, it'll get a warning tip like 
   
   ```shell
   WARNING: using fixed Admin API token has security risk.
   Please modify "admin_key" in conf/config.yaml .
   ```
   
   <!-- Please include a summary of the change and which issue is fixed. -->
   <!-- Please also include relevant motivation and context. -->
   * support single IP address and CIDR format
   * support both IPv4 and IPv6
   * remove hardcode 127.0.0.0/24 when checking
   
   ### Checklist
   
   - [x] I have explained the need for this PR and the problem it solves
   - [x] I have explained the changes or the new features added to this PR
   - [ ] I have added tests corresponding to this change
   - [ ] I have updated the documentation to reflect this change
   - [x] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
   
   <!--
   
   Note
   
   1. Mark the PR as draft until it's ready to be reviewed.
   2. Always add/update tests for any changes unless you have a good reason.
   3. Always update the documentation to reflect the changes made in the PR.
   4. Make a new commit to resolve conversations instead of `push -f`.
   5. To resolve merge conflicts, merge master instead of rebasing.
   6. Use "request review" to notify the reviewer after making changes.
   7. Only a reviewer can mark a conversation as resolved.
   
   -->
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] kwanhur commented on pull request #7049: feat(ops): allow access admin loopback IP address

Posted by GitBox <gi...@apache.org>.
kwanhur commented on PR #7049:
URL: https://github.com/apache/apisix/pull/7049#issuecomment-1126961567

   Test cases cover IPv4 IPv6 CIDR and IPv6 loopback single address.
   
   I'm not sure if need to cover non-loopback address cases.


-- 
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] github-actions[bot] closed pull request #7049: feat(ops): allow access admin loopback IP address

Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #7049: feat(ops): allow access admin loopback IP address
URL: https://github.com/apache/apisix/pull/7049


-- 
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] kwanhur commented on a diff in pull request #7049: feat(ops): allow access admin loopback IP address

Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7049:
URL: https://github.com/apache/apisix/pull/7049#discussion_r879061030


##########
t/cli/test_admin.sh:
##########
@@ -112,6 +113,67 @@ if [ $count -eq 0 ]; then
     exit 1
 fi
 
+echo "
+apisix:
+    enable_admin: true
+    allow_admin:
+        - 127.0.0.0/8
+" > conf/config.yaml
+
+make init
+
+count=`grep -c "allow 127.0.0.0/8" conf/nginx.conf`
+if [ $count -eq 0 ]; then
+    echo "failed: not found 'allow 127.0.0.0/8;' in conf/nginx.conf"
+    exit 1
+fi
+
+echo "
+apisix:
+    enable_admin: true
+    allow_admin:
+        - ::1

Review Comment:
   To make some confirmation, the not relative is that `allow_admin` should be limited on which option?
   
   1. Option A `IPv4 address` and `IPv4 CIDR`, the others are not relative.
   2. Option B `IPv6 address` and `IPv6 CIDR` are not relative, the others are ok.
   3. Option C `all` is not relative.
   
   [allow](https://nginx.org/en/docs/http/ngx_http_access_module.html#allow) (ngx_http_access_module)
   
   ```
   Syntax: | allow address | CIDR | unix: | all;
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] spacewander commented on pull request #7049: feat(ops): allow access admin loopback IP address

Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #7049:
URL: https://github.com/apache/apisix/pull/7049#issuecomment-1126918946

   Would you please add test to cover 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] kwanhur commented on a diff in pull request #7049: feat(ops): allow access admin loopback IP address

Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7049:
URL: https://github.com/apache/apisix/pull/7049#discussion_r875382251


##########
t/cli/test_admin.sh:
##########
@@ -112,6 +112,48 @@ if [ $count -eq 0 ]; then
     exit 1
 fi
 
+echo "

Review Comment:
   Added case to cover it. Plz take a view.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] tzssangglass commented on a diff in pull request #7049: feat(ops): allow access admin loopback IP address

Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7049:
URL: https://github.com/apache/apisix/pull/7049#discussion_r874917476


##########
t/cli/test_admin.sh:
##########
@@ -112,6 +113,66 @@ if [ $count -eq 0 ]; then
     exit 1
 fi
 
+echo "
+apisix:
+    enable_admin: true
+    allow_admin:
+        - 127.0.0.0/8
+" > conf/config.yaml
+
+make init
+
+count=`grep -c "allow 127.0.0.0/8" conf/nginx.conf`
+if [ $count -eq 0 ]; then

Review Comment:
   Not sure I understand what the purpose of using count here is? Worried about other configurations also 
   `listen 127.0.0.0/8` ?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] spacewander commented on a diff in pull request #7049: feat(ops): allow access admin loopback IP address

Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7049:
URL: https://github.com/apache/apisix/pull/7049#discussion_r876520762


##########
t/cli/test_admin.sh:
##########
@@ -112,6 +112,48 @@ if [ $count -eq 0 ]; then
     exit 1
 fi
 
+echo "

Review Comment:
   I mean, your test should walk through the code in this branch: https://github.com/apache/apisix/blob/260529a981e61ffa5db7c0750cfeb5a9b453c839/apisix/cli/ops.lua#L187
   
   It should check the behavior associated with checked_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] spacewander commented on pull request #7049: feat(ops): allow access admin loopback IP address

Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #7049:
URL: https://github.com/apache/apisix/pull/7049#issuecomment-1127296635

   Let's merge master to make 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] kwanhur commented on pull request #7049: feat(ops): allow access admin loopback IP address

Posted by GitBox <gi...@apache.org>.
kwanhur commented on PR #7049:
URL: https://github.com/apache/apisix/pull/7049#issuecomment-1127558445

   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] kwanhur commented on a diff in pull request #7049: feat(ops): allow access admin loopback IP address

Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7049:
URL: https://github.com/apache/apisix/pull/7049#discussion_r873692960


##########
t/cli/test_admin.sh:
##########
@@ -112,6 +112,48 @@ if [ $count -eq 0 ]; then
     exit 1
 fi
 
+echo "

Review Comment:
   Thx. 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] spacewander commented on a diff in pull request #7049: feat(ops): allow access admin loopback IP address

Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7049:
URL: https://github.com/apache/apisix/pull/7049#discussion_r878851838


##########
t/cli/test_admin.sh:
##########
@@ -112,6 +113,67 @@ if [ $count -eq 0 ]; then
     exit 1
 fi
 
+echo "
+apisix:
+    enable_admin: true
+    allow_admin:
+        - 127.0.0.0/8
+" > conf/config.yaml
+
+make init
+
+count=`grep -c "allow 127.0.0.0/8" conf/nginx.conf`
+if [ $count -eq 0 ]; then
+    echo "failed: not found 'allow 127.0.0.0/8;' in conf/nginx.conf"
+    exit 1
+fi
+
+echo "
+apisix:
+    enable_admin: true
+    allow_admin:
+        - ::1

Review Comment:
   Could you rewrite or remove test cases which are not relative to the new feature?



-- 
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] kwanhur commented on a diff in pull request #7049: feat(ops): allow access admin loopback IP address

Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7049:
URL: https://github.com/apache/apisix/pull/7049#discussion_r874773983


##########
t/cli/test_admin.sh:
##########
@@ -112,6 +112,48 @@ if [ $count -eq 0 ]; then
     exit 1
 fi
 
+echo "

Review Comment:
   Ohh, I got what you mean, now all the cases are `checked_admin_key = true`, should make it `checked_admin_key = false`, add more test cases 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] kwanhur commented on a diff in pull request #7049: feat(ops): allow access admin loopback IP address

Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7049:
URL: https://github.com/apache/apisix/pull/7049#discussion_r877044976


##########
t/cli/test_admin.sh:
##########
@@ -112,6 +112,48 @@ if [ $count -eq 0 ]; then
     exit 1
 fi
 
+echo "

Review Comment:
   Case as below I think it's ok.
   
   The detail here, `yaml_conf.apisix.enable_admin` to `true`, `yaml_conf.apisix.allow_admin` not empty, so it through into foreach element to check `loopback` or not, but value `all` still stays `checked_admin_key` to `false`. 
   
   So this case satisfies `if yaml_conf.apisix.enable_admin and not checked_admin_key then` and walks inside.
   
   Am I missing other? Please point it out, thanks very much.
   
   ```shell
   echo "
   apisix:
       enable_admin: true
       allow_admin:
           - all
   " > 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] spacewander commented on a diff in pull request #7049: feat(ops): allow access admin loopback IP address

Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7049:
URL: https://github.com/apache/apisix/pull/7049#discussion_r874431581


##########
t/cli/test_admin.sh:
##########
@@ -112,6 +112,48 @@ if [ $count -eq 0 ]; then
     exit 1
 fi
 
+echo "

Review Comment:
   Err... Even reverting the feature, the newer added tests can still pass, as they don't check the behavior of `checked_admin_key = true`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] spacewander commented on a diff in pull request #7049: feat(ops): allow access admin loopback IP address

Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7049:
URL: https://github.com/apache/apisix/pull/7049#discussion_r873383063


##########
t/cli/test_admin.sh:
##########
@@ -112,6 +112,48 @@ if [ $count -eq 0 ]; then
     exit 1
 fi
 
+echo "

Review Comment:
   The newer added tests don't check the behavior of `checked_admin_key = true`



-- 
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] github-actions[bot] commented on pull request #7049: feat(ops): allow access admin loopback IP address

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #7049:
URL: https://github.com/apache/apisix/pull/7049#issuecomment-1192406223

   This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] kwanhur commented on a diff in pull request #7049: feat(ops): allow access admin loopback IP address

Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7049:
URL: https://github.com/apache/apisix/pull/7049#discussion_r875380927


##########
t/cli/test_admin.sh:
##########
@@ -112,6 +113,66 @@ if [ $count -eq 0 ]; then
     exit 1
 fi
 
+echo "
+apisix:
+    enable_admin: true
+    allow_admin:
+        - 127.0.0.0/8
+" > conf/config.yaml
+
+make init
+
+count=`grep -c "allow 127.0.0.0/8" conf/nginx.conf`
+if [ $count -eq 0 ]; then

Review Comment:
   `count` used to find the target address render or not. It'll place with `allow ip-address or cidr;`.
   
   https://github.com/apache/apisix/blob/6ebe02797fb564c84b30df1865adef7f473880bf/apisix/cli/ngx_tpl.lua#L515-L528



-- 
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] kwanhur commented on a diff in pull request #7049: feat(ops): allow access admin loopback IP address

Posted by GitBox <gi...@apache.org>.
kwanhur commented on code in PR #7049:
URL: https://github.com/apache/apisix/pull/7049#discussion_r878233290


##########
t/cli/test_admin.sh:
##########
@@ -112,6 +112,48 @@ if [ $count -eq 0 ]; then
     exit 1
 fi
 
+echo "

Review Comment:
   Added case value `10.0.0.1`.
   
   The branch `if yaml_conf.apisix.enable_admin and not checked_admin_key then` seems be out of range changed feature from `if allow_ip == "127.0.0.0/24" then` to `if _ip and _ip:is_loopback() then`.
   
   I think the new test case should cover `if _ip and _ip:is_loopback() then`, like the value `all` `10.0.0.1` `127.0.0.9` `127.0.0.1/8` `::1` `::1/128`, these cases are `non-IP address` `non-loopback` `IPv4 loopback address` `IPv4 loopback CIDR` `IPv6 loopback address` `IPv6 loopback CIDR`.
   
   Maybe I'm confused now.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix] spacewander commented on a diff in pull request #7049: feat(ops): allow access admin loopback IP address

Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7049:
URL: https://github.com/apache/apisix/pull/7049#discussion_r877659849


##########
t/cli/test_admin.sh:
##########
@@ -112,6 +112,48 @@ if [ $count -eq 0 ]; then
     exit 1
 fi
 
+echo "

Review Comment:
   The tests can be passed even without the new feature.



-- 
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] github-actions[bot] commented on pull request #7049: feat(ops): allow access admin loopback IP address

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #7049:
URL: https://github.com/apache/apisix/pull/7049#issuecomment-1220492759

   This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org