feat: use env var instead of plain text for vault token #8866

shreemaan-abhishek · 2023-02-15T08:56:30Z

Description

Currently, we use plain text to add vault as a secret resource. Like so:

curl http://127.0.0.1:9180/apisix/admin/secret/vault/1 \
-H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
{
    "uri": "https://127.0.0.1:8200"，
    "prefix": "apisix",
    "token": "root"
}'

This PR implements a feature that would allow the use of environment variables in the following format: $ENV://NAME_OF_ENV_VAR.

This is the final outcome:

curl http://127.0.0.1:9180/apisix/admin/secret/vault/1 \
-H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
{
    "uri": "https://127.0.0.1:8200"，
    "prefix": "apisix",
    "token": "$ENV://VAULT_TOKEN"
}'

A part of #8319

cc: @soulbird

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)

soulbird · 2023-02-15T11:50:04Z

t/config-center-yaml/secret.t

@@ -61,7 +61,7 @@ __DATA__
 secrets:
  - id: vault/1
    prefix: kv/apisix
-    token: root
+    token: "$ENV://VAULT_TOKEN"


So we can no longer use such a configuration?

token: root

Yes, we won't be able to use such a configuration. Let me fix that quickly.

This reverts commit 7f938e2.

soulbird · 2023-02-16T01:14:22Z

ci/centos7-ci.sh

@@ -84,6 +84,7 @@ run_case() {
    export_or_prefix
    make init
    set_coredns
+    export VAULT_TOKEN="root"


refer: https://github.com/apache/apisix/blob/master/t/core/env.t#L17-L20

soulbird · 2023-02-16T02:26:12Z

t/config-center-yaml/secret.t

+--- response_body
+0
+--- error_log
+property "uri" validation failed: failed to match pattern "^[^\\/]+:\\/\\/([\\da-zA-Z.-]+|\\[[\\da-fA-F:]+\\])(:\\d+)?"


Why add this test case? It doesn't look like it has anything to do with your modification？

I just duplicated all the test cases containing the token with just one change: replace the token with the environment variable. Let me do something better. 😂

soulbird · 2023-02-16T02:26:22Z

t/config-center-yaml/secret.t

+--- response_body
+0
+--- error_log
+secret manager not exits


soulbird · 2023-02-16T02:29:00Z

t/config-center-yaml/secret.t

+prefix: kv/apisix
+token: $ENV://VAULT_TOKEN
+uri: http://127.0.0.1:8200
+


There is no need to add this test

soulbird · 2023-02-16T02:31:00Z

t/config-center-yaml/secret.t

+
+
+
+=== TEST 21: secret.fetch_by_uri, no sub key value with the token in an env var


This test case already exists

soulbird · 2023-02-16T02:32:20Z

t/plugin/basic-auth.t

+        }
+    }
+--- response_body
+passed


You only configured the route, you also need to verify whether the route is effective

This reverts commit 62eb6f9.

This reverts commit 6c8cf91.

soulbird · 2023-02-20T01:44:28Z

t/config-center-yaml/secret.t

