Samunroyu commented on code in PR #1706:
URL:
https://github.com/apache/incubator-pegasus/pull/1706#discussion_r1461505986
##########
.github/workflows/lint_and_test_cpp.yaml:
##########
@@ -235,6 +236,7 @@ jobs:
- pegasus_unit_test
- recovery_test
- restore_test
+ - security_test
Review Comment:
> Add it to test_UBSAN even if it's disabled, so there is nothing missed if
the tests enabled one day.
OK, already add it.
##########
src/replica/replica_stub.cpp:
##########
@@ -292,12 +304,60 @@ DSN_DEFINE_int32(
"if tcmalloc reserved but not-used memory exceed this percentage of
application allocated "
"memory, replica server will release the exceeding memory back to
operating system");
+DSN_DEFINE_string(
+ pegasus.server,
+ hadoop_kms_url,
+ "",
+ "Provide the comma-separated list of URLs from which to retrieve the "
+ "file system's server key. Example format:
'hostname1:1234/kms,hostname2:1234/kms'.");
+
DSN_DECLARE_bool(duplication_enabled);
DSN_DECLARE_int32(fd_beacon_interval_seconds);
DSN_DECLARE_int32(fd_check_interval_seconds);
DSN_DECLARE_int32(fd_grace_seconds);
DSN_DECLARE_int32(fd_lease_seconds);
DSN_DECLARE_int32(gc_interval_ms);
+DSN_DECLARE_string(data_dirs);
+DSN_DEFINE_group_validator(encrypt_data_at_rest_pre_check, [](std::string
&message) -> bool {
+ if (!dsn::security::FLAGS_enable_acl && FLAGS_encrypt_data_at_rest) {
+ message = fmt::format("[pegasus.server] encrypt_data_at_rest should be
enabled only if "
+ "[security] enable_acl is enabled.");
+ return false;
+ }
+ return true;
+});
+
+DSN_DEFINE_group_validator(encrypt_data_is_irreversible, [](std::string
&message) -> bool {
+// In some unit test FLAGS_data_dirs may not set.
+#ifdef MOCK_TEST
+ return true;
+#endif
+ std::string data_dirs;
+ data_dirs = FLAGS_data_dirs;
+ std::vector<std::string> dirs;
+ ::absl::StrSplit(data_dirs.c_str(), dirs, ',');
+ std::string kms_path = utils::filesystem::path_combine(dirs[0],
kms_info::kKmsInfo);
Review Comment:
> Isn't dirs possible to be empty?
yes,i use CHECK_EQ_MSG instead of group validator.
##########
src/replica/replica_stub.cpp:
##########
@@ -389,9 +440,39 @@ void replica_stub::initialize(const replication_options
&opts, bool clear /* = f
}
}
+ std::string server_key;
+ dsn::replication::replica_kms_info kms_info;
+ if (FLAGS_encrypt_data_at_rest && !utils::is_empty(FLAGS_hadoop_kms_url)) {
Review Comment:
> Deal the case of "FLAGS_encrypt_data_at_rest &&
utils::is_empty(FLAGS_hadoop_kms_url)"
In the furture TestDefaultKeyProvider of unit test.The FLAGS_hadoop_kms_url
will be empty. So we may not deal the case of "FLAGS_encrypt_data_at_rest &&
utils::is_empty(FLAGS_hadoop_kms_url)" .
##########
src/replica/replica_stub.cpp:
##########
@@ -389,9 +440,39 @@ void replica_stub::initialize(const replication_options
&opts, bool clear /* = f
}
}
+ std::string server_key;
+ dsn::replication::replica_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_encryption_cluster_key_name));
+ auto err = kms_info.load(_options.data_dirs[0]);
Review Comment:
> Add a check before this line:
>
> ```
> CHECK_TRUE(!_options.data_dirs.empty());
> ```
_options.data_dirs have been initialized and CHECK in the
replica_option.initialize(). May not CHECK twice here?
##########
src/test/function_test/security/test_kms_client.cpp:
##########
@@ -0,0 +1,83 @@
+// 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 <stdio.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_string(enable_acl);
+DSN_DECLARE_string(super_users);
+} // namespace security
+
+namespace replication {
+DSN_DECLARE_string(hadoop_kms_url);
+} // namespace replication
+} // namespace dsn
+
+class KmsClientTest : public pegasus::encrypt_data_test_base
+{
+protected:
+ KmsClientTest() : pegasus::encrypt_data_test_base()
+ {
+ dsn::FLAGS_cluster_name = "kudu_cluster_key";
+ dsn::security::FLAGS_enable_acl = "true";
+ dsn::security::FLAGS_super_users = "pegasus";
+ dsn::replication::FLAGS_hadoop_kms_url =
"kms01-throne01.sensorsdata.cn:9292/kms";
+ }
+};
+
+INSTANTIATE_TEST_CASE_P(, KmsClientTest, ::testing::Values(false, true));
+
+TEST_P(KmsClientTest, test_generate_and_decrypt_encryption_key)
+{
+ if (strlen(dsn::replication::FLAGS_hadoop_kms_url) == 0 ||
+ FLAGS_encrypt_data_at_rest == false) {
+ GTEST_SKIP()
+ << "Set kms_client_test.* configs in config-test.ini to enable
kms_client_test.";
+ }
+
+ std::unique_ptr<dsn::security::KMSKeyProvider> key_provider;
+ key_provider.reset(new dsn::security::KMSKeyProvider(
+ ::absl::StrSplit(dsn::replication::FLAGS_hadoop_kms_url, ",",
::absl::SkipEmpty()),
+ dsn::FLAGS_cluster_name));
+ dsn::replication::kms_info kms_info;
+
+ // 1. generate encryption key.
+ printf("generate encryption key.\n");
+ ASSERT_EQ(dsn::ERR_OK,
key_provider->GenerateEncryptionKey(&kms_info).code());
+
+ // 2. decrypt encryption key.
+ printf("decrypt encryption key.\n");
+ std::string server_key;
+ ASSERT_EQ(dsn::ERR_OK, key_provider->DecryptEncryptionKey(kms_info,
&server_key).code());
+ ASSERT_EQ(server_key.size(), kms_info.eek.length());
+ ASSERT_NE(server_key, kms_info.eek);
Review Comment:
> Is it necessary to check they are not equal?
I think it’s necessary. Casuse decrypted key should not be equal with
encryption key.
--
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]
