-
Notifications
You must be signed in to change notification settings - Fork 5k
feat(vault): Added an option to use {vault://} in specific fields of plugins #14775
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.
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
base: master
Are you sure you want to change the base?
Changes from 20 commits
98a6475
e2064f6
5ef5005
7df0b8a
7bd8bb3
a852681
c05fb9b
6d52709
59e6de3
89a263c
f3185d8
9f0e84c
2c83103
8be60d5
33e9a90
eebdbf1
a2a110f
6bdaab0
36828d1
cd8a648
f6ae29d
b8e188f
8aa9286
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| message: Added an option to use {vault://} in specific fields of plugins | ||
| type: feature | ||
| scope: Plugin |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -24,7 +24,7 @@ return { | |||||
| { created_at = typedefs.auto_timestamp_s }, | ||||||
| { consumer = { type = "foreign", reference = "consumers", required = true, on_delete = "cascade", }, }, | ||||||
| { key = { type = "string", required = false, unique = true, auto = true }, }, | ||||||
| { secret = { type = "string", auto = true }, }, | ||||||
| { secret = { type = "string", auto = true, referenceable = true }, }, | ||||||
|
||||||
| { secret = { type = "string", auto = true, referenceable = true }, }, | |
| { secret = { type = "string", auto = true }, }, |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -31,9 +31,9 @@ local oauth2_credentials = { | |||||
| { created_at = typedefs.auto_timestamp_s }, | ||||||
| { consumer = { type = "foreign", reference = "consumers", required = true, on_delete = "cascade", }, }, | ||||||
| { name = { type = "string", required = true }, }, | ||||||
| { client_id = { type = "string", required = false, unique = true, auto = true }, }, | ||||||
| { client_secret = { type = "string", required = false, auto = true, encrypted = true }, }, -- encrypted = true is a Kong Enterprise Exclusive feature. It does nothing in Kong CE | ||||||
| { hash_secret = { type = "boolean", required = true, default = false }, }, | ||||||
| { client_id = { type = "string", required = false, unique = true, auto = true, referenceable = true }, }, | ||||||
| { client_secret = { type = "string", required = false, auto = true, encrypted = true, referenceable = true }, }, -- encrypted = true is a Kong Enterprise Exclusive feature. It does nothing in Kong CE | ||||||
|
||||||
| { client_secret = { type = "string", required = false, auto = true, encrypted = true, referenceable = true }, }, -- encrypted = true is a Kong Enterprise Exclusive feature. It does nothing in Kong CE | |
| { client_secret = { type = "string", required = false, auto = true, encrypted = true }, }, -- encrypted = true is a Kong Enterprise Exclusive feature. It does nothing in Kong CE |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a Boolean type. Don't need to set this field as a referenceable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the referenceable from hash_secret.
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do these fields need to be set as a referenceable field?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed. I thought maybe there is a use-case to pass the headers using ENVAR but looking at it at the moment, there is no actual requirement. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ local string_array = { | |
| type = "array", | ||
| default = {}, | ||
| required = true, | ||
| referenceable = true, | ||
|
||
| elements = { type = "string" }, | ||
| } | ||
|
|
||
|
|
@@ -33,6 +34,7 @@ local colon_string_array = { | |
| type = "array", | ||
| default = {}, | ||
| required = true, | ||
| referenceable = true, | ||
| elements = { type = "string", match = "^[^:]+:.*$" }, | ||
| } | ||
|
|
||
|
|
@@ -53,6 +55,7 @@ local colon_string_record = { | |
| { json_types = { description = "List of JSON type names. Specify the types of the JSON values returned when appending\nJSON properties. Each string element can be one of: boolean, number, or string.", type = "array", | ||
| default = {}, | ||
| required = true, | ||
| referenceable = true, | ||
| elements = { | ||
| type = "string", | ||
| one_of = { "boolean", "number", "string" } | ||
|
|
@@ -66,6 +69,7 @@ local colon_headers_array = { | |
| type = "array", | ||
| default = {}, | ||
| required = true, | ||
| referenceable = true, | ||
| elements = { type = "string", match = "^[^:]+:.*$", custom_validator = validate_colon_headers }, | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,201 @@ | ||||||||||||||||||||||||||||||||||||
| local helpers = require("spec.helpers") | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
| local conf_loader = require("kong.conf_loader") | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| describe("basic-auth: (vault integration)", function() | ||||||||||||||||||||||||||||||||||||
| local get | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| before_each(function() | ||||||||||||||||||||||||||||||||||||
| local conf = assert(conf_loader(nil, { | ||||||||||||||||||||||||||||||||||||
| vaults = "bundled", | ||||||||||||||||||||||||||||||||||||
| })) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| local kong_global = require("kong.global") | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
| _G.kong = kong_global.new() | ||||||||||||||||||||||||||||||||||||
| kong_global.init_pdk(kong, conf) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| get = _G.kong.vault.get | ||||||||||||||||||||||||||||||||||||
| end) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| describe("vault reference resolution", function() | ||||||||||||||||||||||||||||||||||||
| it("should handle all variations of variable name", function() | ||||||||||||||||||||||||||||||||||||
| local env_name = "MY_VAR_NAME" | ||||||||||||||||||||||||||||||||||||
| local env_value = "complex_value_789" | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| finally(function() | ||||||||||||||||||||||||||||||||||||
| helpers.unsetenv(env_name) | ||||||||||||||||||||||||||||||||||||
| end) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| helpers.setenv(env_name, env_value) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| assert.equal(env_value, get("{vault://env/MY_VAR_NAME}")) | ||||||||||||||||||||||||||||||||||||
| assert.equal(env_value, get("{vault://env/MY-VAR-NAME}")) | ||||||||||||||||||||||||||||||||||||
| assert.equal(env_value, get("{vault://env/my_var_name}")) | ||||||||||||||||||||||||||||||||||||
| assert.equal(env_value, get("{vault://env/my-var-name}")) | ||||||||||||||||||||||||||||||||||||
| assert.equal(env_value, get("{vault://env/My_Var_Name}")) | ||||||||||||||||||||||||||||||||||||
| assert.equal(env_value, get("{vault://env/My-Var-Name}")) | ||||||||||||||||||||||||||||||||||||
| end) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
Comment on lines
+20
to
+37
|
||||||||||||||||||||||||||||||||||||
| it("should handle all variations of variable name", function() | |
| local env_name = "MY_VAR_NAME" | |
| local env_value = "complex_value_789" | |
| finally(function() | |
| helpers.unsetenv(env_name) | |
| end) | |
| helpers.setenv(env_name, env_value) | |
| assert.equal(env_value, get("{vault://env/MY_VAR_NAME}")) | |
| assert.equal(env_value, get("{vault://env/MY-VAR-NAME}")) | |
| assert.equal(env_value, get("{vault://env/my_var_name}")) | |
| assert.equal(env_value, get("{vault://env/my-var-name}")) | |
| assert.equal(env_value, get("{vault://env/My_Var_Name}")) | |
| assert.equal(env_value, get("{vault://env/My-Var-Name}")) | |
| end) |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary malformed vault reference test: The malformed reference {vault://env/valid_name?invalid_query= is included in this test but not in any other plugin's vault spec file. All other plugins test the same set of malformed references without this one. Either this should be removed for consistency, or if it's testing an important case, it should be added to all other plugin vault spec files.
| "{vault://env/valid_name?invalid_query=", |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent test structure: This test "should preserve non-vault values unchanged" is unique to basic-auth and doesn't appear in other plugin vault spec files. For consistency across the test suite, either this test should be removed or added to all other plugin vault spec files if it's testing an important behavior.
| it("should preserve non-vault values unchanged", function() | |
| local regular_value = "regular_password" | |
| local res, err = get(regular_value) | |
| if res then | |
| assert.equal(regular_value, res) | |
| else | |
| assert.is_nil(err) | |
| end | |
| end) |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent test structure: This test "should work with special characters in environment variable names" is unique to basic-auth and doesn't appear in the edge cases sections of other plugin vault spec files. For consistency, either this test should be removed or the test approach should be harmonized across all plugin vault spec files.
| it("should work with special characters in environment variable names", function() | |
| local env_name = "SPECIAL_CHARS_(1337@)" | |
| local env_value = "special_value" | |
| finally(function() | |
| helpers.unsetenv(env_name) | |
| end) | |
| helpers.setenv(env_name, env_value) | |
| assert.equal(env_value, get("{vault://env/SPECIAL_CHARS_(1337@)}")) | |
| assert.equal(env_value, get("{vault://env/SPECIAL-CHARS_(1337@)}")) | |
| assert.equal(env_value, get("{vault://env/special-chars_(1337@)}")) | |
| assert.equal(env_value, get("{vault://env/special_chars_(1337@)}")) | |
| end) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking the HMAC credential
secretfield asreferenceablemeans this value will be resolved viakong.vault.geton the data plane. If the vault reference cannot be resolved (for example, a missing or mis-typed environment variable),resolve_referenceinkong.db.schema.initreplaces it with an empty string, sohmac-authwill happily verify signatures using a known empty key, allowing an attacker who guesses the username to forge valid HMAC signatures. This field should fail closed on vault resolution errors (e.g., reject the credential or authentication) instead of silently falling back to an empty secret.