feat(vault): vault lua module, integration with jwt-auth authentication plugin #5745
Conversation
There was a problem hiding this comment.
@bisakhmondal
Please don't ignore the suggestion from the maintainer, thanks!
conf/config-default.yaml
Outdated
| @@ -281,6 +281,17 @@ etcd: | |||
| # the default value is true, e.g. the certificate will be verified strictly. | |||
| #sni: # the SNI for etcd TLS requests. If missed, the host part of the URL will be used. | |||
|
|
|||
| # storage backend for sensitive data storage and retrieval | |||
| vault: | |||
There was a problem hiding this comment.
Let's comment out this section, we do not need to require vault by default.
There was a problem hiding this comment.
Err. Actually, I mean like this :
#vault:
# ...
There was a problem hiding this comment.
Ahh, I get it now 😄
There was a problem hiding this comment.
Updated. Thanks for the review
apisix/plugins/jwt-auth.lua
Outdated
| @@ -28,7 +29,7 @@ local ngx_time = ngx.time | |||
| local sub_str = string.sub | |||
| local plugin_name = "jwt-auth" | |||
| local pcall = pcall | |||
|
|
|||
| local jwt_vault_prefix = "jwt-auth/keys/" | |||
There was a problem hiding this comment.
What about changing it to consumer/<username>/jwt-auth/?
I think about it several times. Although it requires to change several places in this PR, the new format is more extendable, and easier to recognize. People will remember the username better than the key.
There was a problem hiding this comment.
Thanks for the suggestion. Addressed
conf/config-default.yaml
Outdated
| @@ -281,6 +281,17 @@ etcd: | |||
| # the default value is true, e.g. the certificate will be verified strictly. | |||
| #sni: # the SNI for etcd TLS requests. If missed, the host part of the URL will be used. | |||
|
|
|||
| # storage backend for sensitive data storage and retrieval | |||
| vault: | |||
There was a problem hiding this comment.
Err. Actually, I mean like this :
#vault:
# ...
t/plugin/jwt-auth-vault.t
Outdated
| } | ||
| } | ||
| }]], | ||
| [[{ |
There was a problem hiding this comment.
Could we remove the expected response? We don't need to check this output anymore. More other tests already do it.
|
My review is almost done, only a few things need to be addressed. 😄 |
t/plugin/jwt-auth-vault.t
Outdated
|
|
||
|
|
||
| === TEST 6: sign a HS256 jwt and access/verify /secure-endpoint | ||
| --- extra_yaml_config |
There was a problem hiding this comment.
We can set up a common yaml config in this file?
| my $vault_config = $block->extra_yaml_config // <<_EOC_; | ||
| vault: | ||
| host: "http://0.0.0.0:8200" | ||
| timeout: 10 | ||
| prefix: kv/apisix | ||
| token: root | ||
| _EOC_ | ||
|
|
||
| $block->set_value("extra_yaml_config", $vault_config); | ||
|
|
There was a problem hiding this comment.
How is it now? @spacewander
What this PR does / why we need it:
This PR integrates HashiCorp Vault with APISIX jwt-auth authentication plugin.
With this PR
Pre-submission checklist: