feat(AclFamily): add AUTH for acl members #1732

kostasrim · 2023-08-24T10:07:19Z

Extends AUTH command to authenticate ACL users
Add tests

royjacobson · 2023-08-24T11:31:16Z

src/server/acl/user_registry.cc

  }

-  return user->second.HasPassword(password);
+  return {user->second.HasPassword(password) && user->second.IsActive(), user->first};


Suggested change

return {user->second.HasPassword(password) && user->second.IsActive(), user->first};

return {user->second.IsActive() && user->second.HasPassword(password), user->first};

Personal preference, but I don't mind the suggestion. I think the second is more likely than the first but oh well 🤷

From a security perspective I don't want us to check the password if the user is disabled. It doesn't really matter but it's a bad practice IMO

hmmmm, what could go wrong?

'Reject illegal queries as soon as you can know they're illegal' is just a good security principal, even if it doesn't have immediate bad consequences.

Yes but you don't always engineer around security in mind -- any obsession over one principle can backfire pretty fast, imagine if C++ was designed with this in mind, no UB etc, it would be monstrous (and I don't bite the arguments about Rust being a better safe alternative this is so easy to debunk). That being said for execution paths that are guaranteed to be handled I don't see any problem. Again, I like to consider suggestions and since you think that this is a good principle I happily applied it 😄

This is code that handles authentication. It HAS to be designed with security in mind.

royjacobson · 2023-08-24T11:36:51Z

src/server/conn_context.h

@@ -196,6 +196,8 @@ class ConnectionContext : public facade::ConnectionContext {
  // Reference to a FlowInfo for this connection if from a master to a replica.
  FlowInfo* replication_flow;

+  std::optional<std::string_view> authed_username;


IIUC, the underlying std::string_view lives in the user registry and can be de-allocated when that user is deleted.

There's an issue in general of what happens to connections when their user is deleted. But this especially looks like a UAF bug waiting to happen.

Nope. That's not true.

You can't change the username of a user once its created.

You can only delete, but when that happens you first evict && close already opened connections so the lifetime semantics are guaranteed -- you will never access the name of the user once its released

Ok, but please document this next to the field.

royjacobson · 2023-08-24T11:45:10Z

src/server/acl/acl_family.cc

+
+    auto [cat, add] = MaybeParseAclCategory(command);
+    if (!cat) {
+      return ErrorReply(absl::StrCat("Unrecognized paramter", command));


Suggested change

return ErrorReply(absl::StrCat("Unrecognized paramter", command));

return ErrorReply(absl::StrCat("Unrecognized parameter", command));

royjacobson · 2023-08-24T11:46:42Z

src/server/acl/acl_family.cc

+      return ErrorReply(absl::StrCat("Unrecognized paramter", command));
+    }
+
+    auto* acl_field = add ? &req.plus_acl_categories : &req.minus_acl_categories;


This is a bit silly, but do we want to check no category appears both as a minus and as a plus?

This is from the previous PR. What do you mean no caregory? You mean NONE ?

like what happens if I do ACL SETUSER +@JSON -@JSON

So I thought about this before, and for this specific case there won't be an issue because the ADD set is applied Before the REMOVE set and I thought that this redundancy is fine. However, now I am thinking about it, for the reverse case this is not true, that is -@JSON +@JSON won't do what is expected and therefore I think we should only allow a group once per SET USER command (and probably this will require some special handling for some categories, example is -@JSON +@ALL).

Another solution is to apply them one by one in order which is slightly slower (since I can apply them all at once) but I don't expect it will affect the performance and IMO I think this is a better solution since it covers the ordering and we can allow +@JSON -@JSON and vice versa.

I will address this in a separate PR since this is about the AUTH command

Wait... What operations are permitted? If the string is SSO, then any operations on the map (rehashing, shrinking, etc) will invalidate the string_view 🤨

using RegistryType = absl::flat_hash_map<std::string, User>;

PS: I wanted to put this comment below the std::optional<std::string_view> authed_username; discussion 😬

dranikpg · 2023-08-24T21:51:34Z

src/server/acl/acl_family.cc

+      return ErrorReply(absl::StrCat("Unrecognized paramter", command));
+    }
+
+    auto* acl_field = add ? &req.plus_acl_categories : &req.minus_acl_categories;


Wait... What operations are permitted? If the string is SSO, then any operations on the map (rehashing, shrinking, etc) will invalidate the string_view 🤨

using RegistryType = absl::flat_hash_map<std::string, User>;

