feat: support encrypted password file during SASL authentication for ZooKeeper C client #2293

empiredan · 2025-09-10T06:40:06Z

Following configurations are newly added:

[zookeeper]
+ zoo_log_level = "ZOO_LOG_LEVEL_INFO"
+ sasl_password_encryption_scheme = "base64"

…ZooKeeper C client

Copilot

Pull Request Overview

This PR adds support for encrypted password files during SASL authentication for the ZooKeeper C client. The implementation adds a new configuration option sasl_password_encryption_scheme that supports "base64" encoding scheme, while maintaining backward compatibility with plaintext passwords.

Key changes include:

Added password decryption functionality with base64 support using OpenSSL
Refactored ZooKeeper session initialization to handle both encrypted and plaintext passwords
Added comprehensive test coverage for SASL authentication scenarios
Updated build and CI configuration to include new tests

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/zookeeper/zookeeper_session.h	Updated class interface with modern C++ practices and added copy/move protection
src/zookeeper/zookeeper_session.cpp	Added password decryption logic and refactored SASL initialization
src/zookeeper/test/zookeeper_session_test_base.h	New test base class for ZooKeeper session testing
src/zookeeper/test/zookeeper_session_test_base.cpp	Implementation of test base class with common test operations
src/zookeeper/test/zookeeper_session_test.h	Template-based test fixture for parameterized testing
src/zookeeper/test/zookeeper_session_no_auth_test.cpp	Test implementation for non-authenticated scenarios
src/zookeeper/test/sasl_auth/zookeeper_session_sasl_auth_test.cpp	SASL authentication tests including encrypted password scenarios
src/zookeeper/test/sasl_auth/run.sh	Test execution script for SASL auth tests
src/zookeeper/test/sasl_auth/main.cpp	Test application entry point
src/zookeeper/test/sasl_auth/config-test.ini	Configuration file for SASL auth tests
src/zookeeper/test/sasl_auth/CMakeLists.txt	Build configuration for SASL auth tests
src/zookeeper/test/config-test.ini	Minor whitespace cleanup in existing test config
src/zookeeper/test/CMakeLists.txt	Added subdirectory for SASL auth tests
run.sh	Added support for running SASL auth tests with ZooKeeper SASL configuration
admin_tools/start_zk.sh	Enhanced ZooKeeper startup script to support SASL authentication
.github/workflows/lint_and_test_cpp.yaml	Added new test to CI pipeline

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-19T11:55:51Z

src/zookeeper/zookeeper_session.cpp

+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)));


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.

Copilot · 2025-09-19T11:55:51Z

src/zookeeper/zookeeper_session.cpp

+                 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");


The error message uses 'once' which is ambiguous. Consider changing to 'when SASL auth is enabled' for clarity.

Suggested change

"sasl_password_file must be specified once SASL auth is enabled");

"sasl_password_file must be specified when SASL auth is enabled");

Copilot · 2025-09-19T11:55:52Z

src/zookeeper/test/sasl_auth/zookeeper_session_sasl_auth_test.cpp

+    ASSERT_NE(nullptr, f);
+    ASSERT_EQ(password.size(), fwrite(password.c_str(), 1, password.size(), f));
+    ASSERT_EQ(0, fclose(f));
+    f = nullptr;


Setting f = nullptr after fclose() is unnecessary since the variable goes out of scope immediately. The assignment can be removed.

Suggested change

f = nullptr;

Copilot · 2025-09-19T11:55:52Z

.github/workflows/lint_and_test_cpp.yaml

          - restore_test
          - security_test
          - throttle_test
+          - zookeeper_sasl_auth_test 


There are trailing spaces after 'zookeeper_sasl_auth_test' on lines 183 and 271. Remove the trailing whitespace for consistency.

Suggested change

- zookeeper_sasl_auth_test

- zookeeper_sasl_auth_test

Copilot · 2025-09-19T11:55:53Z

.github/workflows/lint_and_test_cpp.yaml

          # it has been optimized.
-#          - throttle_test
+          # - throttle_test
+          - zookeeper_sasl_auth_test 


There are trailing spaces after 'zookeeper_sasl_auth_test' on lines 183 and 271. Remove the trailing whitespace for consistency.

Suggested change

- zookeeper_sasl_auth_test

- zookeeper_sasl_auth_test

Copilot · 2025-09-19T11:55:53Z

.github/workflows/lint_and_test_cpp.yaml

 #          - restore_test
 #          - security_test
 #          - throttle_test
+#          - zookeeper_sasl_auth_test 


There are trailing spaces after 'zookeeper_sasl_auth_test' on lines 183 and 271. Remove the trailing whitespace for consistency.

Suggested change

# - zookeeper_sasl_auth_test

# - zookeeper_sasl_auth_test

github-actions bot added the cpp label Sep 10, 2025

empiredan mentioned this pull request Sep 10, 2025

Support encrypted password file during SASL authentication for ZooKeeper C client #2292

Open

3 tasks

github-actions bot added scripts build github labels Sep 12, 2025

empiredan mentioned this pull request Sep 16, 2025

build(images): install pluggable authentication modules for Cyrus SASL on Ubuntu to provide auth mechanisms required for tests #2296

Merged

empiredan added the type/config-change Added or modified configuration that should be noted on release note of new version. label Sep 18, 2025

empiredan added 12 commits September 19, 2025 16:52

feat: support encrypted password file during SASL authentication for …

929c7d9

…ZooKeeper C client

format and fix compilation

13376f5

fix clang-tidy

d02564f

fix clang-tidy and IWYU

2f720ce

add test cases for both no-auth and sasl-auth

aeedfdc

fix IWYU and enable test for sasl-auth

1e5523f

fix clang-tidy, add tests

c108318

add connect tests and refactor

83236fa

fix clang-tidy and IWYU, refactor, and add comments

c59ffa2

refactor zookeeper_session and zookeeper_session_test

912744b

format

d85e821

add comments

7605304

empiredan force-pushed the encrypted-zk-sasl-passwd branch from d654b02 to 7605304 Compare September 19, 2025 08:53

fix macOS build

db2aba5

acelyc111 requested a review from Copilot September 19, 2025 11:54

Copilot AI reviewed Sep 19, 2025

View reviewed changes

empiredan added 4 commits September 22, 2025 19:36

fix cr and add GetSubNodes test

08c5382

fix clang-tidy and IWYU

0c4798d

fix clang-tidy

7536573

add comments

59c4184

empiredan marked this pull request as ready for review September 23, 2025 08:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: support encrypted password file during SASL authentication for ZooKeeper C client #2293

feat: support encrypted password file during SASL authentication for ZooKeeper C client #2293

Uh oh!

empiredan commented Sep 10, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 19, 2025

Uh oh!

Copilot AI Sep 19, 2025

Uh oh!

Copilot AI Sep 19, 2025

Uh oh!

Copilot AI Sep 19, 2025

Uh oh!

Copilot AI Sep 19, 2025

Uh oh!

Copilot AI Sep 19, 2025

Uh oh!

Uh oh!

	"sasl_password_file must be specified once SASL auth is enabled");
	"sasl_password_file must be specified when SASL auth is enabled");

feat: support encrypted password file during SASL authentication for ZooKeeper C client #2293

Are you sure you want to change the base?

feat: support encrypted password file during SASL authentication for ZooKeeper C client #2293

Uh oh!

Conversation

empiredan commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

empiredan commented Sep 10, 2025 •

edited

Loading