Add ACL support to Valkey Search. #49

jeon1226 · 2025-02-25T01:32:47Z

No description provided.

src/acl.cc

yairgott · 2025-02-26T03:56:32Z

src/acl.cc

+          pattern_len--;
+        }
+        if (pattern_len == 1) {
+          return 1; /* match */


nit: here and in other places, return true/false

yairgott · 2025-02-26T04:02:24Z

testing/acl_test.cc

I've noticed that valkey has fuzz test: https://github.com/valkey-io/valkey/blob/unstable/src/util.c#L199

Can we add it as a unittest?

The purpose of the fuzztest in valkey is to see if there's a crash. The fuzztest we want to add here for ValkeySearch ACL has the same purpose?

Yes. If such exists, I don't see a reason not to just wrap it in a unittest.

yairgott · 2025-02-26T04:18:08Z

src/acl.cc

+    RedisModuleCtx *ctx,
+    const std::unordered_set<absl::string_view> &module_allowed_cmds,
+    const std::vector<std::string> &module_prefixes) {
+  RedisModuleString *username = RedisModule_GetCurrentUserName(ctx);


Please use vmsdk::UniquePtrRedisString to manage the lifecycle of username, instead of calling RedisModule_FreeString: https://github.com/valkey-io/valkey-search/blob/main/vmsdk/src/managed_pointers.h#L52C26-L52C46

yairgott · 2025-02-26T04:19:42Z

src/acl.cc

+    const std::unordered_set<absl::string_view> &module_allowed_cmds,
+    const std::vector<std::string> &module_prefixes) {
+  RedisModuleString *username = RedisModule_GetCurrentUserName(ctx);
+  RedisModuleCallReply *reply =


also vmsdk::UniquePtrRedisCallReply for managing the reply object.

yairgott · 2025-02-26T04:25:55Z

src/acl.cc

+  // to access ALL keys.
+  if (module_prefixes.empty() && IsPrefixAllowed("", acl_keys)) {
+    return absl::OkStatus();
+  } else {


nit: else is not required and just adds indentation to the code.

Personally, in situations like this, I like what was done. The extra indentation isn't excessive.

hmm, this may be related to code styling but note that throughout the code base this guideline is followed.

yairgott · 2025-02-26T04:29:34Z

testing/acl_test.cc

I wonder if it would be useful to port also the valkey unittests to make sure that our implementation is aligned with the engine.

yairgott · 2025-02-26T04:31:28Z

src/acl.cc

+  return AclPrefixCheck(ctx, module_allowed_cmds, module_prefixes);
+}
+
+}  // namespace valkey_search


I think it's saying that an empty line is missing.

yairgott · 2025-02-26T04:34:42Z

src/acl.cc

