feat(vault): Added an option to use {vault://} in specific fields of plugins #14775
Conversation
…t and hash_secret fields
|
|
|
Will it be possible to access certificates? @lordgreg |
|
Hi @mschonmeier , can you give me an example of configuration or what your use case with certificates is. |
|
I've fix the linting error being thrown by CICD. The other 2 errors arent being thrown because of my code changes. |
kong/plugins/oauth2/daos.lua
Outdated
| { 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 | ||
| { hash_secret = { type = "boolean", required = true, default = false, referenceable = true }, }, |
There was a problem hiding this comment.
This is a Boolean type. Don't need to set this field as a referenceable.
There was a problem hiding this comment.
Why do these fields need to be set as a referenceable field?
| type = "array", | ||
| default = {}, | ||
| required = true, | ||
| referenceable = true, |
There was a problem hiding this comment.
Pull request overview
This PR adds vault reference support ({vault://}) to six authentication and transformation plugins, enabling secure credential and configuration management through Kong's vault system. The implementation marks sensitive fields as referenceable = true in plugin schemas/DAOs and includes comprehensive test coverage for vault integration.
- Added vault reference support to authentication plugins (jwt, basic-auth, hmac-auth, oauth2) for credential fields
- Added vault reference support to transformation plugins (request-transformer, response-transformer) for configuration arrays
- Included comprehensive test suites verifying vault reference resolution for each plugin
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| kong/plugins/basic-auth/daos.lua | Marked username and password fields as referenceable for vault support |
| kong/plugins/hmac-auth/daos.lua | Marked username and secret fields as referenceable for vault support |
| kong/plugins/jwt/daos.lua | Marked secret field as referenceable for vault support |
| kong/plugins/oauth2/daos.lua | Marked client_id, client_secret, and hash_secret fields as referenceable for vault support |
| kong/plugins/request-transformer/schema.lua | Marked string arrays, header arrays, and colon string arrays as referenceable for vault support |
| kong/plugins/response-transformer/schema.lua | Marked string arrays, colon string arrays, json_types, and header arrays as referenceable for vault support |
| spec/03-plugins/10-basic-auth/06-vault_spec.lua | Added comprehensive vault integration tests for basic-auth plugin |
| spec/03-plugins/16-jwt/06-vault_spec.lua | Added comprehensive vault integration tests for jwt plugin |
| spec/03-plugins/19-hmac-auth/06-vault_spec.lua | Added comprehensive vault integration tests for hmac-auth plugin |
| spec/03-plugins/25-oauth2/06-vault_spec.lua | Added comprehensive vault integration tests for oauth2 plugin |
| spec/03-plugins/36-request-transformer/06-vault_spec.lua | Added comprehensive vault integration tests for request-transformer plugin |
| spec/03-plugins/15-response-transformer/06-vault_spec.lua | Added comprehensive vault integration tests for response-transformer plugin |
| changelog/unreleased/kong/feat-add-vault-template-support-in-different-plugins.yml | Added changelog entry documenting the feature addition |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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) | ||
|
|
There was a problem hiding this comment.
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) |
| 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) | ||
|
|
There was a problem hiding this comment.
Inconsistent test structure: This test "should handle all variations of variable name" is unique to basic-auth and doesn't appear in other plugin vault spec files. While the test demonstrates environment variable name normalization, it's testing vault behavior rather than basic-auth specific functionality. 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 important behavior.
| 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) |
|
|
||
| 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.
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) |
| @@ -0,0 +1,201 @@ | |||
| local helpers = require("spec.helpers") | |||
There was a problem hiding this comment.
Inconsistent require style: This file uses parentheses in require statements (e.g., require("spec.helpers")), while all other vault spec files use the style without parentheses (e.g., require "spec.helpers"). For consistency across the test suite, this should match the style used in the other vault spec files.
| @@ -0,0 +1,201 @@ | |||
| local helpers = require("spec.helpers") | |||
| local conf_loader = require("kong.conf_loader") | |||
There was a problem hiding this comment.
Inconsistent require style: This file uses parentheses in require statements (e.g., require("kong.conf_loader")), while all other vault spec files use the style without parentheses. For consistency across the test suite, this should match the style used in the other vault spec files.
| vaults = "bundled", | ||
| })) | ||
|
|
||
| local kong_global = require("kong.global") |
There was a problem hiding this comment.
Inconsistent require style: This file uses parentheses in require statements (e.g., require("kong.global")), while all other vault spec files use the style without parentheses. For consistency across the test suite, this should match the style used in the other vault spec files.
| "{vault://env/}", | ||
| "{vault://env}", | ||
| "{vault://}", | ||
| "{vault://env/valid_name?invalid_query=", |
There was a problem hiding this comment.
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=", |
| { username = { type = "string", required = true, unique = true }, }, | ||
| { secret = { type = "string", auto = true }, }, | ||
| { username = { type = "string", required = true, unique = true, referenceable = true }, }, | ||
| { secret = { type = "string", auto = true, referenceable = true }, }, |
There was a problem hiding this comment.
Marking the HMAC credential secret field as referenceable means this value will be resolved via kong.vault.get on the data plane. If the vault reference cannot be resolved (for example, a missing or mis-typed environment variable), resolve_reference in kong.db.schema.init replaces it with an empty string, so hmac-auth will 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.
| { secret = { type = "string", auto = true, referenceable = true }, }, | |
| { secret = { type = "string", auto = true }, }, |
| { 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 }, }, |
There was a problem hiding this comment.
Making the JWT credential secret field referenceable causes it to be dereferenced via kong.vault.get at select time, and on failure resolve_reference replaces the value with an empty string. Because the JWT plugin treats any non-nil jwt_secret.secret as a valid key and passes it directly into jwt:verify_signature, an unresolved vault reference would downgrade the shared secret to an empty string, enabling trivial forgery of JWTs for that key if the vault reference is misconfigured. Vault resolution failures for this field should be treated as fatal (e.g., deny authentication or disable the credential) rather than defaulting to an empty secret.
| { secret = { type = "string", auto = true, referenceable = true }, }, | |
| { secret = { type = "string", 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 |
There was a problem hiding this comment.
By marking oauth2_credentials.client_secret as referenceable, the plugin will resolve secrets from vault at runtime, but if the vault reference cannot be resolved resolve_reference in kong.db.schema.init substitutes an empty string. In the non-hashed branch (hash_secret = false), the OAuth2 plugin then authenticates confidential clients by simple equality client.client_secret == client_secret, so a misconfigured or missing vault secret would reduce the client secret to an empty string and allow any caller presenting an empty secret to be accepted. For this field, vault resolution errors should cause authentication to fail (or the credential/plugin to be rejected) instead of silently falling back to an empty secret.
| { 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 |
raoxiaoyan
left a comment
There was a problem hiding this comment.
For testing all plugins with the vault, please refer here and update all of the test cases.
|
Thank you for your review @raoxiaoyan . I will complete the requested changes next week. |
|
@lordgreg Could you add the vault for key-credentials? |
|
@raoxiaoyan I will add the option too, sure. Which fields you want to be referencable? |
I have asked our PM for this feature. They don't agree to add this option. |
Summary
As found in #14772 , we found out, that several plugins, which are using secrets, password or any other secure string, do not support the
{vault://env/*}templating. This PR adds the support for this feature to the next integrated plugins:Checklist
changelog/unreleased/kongorskip-changeloglabel added on PR if changelog is unnecessary. README.mdIssue reference
Fix #14772
Additional info for Kong development team
The unit tests are almost the same for the plugins mentioned above with the difference of which fields we actually test.
I encountered difficulties running the tests in a Dockerized setup (using a custom Dockerfile with all required tools and a docker-compose configuration including Postgres, as well as mounted caches for Cargo, Lua, etc., to avoid reinstalling dependencies on each run). The main issue was related to LuaJIT and running on an ARM-based Mac, which caused frequent test failures such as lj_mem_realloc: allocated memory address 0xffff9588e010 outside required range. This seems to be a LuaJIT-specific problem.
To improve the developer experience, I’d suggest adding a Dockerfile, docker-compose.yaml, and an updated DEVELOPER.md with instructions for running everything inside a Dockerized environment. Please let me know if this would be considered a useful addition — I’d be happy to open a follow-up PR for it.
Please let me know if additional changes are required to the PR.