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..5c63dfcdb3 100644 --- a/libs/server/ACL/AccessControlList.cs +++ b/libs/server/ACL/AccessControlList.cs @@ -18,16 +18,31 @@ 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) /// User _defaultUser; + /// + /// The s that will receive access control list change notifications. + /// + private readonly ConcurrentDictionary _subscribedSessions = 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. @@ -47,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(); } /// @@ -63,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. /// @@ -73,17 +116,15 @@ 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; + _ = _userLockObjects.TryAdd(user.Name, new object()); + this.NotifySubscribers(user); } /// @@ -98,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; } /// @@ -150,7 +199,7 @@ User CreateDefaultUser(string defaultPassword = "") // Add the user to the user list try { - AddUser(defaultUser); + AddOrReplaceUser(defaultUser); break; } catch (ACLUserAlreadyExistsException) @@ -282,5 +331,36 @@ void Import(StreamReader input, string configurationFile = "") } } } + + /// + /// Registers a to receive notifications when modifications are performed to the . + /// + /// The to register. + internal void Subscribe(RespServerSession respSession) + { + _subscribedSessions[respSession.AclSubscriberKey] = respSession; + } + + /// + /// 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 (RespServerSession respSession in _subscribedSessions.Values) + { + respSession.NotifyAclChange(user); + } + } } } \ No newline at end of file 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..33b15367c5 100644 --- a/libs/server/Resp/ACLCommands.cs +++ b/libs/server/Resp/ACLCommands.cs @@ -163,33 +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); - aclAuthenticator.GetAccessControlList().AddUser(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); - } - } - catch (ACLException exception) - { - logger?.LogDebug("ACLException: {message}", exception.Message); + if (user == null) + { + user = new User(username); + } + else + { + user = new User(user); + } - // Abort command execution - while (!RespWriteUtils.TryWriteError($"ERR {exception.Message}", ref dcurr, dend)) - SendAndReset(); + // Remaining parameters are ACL operations + for (var i = 1; i < parseState.Count; i++) + { + var op = parseState.GetString(i); + ACLParser.ApplyACLOpToUser(ref user, op); + } - return true; + aclAuthenticator.GetAccessControlList().AddOrReplaceUser(user); + } + catch (ACLException exception) + { + // Abort command execution + while (!RespWriteUtils.TryWriteError($"ERR {exception.Message}", ref dcurr, dend)) + SendAndReset(); + + return true; + } } while (!RespWriteUtils.TryWriteDirect(CmdStrings.RESP_OK, ref dcurr, dend)) diff --git a/libs/server/Resp/RespServerSession.cs b/libs/server/Resp/RespServerSession.cs index 84bdf4a349..e0a450f0cd 100644 --- a/libs/server/Resp/RespServerSession.cs +++ b/libs/server/Resp/RespServerSession.cs @@ -102,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; /// @@ -186,6 +192,11 @@ internal sealed unsafe partial class RespServerSession : ServerSessionBase /// public long CreationTicks { get; } + /// + /// Subscriber key for receiving notifications. + /// + public string AclSubscriberKey { get; } + // Track start time (in ticks) of last command for slow log purposes long slowLogStartTime; // Threshold for slow log in ticks (0 means disabled) @@ -207,6 +218,7 @@ public RespServerSession( this.Id = id; this.CreationTicks = Environment.TickCount64; + this.AclSubscriberKey = $"{id}"; logger?.LogDebug("Starting RespServerSession Id={0}", this.Id); @@ -228,6 +240,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; @@ -249,6 +263,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) @@ -296,20 +316,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; @@ -406,6 +413,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. /// @@ -424,6 +437,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 @@ -450,6 +487,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))) @@ -1008,7 +1050,19 @@ public ArgSlice GetCommandAsArgSlice(out bool success) /// Subsequent calls will return false. /// public bool TryKill() - => networkSender.TryClose(); + { + if (!networkSender.TryClose()) + { + return false; + } + + if (_authenticator is GarnetACLAuthenticator aclAuthenticator) + { + aclAuthenticator.GetAccessControlList().Unsubscribe(this); + } + + return true; + } [MethodImpl(MethodImplOptions.AggressiveInlining)] private unsafe bool Write(ref Status s, ref byte* dst, int length) diff --git a/test/Garnet.test/Resp/ACL/ParallelTests.cs b/test/Garnet.test/Resp/ACL/ParallelTests.cs index 129ff99027..54c65f6c23 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,87 @@ 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 test 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 test uses repeat. + /// + /// + [TestCase(128, 2048)] + [Repeat(5)] + public async Task ParallelAclSetUserAvoidsMapContentionTest(int degreeOfParallelism, int iterationsPerSession) + { + string command1 = $"ACL SETUSER {TestUserA} on >{DummyPassword}"; + + 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