feat: Autogenerate admin api key if not passed by Revolyssup · Pull Request #11080 · apache/apisix

Revolyssup · 2024-03-22T08:24:44Z

Description

Fixes # (issue)
This PR is part of the proposal which removes hardcoding of sensitive API keys by autogenerating them at either application level(APISIX) or deployment level.

Notes

The admin api key is autogenerated if it is passed empty.
All the CLI tests now retrieve the key from config.yaml first before sending request to the API server, which is why the change in tests related to CLI.
If key is null or a non empty string, then it will not be changed.
This is not recommended at application level and recommended at infra level. For instance, the helm chart can autogenerate key. Therefore users are advised to pass their own unique key. For simpler local, single host deployments, APISIX can autogenerate the key which the users can read from config.yaml.

Related PRs:
apache/apisix-helm-chart#740
apache/apisix-docker#548

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)

Signed-off-by: Ashish Tiwari <[email protected]>

…isix into revolyssup/autogeneratekey

kayx23

a few suggestions for the docs

starsz · 2024-04-01T08:38:36Z

+        -- Check if deployment.admin.admin_key is not nil and it's an empty string
+        local admin_keys = default_conf.deployment
+            and default_conf.deployment.admin
+            and default_conf.deployment.admin.admin_key


I think you can try core.table.try_read_attr to improve your code.

starsz · 2024-04-01T08:40:29Z

+        local local_conf_path = profile:yaml_path("config")
+        local ok, err = write_file(local_conf_path, yaml_conf)
+        if not ok then
+            log.error(err)


I think you should panic if we can't write file.

moonming · 2024-04-01T09:09:16Z

    access_log: logs/access_stream.log       # Location of the stream access log.
-    access_log_format: "$remote_addr [$time_local] $protocol $status $bytes_sent $bytes_received $session_time" # Customize log format: http://nginx.org/en/docs/varindex.html
+    access_log_format: |
+      "$remote_addr [$time_local] $protocol $status $bytes_sent $bytes_received $session_time" # Customize log format: http://nginx.org/en/docs/varindex.html


This change has nothing to do with this modification. If you think it is necessary, it is recommended to open a new PR to deal with it.

moonming · 2024-04-01T09:09:29Z

+    access_log_format: |
+      "$remote_addr - $remote_user [$time_local] $http_host \"$request\" $status $body_bytes_sent $request_time \"$http_referer\" \"$http_user_agent\" $upstream_addr $upstream_status $upstream_response_time \"$upstream_scheme://$upstream_host$upstream_uri\""


The lyaml.dump function which is being used here to convert lua table into yaml, messes up the formatting and adds a new line automatically. Using | operator avoids this issue by allowing for multiline string

shreemaan-abhishek · 2024-04-05T16:38:43Z

-local open             = io.open
-
-
+local try_read_attr = require("apisix.core.table").try_read_attr


please maintain code beautification (equals on top of each other)

shreemaan-abhishek · 2024-04-05T16:38:54Z

+local ngx_exit = ngx.exit

-local _M = {version = 0.1}
+local _M = { version = 0.1 }


please revert this change.

shreemaan-abhishek · 2024-04-05T16:45:03Z

+        print("Key not found in the YAML file.")
+        return
+    key = key.replace('"', '')
+    print("the key is",key)


Suggested change

print("the key is",key)

print("the key is", key)

shreemaan-abhishek · 2024-04-05T16:50:51Z

        run: |
-          curl http://127.0.0.1:9180/apisix/admin/routes/1 \
-            -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
+            wget https://github.com/mikefarah/yq/releases/download/3.4.1/yq_linux_amd64 -O /usr/bin/yq && sudo chmod +x /usr/bin/yq


this block has 4 indents, where as everywhere in this file there are 2 indents.

shreemaan-abhishek · 2024-04-05T16:51:04Z

-              cat logs/error.log
-              exit 125
-          fi
+            result_code=`curl -I -m 10 -o /dev/null -s -w %{http_code} http://127.0.0.1:9080/get`


this change is not needed.

shreemaan-abhishek · 2024-04-05T16:52:39Z

    access_log: logs/access_stream.log       # Location of the stream access log.
-    access_log_format: "$remote_addr [$time_local] $protocol $status $bytes_sent $bytes_received $session_time" # Customize log format: http://nginx.org/en/docs/varindex.html
+    access_log_format: |
+      "$remote_addr [$time_local] $protocol $status $bytes_sent $bytes_received $session_time" # Customize log format: http://nginx.org/en/docs/varindex.html


@Revolyssup please address this comment.

Revolyssup · 2024-04-09T03:27:46Z

@shreemaan-abhishek Comment addressed here #11080 (comment)

kayx23 · 2024-04-09T04:26:22Z

