feat: use env var instead of plain text for vault token by shreemaan-abhishek · Pull Request #8866 · apache/apisix

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

  - 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

    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

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

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


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

+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

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

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

 # 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

 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?

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

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

Comment thread apisix/secret/vault.lua

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




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

Uh oh!

Conversation

shreemaan-abhishek commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shreemaan-abhishek Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

shreemaan-abhishek Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

soulbird commented Feb 20, 2023

Uh oh!

shreemaan-abhishek commented Feb 20, 2023

Uh oh!

soulbird commented Feb 20, 2023

Uh oh!

spacewander left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shreemaan-abhishek commented Feb 15, 2023 •

edited

Loading

shreemaan-abhishek Feb 15, 2023 •

edited

Loading

shreemaan-abhishek Feb 16, 2023 •

edited

Loading