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/09/15 06:45:12 UTC
[GitHub] [apisix] membphis opened a new pull request #2230: bug: removed default access token for Admin API
membphis opened a new pull request #2230:
URL: https://github.com/apache/apisix/pull/2230
### 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. -->
related PR: #2092
### 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] moonming commented on pull request #2230: bug: removed default access token for Admin API
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2230:
URL: https://github.com/apache/apisix/pull/2230#issuecomment-692518089
why `******` is invalid? Is this hard-coded in the code?
Thanks,
Ming Wen
Twitter: _WenMing
YuanSheng Wang <no...@github.com> 于2020年9月15日周二 下午3:15写道：
> YOUR_OWN_API_TOKEN
>
> that is a bad name, because of the YOUR_OWN_API_TOKEN is a valid Admin
> API key.
>
> the ****** is a invalid Admin API key.
>
> valid config.yaml
>
> apisix:
> admin_key:
> -
> name: "admin"
> key: YOUR_OWN_API_TOKEN # set your Admin API Key
> role: admin
>
> invalid config.yaml
>
> apisix:
> admin_key:
> -
> name: "admin"
> key: ****** # set your Admin API Key
> role: admin
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/apisix/pull/2230#issuecomment-692516876>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AGJZBK6ADQDUQTKZTXUDE4TSF4H7JANCNFSM4RMUDT4Q>
> .
>
----------------------------------------------------------------
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 closed pull request #2230: bug: removed default access token for Admin API
Posted by GitBox <gi...@apache.org>.
membphis closed pull request #2230:
URL: https://github.com/apache/apisix/pull/2230
----------------------------------------------------------------
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] moonming commented on a change in pull request #2230: bug: removed default access token for Admin API
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2230:
URL: https://github.com/apache/apisix/pull/2230#discussion_r488454473
##########
File path: bin/apisix
##########
@@ -643,6 +643,34 @@ local function read_yaml_conf()
merge_conf(default_conf, user_conf)
end
+ -- check the Admin API token
+ if default_conf.apisix.enable_admin then
+ local help = [[
+ERROR: missing valid apisix.admin_key
+
+You can call `]] .. arg[0] .. [[ gen_admin_key` to generate a new Admin API key or
+manually update the `conf/config.yaml` file.
+]]
+ if type(default_conf.apisix.admin_key) ~= "table" or
+ #default_conf.apisix.admin_key == 0
+ then
+ io.stderr:write(help, "\n")
+ os.exit(1)
+ end
+
+ for _, admin in ipairs(default_conf.apisix.admin_key) do
+ if type(admin.key) == "table" then
+ admin.key = ""
+ else
+ admin.key = tostring(admin.key)
+ end
+
+ if admin.key == "" or admin.key:gsub("*", "") == "" then
Review comment:
`*` is invalid, which is too shadowy
----------------------------------------------------------------
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 edited a comment on pull request #2230: bug: removed default access token for Admin API
Posted by GitBox <gi...@apache.org>.
membphis edited a comment on pull request #2230:
URL: https://github.com/apache/apisix/pull/2230#issuecomment-692513326
> Users will encounter errors when running `apisix start`, and this experience is bad.
I think it is good enough for now. We have told the user how to generate the Admin API key.
```shell
./bin/apisix init
ERROR: missing valid apisix.admin_key
You can call `./bin/apisix gen_admin_key` to generate a new Admin API key or
manually update the `conf/config.yaml` file.
```
If you prefer to automatically generate tokens, we can fix it with another new PR maybe.
We need to discuss on the mailing list whether to automatically generate tokens for users when APISIX starts.
The main purpose of this PR is to remove all default tokens. The current PR is already big. I hope this PR will merge as soon as possible instead of being blocked 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #2230: bug: removed default access token for Admin API
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2230:
URL: https://github.com/apache/apisix/pull/2230#issuecomment-693734871
In this pr, we only need to modify one file, and only need to make one change, which is to print a warning when the Admin IP is not 127.0.0.1 and using default 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2230: bug: removed default access token for Admin API
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2230:
URL: https://github.com/apache/apisix/pull/2230#issuecomment-692558053
> I insist on this, if automatic generation token is not possible, I would rather not remove the default token
I think they are two different questions. Let us discuss it in mail list.
----------------------------------------------------------------
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] moonming commented on pull request #2230: bug: removed default access token for Admin API
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2230:
URL: https://github.com/apache/apisix/pull/2230#issuecomment-692515707
I insist on this, if automatic generation token is not possible, I would
rather not remove the default token
Thanks,
Ming Wen
Twitter: _WenMing
YuanSheng Wang <no...@github.com> 于2020年9月15日周二 下午3:07写道：
> Users will encounter errors when running apisix start, and this
> experience is bad.
>
> I think it is enough for now. We have told the user how to generate the
> Admin API key.
>
> ./bin/apisix init
> ERROR: missing valid apisix.admin_key
>
> You can call `./bin/apisix gen_admin_key` to generate a new Admin API key or
> manually update the `conf/config.yaml` file.
>
> If you prefer to automatically generate tokens, we can fix it with another
> new PR maybe.
>
> We need to discuss on the mailing list whether to automatically generate
> tokens for users when APISIX starts.
>
> The main purpose of this PR is to remove all default tokens. The current
> PR is already big. I hope this PR will merge as soon as possible instead of
> being blocked here.
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/apisix/pull/2230#issuecomment-692513326>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AGJZBK2IYEZGLTSW3IMAC2TSF4HCZANCNFSM4RMUDT4Q>
> .
>
----------------------------------------------------------------
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 pull request #2230: bug: removed default access token for Admin API
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2230:
URL: https://github.com/apache/apisix/pull/2230#issuecomment-693481591
> I don't think so, `YOUR_OWN_API_TOKEN` is good as hint, not the real token.
> we can keep the current token
you are right.
the current way, if the APISIX starts with the default Admin API Key, we'll show a WARNING message.
and I updated the default token to a different one.
```
$ ./bin/apisix start
WARNING: using the default Key is very dangerous.
You can call `./bin/apisix gen_admin_key` to generate a new Admin API key or
manually update the `conf/config.yaml` file.
```
----------------------------------------------------------------
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] moonming commented on a change in pull request #2230: bug: removed default access token for Admin API
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2230:
URL: https://github.com/apache/apisix/pull/2230#discussion_r488430805
##########
File path: FAQ.md
##########
@@ -80,7 +80,7 @@ An example, `foo.com/product/index.html?id=204&page=2`, gray release based on `i
here is the way:
```shell
-curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: ******' -X PUT -d '
Review comment:
`******` is not a good hint for user.
----------------------------------------------------------------
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 pull request #2230: bug: removed default access token for Admin API
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2230:
URL: https://github.com/apache/apisix/pull/2230#issuecomment-692513326
> Users will encounter errors when running `apisix start`, and this experience is bad.
I think it is enough for now. We have told the user how to generate the Admin API key.
```shell
./bin/apisix init
ERROR: missing valid apisix.admin_key
You can call `./bin/apisix gen_admin_key` to generate a new Admin API key or
manually update the `conf/config.yaml` file.
```
If you prefer to automatically generate tokens, we can fix it with another new PR maybe.
We need to discuss on the mailing list whether to automatically generate tokens for users when APISIX starts.
The main purpose of this PR is to remove all default tokens. The current PR is already big. I hope this PR will merge as soon as possible instead of being blocked 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #2230: bug: removed default access token for Admin API
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2230:
URL: https://github.com/apache/apisix/pull/2230#issuecomment-693237220
I don't think so, `YOUR_OWN_API_TOKEN` is good as hint, not the real token.
we can keep the current 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on pull request #2230: bug: removed default access token for Admin API
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2230:
URL: https://github.com/apache/apisix/pull/2230#issuecomment-692516876
> YOUR_OWN_API_TOKEN
that is a bad name, because of the `YOUR_OWN_API_TOKEN` is a valid Admin API key.
the `******` is a invalid Admin API key.
> valid `config.yaml`
```yaml
apisix:
admin_key:
-
name: "admin"
key: YOUR_OWN_API_TOKEN # set your Admin API Key
role: admin
```
> invalid `config.yaml`
```yaml
apisix:
admin_key:
-
name: "admin"
key: ****** # set your Admin API Key
role: admin
```
----------------------------------------------------------------
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] moonming commented on pull request #2230: bug: removed default access token for Admin API
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2230:
URL: https://github.com/apache/apisix/pull/2230#issuecomment-692515133
YOUR_OWN_API_TOKEN
Thanks,
Ming Wen
Twitter: _WenMing
YuanSheng Wang <no...@github.com> 于2020年9月15日周二 下午3:09写道：
> *@membphis* commented on this pull request.
> ------------------------------
>
> In FAQ.md
> <https://github.com/apache/apisix/pull/2230#discussion_r488435973>:
>
> > @@ -80,7 +80,7 @@ An example, `foo.com/product/index.html?id=204&page=2` <http://foo.com/product/index.html?id=204&page=2>, gray release based on `i
>
> here is the way:
> ```shell
> -curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
> +curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: ******' -X PUT -d '
>
> what is your advice?
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/apisix/pull/2230#discussion_r488435973>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AGJZBKZSL3D5LOHGX6PE7ALSF4HL3ANCNFSM4RMUDT4Q>
> .
>
----------------------------------------------------------------
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 #2230: bug: removed default access token for Admin API
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2230:
URL: https://github.com/apache/apisix/pull/2230#discussion_r488483897
##########
File path: bin/apisix
##########
@@ -643,6 +643,34 @@ local function read_yaml_conf()
merge_conf(default_conf, user_conf)
end
+ -- check the Admin API token
+ if default_conf.apisix.enable_admin then
+ local help = [[
+ERROR: missing valid apisix.admin_key
+
+You can call `]] .. arg[0] .. [[ gen_admin_key` to generate a new Admin API key or
+manually update the `conf/config.yaml` file.
+]]
+ if type(default_conf.apisix.admin_key) ~= "table" or
+ #default_conf.apisix.admin_key == 0
+ then
+ io.stderr:write(help, "\n")
+ os.exit(1)
+ end
+
+ for _, admin in ipairs(default_conf.apisix.admin_key) do
+ if type(admin.key) == "table" then
+ admin.key = ""
+ else
+ admin.key = tostring(admin.key)
+ end
+
+ if admin.key == "" or admin.key:gsub("*", "") == "" then
Review comment:
In APISIX, the Admin API Key containing only `*` is invalid.
It is shadowy, but it works.
If we use `YOUR_OWN_API_TOKEN` as Key, then we have to disable it here too.
----------------------------------------------------------------
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 pull request #2230: bug: removed default access token for Admin API
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2230:
URL: https://github.com/apache/apisix/pull/2230#issuecomment-692528591
> why `******` is invalid? Is this hard-coded in the code?
you can take a look at this source code in CLI:
https://github.com/apache/apisix/pull/2230/files#diff-62edadffc237f13dc28a694080d293a8R668
----------------------------------------------------------------
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 #2230: bug: removed default access token for Admin API
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2230:
URL: https://github.com/apache/apisix/pull/2230#discussion_r488435973
##########
File path: FAQ.md
##########
@@ -80,7 +80,7 @@ An example, `foo.com/product/index.html?id=204&page=2`, gray release based on `i
here is the way:
```shell
-curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+curl -i http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: ******' -X PUT -d '
Review comment:
what is your advice?
----------------------------------------------------------------
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 pull request #2230: bug: removed default access token for Admin API
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2230:
URL: https://github.com/apache/apisix/pull/2230#issuecomment-693232569
> No, `YOUR_OWN_API_TOKEN` is just hint in doc, not api token itself.
How about we use `YOUR_OWN_API_TOKEN` as the default token?
The APISIX will show a WARNING message when APISIX started, prompt the user to customize a new token if the user does not set a new one.
----------------------------------------------------------------
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] moonming commented on pull request #2230: bug: removed default access token for Admin API
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2230:
URL: https://github.com/apache/apisix/pull/2230#issuecomment-692560517
No, `YOUR_OWN_API_TOKEN` is just hint in doc, not api token itself.
Thanks,
Ming Wen
Twitter: _WenMing
YuanSheng Wang <no...@github.com> 于2020年9月15日周二 下午4:29写道：
> *@membphis* commented on this pull request.
> ------------------------------
>
> In bin/apisix
> <https://github.com/apache/apisix/pull/2230#discussion_r488483897>:
>
> > +]]
> + if type(default_conf.apisix.admin_key) ~= "table" or
> + #default_conf.apisix.admin_key == 0
> + then
> + io.stderr:write(help, "\n")
> + os.exit(1)
> + end
> +
> + for _, admin in ipairs(default_conf.apisix.admin_key) do
> + if type(admin.key) == "table" then
> + admin.key = ""
> + else
> + admin.key = tostring(admin.key)
> + end
> +
> + if admin.key == "" or admin.key:gsub("*", "") == "" then
>
> In APISIX, the Admin API Key containing only * is invalid.
> It is shadowy, but it works.
>
> If we use YOUR_OWN_API_TOKEN as Key, then we have to disable it here too.
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/apisix/pull/2230#discussion_r488483897>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AGJZBK6OLV47AE3TNJ6YQM3SF4QXTANCNFSM4RMUDT4Q>
> .
>
----------------------------------------------------------------
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 pull request #2230: bug: removed default access token for Admin API
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2230:
URL: https://github.com/apache/apisix/pull/2230#issuecomment-693935676
> In this pr, we only need to modify one file, and only need to make one change, which is to print a warning when the Admin IP is not 127.0.0.1 and using default token
will create a new PR in this way. we can close this PR first.
----------------------------------------------------------------
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