PS: I wanted to put this comment below the std::optional<std::string_view> authed_username; discussion 😬

dranikpg · 2023-08-24T21:52:40Z

src/server/acl/user_registry.h

+  std::pair<bool, const std::string_view> AuthUser(std::string_view username,
+                                                   std::string_view password) const;


Please add a comment what the return value means (not obvious without looking at impl how to tell apart non-existing and existing but disabled user)

No needed anymore, we deep copy the username :)

kostasrim · 2023-08-25T14:51:38Z

Wait... What operations are permitted? If the string is SSO, then any operations on the map (rehashing, shrinking, etc) will invalidate the string_view 🤨

The SSO is an issue. I am not happy about this, and honestly I did not consider it -- really good catch here. You can only add users to the registry but you can't really update their usernames. But this doesn't matter because as you say, if the hash table resizes the string_view will dangle. That being said, for now I will add std::string on the connection_context. I was about to say that maybe I should introduce a small string class that doesn't have SSO but this is really unnecessary, (estimating 10k connections * 15 characters each is a few kb -- an insignificant amount of memory)

dranikpg · 2023-08-25T14:57:31Z

I was about to say that maybe I should introduce a small string class that doesn't have SSO but this is really unnecessary,

Thought about this 100500 times, there are cases where SSO is not desired and I just use unique_ptr<char[]>

If we find yet a third use-case lets implement a good wrapper for it 🤣

kostasrim added 10 commits August 22, 2023 20:44

feat(AclFamily): add acl setuser command

f6d8d1e

chore: add tests, add overloaded header, improve implementation

f4dbe93

fix: clang bug on Overloaded

d37290a

Merge branch 'main' into acl_part_4_add_acl_set_user

0cf215e

fix: revert accidental removal of a line in acl_family_test

20412b9

chore: address gh comments

9d3b767

Merge branch 'main' into acl_part_4_add_acl_set_user

cc2432d

chore: use StartsWith instead of string comparison

c0356d8

chore: replace starts_with member function with absl::StartsWith

2251f8e

feat(AclFamily): add AUTH for acl members

ff6e3f0

kostasrim requested review from romange and dranikpg August 24, 2023 10:07

kostasrim self-assigned this Aug 24, 2023

Base automatically changed from acl_part_4_add_acl_set_user to main August 24, 2023 10:24

royjacobson reviewed Aug 24, 2023

View reviewed changes

kostasrim added 3 commits August 24, 2023 14:55

Merge branch 'main' into acl_part_5_add_acl_auth

16aa85f

chore: address gh comments

d98d270

Merge branch 'main' into acl_part_5_add_acl_auth

d84ff8f

dranikpg reviewed Aug 24, 2023

View reviewed changes

chore: apply gh comments

b4a9163

kostasrim requested a review from dranikpg August 25, 2023 15:07

dranikpg previously approved these changes Aug 25, 2023

View reviewed changes

Merge branch 'main' into acl_part_5_add_acl_auth

cdfdb29

kostasrim enabled auto-merge (squash) August 25, 2023 15:09

fix broken test

dba320c

kostasrim dismissed dranikpg’s stale review via dba320c August 25, 2023 15:39

kostasrim requested a review from dranikpg August 25, 2023 15:52

dranikpg approved these changes Aug 25, 2023

View reviewed changes

kostasrim merged commit 50545fc into main Aug 25, 2023

kostasrim deleted the acl_part_5_add_acl_auth branch August 25, 2023 15:56

arukiidou mentioned this pull request Aug 31, 2023

Support ACL[API-6][API-7] #447

Closed

	return {user->second.HasPassword(password) && user->second.IsActive(), user->first};
	return {user->second.IsActive() && user->second.HasPassword(password), user->first};

	return ErrorReply(absl::StrCat("Unrecognized paramter", command));
	return ErrorReply(absl::StrCat("Unrecognized parameter", command));

		std::pair<bool, const std::string_view> AuthUser(std::string_view username,
		std::string_view password) const;

feat(AclFamily): add AUTH for acl members #1732

feat(AclFamily): add AUTH for acl members #1732

Uh oh!

Conversation

kostasrim commented Aug 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dranikpg Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dranikpg Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kostasrim commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dranikpg commented Aug 25, 2023

Uh oh!

Uh oh!

dranikpg Aug 24, 2023 •

edited

Loading

dranikpg Aug 24, 2023 •

edited

Loading

kostasrim commented Aug 25, 2023 •

edited

Loading