Merge remote-tracking branch 'origin/topic/timw/4350-redis-passwords'

* origin/topic/timw/4350-redis-passwords:
  Redis: Add support for sending AUTH commands during connection
  Redis: disconnect cleanly if INFO request fails
  Fix segfault if storage sync open_backend returns bad code
  Add ToStdString and ToStdStringView to ZeekString

CHANGES

@@ -1,3 +1,13 @@
+8.0.0-dev.341 | 2025-06-03 11:54:04 -0700
+
+  * Redis: Add support for sending AUTH commands during connection (Tim Wojtulewicz, Corelight)
+
+  * Redis: disconnect cleanly if INFO request fails (Tim Wojtulewicz, Corelight)
+
+  * Fix segfault if storage sync open_backend returns bad code (Tim Wojtulewicz, Corelight)
+
+  * Add ToStdString and ToStdStringView to ZeekString (Tim Wojtulewicz, Corelight)
+
 8.0.0-dev.335 | 2025-06-02 14:52:44 -0700
 
   * Update libunistd submodule [nomail] (Tim Wojtulewicz, Corelight)

VERSION

@@ -1 +1 @@
-8.0.0-dev.335
+8.0.0-dev.341

scripts/policy/frameworks/storage/backend/redis/main.zeek

@@ -39,6 +39,13 @@ export {
 		## Timeout for operation requests sent to the backend. Operations that
 		## exceed this time will return :zeek:see:`Storage::TIMEOUT`.
 		operation_timeout: interval &default=default_operation_timeout;
+
+		## A username to use for authentication the server is protected by an ACL.
+		username: string &optional;
+
+		## A username to use for authentication the server is protected by an ACL
+		## or by a simple password.
+		password: string &optional;
 	};
 }
 

src/Val.cc

@@ -782,11 +782,9 @@ const char* StringVal::CheckString() const { return string_val->CheckString(); }
 
 std::pair<const char*, size_t> StringVal::CheckStringWithSize() const { return string_val->CheckStringWithSize(); }
 
-string StringVal::ToStdString() const { return {(char*)string_val->Bytes(), static_cast<size_t>(string_val->Len())}; }
+string StringVal::ToStdString() const { return string_val->ToStdString(); }
 
