From 65fabebf40a1f1282fba6f0af1eea8bbb4c34719 Mon Sep 17 00:00:00 2001 From: kbowersox Date: Sat, 1 Feb 2025 09:23:43 -0800 Subject: [PATCH 01/10] Test First: Add Failing ACL SETUSER Parallel Test Cases --- test/Garnet.test/Resp/ACL/ParallelTests.cs | 87 ++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/test/Garnet.test/Resp/ACL/ParallelTests.cs b/test/Garnet.test/Resp/ACL/ParallelTests.cs index 129ff99027..5f48760a78 100644 --- a/test/Garnet.test/Resp/ACL/ParallelTests.cs +++ b/test/Garnet.test/Resp/ACL/ParallelTests.cs @@ -1,6 +1,8 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT license. +using System.Collections.Generic; +using System.Linq; using System.Threading.Tasks; using Garnet.server.ACL; using NUnit.Framework; @@ -68,5 +70,90 @@ public void ParallelPasswordHashTest(int degreeOfParallelism, int iterationsPerS } }); } + + /// + /// Tests that ACL SETUSER works in parallel without corrupting the user's ACL. + /// + /// Test launches multiple clients that apply two simple ACL changes to the same user many times in parallel. + /// Validates that ACL result after each execution is one of the possible valid responses. + /// + /// Race conditions are not deterministic so tests uses repeat. + /// + /// + [TestCase(128, 2048)] + [Repeat(2)] + public async Task ParallelAclSetUserTest(int degreeOfParallelism, int iterationsPerSession) + { + string activeUserWithGetCommand = $"ACL SETUSER {TestUserA} on >{DummyPassword} +get"; + string inactiveUserWithoutGetCommand = $"ACL SETUSER {TestUserA} off >{DummyPassword} -get"; + + string activeUserWithGet = $"user {TestUserA} on #{DummyPasswordHash} +get"; + string inactiveUserWithoutGet = $"user {TestUserA} off #{DummyPasswordHash} -get"; + string inactiveUserWithNoCommands = $"user {TestUserA} off #{DummyPasswordHash}"; + + var c = TestUtils.GetGarnetClientSession(); + c.Connect(); + _ = await c.ExecuteAsync(activeUserWithGetCommand.Split(" ")); + + // Run multiple sessions that stress AUTH + await Parallel.ForAsync(0, degreeOfParallelism, async (t, state) => + { + using var c = TestUtils.GetGarnetClientSession(); + c.Connect(); + + for (uint i = 0; i < iterationsPerSession; i++) + { + await Task.WhenAll( + c.ExecuteAsync(activeUserWithGetCommand.Split(" ")), + c.ExecuteAsync(inactiveUserWithoutGetCommand.Split(" "))); + + var aclListResponse = await c.ExecuteForArrayAsync("ACL", "LIST"); + + if (!aclListResponse.Contains(activeUserWithGet) && + !aclListResponse.Contains(inactiveUserWithoutGet) && + !aclListResponse.Contains(inactiveUserWithNoCommands)) + { + string corruptedAcl = aclListResponse.First(line => line.Contains(TestUserA)); + throw new AssertionException($"Invalid ACL: {corruptedAcl}"); + } + } + }); + } + + /// + /// Tests that ACL SETUSER works in parallel without fatal contention on user in ACL map. + /// + /// Test launches multiple clients that apply the same ACL change to the same user. Creates race to become the + /// the first client to add the user to the ACL. Throws after initial insert into ACL if threading issues exist. + /// + /// Race conditions are not deterministic so tests uses repeat. + /// + /// + [TestCase(128, 2048)] + [Repeat(10)] + public async Task ParallelAclSetUserAvoidsMapContentionTest(int degreeOfParallelism, int iterationsPerSession) + { + string command1 = $"ACL SETUSER {TestUserA} on >{DummyPassword}"; + + var c = TestUtils.GetGarnetClientSession(); + c.Connect(); + + await Parallel.ForAsync(0, degreeOfParallelism, async (t, state) => + { + using var c = TestUtils.GetGarnetClientSession(); + c.Connect(); + + List tasks = new(); + for (uint i = 0; i < iterationsPerSession; i++) + { + // Creates race between threads contending for first insert into ACL. Throws after first ACL insert. + tasks.Add(c.ExecuteAsync(command1.Split(" "))); + } + + await Task.WhenAll(tasks); + }); + + ClassicAssert.Pass(); + } } } \ No newline at end of file From 02b38ee4099760d75e68710cd7042bb9a8d9fe3d Mon Sep 17 00:00:00 2001 From: kbowersox Date: Sat, 1 Feb 2025 09:34:08 -0800 Subject: [PATCH 02/10] Clean up extra calls in test --- test/Garnet.test/Resp/ACL/ParallelTests.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/Garnet.test/Resp/ACL/ParallelTests.cs b/test/Garnet.test/Resp/ACL/ParallelTests.cs index 5f48760a78..10a8a9a467 100644 --- a/test/Garnet.test/Resp/ACL/ParallelTests.cs +++ b/test/Garnet.test/Resp/ACL/ParallelTests.cs @@ -135,9 +135,6 @@ public async Task ParallelAclSetUserAvoidsMapContentionTest(int degreeOfParallel { string command1 = $"ACL SETUSER {TestUserA} on >{DummyPassword}"; - var c = TestUtils.GetGarnetClientSession(); - c.Connect(); - await Parallel.ForAsync(0, degreeOfParallelism, async (t, state) => { using var c = TestUtils.GetGarnetClientSession(); From 6f330f81b5d835dc0fdf9af6ff076007e5770023 Mon Sep 17 00:00:00 2001 From: kbowersox Date: Sat, 1 Feb 2025 09:35:12 -0800 Subject: [PATCH 03/10] Improve ACL SETUSER thread safety --- libs/server/ACL/ACLParser.cs | 6 +- libs/server/ACL/AccessControlList.cs | 43 +++++++++--- .../ACL/IAccessControlListSubscriber.cs | 22 +++++++ libs/server/ACL/User.cs | 13 ++++ libs/server/Auth/GarnetACLAuthenticator.cs | 2 +- libs/server/Resp/ACLCommands.cs | 7 +- libs/server/Resp/AdminCommands.cs | 4 ++ libs/server/Resp/RespServerSession.cs | 66 ++++++++++++++----- test/Garnet.test/Resp/ACL/ParallelTests.cs | 4 +- 9 files changed, 137 insertions(+), 30 deletions(-) create mode 100644 libs/server/ACL/IAccessControlListSubscriber.cs diff --git a/libs/server/ACL/ACLParser.cs b/libs/server/ACL/ACLParser.cs index f033f3042a..9261ab10cd 100644 --- a/libs/server/ACL/ACLParser.cs +++ b/libs/server/ACL/ACLParser.cs @@ -96,7 +96,6 @@ public static User ParseACLRule(string input, AccessControlList acl = null) if (user == null) { user = new User(username); - acl.AddUser(user); } } else @@ -110,6 +109,11 @@ public static User ParseACLRule(string input, AccessControlList acl = null) ApplyACLOpToUser(ref user, tokens[i]); } + if (acl != null) + { + acl.AddOrReplaceUser(user); + } + return user; } diff --git a/libs/server/ACL/AccessControlList.cs b/libs/server/ACL/AccessControlList.cs index 0efd3002b1..e595c0c8c3 100644 --- a/libs/server/ACL/AccessControlList.cs +++ b/libs/server/ACL/AccessControlList.cs @@ -28,6 +28,11 @@ public class AccessControlList /// User _defaultUser; + /// + /// The subscribers who will receive access control list change notifications. + /// + private readonly ConcurrentDictionary _subscribers = new(); + /// /// Creates a new Access Control List from an optional ACL configuration file /// and sets the default user's password, if not provided by the configuration. @@ -73,17 +78,14 @@ public User GetDefaultUser() } /// - /// Adds the given user to the ACL. + /// Adds or replaces the given user in the ACL. /// - /// User to add to the list. - /// Thrown if a user with the given username already exists. - public void AddUser(User user) + /// User to add or replaces in the list. + public void AddOrReplaceUser(User user) { - // If a user with the given name already exists in the ACL, the new user cannot be added - if (!_users.TryAdd(user.Name, user)) - { - throw new ACLUserAlreadyExistsException(user.Name); - } + // If a user with the given name already exists replace the user, otherwise add the new user. + _users[user.Name] = user; + this.NotifySubscribers(user); } /// @@ -150,7 +152,7 @@ User CreateDefaultUser(string defaultPassword = "") // Add the user to the user list try { - AddUser(defaultUser); + AddOrReplaceUser(defaultUser); break; } catch (ACLUserAlreadyExistsException) @@ -282,5 +284,26 @@ void Import(StreamReader input, string configurationFile = "") } } } + + /// + /// Registers a subscriber to receive notifications when modifications are performed to the . + /// + /// The to register. + internal void Subscribe(IAccessControlListSubscriber subscriber) + { + _subscribers[subscriber.AclSubscriberKey] = subscriber; + } + + /// + /// Notify the registered subscribers when modifications are performed to the . + /// + /// The created or updated that triggered the notification. + private void NotifySubscribers(User user) + { + foreach (IAccessControlListSubscriber subscriber in _subscribers.Values) + { + subscriber.NotifyAclChange(user); + } + } } } \ No newline at end of file diff --git a/libs/server/ACL/IAccessControlListSubscriber.cs b/libs/server/ACL/IAccessControlListSubscriber.cs new file mode 100644 index 0000000000..74ecd19391 --- /dev/null +++ b/libs/server/ACL/IAccessControlListSubscriber.cs @@ -0,0 +1,22 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +namespace Garnet.server.ACL +{ + /// + /// An interface for types that subscribe to changes. + /// + internal interface IAccessControlListSubscriber + { + /// + /// A key for the . + /// + public string AclSubscriberKey { get; } + + /// + /// Handle notification received when changes are performed to the . + /// + /// The modified . + public void NotifyAclChange(User user); + } +} diff --git a/libs/server/ACL/User.cs b/libs/server/ACL/User.cs index 699fc0ff5f..5824289b87 100644 --- a/libs/server/ACL/User.cs +++ b/libs/server/ACL/User.cs @@ -44,6 +44,19 @@ public User(string name) _enabledCommands = CommandPermissionSet.None; } + /// + /// Copy constructor for a . + /// + /// The the new is constructed from. + public User(User user) + { + Name = user.Name; + IsEnabled = user.IsEnabled; + IsPasswordless = user.IsPasswordless; + _enabledCommands = user._enabledCommands.Copy(); + _passwordHashes = new HashSet(user._passwordHashes); + } + /// /// Checks whether the user can access the given command. /// diff --git a/libs/server/Auth/GarnetACLAuthenticator.cs b/libs/server/Auth/GarnetACLAuthenticator.cs index 2dbe3e98f2..a2b1b999b0 100644 --- a/libs/server/Auth/GarnetACLAuthenticator.cs +++ b/libs/server/Auth/GarnetACLAuthenticator.cs @@ -88,7 +88,7 @@ public bool Authenticate(ReadOnlySpan password, ReadOnlySpan usernam /// Authorized user or null if not authorized public User GetUser() { - return _user; + return _user == null ? null : _acl.GetUser(_user.Name); } /// diff --git a/libs/server/Resp/ACLCommands.cs b/libs/server/Resp/ACLCommands.cs index 150cf31528..69d44d1b1c 100644 --- a/libs/server/Resp/ACLCommands.cs +++ b/libs/server/Resp/ACLCommands.cs @@ -171,7 +171,10 @@ private bool NetworkAclSetUser() if (user == null) { user = new User(username); - aclAuthenticator.GetAccessControlList().AddUser(user); + } + else + { + user = new User(user); } // Remaining parameters are ACL operations @@ -180,6 +183,8 @@ private bool NetworkAclSetUser() var op = parseState.GetString(i); ACLParser.ApplyACLOpToUser(ref user, op); } + + aclAuthenticator.GetAccessControlList().AddOrReplaceUser(user); } catch (ACLException exception) { diff --git a/libs/server/Resp/AdminCommands.cs b/libs/server/Resp/AdminCommands.cs index 26b791fc24..1934de9c60 100644 --- a/libs/server/Resp/AdminCommands.cs +++ b/libs/server/Resp/AdminCommands.cs @@ -114,6 +114,10 @@ internal bool CheckScriptPermissions(RespCommand cmd) [MethodImpl(MethodImplOptions.AggressiveInlining)] internal bool CheckACLPermissions(RespCommand cmd) { + if (!(!_authenticator.IsAuthenticated || (_user != null))) + { + return false; + } Debug.Assert(!_authenticator.IsAuthenticated || (_user != null)); if ((!_authenticator.IsAuthenticated || !_user.CanAccessCommand(cmd)) && !cmd.IsNoAuth()) diff --git a/libs/server/Resp/RespServerSession.cs b/libs/server/Resp/RespServerSession.cs index 940d147be2..929d3ede58 100644 --- a/libs/server/Resp/RespServerSession.cs +++ b/libs/server/Resp/RespServerSession.cs @@ -7,6 +7,7 @@ using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Text; +using System.Threading; using Garnet.common; using Garnet.common.Parsing; using Garnet.networking; @@ -33,7 +34,7 @@ namespace Garnet.server /// /// RESP server session /// - internal sealed unsafe partial class RespServerSession : ServerSessionBase + internal sealed unsafe partial class RespServerSession : ServerSessionBase, IAccessControlListSubscriber { readonly GarnetSessionMetrics sessionMetrics; readonly GarnetLatencyMetricsSession LatencyMetrics; @@ -101,6 +102,12 @@ internal sealed unsafe partial class RespServerSession : ServerSessionBase /// User _user = null; + /// + /// Flag to indicate user is inconsistent with most recent in + /// the . + /// + bool _isUserAclStale = true; + readonly ILogger logger = null; /// @@ -185,6 +192,11 @@ internal sealed unsafe partial class RespServerSession : ServerSessionBase /// public long CreationTicks { get; } + /// + /// Subscriber key for receiving notifications. + /// + public string AclSubscriberKey { get; } + public RespServerSession( long id, INetworkSender networkSender, @@ -201,6 +213,7 @@ public RespServerSession( this.Id = id; this.CreationTicks = Environment.TickCount64; + this.AclSubscriberKey = $"{id}"; logger?.LogDebug("Starting RespServerSession Id={0}", this.Id); @@ -215,6 +228,7 @@ public RespServerSession( this.storeWrapper = storeWrapper; this.subscribeBroker = subscribeBroker; + this.storeWrapper.accessControlList.Subscribe(this); this._authenticator = authenticator ?? storeWrapper.serverOptions.AuthSettings?.CreateAuthenticator(this.storeWrapper) ?? new GarnetNoAuthAuthenticator(); if (storeWrapper.serverOptions.EnableLua && enableScripts) @@ -288,20 +302,7 @@ public override void Dispose() if (success) { - // Set authenticated user or fall back to default user, if separate users are not supported - // NOTE: Currently only GarnetACLAuthenticator supports multiple users - if (_authenticator is GarnetACLAuthenticator aclAuthenticator) - { - this._user = aclAuthenticator.GetUser(); - } - else - { - this._user = this.storeWrapper.accessControlList.GetDefaultUser(); - } - - // Propagate authentication to cluster session - clusterSession?.SetUser(this._user); - sessionScriptCache?.SetUser(this._user); + RefreshUser(true); } return _authenticator.CanAuthenticate ? success : false; @@ -394,6 +395,12 @@ public override int TryConsumeMessages(byte* reqBuffer, int bytesReceived) return readHead; } + /// + public void NotifyAclChange(User user) + { + _isUserAclStale = _user?.Name == user.Name; + } + /// /// For testing purposes, call and update state accordingly. /// @@ -412,6 +419,30 @@ internal void ExitAndReturnResponseObject() internal void SetTransactionMode(bool enable) => txnManager.state = enable ? TxnState.Running : TxnState.None; + private void RefreshUser(bool force = false) + { + if (!_isUserAclStale && !force) + { + return; + } + + // Set authenticated user or fall back to default user, if separate users are not supported + // NOTE: Currently only GarnetACLAuthenticator supports multiple users + if (_authenticator is GarnetACLAuthenticator aclAuthenticator) + { + this._user = aclAuthenticator.GetUser(); + } + else + { + this._user = this.storeWrapper.accessControlList.GetDefaultUser(); + } + + // Propagate authentication to cluster session + clusterSession?.SetUser(this._user); + sessionScriptCache?.SetUser(this._user); + _isUserAclStale = false; + } + private void ProcessMessages() { // #if DEBUG @@ -438,6 +469,11 @@ private void ProcessMessages() // Check ACL permissions for the command if (cmd != RespCommand.INVALID) { + if (_isUserAclStale && cmd != RespCommand.AUTH) + { + RefreshUser(); + } + var noScriptPassed = true; if (CheckACLPermissions(cmd) && (noScriptPassed = CheckScriptPermissions(cmd))) diff --git a/test/Garnet.test/Resp/ACL/ParallelTests.cs b/test/Garnet.test/Resp/ACL/ParallelTests.cs index 10a8a9a467..317983bbd7 100644 --- a/test/Garnet.test/Resp/ACL/ParallelTests.cs +++ b/test/Garnet.test/Resp/ACL/ParallelTests.cs @@ -77,7 +77,7 @@ public void ParallelPasswordHashTest(int degreeOfParallelism, int iterationsPerS /// Test launches multiple clients that apply two simple ACL changes to the same user many times in parallel. /// Validates that ACL result after each execution is one of the possible valid responses. /// - /// Race conditions are not deterministic so tests uses repeat. + /// Race conditions are not deterministic so test uses repeat. /// /// [TestCase(128, 2048)] @@ -126,7 +126,7 @@ await Task.WhenAll( /// Test launches multiple clients that apply the same ACL change to the same user. Creates race to become the /// the first client to add the user to the ACL. Throws after initial insert into ACL if threading issues exist. /// - /// Race conditions are not deterministic so tests uses repeat. + /// Race conditions are not deterministic so test uses repeat. /// /// [TestCase(128, 2048)] From 21b69e09bc368e9408cc46ddac37b1c9d178b37d Mon Sep 17 00:00:00 2001 From: kbowersox Date: Sat, 1 Feb 2025 10:12:04 -0800 Subject: [PATCH 04/10] Subscribe to ACL notification after authentication of default user --- libs/server/Resp/RespServerSession.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libs/server/Resp/RespServerSession.cs b/libs/server/Resp/RespServerSession.cs index 929d3ede58..6e9bfd6748 100644 --- a/libs/server/Resp/RespServerSession.cs +++ b/libs/server/Resp/RespServerSession.cs @@ -228,7 +228,6 @@ public RespServerSession( this.storeWrapper = storeWrapper; this.subscribeBroker = subscribeBroker; - this.storeWrapper.accessControlList.Subscribe(this); this._authenticator = authenticator ?? storeWrapper.serverOptions.AuthSettings?.CreateAuthenticator(this.storeWrapper) ?? new GarnetNoAuthAuthenticator(); if (storeWrapper.serverOptions.EnableLua && enableScripts) @@ -236,6 +235,8 @@ public RespServerSession( // Associate new session with default user and automatically authenticate, if possible this.AuthenticateUser(Encoding.ASCII.GetBytes(this.storeWrapper.accessControlList.GetDefaultUser().Name)); + this.storeWrapper.accessControlList.Subscribe(this); + txnManager = new TransactionManager(this, storageSession, scratchBufferManager, storeWrapper.serverOptions.EnableCluster, logger); storageSession.txnManager = txnManager; From 7a4a76d63504df9dd0b19f3027727deb0893dc80 Mon Sep 17 00:00:00 2001 From: kbowersox Date: Sat, 1 Feb 2025 13:14:25 -0800 Subject: [PATCH 05/10] Add User Locks for Linearizability of ACL SETUSER --- libs/server/ACL/AccessControlList.cs | 39 ++++++++++++++++ libs/server/Resp/ACLCommands.cs | 53 +++++++++++----------- test/Garnet.test/Resp/ACL/ParallelTests.cs | 2 +- 3 files changed, 67 insertions(+), 27 deletions(-) diff --git a/libs/server/ACL/AccessControlList.cs b/libs/server/ACL/AccessControlList.cs index e595c0c8c3..efc8a2b820 100644 --- a/libs/server/ACL/AccessControlList.cs +++ b/libs/server/ACL/AccessControlList.cs @@ -18,11 +18,21 @@ public class AccessControlList /// const string DefaultUserName = "default"; + /// + /// Arbitrary Key for new user lock object. + /// + const string NewUserLockObjectKey = "441a61e2-4d4e-498e-8ca0-715cf550e5be"; + /// /// Dictionary containing all users defined in the ACL /// ConcurrentDictionary _users = new(); + /// + /// Dictionary containing stable lock objects for each user in the ACL. + /// + ConcurrentDictionary _userLockObjects = new(); + /// /// The currently configured default user (for fast default lookups) /// @@ -52,6 +62,7 @@ public AccessControlList(string defaultPassword = "", string aclConfigurationFil // If no ACL file is defined, only create the default user _defaultUser = CreateDefaultUser(defaultPassword); } + _userLockObjects[NewUserLockObjectKey] = new object(); } /// @@ -68,6 +79,33 @@ public User GetUser(string username) return null; } + /// + /// Returns the lock object for the user with the given name. This allows user level locks, which should only be + /// used for rare cases where modifications must be made to a user object, most notably ACL SETUSER. + /// + /// If modifications to a user are necessary the following pattern is suggested: + /// + /// 1. Obtain the lock object for the user using this method. + /// 2. Immediately take a lock on the object. + /// 3. Read the user from the and make a copy with the copy constructor. + /// 4. Modify the copy of the user object. + /// 5. Replace the user in the using the AddOrReplace(User user) method. + /// + /// Note: This pattern will make the critical section under lock single threaded across all sessions, use very + /// sparingly. + /// + /// Username of the user to retrieve. + /// Matching user lock object. + public object GetUserLockObject(string username) + { + if (_userLockObjects.TryGetValue(username, out var userLockObject)) + { + return userLockObject; + } + + return _userLockObjects[NewUserLockObjectKey]; + } + /// /// Returns the currently configured default user. /// @@ -85,6 +123,7 @@ public void AddOrReplaceUser(User user) { // If a user with the given name already exists replace the user, otherwise add the new user. _users[user.Name] = user; + _ = _userLockObjects.TryAdd(user.Name, new object()); this.NotifySubscribers(user); } diff --git a/libs/server/Resp/ACLCommands.cs b/libs/server/Resp/ACLCommands.cs index 69d44d1b1c..33b15367c5 100644 --- a/libs/server/Resp/ACLCommands.cs +++ b/libs/server/Resp/ACLCommands.cs @@ -163,38 +163,39 @@ private bool NetworkAclSetUser() // REQUIRED: username var username = parseState.GetString(0); - // Modify or create the user with the given username - var user = aclAuthenticator.GetAccessControlList().GetUser(username); - - try + lock (aclAuthenticator.GetAccessControlList().GetUserLockObject(username)) { - if (user == null) - { - user = new User(username); - } - else - { - user = new User(user); - } + // Modify or create the user with the given username + var user = aclAuthenticator.GetAccessControlList().GetUser(username); - // Remaining parameters are ACL operations - for (var i = 1; i < parseState.Count; i++) + try { - var op = parseState.GetString(i); - ACLParser.ApplyACLOpToUser(ref user, op); - } + if (user == null) + { + user = new User(username); + } + else + { + user = new User(user); + } - aclAuthenticator.GetAccessControlList().AddOrReplaceUser(user); - } - catch (ACLException exception) - { - logger?.LogDebug("ACLException: {message}", exception.Message); + // Remaining parameters are ACL operations + for (var i = 1; i < parseState.Count; i++) + { + var op = parseState.GetString(i); + ACLParser.ApplyACLOpToUser(ref user, op); + } - // Abort command execution - while (!RespWriteUtils.TryWriteError($"ERR {exception.Message}", ref dcurr, dend)) - SendAndReset(); + aclAuthenticator.GetAccessControlList().AddOrReplaceUser(user); + } + catch (ACLException exception) + { + // Abort command execution + while (!RespWriteUtils.TryWriteError($"ERR {exception.Message}", ref dcurr, dend)) + SendAndReset(); - return true; + return true; + } } while (!RespWriteUtils.TryWriteDirect(CmdStrings.RESP_OK, ref dcurr, dend)) diff --git a/test/Garnet.test/Resp/ACL/ParallelTests.cs b/test/Garnet.test/Resp/ACL/ParallelTests.cs index 317983bbd7..54c65f6c23 100644 --- a/test/Garnet.test/Resp/ACL/ParallelTests.cs +++ b/test/Garnet.test/Resp/ACL/ParallelTests.cs @@ -130,7 +130,7 @@ await Task.WhenAll( /// /// [TestCase(128, 2048)] - [Repeat(10)] + [Repeat(5)] public async Task ParallelAclSetUserAvoidsMapContentionTest(int degreeOfParallelism, int iterationsPerSession) { string command1 = $"ACL SETUSER {TestUserA} on >{DummyPassword}"; From dc08dcf3e92972cf2e8914045c790655f5c3739e Mon Sep 17 00:00:00 2001 From: kbowersox Date: Sat, 1 Feb 2025 13:33:04 -0800 Subject: [PATCH 06/10] Clean up unused import in RespServerSession --- libs/server/Resp/RespServerSession.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/libs/server/Resp/RespServerSession.cs b/libs/server/Resp/RespServerSession.cs index 6e9bfd6748..c568bfe358 100644 --- a/libs/server/Resp/RespServerSession.cs +++ b/libs/server/Resp/RespServerSession.cs @@ -7,7 +7,6 @@ using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Text; -using System.Threading; using Garnet.common; using Garnet.common.Parsing; using Garnet.networking; From 2872177b5bfba8bf4c947aada05915e9c30b6fb1 Mon Sep 17 00:00:00 2001 From: kbowersox Date: Sat, 1 Feb 2025 13:45:30 -0800 Subject: [PATCH 07/10] Remove newline from file to resolve format violation --- libs/server/ACL/IAccessControlListSubscriber.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/server/ACL/IAccessControlListSubscriber.cs b/libs/server/ACL/IAccessControlListSubscriber.cs index 74ecd19391..39abd111db 100644 --- a/libs/server/ACL/IAccessControlListSubscriber.cs +++ b/libs/server/ACL/IAccessControlListSubscriber.cs @@ -19,4 +19,4 @@ internal interface IAccessControlListSubscriber /// The modified . public void NotifyAclChange(User user); } -} +} \ No newline at end of file From b9ad4b21ef9eb46a57e80c88fbf14abd7ca23431 Mon Sep 17 00:00:00 2001 From: kbowersox Date: Sat, 1 Feb 2025 15:38:00 -0800 Subject: [PATCH 08/10] Remove user lock object from dictionary upon deletion --- libs/server/ACL/AccessControlList.cs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/libs/server/ACL/AccessControlList.cs b/libs/server/ACL/AccessControlList.cs index efc8a2b820..a0a957c6e9 100644 --- a/libs/server/ACL/AccessControlList.cs +++ b/libs/server/ACL/AccessControlList.cs @@ -139,7 +139,15 @@ public bool DeleteUser(string username) { throw new ACLException("The special 'default' user cannot be removed from the system"); } - return _users.TryRemove(username, out _); + + bool userDeleted = _users.TryRemove(username, out _); + + if (userDeleted) + { + _userLockObjects.TryRemove(username, out _); + } + + return userDeleted; } /// From d5629a7fc07439497a0909e65487eb331bc62f62 Mon Sep 17 00:00:00 2001 From: kbowersox Date: Mon, 3 Feb 2025 12:51:59 -0800 Subject: [PATCH 09/10] Address feedback on PR regarding races, interfaces and errant debug code --- libs/server/ACL/AccessControlList.cs | 28 +++++++++++++------ .../ACL/IAccessControlListSubscriber.cs | 22 --------------- libs/server/Resp/AdminCommands.cs | 4 --- libs/server/Resp/RespServerSession.cs | 22 +++++++++++++-- 4 files changed, 39 insertions(+), 37 deletions(-) delete mode 100644 libs/server/ACL/IAccessControlListSubscriber.cs diff --git a/libs/server/ACL/AccessControlList.cs b/libs/server/ACL/AccessControlList.cs index a0a957c6e9..5c63dfcdb3 100644 --- a/libs/server/ACL/AccessControlList.cs +++ b/libs/server/ACL/AccessControlList.cs @@ -39,9 +39,9 @@ public class AccessControlList User _defaultUser; /// - /// The subscribers who will receive access control list change notifications. + /// The s that will receive access control list change notifications. /// - private readonly ConcurrentDictionary _subscribers = new(); + private readonly ConcurrentDictionary _subscribedSessions = new(); /// /// Creates a new Access Control List from an optional ACL configuration file @@ -333,23 +333,33 @@ void Import(StreamReader input, string configurationFile = "") } /// - /// Registers a subscriber to receive notifications when modifications are performed to the . + /// Registers a to receive notifications when modifications are performed to the . /// - /// The to register. - internal void Subscribe(IAccessControlListSubscriber subscriber) + /// The to register. + internal void Subscribe(RespServerSession respSession) { - _subscribers[subscriber.AclSubscriberKey] = subscriber; + _subscribedSessions[respSession.AclSubscriberKey] = respSession; } /// - /// Notify the registered subscribers when modifications are performed to the . + /// Unregisters a to receive notifications when modifications are performed to the . + /// + /// The to register. + internal void Unsubscribe(RespServerSession respSession) + { + _ = _subscribedSessions.TryRemove(respSession.AclSubscriberKey, out _); + } + + + /// + /// Notify the registered when modifications are performed to the . /// /// The created or updated that triggered the notification. private void NotifySubscribers(User user) { - foreach (IAccessControlListSubscriber subscriber in _subscribers.Values) + foreach (RespServerSession respSession in _subscribedSessions.Values) { - subscriber.NotifyAclChange(user); + respSession.NotifyAclChange(user); } } } diff --git a/libs/server/ACL/IAccessControlListSubscriber.cs b/libs/server/ACL/IAccessControlListSubscriber.cs deleted file mode 100644 index 39abd111db..0000000000 --- a/libs/server/ACL/IAccessControlListSubscriber.cs +++ /dev/null @@ -1,22 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -namespace Garnet.server.ACL -{ - /// - /// An interface for types that subscribe to changes. - /// - internal interface IAccessControlListSubscriber - { - /// - /// A key for the . - /// - public string AclSubscriberKey { get; } - - /// - /// Handle notification received when changes are performed to the . - /// - /// The modified . - public void NotifyAclChange(User user); - } -} \ No newline at end of file diff --git a/libs/server/Resp/AdminCommands.cs b/libs/server/Resp/AdminCommands.cs index 1934de9c60..26b791fc24 100644 --- a/libs/server/Resp/AdminCommands.cs +++ b/libs/server/Resp/AdminCommands.cs @@ -114,10 +114,6 @@ internal bool CheckScriptPermissions(RespCommand cmd) [MethodImpl(MethodImplOptions.AggressiveInlining)] internal bool CheckACLPermissions(RespCommand cmd) { - if (!(!_authenticator.IsAuthenticated || (_user != null))) - { - return false; - } Debug.Assert(!_authenticator.IsAuthenticated || (_user != null)); if ((!_authenticator.IsAuthenticated || !_user.CanAccessCommand(cmd)) && !cmd.IsNoAuth()) diff --git a/libs/server/Resp/RespServerSession.cs b/libs/server/Resp/RespServerSession.cs index c568bfe358..d5c0bfb2ea 100644 --- a/libs/server/Resp/RespServerSession.cs +++ b/libs/server/Resp/RespServerSession.cs @@ -33,7 +33,7 @@ namespace Garnet.server /// /// RESP server session /// - internal sealed unsafe partial class RespServerSession : ServerSessionBase, IAccessControlListSubscriber + internal sealed unsafe partial class RespServerSession : ServerSessionBase { readonly GarnetSessionMetrics sessionMetrics; readonly GarnetLatencyMetricsSession LatencyMetrics; @@ -255,6 +255,12 @@ public RespServerSession( if (this.networkSender.GetMaxSizeSettings?.MaxOutputSize < sizeof(int)) this.networkSender.GetMaxSizeSettings.MaxOutputSize = sizeof(int); } + + // Ensure user was not updated during initialization. + if (_authenticator is GarnetACLAuthenticator aclAuthenticator && _user != aclAuthenticator.GetUser()) + { + this.RefreshUser(); + } } internal void SetUser(User user) @@ -1031,7 +1037,19 @@ public ArgSlice GetCommandAsArgSlice(out bool success) /// Subsequent calls will return false. /// public bool TryKill() - => networkSender.TryClose(); + { + if (networkSender.TryClose()) + { + if (_authenticator is GarnetACLAuthenticator aclAuthenticator) + { + aclAuthenticator.GetAccessControlList().Unsubscribe(this); + } + + return true; + } + + return false; + } [MethodImpl(MethodImplOptions.AggressiveInlining)] private unsafe bool Write(ref Status s, ref byte* dst, int length) From 4ca6db52373090c3b124adaec30b230695b9b23c Mon Sep 17 00:00:00 2001 From: kbowersox Date: Mon, 3 Feb 2025 12:54:16 -0800 Subject: [PATCH 10/10] Reformat unsubscribe logic --- libs/server/Resp/RespServerSession.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/libs/server/Resp/RespServerSession.cs b/libs/server/Resp/RespServerSession.cs index d5c0bfb2ea..82d1dca14b 100644 --- a/libs/server/Resp/RespServerSession.cs +++ b/libs/server/Resp/RespServerSession.cs @@ -1038,17 +1038,17 @@ public ArgSlice GetCommandAsArgSlice(out bool success) /// public bool TryKill() { - if (networkSender.TryClose()) + if (!networkSender.TryClose()) { - if (_authenticator is GarnetACLAuthenticator aclAuthenticator) - { - aclAuthenticator.GetAccessControlList().Unsubscribe(this); - } + return false; + } - return true; + if (_authenticator is GarnetACLAuthenticator aclAuthenticator) + { + aclAuthenticator.GetAccessControlList().Unsubscribe(this); } - return false; + return true; } [MethodImpl(MethodImplOptions.AggressiveInlining)]