acelyc111 commented on code in PR #1706:
URL:
https://github.com/apache/incubator-pegasus/pull/1706#discussion_r1417011516
##########
src/utils/env.cpp:
##########
@@ -49,6 +49,11 @@ DSN_DEFINE_string(pegasus.server,
"The encryption method to use in the filesystem. Now "
"supports AES128CTR, AES192CTR, AES256CTR and SM4CTR.");
+DSN_DEFINE_string(pegasus.server,
+ server_key,
Review Comment:
Is the existed `server_key_for_testing` used any more? How about reusing and
renaming it?
##########
src/replica/replication_app_base.h:
##########
@@ -111,6 +111,30 @@ class replica_init_info
error_code store_json(const std::string &fname);
};
+class replica_kms_info
Review Comment:
It's not suitable to place the replica_kms_info class in this file.
##########
src/runtime/security/kms_client.h:
##########
@@ -0,0 +1,56 @@
+// 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.
+
+#pragma once
+
+#include <string>
+#include <utility>
+#include <vector>
+
+#include "absl/strings/str_split.h"
+#include "utils/errors.h"
+
+namespace dsn {
+namespace security {
+class PegasusKMSClient
Review Comment:
```suggestion
class KMSClient
```
"Pegasus" is duplicate in the Pegasus project.
##########
src/replica/replication_app_base.cpp:
##########
@@ -124,6 +125,56 @@ std::string replica_init_info::to_string()
return oss.str();
}
+error_code replica_kms_info::load(const std::string &dir)
Review Comment:
There are some duplicaten code, a further refactor is possible and needed
IMO.
The only difference is the type, you can use template class/function to
implement this.
##########
src/replica/replica_stub.cpp:
##########
@@ -215,6 +227,11 @@ replica_stub::replica_stub(replica_state_subscriber
subscriber /*= nullptr*/,
_log = nullptr;
_primary_address_str[0] = '\0';
install_perf_counters();
+ if (FLAGS_encrypt_data_at_rest) {
+ // TODO: check enable_acl whether be true
Review Comment:
You can use DSN_DEFINE_validator to check these flags.
##########
src/replica/replica_stub.cpp:
##########
@@ -573,6 +590,35 @@ void replica_stub::initialize(bool clear /* = false*/)
_access_controller = std::make_unique<dsn::security::access_controller>();
}
+dsn::error_s store_kms_key(std::string data_dir,
Review Comment:
Different from Java, C++ passes a copy of value for parameters, so it's
better to use const reference in this case.
##########
src/replica/test/defaul_key_provider_test.cpp:
##########
@@ -0,0 +1,46 @@
+// 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 <string>
+
+#include "gtest/gtest.h"
+#include "replica/default_key_provider.h"
+
+using std::string;
+
+namespace dsn {
+namespace security {
+
+class DefaultKeyProviderTest : public testing::Test
+{
+protected:
+ DefaultKeyProvider key_provider;
Review Comment:
The class is only used in it's own test, it is meaningless, it should be use
in other tests to check the eek can be generated and restored when reboot
server. Right?
##########
src/runtime/security/kms_client.h:
##########
@@ -0,0 +1,56 @@
+// 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.
+
+#pragma once
+
+#include <string>
+#include <utility>
+#include <vector>
+
+#include "absl/strings/str_split.h"
+#include "utils/errors.h"
+
+namespace dsn {
+namespace security {
+class PegasusKMSClient
Review Comment:
It's necessary to add comments to class, public APIs, member variables to
describe what it is, how to use it.
##########
src/replica/replication_app_base.cpp:
##########
@@ -61,6 +61,7 @@ namespace dsn {
namespace replication {
const std::string replica_init_info::kInitInfo = ".init-info";
+const std::string replica_kms_info::kFileName = ".kms-info";
Review Comment:
"FileName" is too generic, it would be better to mention "KMS" which is more
special.
##########
src/replica/replica_stub.cpp:
##########
@@ -600,9 +646,30 @@ void replica_stub::initialize(const replication_options
&opts, bool clear /* = f
}
}
+ std::string encryption_key;
+ std::string iv;
+ std::string key_version;
+ std::string server_key;
+ // get and store eek from kms
Review Comment:
Add comments to describe what is "eek" short for.
##########
src/replica/replica_stub.cpp:
##########
@@ -573,6 +590,35 @@ void replica_stub::initialize(bool clear /* = false*/)
_access_controller = std::make_unique<dsn::security::access_controller>();
}
+dsn::error_s store_kms_key(std::string data_dir,
+ std::string encryption_key,
+ std::string iv,
+ std::string key_version)
+{
+ replica_kms_info kms_info(encryption_key, iv, key_version);
+ auto err = kms_info.store(data_dir);
+ if (dsn::ERR_OK == err) {
+ return dsn::error_s::ok();
+ } else {
+ return dsn::error_s::make(err, "Can't open replica_encrypted_key file
to write");
Review Comment:
The error message is not correct.
##########
src/replica/replica_stub.cpp:
##########
@@ -573,6 +590,35 @@ void replica_stub::initialize(bool clear /* = false*/)
_access_controller = std::make_unique<dsn::security::access_controller>();
}
+dsn::error_s store_kms_key(std::string data_dir,
+ std::string encryption_key,
+ std::string iv,
+ std::string key_version)
+{
+ replica_kms_info kms_info(encryption_key, iv, key_version);
+ auto err = kms_info.store(data_dir);
+ if (dsn::ERR_OK == err) {
+ return dsn::error_s::ok();
+ } else {
+ return dsn::error_s::make(err, "Can't open replica_encrypted_key file
to write");
+ }
+}
+
+void get_kms_key(std::string data_dir,
+ std::string *encryption_key,
+ std::string *iv,
+ std::string *key_version)
+{
+ replica_kms_info kms_info;
+ auto err = kms_info.load(data_dir);
+ *encryption_key = kms_info.encryption_key;
+ *iv = kms_info.iv;
+ *key_version = kms_info.key_version;
+ if (dsn::ERR_OK != err) {
+ CHECK(err, "Can't open replica_encrypted_key file to read");
+ }
Review Comment:
Use CHECK_OK.
##########
src/replica/replica_stub.cpp:
##########
@@ -600,9 +646,30 @@ void replica_stub::initialize(const replication_options
&opts, bool clear /* = f
}
}
+ std::string encryption_key;
+ std::string iv;
+ std::string key_version;
+ std::string server_key;
+ // get and store eek from kms
+ if (key_provider && !utils::is_empty(FLAGS_hadoop_kms_url)) {
+ get_kms_key(_options.data_dirs[0], &encryption_key, &iv, &key_version);
+ if (encryption_key.empty()) {
Review Comment:
Add comments to describe when it's empty and non-empty, and what will do in
the both cases.
##########
src/replica/replica_stub.cpp:
##########
@@ -573,6 +590,35 @@ void replica_stub::initialize(bool clear /* = false*/)
_access_controller = std::make_unique<dsn::security::access_controller>();
}
+dsn::error_s store_kms_key(std::string data_dir,
+ std::string encryption_key,
+ std::string iv,
+ std::string key_version)
+{
+ replica_kms_info kms_info(encryption_key, iv, key_version);
+ auto err = kms_info.store(data_dir);
+ if (dsn::ERR_OK == err) {
Review Comment:
Use circuit return to reduce if-else statements, which is more readable.
```
if (not ok) {
return non_ok;
}
return ok;
```
##########
src/replica/replica_stub.cpp:
##########
@@ -600,9 +646,30 @@ void replica_stub::initialize(const replication_options
&opts, bool clear /* = f
}
}
+ std::string encryption_key;
+ std::string iv;
+ std::string key_version;
+ std::string server_key;
+ // get and store eek from kms
+ if (key_provider && !utils::is_empty(FLAGS_hadoop_kms_url)) {
+ get_kms_key(_options.data_dirs[0], &encryption_key, &iv, &key_version);
Review Comment:
Check _options.data_dirs is not empty at first, or it may crash without any
logs.
##########
src/replica/replica_stub.cpp:
##########
@@ -178,6 +182,14 @@ DSN_DEFINE_int32(
10,
"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,
+ encryption_cluster_key_name,
+ "pegasus",
+ "The cluster name of encrypted server which use to get
server key from kms.");
+DSN_DEFINE_string(pegasus.server,
+ hadoop_kms_url,
+ "",
+ "Where the server encrypted key of file system can get
from.");
Review Comment:
Unify the key name to "server encrypted key", line 188 is called "server
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]