+  return acl_views;
+}
+
+absl::Status AclManager::AclPrefixCheck(


I wonder how expensive this function is. Maybe there is a way to cache the results and be notified on acl changes to flush the cache?

This is probably more applicable while we'll support non-vector queries. For now, I suggest that we just run sanity performance regression test to make sure there are no surprises. This perf test shouldn't take it to the extreme, decent amount of ACLs with an index using a dozen or so prefixes.

Not sure how to get notified on acl changes - is there a built in topic for acl change? If not we need to change the server code.

The ValkeySearch ACL RFC states that no security information is cached. https://github.com/valkey-io/valkey-rfc/pull/18/files#diff-ecbdf00816a3a420fd7225875f5d296283e9fada434cef7baf279184f34b9679R43 So I didn't consider to store any acl information.

I believe you could hook out the ACL update functions.

@allenss-amazon Do you mean the hook (a server event) should be added to Valkey? I cannot find the current Module API for an ACL update event.
https://valkey.io/topics/modules-api-ref/#section-server-hooks-implementation

Not sure if there is a better way but one approach is to override the ACL command and proxy it between the engine and the caller. Before returning the response back to the caller, fetch and cache the ACLs from the engine using ValkeyModule_Call.

yairgott · 2025-02-27T03:06:09Z

src/acl.h

+  Internally, we use RedisModule_Call API to get the ACL ruls from the server,
+  i.e. "ACL GETUSER alice"
+
+  The reply of RedisModule_Call API is complicated,


It looks like the main reason for AclManager is to avoid mocking the RedisModule_* APIs. In general, testing dictating the design is suboptimal. In addition, by overriding AclManager::GetAclViewFromCallReply we're missing coverage on key code areas, which will force more integration tests later.

I think that a better approach is to:

mock RedisModule_* . Note that we already have solid framework for doing so. Most of those APIs already have mocking support and adding mocks for the remaining APIs should be relatively simple.

To simplify mock usage, we can represent the expected reply as a json string.

During the mock API calls, we can use the parsing context to generate JSON path queries and validate the calls against the JSON.

Happy to further discuss this.

yairgott · 2025-02-27T03:21:51Z

src/commands/ft_create.cc

@@ -40,6 +41,8 @@ absl::Status FTCreateCmd(RedisModuleCtx *ctx, RedisModuleString **argv,
                         int argc) {
  VMSDK_ASSIGN_OR_RETURN(auto index_schema_proto,
                         ParseFTCreateArgs(ctx, argv + 1, argc - 1));
+  VMSDK_RETURN_IF_ERROR(AclManager::Instance().AclPrefixCheck(
+      ctx, kCommandCategories.at(kCreate), index_schema_proto));


Here and in all other AclPrefixCheck calls: I wonder if using ctx might lead RedisModule_Call to fail due to ACL enforced by the engine. The alternative is to use valkey_search::ctx_

allenss-amazon · 2025-02-28T23:34:01Z

src/acl.cc

+ match. It returns true when (1) the pattern matches the string, AND (2) the
+ pattern ends with wildcards.
+ */
+bool StringEndsWithWildCardMatch(const char *pattern, int pattern_len,


This code look quite brittle. Tracking the pointers and lengths should be combined into convenience functions. I'd recommend using string_view here with it's associated manipulation functions.

My biggest worry is, a hand crafted pattern matching code is doing something weird, while the original code is verified to work well. In this case I want to use the library code as much as possible. If that is not an option, I'll copy and paste the library code and modify it as small as possible.

The code here is from valkey/src/util.c. I agree that the pointer and length approach is scary, but deviating from the original code too much could be more dangerous, considering this is supposed to be a temp solution (after this this should be changed into some valkey module api with the same functionality). The safety issue (crash) can be handled by fuzz testing.

allenss-amazon · 2025-02-28T23:34:55Z

src/acl.cc

+  while (pattern_len && string_len) {
+    switch (pattern[0]) {
+      case '*':
+        while (pattern_len && pattern[1] == '*') {


This looks like a potential memory violation. How do we know if [1] is valid?

pattern_len is not zero, and in this case we believe pattern_len is maintained properly as a length of pattern

Oh, it is not a null terminated string, so you're right.

allenss-amazon · 2025-02-28T23:38:12Z

src/acl.cc

+        }
+        match = 0;
+        while (1) {
+          if (pattern[0] == '\\' && pattern_len >= 2) {


Please reorder to avoid accessing [0] before the length check.

allenss-amazon · 2025-02-28T23:42:56Z

src/acl.cc

+      } else {
+        // This should not happen. All ACL command expression starts with +, -,
+        // or all/no command alias.
+        abort();


RedisModule_Assert

Actually, the recommended way is to use absl CHECK

allenss-amazon · 2025-02-28T23:44:51Z

src/acl.cc

+  bool result = false;
+  for (const auto &acl_key : acl_keys) {
+    if (acl_key == "allkeys") {
+      result = true;


return true?

The key patterns of Valkey Acl rule is cumulative and path dependent.

"allkeys nokeys allkeys" is all key allowed, however,
"allkeys nokeys allkeys nokeys" is no key allowed

so we need to see all items in acl_keys through the end.

allenss-amazon · 2025-02-28T23:45:35Z

src/acl.cc

+      continue;
+    } else {
+      int offset = acl_key.find('~') + 1;  // either one of ~, %R~, and %RW~
+      result = result || StringEndsWithWildCardMatch(


Once you get a true, why bother continuing in the loop?

The same logic as above. Even if we got all permissions at this point, if the last acl_key is nokeys or -@all no keys are allowed.

allenss-amazon · 2025-02-28T23:49:40Z

src/acl.cc

+  return acl_views;
+}
+
+absl::Status AclManager::AclPrefixCheck(


I believe you could hook out the ACL update functions.

allenss-amazon · 2025-02-28T23:53:40Z

src/acl.cc

+  // to access ALL keys.
+  if (module_prefixes.empty() && IsPrefixAllowed("", acl_keys)) {
+    return absl::OkStatus();
+  } else {


Personally, in situations like this, I like what was done. The extra indentation isn't excessive.

Add ACL support to Valkey Search.

2f5d451

yairgott requested changes Feb 26, 2025

View reviewed changes

jeon1226 linked an issue Feb 26, 2025 that may be closed by this pull request

Implement Valkey 7.2 compatible ACL support #26

Closed

yairgott reviewed Feb 27, 2025

View reviewed changes

allenss-amazon reviewed Feb 28, 2025

View reviewed changes

yairgott assigned jeon1226 Mar 6, 2025

yairgott mentioned this pull request Mar 6, 2025

Implement Valkey 7.2 compatible ACL support #26

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ACL support to Valkey Search. #49

Add ACL support to Valkey Search. #49

jeon1226 commented Feb 25, 2025

yairgott Feb 26, 2025

yairgott Feb 26, 2025

jeon1226 Feb 28, 2025

yairgott Mar 3, 2025

yairgott Feb 26, 2025

yairgott Feb 26, 2025

yairgott Feb 26, 2025

allenss-amazon Feb 28, 2025

yairgott Mar 3, 2025

yairgott Feb 26, 2025

yairgott Feb 26, 2025

yairgott Feb 26, 2025

yairgott Feb 26, 2025

jeon1226 Feb 28, 2025

allenss-amazon Feb 28, 2025

jeon1226 Mar 3, 2025

yairgott Mar 7, 2025

yairgott Feb 27, 2025

yairgott Feb 27, 2025 •

edited

Loading

allenss-amazon Feb 28, 2025

jeon1226 Mar 6, 2025

allenss-amazon Feb 28, 2025

jeon1226 Mar 3, 2025

jeon1226 Mar 6, 2025

allenss-amazon Feb 28, 2025

allenss-amazon Feb 28, 2025

yairgott Mar 3, 2025 •

edited

Loading

allenss-amazon Feb 28, 2025

jeon1226 Mar 3, 2025

allenss-amazon Feb 28, 2025

jeon1226 Mar 3, 2025

allenss-amazon Feb 28, 2025

allenss-amazon Feb 28, 2025

Add ACL support to Valkey Search. #49

Are you sure you want to change the base?

Add ACL support to Valkey Search. #49

Conversation

jeon1226 commented Feb 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yairgott Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yairgott Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yairgott Feb 27, 2025 •

edited

Loading

yairgott Mar 3, 2025 •

edited

Loading