Copilot commented on code in PR #2293:
URL:
https://github.com/apache/incubator-pegasus/pull/2293#discussion_r2362654269
##########
src/zookeeper/zookeeper_session.cpp:
##########
@@ -191,69 +211,133 @@ const char *zookeeper_session::string_zoo_state(int
zoo_state)
return "invalid_state";
}
-zookeeper_session::~zookeeper_session() {}
+zookeeper_session::zookeeper_session(service_app_info info)
+ : _info(std::move(info)), _handle(nullptr)
+{
+}
+
+namespace {
+
+int decode_base64(
+ const char *content, size_t content_len, char *buf, size_t buf_len, size_t
*passwd_len)
+{
+ if (content_len == 0) {
+ return SASL_BADPARAM;
+ }
+
+ BIO *bio = BIO_new_mem_buf(content, static_cast<int>(content_len));
+ if (bio == nullptr) {
+ return SASL_FAIL;
+ }
+
+ BIO *b64 = BIO_new(BIO_f_base64());
+ if (b64 == nullptr) {
+ BIO_free(bio);
+ return SASL_FAIL;
+ }
+
+ BIO_set_flags(b64, BIO_FLAGS_BASE64_NO_NL);
+
+ bio = BIO_push(b64, bio);
+ const auto cleanup = dsn::defer([bio]() { BIO_free_all(bio); });
+
+ const int read_len = BIO_read(bio, buf, static_cast<int>(buf_len));
+ if (read_len <= 0) {
+ return SASL_FAIL;
+ }
+
+ *passwd_len = read_len;
+
+ return SASL_OK;
+}
+
+int zookeeper_password_decoder(const char *content,
+ size_t content_len,
+ void *context,
+ char *buf,
+ size_t buf_len,
+ size_t *passwd_len)
+{
+ if (dsn::utils::iequals(FLAGS_sasl_password_encryption_scheme, "base64")) {
+ return decode_base64(content, content_len, buf, buf_len, passwd_len);
+ }
+
+ return SASL_BADPARAM;
+}
-zookeeper_session::zookeeper_session(const service_app_info &node) :
_handle(nullptr)
+bool is_password_file_plaintext()
{
- _srv_node = node;
+ return dsn::utils::is_empty(FLAGS_sasl_password_encryption_scheme) ||
+ dsn::utils::iequals(FLAGS_sasl_password_encryption_scheme,
"plaintext");
+}
+
+zhandle_t *create_zookeeper_handle(watcher_fn watcher, void *context)
+{
+ zoo_set_debug_level(enum_from_string(FLAGS_zoo_log_level,
static_cast<ZooLogLevel>(0)));
Review Comment:
The function `enum_from_string` may fail if `FLAGS_zoo_log_level` contains
an invalid value, but there's no error handling. Consider adding validation or
using a default fallback value explicitly.
##########
.github/workflows/lint_and_test_cpp.yaml:
##########
@@ -353,6 +355,7 @@ jobs:
# - restore_test
# - security_test
# - throttle_test
+# - zookeeper_sasl_auth_test
Review Comment:
There are trailing spaces after 'zookeeper_sasl_auth_test' on lines 183 and
271. Remove the trailing whitespace for consistency.
```suggestion
# - zookeeper_sasl_auth_test
```
##########
.github/workflows/lint_and_test_cpp.yaml:
##########
@@ -266,7 +267,8 @@ jobs:
- security_test
# TODO(yingchun): Disable it because we find it's too flaky, we will
re-enable it after
# it has been optimized.
-# - throttle_test
+ # - throttle_test
+ - zookeeper_sasl_auth_test
Review Comment:
There are trailing spaces after 'zookeeper_sasl_auth_test' on lines 183 and
271. Remove the trailing whitespace for consistency.
```suggestion
- zookeeper_sasl_auth_test
```
##########
src/zookeeper/zookeeper_session.cpp:
##########
@@ -191,69 +211,133 @@ const char *zookeeper_session::string_zoo_state(int
zoo_state)
return "invalid_state";
}
-zookeeper_session::~zookeeper_session() {}
+zookeeper_session::zookeeper_session(service_app_info info)
+ : _info(std::move(info)), _handle(nullptr)
+{
+}
+
+namespace {
+
+int decode_base64(
+ const char *content, size_t content_len, char *buf, size_t buf_len, size_t
*passwd_len)
+{
+ if (content_len == 0) {
+ return SASL_BADPARAM;
+ }
+
+ BIO *bio = BIO_new_mem_buf(content, static_cast<int>(content_len));
+ if (bio == nullptr) {
+ return SASL_FAIL;
+ }
+
+ BIO *b64 = BIO_new(BIO_f_base64());
+ if (b64 == nullptr) {
+ BIO_free(bio);
+ return SASL_FAIL;
+ }
+
+ BIO_set_flags(b64, BIO_FLAGS_BASE64_NO_NL);
+
+ bio = BIO_push(b64, bio);
+ const auto cleanup = dsn::defer([bio]() { BIO_free_all(bio); });
+
+ const int read_len = BIO_read(bio, buf, static_cast<int>(buf_len));
+ if (read_len <= 0) {
+ return SASL_FAIL;
+ }
+
+ *passwd_len = read_len;
+
+ return SASL_OK;
+}
+
+int zookeeper_password_decoder(const char *content,
+ size_t content_len,
+ void *context,
+ char *buf,
+ size_t buf_len,
+ size_t *passwd_len)
+{
+ if (dsn::utils::iequals(FLAGS_sasl_password_encryption_scheme, "base64")) {
+ return decode_base64(content, content_len, buf, buf_len, passwd_len);
+ }
+
+ return SASL_BADPARAM;
+}
-zookeeper_session::zookeeper_session(const service_app_info &node) :
_handle(nullptr)
+bool is_password_file_plaintext()
{
- _srv_node = node;
+ return dsn::utils::is_empty(FLAGS_sasl_password_encryption_scheme) ||
+ dsn::utils::iequals(FLAGS_sasl_password_encryption_scheme,
"plaintext");
+}
+
+zhandle_t *create_zookeeper_handle(watcher_fn watcher, void *context)
+{
+ zoo_set_debug_level(enum_from_string(FLAGS_zoo_log_level,
static_cast<ZooLogLevel>(0)));
+
+ // SASL auth is enabled iff FLAGS_sasl_mechanisms_type is non-empty.
+ if (dsn::utils::is_empty(FLAGS_sasl_mechanisms_type)) {
+ return zookeeper_init(FLAGS_hosts_list, watcher, FLAGS_timeout_ms,
nullptr, context, 0);
+ }
+
+ const int err = sasl_client_init(nullptr);
+ CHECK_EQ_MSG(err,
+ SASL_OK,
+ "Unable to initialize SASL library {}",
+ sasl_errstring(err, nullptr, nullptr));
+
+ CHECK(!dsn::utils::is_empty(FLAGS_sasl_password_file),
+ "sasl_password_file must be specified once SASL auth is enabled");
Review Comment:
The error message uses 'once' which is ambiguous. Consider changing to 'when
SASL auth is enabled' for clarity.
```suggestion
"sasl_password_file must be specified when SASL auth is enabled");
```
##########
.github/workflows/lint_and_test_cpp.yaml:
##########
@@ -172,14 +172,15 @@ jobs:
- dsn.zookeeper.tests
# TODO(yingchun): Disable it because we find it's too flaky, we will
re-enable it after
# it has been optimized.
-# - partition_split_test
+ # - partition_split_test
- pegasus_geo_test
- pegasus_rproxy_test
- pegasus_unit_test
- recovery_test
- restore_test
- security_test
- throttle_test
+ - zookeeper_sasl_auth_test
Review Comment:
There are trailing spaces after 'zookeeper_sasl_auth_test' on lines 183 and
271. Remove the trailing whitespace for consistency.
```suggestion
- zookeeper_sasl_auth_test
```
##########
src/zookeeper/test/sasl_auth/zookeeper_session_sasl_auth_test.cpp:
##########
@@ -0,0 +1,126 @@
+// 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 <zookeeper/zookeeper.h>
+#include <cstdio>
+#include <string>
+
+#include "gtest/gtest.h"
+#include "utils/flags.h"
+#include "zookeeper/test/zookeeper_session_test_base.h"
+#include "zookeeper_session_test.h"
+
+DSN_DECLARE_string(sasl_mechanisms_type);
+DSN_DECLARE_string(sasl_user_name);
+DSN_DECLARE_string(sasl_password_file);
+DSN_DECLARE_string(sasl_password_encryption_scheme);
+
+namespace dsn::dist {
+
+namespace {
+
+// The correct password plaintext and its encrypted strings.
+const std::string kPlaintextPassword("mypassword");
+const std::string kBase64Password("bXlwYXNzd29yZA==");
+
+// Write the given `password` into the file. The `password` can be either
unencrypted
+// or encrypted.
+void write_password(const std::string &password)
+{
+ FILE *f = fopen("sasl_auth.password", "w");
+ ASSERT_NE(nullptr, f);
+ ASSERT_EQ(password.size(), fwrite(password.c_str(), 1, password.size(),
f));
+ ASSERT_EQ(0, fclose(f));
+ f = nullptr;
Review Comment:
Setting `f = nullptr` after `fclose()` is unnecessary since the variable
goes out of scope immediately. The assignment can be removed.
```suggestion
```
--
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]