-string_view StringVal::ToStdStringView() const {
-    return {(char*)string_val->Bytes(), static_cast<size_t>(string_val->Len())};
-}
+string_view StringVal::ToStdStringView() const { return string_val->ToStdStringView(); }
 
 StringVal* StringVal::ToUpper() {
     string_val->ToUpper();

src/ZeekString.cc

@@ -177,6 +177,10 @@ std::pair<const char*, size_t> String::CheckStringWithSize() const {
 
 const char* String::CheckString() const { return CheckStringWithSize().first; }
 
+std::string String::ToStdString() const { return {(char*)Bytes(), static_cast<size_t>(Len())}; }
+
+std::string_view String::ToStdStringView() const { return {(char*)Bytes(), static_cast<size_t>(Len())}; }
+
 char* String::Render(int format, int* len) const {
     // Maximum character expansion is as \xHH, so a factor of 4.
     char* s = new char[n * 4 + 1]; // +1 is for final '\0'

src/ZeekString.h

@@ -91,6 +91,17 @@ public:
      */
     std::pair<const char*, size_t> CheckStringWithSize() const;
 
+    /**
+     * Returns the string data as a std::string. This makes a copy of the
+     * string data.
+     */
+    std::string ToStdString() const;
+
+    /**
+     * Returns the string data as a std::string_view.
+     */
+    std::string_view ToStdStringView() const;
+
     enum render_style {
         ESC_NONE = 0,
         ESC_ESC = (1 << 1),  // '\' -> "\\"

src/storage/backend/redis/Redis.cc

@@ -124,18 +124,31 @@ void redisGeneric(redisAsyncContext* ctx, void* reply, void* privdata) {
 }
 
 /**
- * Callback handler for ZADD commands.
+ * Callback handler for INFO commands.
  *
  * @param ctx The async context that called this callback.
  * @param reply The reply from the server for the command.
  * @param privdata A pointer to private data passed in the command.
  */
 void redisINFO(redisAsyncContext* ctx, void* reply, void* privdata) {
-    auto t = Tracer("generic");
+    auto t = Tracer("info");
     auto backend = static_cast<zeek::storage::backend::redis::Redis*>(ctx->data);
     backend->HandleInfoResult(static_cast<redisReply*>(reply));
 }
 
+/**
+ * Callback handler for AUTH commands.
+ *
+ * @param ctx The async context that called this callback.
+ * @param reply The reply from the server for the command.
+ * @param privdata A pointer to private data passed in the command.
+ */
+void redisAUTH(redisAsyncContext* ctx, void* reply, void* privdata) {
+    auto t = Tracer("auth");
+    auto backend = static_cast<zeek::storage::backend::redis::Redis*>(ctx->data);
+    backend->HandleAuthResult(static_cast<redisReply*>(reply));
+}
+
 // Because we called redisPollAttach in DoOpen(), privdata here is a
 // redisPollEvents object. We can go through that object to get the context's
 // data, which contains the backend. Because we overrode these callbacks in
@@ -246,6 +259,10 @@ OperationResult Redis::DoOpen(OpenResultCallback* cb, RecordValPtr options) {
     StringValPtr host = backend_options->GetField<StringVal>("server_host");
     if ( host ) {
         PortValPtr port = backend_options->GetField<PortVal>("server_port");
+        if ( ! port )
+            return {ReturnCode::CONNECTION_FAILED,
+                    "server_port must be set if server_host is set in Redis options record"};
+
         server_addr = util::fmt("%s:%d", host->ToStdString().c_str(), port->Port());
         REDIS_OPTIONS_SET_TCP(&opt, host->ToStdStringView().data(), port->Port());
     }
@@ -267,6 +284,14 @@ OperationResult Redis::DoOpen(OpenResultCallback* cb, RecordValPtr options) {
     struct timeval timeout = util::double_to_timeval(connect_timeout_opt);
     opt.connect_timeout = &timeout;
 
+    auto username_field = backend_options->GetField<StringVal>("username");
+    if ( username_field )
+        username = username_field->ToStdString();
+
+    auto password_field = backend_options->GetField<StringVal>("password");
+    if ( password_field )
+        password = password_field->ToStdString();
+
     // The connection request below should be operation #1.
     active_ops = 1;
 
@@ -626,32 +651,86 @@ void Redis::HandleInfoResult(redisReply* reply) {
         }
     }
 
-    if ( ! connected && res.err_str.empty() )
-        res.err_str = "INFO command did not return server version";
+    if ( ! connected ) {
+        if ( res.err_str.empty() )
+            res.err_str = "INFO command did not return server version";
+
+        disconnect_reason = res.err_str;
+        redisAsyncDisconnect(async_ctx);
+    }
 
     freeReplyObject(reply);
     CompleteCallback(open_cb, res);
 }
 
+void Redis::HandleAuthResult(redisReply* reply) {
+    DBG_LOG(DBG_STORAGE, "Redis backend: auth event");
+    --active_ops;
+
+    if ( strncmp(reply->str, "OK", 2) != 0 ) {
+        std::string reason = util::fmt("AUTH command failed to authenticate: %s", reply->str);
+        CompleteCallback(open_cb, {ReturnCode::CONNECTION_FAILED, reason});
+        freeReplyObject(reply);
+
+        disconnect_reason = reason;
+        redisAsyncDisconnect(async_ctx);
+        return;
+    }
+
+    freeReplyObject(reply);
+    SendInfoRequest();
+}
+
+void Redis::SendInfoRequest() {
+    DBG_LOG(DBG_STORAGE, "Redis backend: Sending INFO request");
+
+    // Request the INFO block from the server that should contain the version information.
+    int status = redisAsyncCommand(async_ctx, redisINFO, NULL, "INFO server");
+
+    if ( status == REDIS_ERR ) {
+        // TODO: do something with the error?
+        DBG_LOG(DBG_STORAGE, "INFO command failed: %s err=%d", async_ctx->errstr, async_ctx->err);
+        CompleteCallback(open_cb, {ReturnCode::OPERATION_FAILED,
+                                   util::fmt("INFO command failed to retrieve server info: %s", async_ctx->errstr)});
+        return;
+    }
+
+    ++active_ops;
+}
+
 void Redis::OnConnect(int status) {
     DBG_LOG(DBG_STORAGE, "Redis backend: connection event, status=%d", status);
     --active_ops;
 
     connected = false;
     if ( status == REDIS_OK ) {
-        // Request the INFO block from the server that should contain the version information.
-        status = redisAsyncCommand(async_ctx, redisINFO, NULL, "INFO server");
+        bool made_auth_request = false;
 
+        // If the username and/or password are set, send an AUTH command. Fail to
+        // connect if the authentication fails. We want to pause here while opening.
+        if ( ! username.empty() && ! password.empty() ) {
+            status = redisAsyncCommand(async_ctx, redisAUTH, NULL, "AUTH %s %s", username.c_str(), password.c_str());
+            made_auth_request = true;
+        }
+        else if ( ! password.empty() ) {
+            status = redisAsyncCommand(async_ctx, redisAUTH, NULL, "AUTH %s", password.c_str());
+            made_auth_request = true;
+        }
+
+        // This will be reset by the sync calls above if we make them and they fail. The
+        // condition will always be false if we don't make any auth call.
         if ( status == REDIS_ERR ) {
-            // TODO: do something with the error?
-            DBG_LOG(DBG_STORAGE, "INFO command failed: %s err=%d", async_ctx->errstr, async_ctx->err);
-            CompleteCallback(open_cb,
-                             {ReturnCode::OPERATION_FAILED,
-                              util::fmt("INFO command failed to retrieve server info: %s", async_ctx->errstr)});
+            DBG_LOG(DBG_STORAGE, "AUTH command failed: %s err=%d", async_ctx->errstr, async_ctx->err);
+            CompleteCallback(open_cb, {ReturnCode::OPERATION_FAILED,
+                                       util::fmt("AUTH command failed to queue: %s", async_ctx->errstr)});
             return;
         }
 
-        ++active_ops;
+        if ( made_auth_request )
+            ++active_ops;
+        else
+            // This will be called from the handler of the auth event if that succeeds.
+            SendInfoRequest();
     }
     else {
         DBG_LOG(DBG_STORAGE, "Redis backend: connection failed: %s err=%d", async_ctx->errstr, async_ctx->err);
@@ -673,9 +752,12 @@ void Redis::OnDisconnect(int status) {
     }
     else {
         --active_ops;
+        std::string msg =
+            util::fmt("Client disconnected%s%s", disconnect_reason.empty() ? "" : ": ", disconnect_reason.c_str());
+        EnqueueBackendLost(msg);
 
-        EnqueueBackendLost("Client disconnected");
-        CompleteCallback(close_cb, {ReturnCode::SUCCESS});
+        if ( close_cb )
+            CompleteCallback(close_cb, {ReturnCode::SUCCESS});
     }
 
     redisAsyncFree(async_ctx);

src/storage/backend/redis/Redis.h

@@ -42,6 +42,7 @@ public:
     void HandleEraseResult(redisReply* reply, ResultCallback* callback);
     void HandleGeneric(redisReply* reply);
     void HandleInfoResult(redisReply* reply);
+    void HandleAuthResult(redisReply* reply);
 
     /**
      * Returns whether the backend is opened.
@@ -63,6 +64,8 @@ private:
     OperationResult ParseReplyError(std::string_view op_str, std::string_view reply_err_str) const;
     OperationResult CheckServerVersion();
 
+    void SendInfoRequest();
+
     redisAsyncContext* async_ctx = nullptr;
 
     // When running in sync mode, this is used to keep a queue of replies as
@@ -70,12 +73,15 @@ private:
     // poll.
     std::deque<redisReply*> reply_queue;
 
-    OpenResultCallback* open_cb;
-    ResultCallback* close_cb;
+    OpenResultCallback* open_cb = nullptr;
+    ResultCallback* close_cb = nullptr;
     std::mutex expire_mutex;
 
     std::string server_addr;
     std::string key_prefix;
+    std::string disconnect_reason;
+    std::string username;
+    std::string password;
 
     std::atomic<bool> connected = false;
     std::atomic<bool> expire_running = false;

src/storage/storage-sync.bif

@@ -7,6 +7,13 @@
 
 using namespace zeek;
 using namespace zeek::storage;
+
+static void wait_for_result(OperationResult& op_result, BackendPtr& backend, ResultCallback* cb) {
+	if ( op_result.code == ReturnCode::IN_PROGRESS && ! backend->SupportsSync() ) {
+		backend->Poll();
+		op_result = cb->Result();
+	}
+}
 %%}
 
 module Storage::Sync;
@@ -31,12 +38,8 @@ function Storage::Sync::__open_backend%(btype: Storage::Backend, options: any, k
 	auto options_val = IntrusivePtr<RecordVal>{NewRef{}, options->AsRecordVal()};
 	auto op_result = storage_mgr->OpenBackend(b.value(), cb, options_val, kt, vt);
 
-	// If the backend only supports async, block until it's ready and then pull the result out of
-	// the callback.
-	if ( ! b.value()->SupportsSync() ) {
-		b.value()->Poll();
-		op_result = cb->Result();
-	}
+	// Potentially wait for a result if the backend only supports async.
+	wait_for_result(op_result, b.value(), cb);
 
 	delete cb;
 
@@ -54,12 +57,8 @@ function Storage::Sync::__close_backend%(backend: opaque of Storage::BackendHand
 		auto cb = new ResultCallback();
 		op_result = storage_mgr->CloseBackend((*b)->backend, cb);
 
-		// If the backend only supports async, block until it's ready and then pull the result out of
-		// the callback.
-		if ( ! (*b)->backend->SupportsSync() ) {
-		   (*b)->backend->Poll();
-		   op_result = cb->Result();
-		}
+		// Potentially wait for a result if the backend only supports async.
+		wait_for_result(op_result, (*b)->backend, cb);
 
 		delete cb;
 	}
@@ -84,12 +83,8 @@ function Storage::Sync::__put%(backend: opaque of Storage::BackendHandle, key: a
 		auto val_v = IntrusivePtr<Val>{NewRef{}, value};
 		op_result = (*b)->backend->Put(cb, key_v, val_v, overwrite, expire_time);
 
-		// If the backend only supports async, block until it's ready and then pull the result out of
-		// the callback.
-		if ( ! (*b)->backend->SupportsSync() ) {
-			(*b)->backend->Poll();
-			op_result = cb->Result();
-		}
+		// Potentially wait for a result if the backend only supports async.
+		wait_for_result(op_result, (*b)->backend, cb);
 
 		delete cb;
 	}
@@ -109,12 +104,8 @@ function Storage::Sync::__get%(backend: opaque of Storage::BackendHandle, key: a
 		auto key_v = IntrusivePtr<Val>{NewRef{}, key};
 		op_result = (*b)->backend->Get(cb, key_v);
 
-		// If the backend only supports async, block until it's ready and then pull the result out of
-		// the callback.
-		if ( ! (*b)->backend->SupportsSync() ) {
-			(*b)->backend->Poll();
-			op_result = cb->Result();
-		}
+		// Potentially wait for a result if the backend only supports async.
+		wait_for_result(op_result, (*b)->backend, cb);
 
 		delete cb;
 	}
@@ -134,12 +125,8 @@ function Storage::Sync::__erase%(backend: opaque of Storage::BackendHandle, key:
 		auto key_v = IntrusivePtr<Val>{NewRef{}, key};
 		op_result = (*b)->backend->Erase(cb, key_v);
 
-		// If the backend only supports async, block until it's ready and then pull the result out of
-		// the callback.
-		if ( ! (*b)->backend->SupportsSync() ) {
-			(*b)->backend->Poll();
-			op_result = cb->Result();
-		}
+		// Potentially wait for a result if the backend only supports async.
+		wait_for_result(op_result, (*b)->backend, cb);
 
 		delete cb;
 	}

testing/btest/Baseline/scripts.base.frameworks.storage.redis-auth/out

@@ -0,0 +1,4 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+open 1, [code=Storage::CONNECTION_FAILED, error_str=AUTH command failed to authenticate: WRONGPASS invalid username-password pair or user is disabled., value=<opaque of BackendHandleVal>]
+open 2, [code=Storage::SUCCESS, error_str=<uninitialized>, value=<opaque of BackendHandleVal>]
+close, [code=Storage::SUCCESS, error_str=<uninitialized>, value=<uninitialized>]

testing/btest/Baseline/scripts.base.frameworks.storage.redis-disconnect/out

@@ -1,4 +1,4 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
 open_result, [code=Storage::SUCCESS, error_str=<uninitialized>, value=<opaque of BackendHandleVal>]
-Storage::backend_opened, Storage::STORAGE_BACKEND_REDIS, [serializer=Storage::STORAGE_SERIALIZER_JSON, redis=[server_host=127.0.0.1, server_port=xxxx/tcp, server_unix_socket=<uninitialized>, key_prefix=testing, connect_timeout=5.0 secs, operation_timeout=5.0 secs]]
-Storage::backend_lost, Storage::STORAGE_BACKEND_REDIS, [serializer=Storage::STORAGE_SERIALIZER_JSON, redis=[server_host=127.0.0.1, server_port=xxxx/tcp, server_unix_socket=<uninitialized>, key_prefix=testing, connect_timeout=5.0 secs, operation_timeout=5.0 secs]], Server closed the connection
+Storage::backend_opened, Storage::STORAGE_BACKEND_REDIS, [serializer=Storage::STORAGE_SERIALIZER_JSON, redis=[server_host=127.0.0.1, server_port=xxxx/tcp, server_unix_socket=<uninitialized>, key_prefix=testing, connect_timeout=5.0 secs, operation_timeout=5.0 secs, acl_username=<uninitialized>, password=<uninitialized>]]
+Storage::backend_lost, Storage::STORAGE_BACKEND_REDIS, [serializer=Storage::STORAGE_SERIALIZER_JSON, redis=[server_host=127.0.0.1, server_port=xxxx/tcp, server_unix_socket=<uninitialized>, key_prefix=testing, connect_timeout=5.0 secs, operation_timeout=5.0 secs, acl_username=<uninitialized>, password=<uninitialized>]], Server closed the connection

testing/btest/Baseline/scripts.base.frameworks.storage.redis-sync/out

@@ -9,5 +9,5 @@ get result same as originally inserted, T
 put result, [code=Storage::SUCCESS, error_str=<uninitialized>, value=<uninitialized>]
 get result, [code=Storage::SUCCESS, error_str=<uninitialized>, value=value2345]
 get result same as overwritten, T
-Storage::backend_opened, Storage::STORAGE_BACKEND_REDIS, [serializer=Storage::STORAGE_SERIALIZER_JSON, redis=[server_host=127.0.0.1, server_port=xxxx/tcp, server_unix_socket=<uninitialized>, key_prefix=testing, connect_timeout=5.0 secs, operation_timeout=5.0 secs]]
-Storage::backend_lost, Storage::STORAGE_BACKEND_REDIS, [serializer=Storage::STORAGE_SERIALIZER_JSON, redis=[server_host=127.0.0.1, server_port=xxxx/tcp, server_unix_socket=<uninitialized>, key_prefix=testing, connect_timeout=5.0 secs, operation_timeout=5.0 secs]], Client disconnected
+Storage::backend_opened, Storage::STORAGE_BACKEND_REDIS, [serializer=Storage::STORAGE_SERIALIZER_JSON, redis=[server_host=127.0.0.1, server_port=xxxx/tcp, server_unix_socket=<uninitialized>, key_prefix=testing, connect_timeout=5.0 secs, operation_timeout=5.0 secs, acl_username=<uninitialized>, password=<uninitialized>]]
+Storage::backend_lost, Storage::STORAGE_BACKEND_REDIS, [serializer=Storage::STORAGE_SERIALIZER_JSON, redis=[server_host=127.0.0.1, server_port=xxxx/tcp, server_unix_socket=<uninitialized>, key_prefix=testing, connect_timeout=5.0 secs, operation_timeout=5.0 secs, acl_username=<uninitialized>, password=<uninitialized>]], Client disconnected

testing/btest/scripts/base/frameworks/storage/redis-auth.zeek

@@ -0,0 +1,40 @@
+# @TEST-DOC: Tests basic Redis storage backend functions in async mode
+
+# @TEST-REQUIRES: have-redis
+# @TEST-PORT: REDIS_PORT
+
+# @TEST-EXEC: btest-bg-run redis-server run-redis-server ${REDIS_PORT%/tcp} testpassword
+# @TEST-EXEC: zeek -b %INPUT > out
+# @TEST-EXEC: btest-bg-wait -k 0
+
+# @TEST-EXEC: btest-diff out
+
+@load base/frameworks/storage/sync
+@load policy/frameworks/storage/backend/redis
+
+event zeek_init()
+	{
+	local opts: Storage::BackendOptions;
+	opts$redis = [ $server_host="127.0.0.1", $server_port=to_port(getenv(
+	    "REDIS_PORT")), $key_prefix="testing", $password="notthepassword" ];
+
+	local key = "key1234";
+	local value = "value5678";
+
+	# This should fail because the password doesn't match.
+	local res = Storage::Sync::open_backend(Storage::STORAGE_BACKEND_REDIS, opts, string, string);
+	print "open 1", res;
+	if ( res$code == Storage::SUCCESS )
+		return;
+
+	opts$redis$password = "testpassword";
+	res = Storage::Sync::open_backend(Storage::STORAGE_BACKEND_REDIS, opts, string, string);
+	print "open 2", res;
+
+	if ( res$code != Storage::SUCCESS )
+		return;
+
+	local backend = res$value;
+	res = Storage::Sync::close_backend(backend);
+	print "close", res;
+	}

testing/scripts/run-redis-server

@@ -5,17 +5,28 @@ if ! redis-server --version; then
     exit 1
 fi
 
-if [ $# -ne 1 ]; then
-    echo "Usage $0 <listen_port>" >2
+if [ $# -lt 1 ]; then
+    echo "Usage $0 <listen_port> (<password>)" >2
     exit 1
 fi
 
 listen_port=$1
 
-exec redis-server \
-    --bind 127.0.0.1 \
-    --port ${listen_port} \
-    --loglevel verbose \
-    --logfile redis.log \
-    --pidfile redis.pid \
-    --databases 1
+if [ $# -eq 1 ]; then
+    exec redis-server \
+        --bind 127.0.0.1 \
+        --port ${listen_port} \
+        --loglevel verbose \
+        --logfile redis.log \
+        --pidfile redis.pid \
+        --databases 1
+elif [ $# -eq 2 ]; then
+    exec redis-server \
+        --bind 127.0.0.1 \
+        --port ${listen_port} \
+        --loglevel verbose \
+        --logfile redis.log \
+        --pidfile redis.pid \
+        --databases 1 \
+        --requirepass $2
+fi