Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ACL GETUSER Command Initial Implementation #959

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions libs/resources/RespCommandsDocs.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,22 @@
}
]
},
{
"Command": "ACL_GETUSER",
"Name": "ACL|GETUSER",
"Summary": "Returns the rules defined for an ACL user.",
"Group": "Server",
"Complexity": "O(1) amortized time considering the typical user.",
"Arguments": [
{
"TypeDiscriminator": "RespCommandBasicArgument",
"Name": "USERNAME",
"DisplayText": "username",
"Type": "String",
"ArgumentFlags": "Multiple"
}
]
},
{
"Command": "ACL_LIST",
"Name": "ACL|LIST",
Expand Down
11 changes: 11 additions & 0 deletions libs/resources/RespCommandsInfo.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,17 @@
"response_policy:all_succeeded"
]
},
{
"Command": "ACL_GETUSER",
"Name": "ACL|GETUSER",
"Arity": -3,
"Flags": "Admin, Loading, NoScript, Stale",
"AclCategories": "Admin, Dangerous, Slow",
"Tips": [
"request_policy:all_nodes",
"response_policy:all_succeeded"
]
},
{
"Command": "ACL_LIST",
"Name": "ACL|LIST",
Expand Down
35 changes: 31 additions & 4 deletions libs/server/ACL/User.cs
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public void AddCategory(RespAclCategories category)

/// <summary>
/// Adds the given command to the user.
///
///
/// If the command has subcommands, and no specific subcommand is indicated, adds all subcommands too.
/// </summary>
/// <param name="command">Command to add.</param>
Expand Down Expand Up @@ -246,7 +246,7 @@ public void RemoveCategory(RespAclCategories category)

/// <summary>
/// Removes the given command from the user.
///
///
/// If the command has subcommands, and no specific subcommand is indicated, removes all subcommands too.
/// </summary>
/// <param name="command">Command to remove.</param>
Expand Down Expand Up @@ -420,6 +420,33 @@ public string DescribeUser()
return stringBuilder.ToString();
}

/// <summary>
/// Returns flags for the <see cref="User"/>.
/// </summary>
/// <returns>A <see cref="List{T}"/> of <see cref="string"/> representing flags for the user.</returns>
public List<string> GetFlags()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why allocate here? It's just switching between two constants.

{
return new() { IsEnabled ? "on" : "off" };
}

/// <summary>
/// Returns password hashes for the <see cref="User"/>.
/// </summary>
/// <returns>A <see cref="List{T}"/> of <see cref="string"/> representing password hashes for the user.</returns>
public List<string> GetPasswordHashes()
{
return _passwordHashes.Select(hash => $"#{hash}").ToList();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not thread safe, _passwordHashes is mutable.

Also, there's no need to allocate here, we're just prepending a # so that can be done while writing the response out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you onboard with making this field accessible to clients to avoid the allocations?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the thread safety concerns, I don't think exposing the field is appropriate.

}

/// <summary>
/// Returns a <see cref="string"/> containing the enabled commands.
/// </summary>
/// <returns>A <see cref="string"/> containing the enabled commands.</returns>
public string GetEnabledCommandsDescription()
{
return _enabledCommands.Description;
}

/// <summary>
/// Determine the command / sub command pairs that are associated with this command information entries
/// </summary>
Expand Down Expand Up @@ -448,7 +475,7 @@ internal static IEnumerable<RespCommand> DetermineCommandDetails(IReadOnlyList<R

/// <summary>
/// Check to see if any tokens from a description can be removed without modifying the effective permissions.
///
///
/// This is an expensive method, but ACL modifications are rare enough it's hopefully not a problem.
/// </summary>
private static string RationalizeACLDescription(CommandPermissionSet set, string description)
Expand Down Expand Up @@ -492,7 +519,7 @@ internal CommandPermissionSet CopyCommandPermissionSet()

/// <summary>
/// A set of all allowed _passwordHashes for the user.
///
///
/// NOTE: HashSet is not thread-safe, so accesses need to be synchronized
/// </summary>
readonly HashSet<ACLPassword> _passwordHashes = [];
Expand Down
85 changes: 85 additions & 0 deletions libs/server/Resp/ACLCommands.cs
Original file line number Diff line number Diff line change
Expand Up @@ -366,5 +366,90 @@ private bool NetworkAclSave()

return true;
}