@@ -14,6 +14,10 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
+BEGIN {
+    $ENV{VAULT_TOKEN} = "root";


Why was this added?

soulbird · 2023-02-20T01:48:22Z

IMO, just adding a test case for t/secret/vault.t is enough. The test cases under t/plugin seem redundant, don't you think?

shreemaan-abhishek · 2023-02-20T03:25:30Z

The test cases under t/plugin seem redundant, don't you think?

I thought the same. But then I thought they would provide better safety from potential nasty bugs in the future.

soulbird · 2023-02-20T13:34:54Z

The test cases under t/plugin seem redundant, don't you think?

I thought the same. But then I thought they would provide better safety from potential nasty bugs in the future.

ok

spacewander

Let's update https://github.com/apache/apisix/blob/master/docs/en/latest/terminology/secret.md

spacewander · 2023-02-21T02:19:20Z

apisix/secret/vault.lua

@@ -26,6 +26,7 @@ local norm_path = require("pl.path").normpath
 local sub        = core.string.sub
 local rfind_char = core.string.rfind_char

+local env        = require("apisix.core.env")


Can't we import it like core.string.sub?

apisix/secret/vault.lua

spacewander · 2023-02-22T01:28:42Z

apisix/secret/vault.lua

@@ -26,6 +26,7 @@ local norm_path = require("pl.path").normpath
 local sub        = core.string.sub
 local rfind_char = core.string.rfind_char



We can avoid the blank line here? Other files don't add a blank line among localized variables.

shreemaan-abhishek added 3 commits February 15, 2023 14:17

feat: use env var instead of plain text

aa8b1bd

modify tests to use env var

7f938e2

export env var

c13c6ec

shreemaan-abhishek marked this pull request as ready for review February 15, 2023 09:03

export env var for centos

6bc6d61

soulbird reviewed Feb 15, 2023

View reviewed changes

shreemaan-abhishek added 3 commits February 15, 2023 18:57

fix breaking change, allow plain text env vars

111ac11

Revert "modify tests to use env var"

2d988b7

This reverts commit 7f938e2.

add tests

62eb6f9

soulbird reviewed Feb 16, 2023

View reviewed changes

place env var in correct place

26627b7

soulbird reviewed Feb 16, 2023

View reviewed changes

shreemaan-abhishek added 12 commits February 16, 2023 08:54

Revert "add tests"

0868b43

This reverts commit 62eb6f9.

add tests

c562216

fix tests

206dd90

Merge branch 'master' into vault-token-env-var

3aeed0d

add quotes

6c8cf91

use print instead of "say"

756f3b4

Revert "add quotes"

37a3406

This reverts commit 6c8cf91.

add line break

d4d5fb3

replace with \R

30a296b

remove directory added by mistake

1626dce

use regex

46b992e

trigger build

f2c1c27

soulbird reviewed Feb 20, 2023

View reviewed changes

remove unnecessary env var

9bfaede

soulbird previously approved these changes Feb 20, 2023

View reviewed changes

spacewander reviewed Feb 21, 2023

View reviewed changes

clean way to import

234346a

shreemaan-abhishek dismissed soulbird’s stale review via 234346a February 21, 2023 03:01

shreemaan-abhishek requested a review from spacewander February 21, 2023 06:41

soulbird reviewed Feb 21, 2023

View reviewed changes

apisix/secret/vault.lua Show resolved Hide resolved

soulbird previously approved these changes Feb 22, 2023

View reviewed changes

spacewander reviewed Feb 22, 2023

View reviewed changes

remove newline

8b72f12

shreemaan-abhishek dismissed soulbird’s stale review via 8b72f12 February 22, 2023 02:54

shreemaan-abhishek requested a review from spacewander February 22, 2023 02:55

trigger build

74b3e66

spacewander approved these changes Feb 23, 2023

View reviewed changes

soulbird approved these changes Feb 23, 2023

View reviewed changes

spacewander merged commit 14727fc into apache:master Feb 24, 2023

shreemaan-abhishek deleted the vault-token-env-var branch April 27, 2023 04:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use env var instead of plain text for vault token #8866

feat: use env var instead of plain text for vault token #8866

shreemaan-abhishek commented Feb 15, 2023 •

edited

Loading

soulbird Feb 15, 2023

shreemaan-abhishek Feb 15, 2023 •

edited

Loading

soulbird Feb 16, 2023

soulbird Feb 16, 2023

shreemaan-abhishek Feb 16, 2023 •

edited

Loading

soulbird Feb 16, 2023

soulbird Feb 16, 2023

soulbird Feb 16, 2023

soulbird Feb 16, 2023

soulbird Feb 20, 2023

soulbird commented Feb 20, 2023

shreemaan-abhishek commented Feb 20, 2023

soulbird commented Feb 20, 2023

spacewander left a comment

spacewander Feb 21, 2023

spacewander Feb 22, 2023




		=== TEST 21: secret.fetch_by_uri, no sub key value with the token in an env var

		@@ -26,6 +26,7 @@ local norm_path = require("pl.path").normpath
		local sub = core.string.sub
		local rfind_char = core.string.rfind_char

feat: use env var instead of plain text for vault token #8866

feat: use env var instead of plain text for vault token #8866

Conversation

shreemaan-abhishek commented Feb 15, 2023 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

shreemaan-abhishek Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreemaan-abhishek Feb 16, 2023 • edited Loading

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

soulbird commented Feb 20, 2023

shreemaan-abhishek commented Feb 20, 2023

soulbird commented Feb 20, 2023

spacewander left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreemaan-abhishek commented Feb 15, 2023 •

edited

Loading

shreemaan-abhishek Feb 15, 2023 •

edited

Loading

shreemaan-abhishek Feb 16, 2023 •

edited

Loading