+You can fetch the `admin_key` from `config.yaml` and save to an environment variable with the following command:
+
+```bash
+ admin_key=$(yq '.deployment.admin.admin_key[0].key' conf/config.yaml | sed 's/"//g')


Suggested change

admin_key=$(yq '.deployment.admin.admin_key[0].key' conf/config.yaml | sed 's/"//g')

admin_key=$(yq '.deployment.admin.admin_key[0].key' conf/config.yaml | sed 's/"//g')

Remove extra space. Please replace all.

kayx23

Other doc changes LGTM.

Signed-off-by: Ashish Tiwari <[email protected]>

kayx23

doc changes LGTM

feat: Autogenerate admin api key if not passed

33d8f9c

Revolyssup marked this pull request as draft March 22, 2024 08:24

Revolyssup added 5 commits March 24, 2024 19:52

fix function

73efe8b

fix function

5f3f06f

use lyaml dump

cd4160c

Change makefile

2054e73

Signed-off-by: Ashish Tiwari <[email protected]>

Change rockspec

f1fe58b

Signed-off-by: Ashish Tiwari <[email protected]>

Revolyssup marked this pull request as ready for review March 27, 2024 08:39

Revolyssup added 22 commits March 27, 2024 14:20

fix merge conflict

4b64ef7

fix CI

05ead26

fix ci

9d18f3a

change install prefix

4449ed1

add YAML_DIR in luaconfig CI

d80ba97

install libyaml-devel

4c7d2b7

fix CI

336a63f

use flag to install libyaml-devel

6fa433d

fix name on ubuntu

b3ab95c

fix lint

9dd6a1f

fix lint

ef88059

fix lint

7d2093f

add function to extract $admin_key

e6d34a9

echo for debug

e07d3f5

add log for debug

e0a7d45

Signed-off-by: Ashish Tiwari <[email protected]>

add log

ea4f4f7

add verbose flag

063afb2

fix CI by export

4cda3f3

fix ci

e5def8e

use yq

2cbad3a

download yq

d2fc7eb

add yq

465c276

Revolyssup added 5 commits April 1, 2024 12:21

Merge branch 'revolyssup/autogeneratekey' of github.com:Revolyssup/ap…

3df4978

…isix into revolyssup/autogeneratekey

fix lint

6357411

remove unnecessary code from generate_yaml

c851820

update docs

df9dc76

fix doc lint

07afe44

kayx23 requested changes Apr 1, 2024

View reviewed changes

Comment thread docs/en/latest/admin-api.md

Comment thread docs/en/latest/admin-api.md Outdated

Comment thread docs/zh/latest/plugins/proxy-mirror.md Outdated

starsz reviewed Apr 1, 2024

View reviewed changes

moonming reviewed Apr 1, 2024

View reviewed changes

Revolyssup added 3 commits April 2, 2024 12:57

apply doc suggestion

ec03ce8

apply suggestions

aacb574

fix lint

258e1cf

shreemaan-abhishek requested a review from moonming April 4, 2024 06:40

moonming previously approved these changes Apr 4, 2024

View reviewed changes

shreemaan-abhishek reviewed Apr 5, 2024

View reviewed changes

apply code suggestions from review

277cc74

Revolyssup dismissed moonming’s stale review via 277cc74 April 9, 2024 03:32

kayx23 reviewed Apr 9, 2024

View reviewed changes

remove space from doc

93c0e01

shreemaan-abhishek reviewed Apr 9, 2024

View reviewed changes

Comment thread apisix/core/id.lua Outdated

shreemaan-abhishek reviewed Apr 9, 2024

View reviewed changes

Comment thread apisix/core/id.lua Outdated

shreemaan-abhishek reviewed Apr 9, 2024

View reviewed changes

Comment thread apisix/core/id.lua

apply suggestion

612ef88

Signed-off-by: Ashish Tiwari <[email protected]>

shreemaan-abhishek approved these changes Apr 9, 2024

View reviewed changes

kayx23 approved these changes Apr 9, 2024

View reviewed changes

Revolyssup requested review from moonming and starsz April 9, 2024 13:15

moonming merged commit c6b9f99 into apache:master Apr 10, 2024

dragonflylee mentioned this pull request Feb 18, 2025

bug: docker compose 启动报错 #11834

Closed

		access_log_format: \|
		"$remote_addr - $remote_user [$time_local] $http_host \"$request\" $status $body_bytes_sent $request_time \"$http_referer\" \"$http_user_agent\" $upstream_addr $upstream_status $upstream_response_time \"$upstream_scheme://$upstream_host$upstream_uri\""

		local open = io.open


		local try_read_attr = require("apisix.core.table").try_read_attr

	admin_key=$(yq '.deployment.admin.admin_key[0].key' conf/config.yaml \| sed 's/"//g')
	admin_key=$(yq '.deployment.admin.admin_key[0].key' conf/config.yaml \| sed 's/"//g')

Uh oh!

Conversation

Revolyssup commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes

Checklist

Uh oh!

kayx23 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Revolyssup commented Apr 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kayx23 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kayx23 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Revolyssup commented Mar 22, 2024 •

edited

Loading