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