/// <summary>
/// Processes ACL GETUSER subcommand.
/// </summary>
/// <returns>true if parsing succeeded correctly, false if not all tokens could be consumed and further processing is necessary.</returns>
private bool NetworkAclGetUser()
{
// Have to have at least the username
if (parseState.Count != 1)
{
while (!RespWriteUtils.TryWriteError($"ERR Unknown subcommand or wrong number of arguments for ACL GETUSER.", ref dcurr, dend))
SendAndReset();
}
else
{
if (!ValidateACLAuthenticator())
return true;

var aclAuthenticator = (GarnetACLAuthenticator)_authenticator;
User user = null;

try
{
user = aclAuthenticator
.GetAccessControlList()
.GetUser(parseState.GetString(0));
}
catch (ACLException exception)
{
logger?.LogDebug("ACLException: {message}", exception.Message);

// Abort command execution
while (!RespWriteUtils.TryWriteError($"ERR {exception.Message}", ref dcurr, dend))
SendAndReset();

return true;
}

if (user is null)
{
while (!RespWriteUtils.TryWriteNull(ref dcurr, dend))
SendAndReset();
}
else
{
while (!RespWriteUtils.TryWriteArrayLength(6, ref dcurr, dend))
SendAndReset();

var flags = user.GetFlags();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think broadly this will run into issues if the user is modified while the command is running.

GetFlags(), GetPasswordHashes(), and GetEnabledCommandsDescription() won't necessarily return consistent results.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that enabledCommands has a copy method. What are your thoughts on returning a copy of the commands back to the client to avoid any issues with modifications?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some variant of "one method (that internally takes appropriate) locks, and copies the relevant data into the return"-is the only path forward.

I'd be tempted to try and fit things into Span's created by the caller to avoid allocations (or rent some arrays, or something), so the there's some flexibility in the implementation.

var passwordHashes = user.GetPasswordHashes();

while (!RespWriteUtils.TryWriteAsciiBulkString("flags", ref dcurr, dend))
SendAndReset();

while (!RespWriteUtils.TryWriteArrayLength(flags.Count, ref dcurr, dend))
SendAndReset();

foreach (var flag in flags)
{
while (!RespWriteUtils.TryWriteAsciiBulkString(flag, ref dcurr, dend))
SendAndReset();
}

while (!RespWriteUtils.TryWriteAsciiBulkString("passwords", ref dcurr, dend))
SendAndReset();

while (!RespWriteUtils.TryWriteArrayLength(passwordHashes.Count, ref dcurr, dend))
SendAndReset();

foreach (var passwordHash in passwordHashes)
{
while (!RespWriteUtils.TryWriteAsciiBulkString(passwordHash, ref dcurr, dend))
SendAndReset();
}

while (!RespWriteUtils.TryWriteAsciiBulkString("commands", ref dcurr, dend))
SendAndReset();

while (!RespWriteUtils.TryWriteAsciiBulkString(user.GetEnabledCommandsDescription(), ref dcurr, dend))
SendAndReset();
}
}

return true;
}
}
}
4 changes: 2 additions & 2 deletions libs/server/Resp/AdminCommands.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Licensed under the MIT license.

using System;
using System.Collections;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
Expand Down Expand Up @@ -56,6 +55,7 @@ RespCommand.MIGRATE or
RespCommand.HCOLLECT => NetworkHCOLLECT(ref storageApi),
RespCommand.MONITOR => NetworkMonitor(),
RespCommand.ACL_DELUSER => NetworkAclDelUser(),
RespCommand.ACL_GETUSER => NetworkAclGetUser(),
RespCommand.ACL_LIST => NetworkAclList(),
RespCommand.ACL_LOAD => NetworkAclLoad(),
RespCommand.ACL_SETUSER => NetworkAclSetUser(),
Expand Down Expand Up @@ -126,7 +126,7 @@ internal bool CheckACLPermissions(RespCommand cmd)

/// <summary>
/// Handle ACL or NoScript failures.
///
///
/// Failing should be rare, and is not important for performance so hide this behind
/// a method call to keep icache pressure down
/// </summary>
Expand Down
3 changes: 2 additions & 1 deletion libs/server/Resp/CmdStrings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,7 @@ static partial class CmdStrings
// subcommand parsing strings
public static ReadOnlySpan<byte> CAT => "CAT"u8;
public static ReadOnlySpan<byte> DELUSER => "DELUSER"u8;
public static ReadOnlySpan<byte> GETUSER => "GETUSER"u8;
public static ReadOnlySpan<byte> LOAD => "LOAD"u8;
public static ReadOnlySpan<byte> LOADCS => "LOADCS"u8;
public static ReadOnlySpan<byte> SETUSER => "SETUSER"u8;
Expand Down Expand Up @@ -344,7 +345,7 @@ static partial class CmdStrings
public static ReadOnlySpan<byte> NO => "NO"u8;

// Cluster subcommands which are internal and thus undocumented
//
//
// Because these are internal, they have lower case property names
public static ReadOnlySpan<byte> gossip => "GOSSIP"u8;
public static ReadOnlySpan<byte> myparentid => "MYPARENTID"u8;
Expand Down
10 changes: 8 additions & 2 deletions libs/server/Resp/Parser/RespCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ public enum RespCommand : ushort
ACL,
ACL_CAT,
ACL_DELUSER,
ACL_GETUSER,
ACL_LIST,
ACL_LOAD,
ACL_SAVE,
Expand Down Expand Up @@ -377,6 +378,7 @@ public static class RespCommandExtensions
// ACL
RespCommand.ACL_CAT,
RespCommand.ACL_DELUSER,
RespCommand.ACL_GETUSER,
RespCommand.ACL_LIST,
RespCommand.ACL_LOAD,
RespCommand.ACL_SAVE,
Expand Down Expand Up @@ -1641,7 +1643,7 @@ private RespCommand FastParseArrayCommand(ref int count, ref ReadOnlySpan<byte>
/// <summary>
/// Parses the receive buffer, starting from the current read head, for all command names that are
/// not covered by FastParseArrayCommand() and advances the read head to the end of the command name.
///
///
/// NOTE: Assumes the input command names have already been converted to upper-case.
/// </summary>
/// <param name="count">Reference to the number of remaining tokens in the packet. Will be reduced to number of command arguments.</param>
Expand Down Expand Up @@ -2122,6 +2124,10 @@ private RespCommand SlowParseCommand(ref int count, ref ReadOnlySpan<byte> speci
{
return RespCommand.ACL_DELUSER;
}
else if (subCommand.SequenceEqual(CmdStrings.GETUSER))
{
return RespCommand.ACL_GETUSER;
}
else if (subCommand.SequenceEqual(CmdStrings.LIST))
{
return RespCommand.ACL_LIST;
Expand Down Expand Up @@ -2326,7 +2332,7 @@ private void HandleAofCommitMode(RespCommand cmd)
if (txnManager.state == TxnState.Started)
return;

/*
/*
If a previous command marked AOF for blocking we should not change AOF blocking flag.
If no previous command marked AOF for blocking, then we only change AOF flag to block
if the current command is AOF dependent.
Expand Down
Loading
Loading