Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(cli): support bypassing Admin API Auth by configuration #9147

Merged
merged 21 commits into from
Apr 10, 2023
Merged

feat(cli): support bypassing Admin API Auth by configuration #9147

merged 21 commits into from
Apr 10, 2023

Conversation

An-DJ
Copy link
Contributor

@An-DJ An-DJ commented Mar 22, 2023 •
edited
Loading

Description

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 the configuration below.

deployment:
  admin:
    admin_key_required: false    # Default value is true

Fixes #9071

image

Checklist

  • I have explained the need for this PR and the problem it solves
  • 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
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

moonming
moonming reviewed Mar 22, 2023
View reviewed changes
apisix/cli/ngx_tpl.lua Outdated Show resolved Hide resolved
@monkeyDluffy6017
Copy link
Contributor

monkeyDluffy6017 commented Mar 24, 2023 •
edited
Loading

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

@leslie-tsang
Copy link
Member

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.

@leslie-tsang leslie-tsang self-requested a review March 28, 2023 02:57
@An-DJ
Copy link
Contributor Author

An-DJ commented Mar 28, 2023

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.

moonming
moonming requested changes Mar 29, 2023
View reviewed changes
apisix/cli/ngx_tpl.lua Outdated Show resolved Hide resolved
@monkeyDluffy6017
Copy link
Contributor

Please make the CI pass

@An-DJ An-DJ changed the title feat(cli): support APISIX_ALLOW_NONE_AUTHENTICATION feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH Mar 29, 2023
moonming
moonming reviewed Mar 29, 2023
View reviewed changes
apisix/admin/init.lua Outdated Show resolved Hide resolved
apisix/cli/ops.lua Outdated
Comment on lines 193 to 201
-- 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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are there duplicate codes?

Copy link
Contributor Author

@An-DJ An-DJ Mar 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@An-DJ An-DJ Mar 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is repeated code, then it needs to be abstracted into a function

t/admin/token.t Outdated Show resolved Hide resolved
moonming
moonming reviewed Mar 29, 2023
View reviewed changes
apisix/cli/ngx_tpl.lua Outdated Show resolved Hide resolved
@An-DJ An-DJ changed the title feat(cli): support APISIX_BYPASS_ADMIN_API_AUTH feat(cli): support bypassing Admin API Auth by configuration Mar 30, 2023
leslie-tsang
leslie-tsang previously approved these changes Mar 31, 2023
View reviewed changes
moonming
moonming requested changes Mar 31, 2023
View reviewed changes
apisix/admin/init.lua Outdated Show resolved Hide resolved
apisix/cli/ops.lua Outdated
Comment on lines 193 to 198
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicate code

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an issue for the duplicate code and fix it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 )

conf/config-default.yaml Outdated Show resolved Hide resolved
apisix/admin/init.lua Outdated Show resolved Hide resolved
t/admin/api.t Outdated Show resolved Hide resolved
@An-DJ An-DJ dismissed stale reviews from leslie-tsang and monkeyDluffy6017 via bffbf56 April 4, 2023 08:54
moonming
moonming previously approved these changes Apr 6, 2023
View reviewed changes
moonming
moonming reviewed Apr 6, 2023
View reviewed changes
t/admin/api.t Outdated
Comment on lines 175 to 193
=== 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these two test cases the same?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be Access with api key, but admin_key_required=false

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@An-DJ
Copy link
Contributor Author

An-DJ commented Apr 6, 2023

Test cases are designed as below:

image

CC @moonming

@An-DJ An-DJ requested a review from moonming April 6, 2023 09:46
@monkeyDluffy6017 monkeyDluffy6017 merged commit 2c5639b into apache:master Apr 10, 2023
hongbinhsu added a commit to fitphp/apix that referenced this pull request Apr 11, 2023
@hongbinhsu
Merge branch 'upstream/master' into github/master
a6808a0 
* upstream/master: (25 commits)
  fix: upgrade lua-resty-ldap to 0.2.2 (apache#9254)
  feat(cli): support bypassing Admin API Auth by configuration (apache#9147)
  fix(ci): write version into xds first (apache#9274)
  fix: skip warning log when apisix.data_encryption.enable is false (apache#9057)
  docs: add-api7-information (apache#9260)
  docs: Fixed typo (apache#9244)
  docs: clarify what is client.ca in client-to-apisix-mtls.md (apache#9221)
  docs: Corrected typos and grammatical errors (apache#9216)
  docs: updated ssl sni parameter requirement in admin-api.md (apache#9176)
  fix: check upstream reference in traffic-split plugin when delete upstream (apache#9044)
  docs: Update proxy-rewrite headers.add docs (apache#9220)
  feat: suppot header injection for fault-injection plugin (apache#9039)
  fix: upgrade lua-resty-etcd to 1.10.4 (apache#9235)
  docs: fix incorrect semantic.yml link (apache#9231)
  feat: Upstream status report (apache#9151)
  fix: host_hdr should not be false (apache#9150)
  docs: remove APISIX base instruction (apache#9117)
  fix(cli): prevent non-`127.0.0.0/24` to access admin api with empty admin_key (apache#9146)
  docs: fix 404 link (apache#9160)
  fix(cors): consider using `allow_origins_by_regex` only when it is not `nil` (apache#9028)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monkeyDluffy6017 monkeyDluffy6017 monkeyDluffy6017 approved these changes

@moonming moonming moonming approved these changes

@leslie-tsang leslie-tsang leslie-tsang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@An-DJ @monkeyDluffy6017 @leslie-tsang @moonming