acelyc111 commented on code in PR #1706: URL: https://github.com/apache/incubator-pegasus/pull/1706#discussion_r1462689839
########## src/test/function_test/security/config.ini: ########## @@ -0,0 +1,92 @@ +; 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. +[apps..default] +run = true +count = 1 +;network.client.RPC_CHANNEL_TCP = dsn::tools::sim_network_provider, 65536 +;network.client.RPC_CHANNEL_UDP = dsn::tools::sim_network_provider, 65536 +;network.server.0.RPC_CHANNEL_TCP = NET_HDR_DSN, dsn::tools::sim_network_provider, 65536 + +[apps.mimic] +name = mimic +type = dsn.app.mimic +arguments = +pools = THREAD_POOL_DEFAULT,THREAD_POOL_META_SERVER +run = true +count = 1 + +[core] +;tool = simulator +tool = nativerun +;toollets = tracer +;toollets = tracer, profiler, fault_injector +pause_on_start = false + +logging_start_level = LOG_LEVEL_INFO +logging_factory_name = dsn::tools::simple_logger +;logging_factory_name = dsn::tools::screen_logger + +enable_default_app_mimic = true + +[tools.simple_logger] +fast_flush = true +max_number_of_log_files_on_disk = 1000 + +[tools.simulator] +random_seed = 0 + +[network] +; how many network threads for network library(used by asio) +io_service_worker_count = 4 + +; specification for each thread pool +[threadpool..default] +worker_count = 4 + +[threadpool.THREAD_POOL_DEFAULT] +name = default +partitioned = false +worker_priority = THREAD_xPRIORITY_NORMAL +worker_count = 8 + +[task..default] +is_trace = false +is_profile = false +allow_inline = false +rpc_call_channel = RPC_CHANNEL_TCP +rpc_call_header_format = NET_HDR_DSN +rpc_timeout_milliseconds = 5000 + +[replication] +lb_interval_ms = 3000 +cluster_name = pegasus_cluster_key + +[pegasus.clusters] +onebox = 127.0.0.1:34601,127.0.0.1:34602,127.0.0.1:34603 +single_master_cluster = 127.0.0.1:34601 + +[pegasus.server] +encrypt_data_at_rest = false +hadoop_kms_url = + +[security] +enable_acl = +super_users = pegasus + +[function_test.throttle_test] +throttle_test_medium_value_kb = 20 +throttle_test_large_value_kb = 50 Review Comment: ```suggestion ``` ########## src/replica/replica_stub.cpp: ########## @@ -389,9 +428,56 @@ void replica_stub::initialize(const replication_options &opts, bool clear /* = f } } + std::string kms_path = + utils::filesystem::path_combine(_options.data_dirs[0], kms_info::kKmsInfo); + // FLAGS_data_dirs may be empty when load configuration, use CHECK_EQ_MSG instead of group + // validator Review Comment: ```suggestion // validator. ``` ########## src/replica/replica_stub.cpp: ########## @@ -389,9 +428,56 @@ void replica_stub::initialize(const replication_options &opts, bool clear /* = f } } + std::string kms_path = + utils::filesystem::path_combine(_options.data_dirs[0], kms_info::kKmsInfo); + // FLAGS_data_dirs may be empty when load configuration, use CHECK_EQ_MSG instead of group + // validator + if (!FLAGS_encrypt_data_at_rest && utils::filesystem::path_exists(kms_path)) { + CHECK_EQ_MSG(FLAGS_encrypt_data_at_rest, + true, + "The kms_info file exists at ({}), but [pegasus.server] " + "encrypt_data_at_rest is set to ({})." + "Encryption in Pegasus is irreversible after its initial activation.", + kms_path, + FLAGS_encrypt_data_at_rest); Review Comment: ```suggestion LOG_FATAL("The kms_info file exists at ({}), but [pegasus.server] " "encrypt_data_at_rest is disabled." "Encryption in Pegasus is irreversible after its initial activation.", kms_path); ``` ########## src/replica/replica_stub.cpp: ########## @@ -389,9 +428,56 @@ void replica_stub::initialize(const replication_options &opts, bool clear /* = f } } + std::string kms_path = + utils::filesystem::path_combine(_options.data_dirs[0], kms_info::kKmsInfo); + // FLAGS_data_dirs may be empty when load configuration, use CHECK_EQ_MSG instead of group + // validator + if (!FLAGS_encrypt_data_at_rest && utils::filesystem::path_exists(kms_path)) { + CHECK_EQ_MSG(FLAGS_encrypt_data_at_rest, + true, + "The kms_info file exists at ({}), but [pegasus.server] " + "encrypt_data_at_rest is set to ({})." + "Encryption in Pegasus is irreversible after its initial activation.", + kms_path, + FLAGS_encrypt_data_at_rest); + } + + std::string server_key; + dsn::replication::kms_info kms_info; + if (FLAGS_encrypt_data_at_rest && !utils::is_empty(FLAGS_hadoop_kms_url)) { + key_provider.reset(new dsn::security::KMSKeyProvider( + ::absl::StrSplit(FLAGS_hadoop_kms_url, ",", ::absl::SkipEmpty()), FLAGS_cluster_name)); + auto ec = dsn::utils::load_rjobj_from_file( + kms_path, dsn::utils::FileDataType::kNonSensitive, &kms_info); + if (ec != dsn::ERR_PATH_NOT_FOUND && ec != dsn::ERR_OK) { + CHECK_EQ_MSG(dsn::ERR_OK, ec, "Can't load kms key from kms-info file"); + } + // Upon the first launch, the encryption key should be empty. The process will then retrieve + // EEK, IV, and KV from KMS. + // After the first launch, the encryption key, obtained from the kms-info file, should not + // be empty. The process will then acquire the DEK from KMS. + if (ec == dsn::ERR_PATH_NOT_FOUND) { + LOG_WARNING("It's normal to encounter a temporary inability to open the kms-info file " + "during the first process launch. error_code = {}", + ec); + CHECK_OK(key_provider->GenerateEncryptionKey(&kms_info), + "Generate encryption key from kms failed"); + } + CHECK_OK(key_provider->DecryptEncryptionKey(kms_info, &server_key), + "Get decryption key failed from {}", + kms_path); + FLAGS_server_key = server_key.c_str(); + } + // Initialize the file system manager. _fs_manager.initialize(_options.data_dirs, _options.data_dir_tags); + if (key_provider && !utils::filesystem::path_exists(kms_path)) { + auto err = dsn::utils::dump_rjobj_to_file( + kms_info, dsn::utils::FileDataType::kNonSensitive, kms_path); + CHECK_EQ_MSG(dsn::ERR_OK, err, "Can't store kms key to kms-info file, err = {}", err); Review Comment: The error code has been printed by `CHECK_EQ_MSG` already. ```suggestion CHECK_EQ_MSG(dsn::ERR_OK, err, "Can't store kms key to kms-info file"); ``` ########## src/replica/replica_stub.cpp: ########## @@ -389,9 +428,56 @@ void replica_stub::initialize(const replication_options &opts, bool clear /* = f } } + std::string kms_path = + utils::filesystem::path_combine(_options.data_dirs[0], kms_info::kKmsInfo); + // FLAGS_data_dirs may be empty when load configuration, use CHECK_EQ_MSG instead of group + // validator + if (!FLAGS_encrypt_data_at_rest && utils::filesystem::path_exists(kms_path)) { + CHECK_EQ_MSG(FLAGS_encrypt_data_at_rest, + true, + "The kms_info file exists at ({}), but [pegasus.server] " + "encrypt_data_at_rest is set to ({})." + "Encryption in Pegasus is irreversible after its initial activation.", + kms_path, + FLAGS_encrypt_data_at_rest); + } + + std::string server_key; + dsn::replication::kms_info kms_info; + if (FLAGS_encrypt_data_at_rest && !utils::is_empty(FLAGS_hadoop_kms_url)) { + key_provider.reset(new dsn::security::KMSKeyProvider( + ::absl::StrSplit(FLAGS_hadoop_kms_url, ",", ::absl::SkipEmpty()), FLAGS_cluster_name)); + auto ec = dsn::utils::load_rjobj_from_file( + kms_path, dsn::utils::FileDataType::kNonSensitive, &kms_info); + if (ec != dsn::ERR_PATH_NOT_FOUND && ec != dsn::ERR_OK) { + CHECK_EQ_MSG(dsn::ERR_OK, ec, "Can't load kms key from kms-info file"); + } + // Upon the first launch, the encryption key should be empty. The process will then retrieve + // EEK, IV, and KV from KMS. + // After the first launch, the encryption key, obtained from the kms-info file, should not + // be empty. The process will then acquire the DEK from KMS. + if (ec == dsn::ERR_PATH_NOT_FOUND) { + LOG_WARNING("It's normal to encounter a temporary inability to open the kms-info file " + "during the first process launch. error_code = {}", + ec); Review Comment: Of course it's not found. ```suggestion "during the first process launch"); ``` ########## src/test/function_test/security/config.ini: ########## @@ -0,0 +1,92 @@ +; 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. +[apps..default] +run = true +count = 1 +;network.client.RPC_CHANNEL_TCP = dsn::tools::sim_network_provider, 65536 +;network.client.RPC_CHANNEL_UDP = dsn::tools::sim_network_provider, 65536 +;network.server.0.RPC_CHANNEL_TCP = NET_HDR_DSN, dsn::tools::sim_network_provider, 65536 Review Comment: Remove the comments. ########## src/test/function_test/security/config.ini: ########## @@ -0,0 +1,92 @@ +; 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. +[apps..default] +run = true +count = 1 +;network.client.RPC_CHANNEL_TCP = dsn::tools::sim_network_provider, 65536 +;network.client.RPC_CHANNEL_UDP = dsn::tools::sim_network_provider, 65536 +;network.server.0.RPC_CHANNEL_TCP = NET_HDR_DSN, dsn::tools::sim_network_provider, 65536 + +[apps.mimic] +name = mimic +type = dsn.app.mimic +arguments = +pools = THREAD_POOL_DEFAULT,THREAD_POOL_META_SERVER +run = true +count = 1 + +[core] +;tool = simulator +tool = nativerun +;toollets = tracer +;toollets = tracer, profiler, fault_injector +pause_on_start = false + +logging_start_level = LOG_LEVEL_INFO +logging_factory_name = dsn::tools::simple_logger +;logging_factory_name = dsn::tools::screen_logger + +enable_default_app_mimic = true + +[tools.simple_logger] +fast_flush = true +max_number_of_log_files_on_disk = 1000 + +[tools.simulator] +random_seed = 0 + +[network] +; how many network threads for network library(used by asio) +io_service_worker_count = 4 + +; specification for each thread pool +[threadpool..default] +worker_count = 4 + +[threadpool.THREAD_POOL_DEFAULT] +name = default +partitioned = false +worker_priority = THREAD_xPRIORITY_NORMAL +worker_count = 8 + +[task..default] +is_trace = false +is_profile = false +allow_inline = false +rpc_call_channel = RPC_CHANNEL_TCP +rpc_call_header_format = NET_HDR_DSN +rpc_timeout_milliseconds = 5000 + +[replication] +lb_interval_ms = 3000 +cluster_name = pegasus_cluster_key + +[pegasus.clusters] +onebox = 127.0.0.1:34601,127.0.0.1:34602,127.0.0.1:34603 +single_master_cluster = 127.0.0.1:34601 Review Comment: ```suggestion ``` ########## src/test/function_test/security/config.ini: ########## @@ -0,0 +1,92 @@ +; 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. +[apps..default] +run = true +count = 1 +;network.client.RPC_CHANNEL_TCP = dsn::tools::sim_network_provider, 65536 +;network.client.RPC_CHANNEL_UDP = dsn::tools::sim_network_provider, 65536 +;network.server.0.RPC_CHANNEL_TCP = NET_HDR_DSN, dsn::tools::sim_network_provider, 65536 + +[apps.mimic] +name = mimic +type = dsn.app.mimic +arguments = +pools = THREAD_POOL_DEFAULT,THREAD_POOL_META_SERVER +run = true +count = 1 + +[core] +;tool = simulator +tool = nativerun +;toollets = tracer +;toollets = tracer, profiler, fault_injector +pause_on_start = false + +logging_start_level = LOG_LEVEL_INFO +logging_factory_name = dsn::tools::simple_logger +;logging_factory_name = dsn::tools::screen_logger Review Comment: ```suggestion logging_factory_name = dsn::tools::screen_logger ``` ########## src/test/function_test/security/test_kms_client.cpp: ########## @@ -0,0 +1,75 @@ +// 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. + +#include <absl/strings/str_split.h> +#include <string.h> +#include <memory> +#include <string> + +#include "gtest/gtest.h" +#include "replica/kms_key_provider.h" +#include "replica/replication_app_base.h" +#include "test_util/test_util.h" +#include "utils/error_code.h" +#include "utils/errors.h" +#include "utils/flags.h" + +namespace dsn { +DSN_DECLARE_string(cluster_name); +namespace security { +DSN_DECLARE_bool(enable_acl); +} // namespace security +namespace replication { +DSN_DECLARE_string(hadoop_kms_url); +} // namespace replication +} // namespace dsn + +class KmsClientTest : public pegasus::encrypt_data_test_base Review Comment: The test only check the functionality of when FLAGS_encrypt_data_at_rest is true IMO, the KMS client will not be used if it's disabled, right? ########## src/test/function_test/security/config.ini: ########## @@ -0,0 +1,92 @@ +; 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. +[apps..default] +run = true +count = 1 +;network.client.RPC_CHANNEL_TCP = dsn::tools::sim_network_provider, 65536 +;network.client.RPC_CHANNEL_UDP = dsn::tools::sim_network_provider, 65536 +;network.server.0.RPC_CHANNEL_TCP = NET_HDR_DSN, dsn::tools::sim_network_provider, 65536 + +[apps.mimic] +name = mimic +type = dsn.app.mimic +arguments = +pools = THREAD_POOL_DEFAULT,THREAD_POOL_META_SERVER +run = true +count = 1 + +[core] +;tool = simulator +tool = nativerun +;toollets = tracer +;toollets = tracer, profiler, fault_injector +pause_on_start = false + +logging_start_level = LOG_LEVEL_INFO +logging_factory_name = dsn::tools::simple_logger +;logging_factory_name = dsn::tools::screen_logger + +enable_default_app_mimic = true + +[tools.simple_logger] +fast_flush = true +max_number_of_log_files_on_disk = 1000 + +[tools.simulator] +random_seed = 0 + +[network] +; how many network threads for network library(used by asio) +io_service_worker_count = 4 + +; specification for each thread pool +[threadpool..default] +worker_count = 4 + +[threadpool.THREAD_POOL_DEFAULT] +name = default +partitioned = false +worker_priority = THREAD_xPRIORITY_NORMAL +worker_count = 8 + +[task..default] +is_trace = false +is_profile = false +allow_inline = false +rpc_call_channel = RPC_CHANNEL_TCP +rpc_call_header_format = NET_HDR_DSN +rpc_timeout_milliseconds = 5000 + +[replication] +lb_interval_ms = 3000 Review Comment: ```suggestion ``` ########## src/test/function_test/security/config.ini: ########## @@ -0,0 +1,92 @@ +; 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. +[apps..default] Review Comment: Leave a blank line. ########## src/test/function_test/security/config.ini: ########## @@ -0,0 +1,92 @@ +; 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. +[apps..default] +run = true Review Comment: Minimize the config file, some of the fields are not necessary. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
