[GitHub] [apisix] spacewander commented on a change in pull request #5745: feat(vault): vault lua module, integration with jwt-auth authentication plugin
spacewander commented on a change in pull request #5745:
URL: https://github.com/apache/apisix/pull/5745#discussion_r767475036
##
File path: t/plugin/jwt-auth-vault.t
##
@@ -39,6 +39,16 @@ _EOC_
$block->set_value("http_config", $http_config);
+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);
+
Review comment:
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@apisix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5745: feat(vault): vault lua module, integration with jwt-auth authentication plugin
spacewander commented on a change in pull request #5745:
URL: https://github.com/apache/apisix/pull/5745#discussion_r767416526
##
File path: t/plugin/jwt-auth-vault.t
##
@@ -0,0 +1,381 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+
+add_block_preprocessor(sub {
+my ($block) = @_;
+
+my $http_config = $block->http_config // <<_EOC_;
+
+server {
+listen 8777;
+
+location /secure-endpoint {
+content_by_lua_block {
+ngx.say("successfully invoked secure endpoint")
+}
+}
+}
+_EOC_
+
+$block->set_value("http_config", $http_config);
+
+if (!$block->request) {
+$block->set_value("request", "GET /t");
+}
+if (!$block->no_error_log && !$block->error_log) {
+$block->set_value("no_error_log", "[error]\n[alert]");
+}
+});
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: schema check
+--- config
+location /t {
+content_by_lua_block {
+local plugin = require("apisix.plugins.jwt-auth")
+local core = require("apisix.core")
+for _, conf in ipairs({
+{
+-- public and private key are not provided for RS256,
returns error
+key = "key-1",
+algorithm = "RS256"
+},
+{
+-- public and private key are not provided but vault
config is enabled.
+key = "key-1",
+algorithm = "RS256",
+vault = {}
+}
+}) do
+local ok, err = plugin.check_schema(conf,
core.schema.TYPE_CONSUMER)
+if not ok then
+ngx.say(err)
+else
+ngx.say("ok")
+end
+end
+}
+}
+--- response_body
+failed to validate dependent schema for "algorithm": value should match only
one schema, but matches none
+ok
+
+
+
+=== TEST 2: create a consumer with plugin and username
+--- config
+location /t {
+content_by_lua_block {
+local t = require("lib.test_admin").test
+local code, body = t('/apisix/admin/consumers',
+ngx.HTTP_PUT,
+[[{
+"username": "jack",
+"plugins": {
+"jwt-auth": {
+"key": "key-hs256",
+"algorithm": "HS256",
+"vault":{}
+}
+}
+}]]
+)
+
+if code >= 300 then
+ngx.status = code
+end
+ngx.say(body)
+}
+}
+--- response_body
+passed
+
+
+
+=== TEST 3: enable jwt auth plugin using admin api
+--- config
+location /t {
+content_by_lua_block {
+local t = require("lib.test_admin").test
+local code, body = t('/apisix/admin/routes/1',
+ngx.HTTP_PUT,
+[[{
+"plugins": {
+"jwt-auth": {}
+},
+"upstream": {
+"nodes": {
+"127.0.0.1:8777": 1
+},
+"type": "roundrobin"
+},
+"uri": "/secure-endpoint"
+}]]
+)
+
+if code >= 300 then
+ngx.status = code
+end
+ngx.say(body)
+}
+}
+--- response_body
+passed
+
+
+
+=== TEST 4: sign a jwt and access/verify /secure-endpoint, fails as no secret
entry into vault
+--- extra_yaml_config
+vault:
+ host: "http://0.0.0.0:8200;
+ timeout: 10
+ prefix: kv/apisix
+ token: root
+#END
+--- config
+location /t {
+content_by_lua_block {
+local t = require("lib.test_admin").test
+local code, err, sign = t('/apisix/plugin/jwt/sign?key=key-hs256',
+ngx.HTTP_GET
+)
+
+if code > 200 then
+
[GitHub] [apisix] spacewander commented on a change in pull request #5745: feat(vault): vault lua module, integration with jwt-auth authentication plugin
spacewander commented on a change in pull request #5745: URL: https://github.com/apache/apisix/pull/5745#discussion_r767363371 ## File path: t/plugin/jwt-auth-vault.t ## @@ -163,6 +148,13 @@ passed === TEST 4: sign a jwt and access/verify /secure-endpoint, fails as no secret entry into vault +--- yaml_config Review comment: We can share the config via extra_yaml_config See https://github.com/apache/apisix/blob/71c256be81d95d56ea38f9874699801cbbad6a2e/t/plugin/ext-plugin/http-req-call.t#L47 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@apisix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5745: feat(vault): vault lua module, integration with jwt-auth authentication plugin
spacewander commented on a change in pull request #5745:
URL: https://github.com/apache/apisix/pull/5745#discussion_r767266639
##
File path: t/plugin/jwt-auth-vault.t
##
@@ -0,0 +1,368 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+
+add_block_preprocessor(sub {
+my ($block) = @_;
+
+my $http_config = $block->http_config // <<_EOC_;
+
+server {
+listen 8777;
+
+location /secure-endpoint {
+content_by_lua_block {
+ngx.say("successfully invoked secure endpoint")
+}
+}
+}
+_EOC_
+
+$block->set_value("http_config", $http_config);
+
+if (!$block->request) {
+$block->set_value("request", "GET /t");
+}
+if (!$block->no_error_log && !$block->error_log) {
+$block->set_value("no_error_log", "[error]\n[alert]");
+}
+});
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: schema check
+--- config
+location /t {
+content_by_lua_block {
+local plugin = require("apisix.plugins.jwt-auth")
+local core = require("apisix.core")
+for _, conf in ipairs({
+{
+-- public and private key are not provided for RS256,
returns error
+key = "key-1",
+algorithm = "RS256"
+},
+{
+-- public and private key are not provided but vault
config is enabled.
+key = "key-1",
+algorithm = "RS256",
+vault = {}
+}
+}) do
+local ok, err = plugin.check_schema(conf,
core.schema.TYPE_CONSUMER)
+if not ok then
+ngx.say(err)
+else
+ngx.say("ok")
+end
+end
+}
+}
+--- response_body
+failed to validate dependent schema for "algorithm": value should match only
one schema, but matches none
+ok
+
+
+
+=== TEST 2: create a consumer with plugin and username
+--- config
+location /t {
+content_by_lua_block {
+local t = require("lib.test_admin").test
+local code, body = t('/apisix/admin/consumers',
+ngx.HTTP_PUT,
+[[{
+"username": "jack",
+"plugins": {
+"jwt-auth": {
+"key": "key-hs256",
+"algorithm": "HS256",
+"vault":{}
+}
+}
+}]],
+[[{
Review comment:
Could we remove the expected response? We don't need to check this
output anymore. More other tests already do it.
##
File path: conf/config-default.yaml
##
@@ -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:
Review comment:
Err. Actually, I mean like this :
```
#vault:
# ...
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@apisix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5745: feat(vault): vault lua module, integration with jwt-auth authentication plugin
spacewander commented on a change in pull request #5745:
URL: https://github.com/apache/apisix/pull/5745#discussion_r766469765
##
File path: t/plugin/jwt-auth-vault.t
##
@@ -0,0 +1,369 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+
+add_block_preprocessor(sub {
+my ($block) = @_;
+
+my $http_config = $block->http_config // <<_EOC_;
+
+server {
+listen 8777;
+
+location /secure-endpoint {
+content_by_lua_block {
+ngx.say("successfully invoked secure endpoint")
+}
+}
+}
+_EOC_
+
+$block->set_value("http_config", $http_config);
+
+if (!$block->request) {
+$block->set_value("request", "GET /t");
+}
+if (!$block->no_error_log && !$block->error_log) {
+$block->set_value("no_error_log", "[error]\n[alert]");
+}
+});
+
+run_tests;
+
+__DATA__
+
+=== TEST 1: schema - if public and private key are not provided for RS256
+--- config
+location /t {
+content_by_lua_block {
+local plugin = require("apisix.plugins.jwt-auth")
+local core = require("apisix.core")
+local conf = {
+key = "key-1",
+algorithm = "RS256"
+}
+
+local ok, err = plugin.check_schema(conf,
core.schema.TYPE_CONSUMER)
Review comment:
We can use table drive test for check_schema. You can take this as an
example:
https://github.com/apache/apisix/blob/c178435d7ada4eeb713d9a1688fb5f54f971abdf/t/plugin/gzip.t#L494
##
File path: docs/en/latest/plugins/jwt-auth.md
##
@@ -51,6 +55,9 @@ For more information on JWT, refer to [JWT](https://jwt.io/)
for more informatio
| algorithm | string | optional| "HS256" | ["HS256", "HS512",
"RS256"] | encryption algorithm.
|
| exp | integer | optional| 86400 | [1,...]
| token's expire time, in seconds
|
| base64_secret | boolean | optional| false |
| whether secret is base64 encoded
|
+| vault | dictionary | optional|| |
whether vault to be used for secret (secret for HS256/HS512 or public_key and
private_key for RS256) storage and retrieval. The plugin by default uses the
vault path as `kv/apisix/jwt-auth/keys/` for secret retrieval. |
Review comment:
```suggestion
| vault | object | optional|| | whether
vault to be used for secret (secret for HS256/HS512 or public_key and
private_key for RS256) storage and retrieval. The plugin by default uses the
vault path as `kv/apisix/jwt-auth/keys/` for secret retrieval. |
```
##
File path: apisix/plugins/jwt-auth.lua
##
@@ -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/"
Review comment:
What about changing it to `consumer//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.
##
File path: docs/en/latest/plugins/jwt-auth.md
##
@@ -51,6 +55,9 @@ For more information on JWT, refer to [JWT](https://jwt.io/)
for more informatio
| algorithm | string | optional| "HS256" | ["HS256", "HS512",
"RS256"] | encryption algorithm.
|
| exp | integer | optional| 86400 | [1,...]
| token's expire time, in seconds
[GitHub] [apisix] spacewander commented on a change in pull request #5745: feat(vault): vault lua module, integration with jwt-auth authentication plugin
spacewander commented on a change in pull request #5745:
URL: https://github.com/apache/apisix/pull/5745#discussion_r766274062
##
File path: apisix/plugins/jwt-auth.lua
##
@@ -76,7 +84,23 @@ local consumer_schema = {
},
},
required = {"public_key", "private_key"},
-}
+},
+{
+properties = {
+vault = {
+type = "object",
+properties = {
+path = {type = "string"},
+add_prefix = {type = "boolean"}
Review comment:
Please remove the path & add_prefix, which is too flexible, and made
future evolution harder. Let's be strict from the beginning. It's easier to
relax the rule than be more strict with the rule. If people really need this
feature, we can add it at that time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@apisix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5745: feat(vault): vault lua module, integration with jwt-auth authentication plugin
spacewander commented on a change in pull request #5745: URL: https://github.com/apache/apisix/pull/5745#discussion_r765465329 ## File path: apisix/plugins/jwt-auth.lua ## @@ -119,29 +143,74 @@ function _M.check_schema(conf, schema_type) if schema_type == core.schema.TYPE_CONSUMER then ok, err = core.schema.check(consumer_schema, conf) else -ok, err = core.schema.check(schema, conf) +return core.schema.check(schema, conf) end if not ok then return false, err end -if schema_type == core.schema.TYPE_CONSUMER then -if conf.algorithm ~= "RS256" and not conf.secret then -conf.secret = ngx_encode_base64(resty_random.bytes(32, true)) +-- in nginx init_worker_by_lua context API calls are disabled, +-- also that is a costly operation during system startup. +if ngx.get_phase() == "init_worker" then Review comment: > The full sync is done in apisix startup that is init_worker_by_lua phase. Not exactly. The full sync can happen elsewhere like the data in etcd is compacted. BTW, even the sync timer is triggered in the init_worker_by_lua, it runs in a separate "timer" phase. Currently, there is no way to distinguish a DP side check_schema out of the CP side. Even we have successfully checked the data, its benefit is limited. Maybe we can just leave a note in the doc? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@apisix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5745: feat(vault): vault lua module, integration with jwt-auth authentication plugin
spacewander commented on a change in pull request #5745:
URL: https://github.com/apache/apisix/pull/5745#discussion_r765462264
##
File path: apisix/plugins/jwt-auth.lua
##
@@ -76,7 +84,23 @@ local consumer_schema = {
},
},
required = {"public_key", "private_key"},
-}
+},
+{
+properties = {
+vault = {
+type = "object",
+properties = {
+path = {type = "string"},
+add_prefix = {type = "boolean"}
Review comment:
There are two cases people what to set a common path (or ref):
1. they already have the data under another key
2. they want to share the same data among configurations
For reason 1, since we already enforce the attributes name (secret, public
key, .etc), it is not suitable to reuse the data format. And we may add new
attribute requirements, which might conflict with the data already there.
For reason 2, it is quite strange to share the same configuration among
consumers. Maybe we can add this feature when it's needed. (More features
require more doc & test, and more bugs sometimes).
> Do you mean vault namespace? That's an enterprise feature [even I can't
test it], we can look into it in the next version. WDYT?
Sorry for misleading you. Actually, I mean the keys can have a common
prefix, like `/kv/apisix/`, which is easier to recognize and manage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@apisix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5745: feat(vault): vault lua module, integration with jwt-auth authentication plugin
spacewander commented on a change in pull request #5745: URL: https://github.com/apache/apisix/pull/5745#discussion_r765460002 ## File path: apisix/admin/consumers.lua ## @@ -102,6 +103,33 @@ function _M.get(consumer_name) end utils.fix_count(res.body, consumer_name) + +if consumer_name then +-- if data is queried for a single consumer, and there is any plugin where the vault config +-- is enabled - it fetches vault data and returns combined with etcd response. Review comment: I think we can remove it. BTW, when people store their data in protection storage, easier to access or can be written is no longer a plus for them. Restriction access is more helpful. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@apisix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #5745: feat(vault): vault lua module, integration with jwt-auth authentication plugin
spacewander commented on a change in pull request #5745:
URL: https://github.com/apache/apisix/pull/5745#discussion_r765369332
##
File path: apisix/plugins/jwt-auth.lua
##
@@ -76,7 +84,23 @@ local consumer_schema = {
},
},
required = {"public_key", "private_key"},
-}
+},
+{
+properties = {
+vault = {
+type = "object",
+properties = {
+path = {type = "string"},
+add_prefix = {type = "boolean"}
Review comment:
We should not allow users to specify path & add_prefix in the MVP, which
will make thing very complex.
First, people may make mistakes, like using the same path according to
multiple consumers.
Second, we need to enforce users to put their data under the same namespace,
so we can provide a consistent token for APISIX to read the data.
##
File path: apisix/admin/consumers.lua
##
@@ -102,6 +103,33 @@ function _M.get(consumer_name)
end
utils.fix_count(res.body, consumer_name)
+
+if consumer_name then
+-- if data is queried for a single consumer, and there is any plugin
where the vault config
+-- is enabled - it fetches vault data and returns combined with etcd
response.
Review comment:
IMHO, it is not a good idea to merge the vault data with etcd response.
Now everyone who can access Admin API could access the vault data, which is
against the reason to use vault.
##
File path: apisix/plugins/jwt-auth.lua
##
@@ -198,7 +312,11 @@ end
local function sign_jwt_with_HS(key, auth_conf, payload)
-local auth_secret = get_secret(auth_conf)
+local auth_secret, err = get_secret(auth_conf)
+if not auth_secret then
+core.log.error("failed to sign jwt, err: ", err)
+core.response.exit(500, "failed to sign jwt")
Review comment:
```suggestion
core.response.exit(503, "failed to sign jwt")
```
Use 503 for known failure.
##
File path: apisix/core/vault.lua
##
@@ -0,0 +1,116 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+local core = require("apisix.core")
+local http = require("resty.http")
+local json = require("cjson")
+
+local fetch_local_conf = require("apisix.core.config_local").local_conf
+local norm_path = require("pl.path").normpath
+
+local _M = {}
+
+local function fetch_vault_conf()
+local conf, err = fetch_local_conf()
+if not conf then
+return nil, "failed to fetch vault configuration from config yaml: "
.. err
+end
+
+if not conf.vault then
+return nil, "accessing vault data requires configuration information"
+end
+return conf.vault
+end
+
+
+local function make_request_to_vault(method, key, rel_path, data)
+local vault, err = fetch_vault_conf()
+if not vault then
+return nil, err
+end
+
+local httpc = http.new()
+-- config timeout or default to 5000 ms
+httpc:set_timeout((vault.timeout or 5)*1000)
+
+local req_addr = vault.host
+if rel_path then
+req_addr = req_addr .. norm_path("/v1/"
+.. vault.prefix .. "/" .. key)
+else
+req_addr = req_addr .. norm_path("/" .. key)
+end
+
+local res, err = httpc:request_uri(req_addr, {
+method = method,
+headers = {
+["X-Vault-Token"] = vault.token
+},
+body = core.json.encode(data or {}, true)
+})
+if not res then
+return nil, err
+end
+
+return res.body
+end
+
+-- key is the vault kv engine path, joined with config yaml vault prefix
+local function get(key, rel_path)
+core.log.info("fetching data from vault for key: ", key)
+
+local res, err = make_request_to_vault("GET", key, rel_path)
+if not res or err then
+return nil, "failed to retrtive data from vault kv engine " .. err
+end
+
+return json.decode(res)
+end
+
+_M.get = get
+
+-- key is the vault kv engine path, data is json key vaule pair
+local function set(key, data,
