feat: Autogenerate admin api key if not passed #11080

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 <ashishjaitiwari15112000@gmail.com>

…isix into revolyssup/autogeneratekey

kayx23

a few suggestions for the docs

docs/en/latest/admin-api.md

docs/zh/latest/plugins/proxy-mirror.md

apisix/core/id.lua

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

apisix/core/id.lua

+        -- 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

apisix/core/id.lua

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

apisix/core/id.lua

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

conf/config-default.yaml

@@ -170,7 +170,8 @@ nginx_config:                     # Config for render the template to generate n
  stream:
    enable_access_log: false                 # Enable stream proxy access logging.
    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

conf/config-default.yaml

+    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

apisix/core/id.lua

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

apisix/core/id.lua

-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

apisix/core/id.lua


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


please revert this change.

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

t/fuzzing/simple_http.py

+        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

.github/workflows/source-install.yml

@@ -88,8 +88,15 @@ jobs:

      - name: Test apisix
        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

.github/workflows/source-install.yml

-              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

conf/config-default.yaml

@@ -170,7 +170,8 @@ nginx_config:                     # Config for render the template to generate n
  stream:
    enable_access_log: false                 # Enable stream proxy access logging.
    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

docs/en/latest/plugins/api-breaker.md

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

apisix/core/id.lua

Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>

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 <ashishjaitiwari15112000@gmail.com>

Change rockspec

f1fe58b

Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>

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 <ashishjaitiwari15112000@gmail.com>

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 6 commits April 1, 2024 12:20

apply suggestions from code review

fae943b

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

docs/en/latest/admin-api.md Show resolved Hide resolved

docs/en/latest/admin-api.md Outdated Show resolved Hide resolved

docs/zh/latest/plugins/proxy-mirror.md Outdated Show resolved Hide resolved

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

apisix/core/id.lua Outdated Show resolved Hide resolved

shreemaan-abhishek reviewed Apr 9, 2024

View reviewed changes

apisix/core/id.lua Outdated Show resolved Hide resolved

shreemaan-abhishek reviewed Apr 9, 2024

View reviewed changes

apisix/core/id.lua Show resolved Hide resolved

apply suggestion

612ef88

Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>

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
56 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Autogenerate admin api key if not passed #11080

feat: Autogenerate admin api key if not passed #11080

Revolyssup commented Mar 22, 2024 •

edited

Loading

kayx23 left a comment

starsz Apr 1, 2024

starsz Apr 1, 2024

moonming Apr 1, 2024

moonming Apr 1, 2024

Revolyssup Apr 1, 2024

shreemaan-abhishek Apr 5, 2024

Revolyssup Apr 9, 2024

shreemaan-abhishek Apr 5, 2024

shreemaan-abhishek Apr 5, 2024

shreemaan-abhishek Apr 5, 2024

Revolyssup Apr 9, 2024

shreemaan-abhishek Apr 5, 2024

Revolyssup Apr 9, 2024

shreemaan-abhishek Apr 5, 2024

Revolyssup commented Apr 9, 2024

kayx23 Apr 9, 2024

Revolyssup Apr 9, 2024

kayx23 left a comment

kayx23 left a comment

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

feat: Autogenerate admin api key if not passed #11080

feat: Autogenerate admin api key if not passed #11080

Conversation

Revolyssup commented Mar 22, 2024 • edited Loading

Description

Notes

Checklist

kayx23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Revolyssup commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kayx23 left a comment

Choose a reason for hiding this comment

kayx23 left a comment

Choose a reason for hiding this comment

Revolyssup commented Mar 22, 2024 •

edited

Loading