diff --git a/apisix/admin/plugin_metadata.lua b/apisix/admin/plugin_metadata.lua index c4b54be5568f..79accdd6a093 100644 --- a/apisix/admin/plugin_metadata.lua +++ b/apisix/admin/plugin_metadata.lua @@ -94,7 +94,10 @@ local function encrypt_conf(plugin_name, conf) inject_metadata_schema(plugin_object) local schema = plugin_object.metadata_schema - if schema['$comment'] ~= injected_mark and plugin_object.check_schema then + -- encrypt whenever the plugin has a real metadata schema; do not gate on + -- check_schema, or a plugin that validates via core.schema.check would leak + -- its metadata encrypt_fields in plaintext + if schema['$comment'] ~= injected_mark then plugin_encrypt_conf(plugin_name, conf, core.schema.TYPE_METADATA) end end diff --git a/apisix/admin/resource.lua b/apisix/admin/resource.lua index 5de0a9beb89a..1d6bd7fb5570 100644 --- a/apisix/admin/resource.lua +++ b/apisix/admin/resource.lua @@ -127,19 +127,21 @@ function _M:check_conf(id, conf, need_id, typ, allow_time, sub_path) local ok, err = self.checker(id, conf_for_check, need_id, self.schema, {secret_type = typ, sub_path = sub_path}) + if not ok then + return ok, err + end + + -- encrypt the real conf only after a successful check; encrypting + -- unvalidated input can crash on malformed structures (e.g. plugins + -- as a string), and invalid configs are never persisted anyway if self.encrypt_conf then self.encrypt_conf(id, conf) end - if not ok then - return ok, err - else - if no_id_res[self.name] then - return ok - else - return need_id and id or true - end + if no_id_res[self.name] then + return ok end + return need_id and id or true end diff --git a/t/admin/encrypt-after-validation.t b/t/admin/encrypt-after-validation.t new file mode 100644 index 000000000000..8842c9576545 --- /dev/null +++ b/t/admin/encrypt-after-validation.t @@ -0,0 +1,105 @@ +# +# 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(); +log_level("info"); + +add_block_preprocessor(sub { + my ($block) = @_; + + 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: invalid non-object plugins is rejected with 400, not a 500 +--- 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": "not-an-object", + "upstream": { + "nodes": {"127.0.0.1:1980": 1}, + "type": "roundrobin" + }, + "uri": "/hello" + }]] + ) + + ngx.status = code + ngx.say(code) + } + } +--- error_code: 400 +--- response_body +400 + + + +=== TEST 2: valid config still encrypts secret fields on write +--- yaml_config +apisix: + data_encryption: + enable: true + keyring: + - edd1c9f0985e76a2 +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local etcd = require("apisix.core.etcd") + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "encfoo", + "plugins": { + "key-auth": { + "key": "plain-secret-key" + } + } + }]] + ) + if code >= 300 then + ngx.status = code + ngx.say(body) + return + end + + -- the stored key must be ciphertext, not the plaintext we sent + local res = assert(etcd.get('/consumers/encfoo')) + local stored = res.body.node.value.plugins["key-auth"].key + assert(stored ~= "plain-secret-key", "key must be encrypted at rest") + ngx.say("encrypted") + } + } +--- response_body +encrypted