Skip to content

Replace Auth::User IP-cache with a ClpMap cache #2221

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.

Sign up for GitHub

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

Open
yadij wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Replace Auth::User IP-cache with a ClpMap cache #2221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
06dcb3f
Replace Auth::User IP-cache with a ClpMap cache
yadij Sep 7, 2025
298daf0
Drop #if 0 section
yadij Sep 8, 2025
5dad117
Update code comments
yadij Sep 12, 2025
8ad7b0c
Add ClpMap::clear() method
yadij Sep 12, 2025
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
1 change: 1 addition & 0 deletions src/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1711,6 +1711,7 @@ nodist_tests_testACLMaxUserIP_SOURCES = \
tests/stub_libhttp.cc \
tests/stub_libmem.cc \
tests/stub_libsecurity.cc \
tests/stub_libtime.cc \
tests/stub_neighbors.cc
tests_testACLMaxUserIP_LDADD = \
$(AUTH_ACL_LIBS) \
Expand Down
155 changes: 24 additions & 131 deletions src/auth/User.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,24 @@
#include "globals.h"
#include "Store.h"

constexpr auto SizeOfUserIPCacheEntry = (sizeof(SBuf)+MAX_IPSTRLEN) /* key */ +
sizeof(Ip::Address) /* value */ +
sizeof(time_t) /* expiry */;

Auth::User::User(Auth::SchemeConfig *aConfig, const char *aRequestRealm) :
auth_type(Auth::AUTH_UNKNOWN),
config(aConfig),
ipcount(0),
expiretime(0),
credentials_state(Auth::Unchecked),
username_(nullptr),
requestRealm_(aRequestRealm)
requestRealm_(aRequestRealm),
/* IPv6 "anonymization" rotates IPv6 every 5min.
* This 1200 cache entries allows each user 4 such devices
* with 24hrs of TTL before the cache starts to trim records.
*/
ipList(1200 * SizeOfUserIPCacheEntry)
{
proxy_match_cache.head = proxy_match_cache.tail = nullptr;
ip_list.head = ip_list.tail = nullptr;
debugs(29, 5, "Initialised auth_user '" << this << "'.");
}

Expand Down Expand Up @@ -69,52 +76,7 @@ Auth::User::absorb(Auth::User::Pointer from)
notes.appendNewOnly(&from->notes);
Copy link
Contributor

@rousskov rousskov Sep 8, 2025

Choose a reason for hiding this comment

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

Here, we ignore any same-key annotations found from. In merge() below, we use same-key annotations in from to replace the ones in this. If this difference is intentional, please add a C++ comment to highlight and explain the difference in merging two containers (notes and ipList).

Copy link
Contributor Author

@yadij yadij Sep 12, 2025

Choose a reason for hiding this comment

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

Not sure why you are equating annotations with IP addresses. They are different, their handling is different.


/* absorb the list of IP address sources (for max_user_ip controls) */
AuthUserIP *new_ipdata;
while (from->ip_list.head != nullptr) {
new_ipdata = static_cast<AuthUserIP *>(from->ip_list.head->data);

/* If this IP has expired - ignore the expensive merge actions. */
if (new_ipdata->ip_expiretime <= squid_curtime) {
/* This IP has expired - remove from the source list */
dlinkDelete(&new_ipdata->node, &(from->ip_list));
delete new_ipdata;
/* catch incipient underflow */
-- from->ipcount;
} else {
/* add to our list. replace if already present. */
AuthUserIP *ipdata = static_cast<AuthUserIP *>(ip_list.head->data);
bool found = false;
while (ipdata) {
AuthUserIP *tempnode = static_cast<AuthUserIP *>(ipdata->node.next->data);

if (ipdata->ipaddr == new_ipdata->ipaddr) {
/* This IP has already been seen. */
found = true;
/* update IP ttl and stop searching. */
ipdata->ip_expiretime = max(ipdata->ip_expiretime, new_ipdata->ip_expiretime);
break;
} else if (ipdata->ip_expiretime <= squid_curtime) {
/* This IP has expired - cleanup the destination list */
dlinkDelete(&ipdata->node, &ip_list);
delete ipdata;
/* catch incipient underflow */
assert(ipcount);
-- ipcount;
}

ipdata = tempnode;
}

if (!found) {
/* This ip is not in the seen list. Add it. */
dlinkAddTail(&new_ipdata->node, &ipdata->node, &ip_list);
++ipcount;
/* remove from the source list */
dlinkDelete(&new_ipdata->node, &(from->ip_list));
++from->ipcount;
}
}
}
ipList.merge(from->ipList);
Copy link
Contributor

@rousskov rousskov Sep 8, 2025

Choose a reason for hiding this comment

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

I assume you have tested this new code with both found and !found cases (using old code terminology). Please apply the same test to the old code to explain why the found case avoided the infinite while (from->ip_list.head != nullptr) loop.

Copy link
Contributor Author

@yadij yadij Sep 12, 2025

Choose a reason for hiding this comment

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

I am relying on the fact that we exhaustively audited and tested ClpMap before it merged not to have that type of fundamental error. All this Auth::User code does now is call add(IP, TTL) whenever a login occurs.

}

Auth::User::~User()
Expand All @@ -125,104 +87,35 @@ Auth::User::~User()
/* free cached acl results */
aclCacheMatchFlush(&proxy_match_cache);

/* free seen ip address's */
clearIp();

if (username_)
xfree((char*)username_);

/* prevent accidental reuse */
auth_type = Auth::AUTH_UNKNOWN;
}

void
Auth::User::clearIp()
/// generate the cache key for an ipList entry
static const SBuf
BuildIpKey(const Ip::Address &ip)
{
AuthUserIP *ipdata, *tempnode;

ipdata = (AuthUserIP *) ip_list.head;

while (ipdata) {
tempnode = (AuthUserIP *) ipdata->node.next;
/* walk the ip list */
dlinkDelete(&ipdata->node, &ip_list);
delete ipdata;
/* catch incipient underflow */
assert(ipcount);
-- ipcount;
ipdata = tempnode;
}

/* integrity check */
assert(ipcount == 0);
SBuf key;
auto *buf = key.rawAppendStart(MAX_IPSTRLEN);
const auto len = ip.toHostStr(buf, MAX_IPSTRLEN);
key.rawAppendFinish(buf, len);
return key;
Comment on lines +101 to +105
Copy link
Contributor

@kinkie kinkie Sep 8, 2025

Choose a reason for hiding this comment

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

TBH I feel like this would belong better to the Ip::Address class.

Copy link
Contributor

@rousskov rousskov Sep 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW, I recommend addressing my change request about ipList key type before resuming work on this function.

}

void
Auth::User::removeIp(Ip::Address ipaddr)
Auth::User::removeIp(const Ip::Address &ip)
{
AuthUserIP *ipdata = (AuthUserIP *) ip_list.head;

while (ipdata) {
/* walk the ip list */

if (ipdata->ipaddr == ipaddr) {
/* remove the node */
dlinkDelete(&ipdata->node, &ip_list);
delete ipdata;
/* catch incipient underflow */
assert(ipcount);
-- ipcount;
return;
}

ipdata = (AuthUserIP *) ipdata->node.next;
}

ipList.del(BuildIpKey(ip));
}

void
Auth::User::addIp(Ip::Address ipaddr)
Auth::User::addIp(const Ip::Address &ip)
{
AuthUserIP *ipdata = (AuthUserIP *) ip_list.head;
int found = 0;

/*
* we walk the entire list to prevent the first item in the list
* preventing old entries being flushed and locking a user out after
* a timeout+reconfigure
*/
while (ipdata) {
AuthUserIP *tempnode = (AuthUserIP *) ipdata->node.next;
/* walk the ip list */

if (ipdata->ipaddr == ipaddr) {
/* This ip has already been seen. */
found = 1;
/* update IP ttl */
ipdata->ip_expiretime = squid_curtime + Auth::TheConfig.ipTtl;
} else if (ipdata->ip_expiretime <= squid_curtime) {
/* This IP has expired - remove from the seen list */
dlinkDelete(&ipdata->node, &ip_list);
delete ipdata;
/* catch incipient underflow */
assert(ipcount);
-- ipcount;
}

ipdata = tempnode;
}

if (found)
return;

/* This ip is not in the seen list */
ipdata = new AuthUserIP(ipaddr, squid_curtime + Auth::TheConfig.ipTtl);

dlinkAddTail(ipdata, &ipdata->node, &ip_list);

++ipcount;

debugs(29, 2, "user '" << username() << "' has been seen at a new IP address (" << ipaddr << ")");
ipList.add(BuildIpKey(ip), ip, Auth::TheConfig.ipTtl);
debugs(29, 2, "user '" << username() << "' has been seen at a new IP address (" << ip << ")");
}

SBuf
Expand Down
17 changes: 11 additions & 6 deletions src/auth/User.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@
#include "auth/forward.h"
#include "auth/Type.h"
#include "base/CbcPointer.h"
#include "base/ClpMap.h"
#include "base/RefCount.h"
#include "dlink.h"
#include "ip/Address.h"
#include "Notes.h"
#include "sbuf/Algorithms.h"
#include "sbuf/SBuf.h"

class StoreEntry;
Expand Down Expand Up @@ -49,7 +51,6 @@ class User : public RefCountable
/** the config for this user */
Auth::SchemeConfig *config;
dlink_list proxy_match_cache;
size_t ipcount;
long expiretime;

/// list of key=value pairs the helper produced
Expand All @@ -72,9 +73,13 @@ class User : public RefCountable
virtual int32_t ttl() const = 0;

/* Manage list of IPs using this username */
void clearIp();
void removeIp(Ip::Address);
void addIp(Ip::Address);
void clearIp() { ipList.clear(); }
void removeIp(const Ip::Address &);
void addIp(const Ip::Address &);
/// How many unique IPs this client has been seen at.
/// Count may be limited to the prior authenticate_ip_ttl seconds
/// if more than 1200 IPs are seen.
size_t ipCount() const { return ipList.entries(); }

/// add the Auth::User to the protocol-specific username cache.
virtual void addToNameCache() = 0;
Expand Down Expand Up @@ -117,8 +122,8 @@ class User : public RefCountable
*/
SBuf userKey_;

/** what ip addresses has this user been seen at?, plus a list length cache */
dlink_list ip_list;
/// what IP addresses has this user been seen at?
ClpMap<SBuf, Ip::Address> ipList;
Copy link
Contributor

@rousskov rousskov Sep 8, 2025

Choose a reason for hiding this comment

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

I do not understand what this map key (represented by SBuf type) is. In other words, I do not understand what ipList maps to an IP address. Please clarify.

PR BuildIpKey() code suggests that the key is an IP address (represented by Ip::Address), but that does not help answering the above question and raises another: Why convert Ip::Address to SBuf instead of using Ip::Address itself?

Copy link
Contributor Author

@yadij yadij Sep 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

key is how the map is searched. In this cache it is a representation of the IP (only). The stored value includes details like address family which are not relevant to the map lookup.

In theory we could use struct in6_addr as a cache key (I tried). However the memory management logic of ClpMap need to use some methods of STL container API (SBuf provides those).

Copy link
Contributor

@rousskov rousskov Sep 12, 2025

Choose a reason for hiding this comment

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

key is ... a representation of the IP (only). The stored value includes details like address family which are not relevant to the map lookup.

If this map key is an IP address, we should use Ip::Address instead of SBuf for its type (more on that below).

However, that raises a second question: If the key is an IP address and the value is the same IP address, then why map X to X?! AFAICT, this PR proposes to use a map for what is really a set of unique IP addresses (with LRU-driven expiration and other management features implemented by ClpMap). If so, then we should refactor to clarify this ClpMap abuse and reduce its overheads (unless you want to implement a ClpSet in a prerequisite PR). I can help with that refactoring, but I want to make sure we are on the same page here before we discuss specific code adjustments.

In theory we could use struct in6_addr as a cache key (I tried).

I do not think we should use in6_addr directly because high-level Squid code uses Ip::Address to represent IP addresses. This authentication cache is high-level code (for this discussion context). It is regrettable that Ip::Address contains port information as well, but we can (and should) ignore that extra information in this PR context.

Can Ip::Address be uses as a ClpMap key type already? If not, what APIs do we need to add or adjust?

Copy link
Contributor Author

@yadij yadij Dec 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Can Ip::Address be uses as a ClpMap key type already? If not, what APIs do we need to add or adjust?

Per ClpMap.h documentation:

/// Key must come with std::hash<Key> and std::equal_to<Key> instantiations.
/// Key::length() must return the number of memory bytes in use by the key.
  • essentially any STL container type, or custom types implementing the STL container API.

Copy link
Contributor

@rousskov rousskov Dec 29, 2025

Choose a reason for hiding this comment

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

Please note that the minor questions discussed below are secondary to the primary concern raised in this change request. Again, we need to "make sure that we are on the same page here before we discuss specific code adjustments". Please see "If the key is an IP address and the value is the same IP address, then why map X to X?!" paragraph for more details.

Alex: Can Ip::Address be uses as a ClpMap key type already? If not, what APIs do we need to add or adjust?

/// Key must come with std::hash<Key> and std::equal_to<Key> instantiations.

std::hash<Key> should be easy to provide.

I hope that std::equal_to<Key> can be provided without significant problems as well, despite Ip::Address idiosyncrasies related to address equality, but we will need to tread carefully there. Some work may be required.

/// Key::length() must return the number of memory bytes in use by the key.

I suggest investigating replacing the existing k.length() call with MemoryUsedBy(k) to improve/fix this overly restrictive interface.

};

} // namespace Auth
Expand Down
2 changes: 1 addition & 1 deletion src/auth/UserRequest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ authenticateAuthUserRequestIPCount(Auth::UserRequest::Pointer auth_user_request)
{
assert(auth_user_request != nullptr);
assert(auth_user_request->user() != nullptr);
return auth_user_request->user()->ipcount;
return auth_user_request->user()->ipCount();
}

/*
Expand Down
23 changes: 0 additions & 23 deletions src/auth/UserRequest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,29 +30,6 @@ class HttpRequest;
// XXX: Keep in sync with all others: bzr grep 'define MAX_AUTHTOKEN_LEN'
#define MAX_AUTHTOKEN_LEN 65535

/**
* Node used to link an IP address to some user credentials
* for the max_user_ip ACL feature.
*/
class AuthUserIP
{
MEMPROXY_CLASS(AuthUserIP);

public:
AuthUserIP(const Ip::Address &ip, time_t t) : ipaddr(ip), ip_expiretime(t) {}

dlink_node node;

/// IP address this user authenticated from
Ip::Address ipaddr;

/** When this IP should be forgotten.
* Set to the time of last request made from this
* (user,IP) pair plus authenticate_ip_ttl seconds
*/
time_t ip_expiretime;
};

// TODO: make auth schedule AsyncCalls?
typedef void AUTHCB(void*);

Expand Down
34 changes: 34 additions & 0 deletions src/base/ClpMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,14 @@ class ClpMap
/// Remove the corresponding entry (if any)
void del(const Key &);

/// Merge entries from another ClpMap.
/// * Entries imported from the other list will retain their LRU order
/// and be placed before existing entries.
/// * Entries which have expired will be ignored.
/// * When an key collision occurs the existing entry will be replaced
/// and the new entry given the largest of the two TTL values.
void merge(const ClpMap<Key, Value, MemoryUsedBy> &other);
Copy link
Contributor

@rousskov rousskov Sep 8, 2025

Choose a reason for hiding this comment

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

Please rename this method to importFresh(). There are many reasonable ways to merge two maps, and we should give this specific merging algorithm a more specific name. While renaming, please avoid the term "list" when mentioning this new method argument -- ClpMap is an associative container (i.e. a map) rather than a list.

Copy link
Contributor Author

@yadij yadij Sep 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure that name is any better.

  • import has just as many "reasonable ways to import" variations as merge does, and
  • fresh has implications about the timing information which may be false (already expired/stale, or earlier-than-current expiry time) due to helper delays and parallel transactions.

If you want to be explicit about what operation the merge is doing. How about addOrUpdate() ?

FWIW, I have tried to implement std::list::merge() behaviour with only small changes appropriate for ClpMap LRU sorting and uniqueness.

Copy link
Contributor

@rousskov rousskov Sep 12, 2025

Choose a reason for hiding this comment

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

import has just as many "reasonable ways to import" variations as merge does, and

Yes, and that is why nobody is proposing to name this method import(). mergeFresh() or addFresh() would work.

fresh has implications about the timing information which may be false (already expired/stale, or earlier-than-current expiry time) due to helper delays and parallel transactions.

This code does not add expired entries (from this code point of view), so "fresh" is very appropriate IMO.

I still think importFresh() is better than addOrUpdate() [because it more clearly indicates which map dominates], but addOrUpdate() is better than merge(). Using either of those two names will address this change request.

"Update" clashes with the method description that promises to replace (rather than update) existing same-key entries. Please rephrase that bullet if you decide to go with addOrUpdate().

I have tried to implement std::list::merge() behaviour with only small changes appropriate for ClpMap LRU sorting and uniqueness.

Those "small changes" are actually what differentiates the proposed method from other possible merging methods. That "sorting and uniqueness" is why we use ClpMap instead of std::map (and its merge() method) here. The name should inform the caller which of the two maps dominates as far as LRU replacements are concerned. If we were to call this merge(), then the method would have to truly merge (i.e. interleave as needed to preserve relative use order) LRU indexes instead (and our ClpMap does not have enough information to do that IIRC).


/// Reset the memory capacity for this map, purging if needed
void setMemLimit(uint64_t newLimit);

Expand All @@ -108,6 +116,9 @@ class ClpMap
/// The number of currently stored entries, including expired ones
size_t entries() const { return entries_.size(); }

/// Erases all elements from the container. After this call, entries() returns zero.
void clear();

/// Read-only traversal of all cached entries in LRU order, least recently
/// used entry first. Stored expired entries (if any) are included. Any map
/// modification may invalidate these iterators and their derivatives.
Expand Down Expand Up @@ -162,6 +173,15 @@ ClpMap<Key, Value, MemoryUsedBy>::setMemLimit(const uint64_t newLimit)
memLimit_ = newLimit;
}

template <class Key, class Value, uint64_t MemoryUsedBy(const Value &)>
void
ClpMap<Key, Value, MemoryUsedBy>::clear()
{
index.clear();
entries.clear();
memUsed_ = 0;
}

/// \returns the index position of an entry identified by its key (or end())
template <class Key, class Value, uint64_t MemoryUsedBy(const Value &)>
typename ClpMap<Key, Value, MemoryUsedBy>::IndexIterator
Expand Down Expand Up @@ -272,6 +292,20 @@ ClpMap<Key, Value, MemoryUsedBy>::del(const Key &key)
erase(i);
}

template <class Key, class Value, uint64_t MemoryUsedBy(const Value &)>
void
ClpMap<Key, Value, MemoryUsedBy>::merge(const ClpMap<Key, Value, MemoryUsedBy> &other)
{
// reverse-order across other - to preserve old LRU
for (auto i = other.entries_.rbegin(); i != other.entries_.rend(); ++i) {
if (!i->expired()) {
const auto ours = find(i->key);
const auto newTtl = (ours != index_.cend() ? max(i->expires, ours->second->expires) : i->expires) - squid_curtime;
Copy link
Contributor

@rousskov rousskov Sep 8, 2025

Choose a reason for hiding this comment

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

If this merge() method survives, we should try to refactor so that its code does not convert expiration values to TTL values (here) and then back to the original expiration value (in Entry constructor). In other words, there should be no squid_curtime involvement; just expires manipulation.

add(i->key, i->value, newTtl);
}
}
}

/// purges entries to make free memory large enough to fit wantSpace bytes
template <class Key, class Value, uint64_t MemoryUsedBy(const Value &)>
void
Expand Down
6 changes: 3 additions & 3 deletions src/tests/stub_libauth.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,14 @@ void Auth::Scheme::FreeAll() STUB
void Auth::SchemesConfig::expand() STUB

#include "auth/User.h"
Auth::User::User(Auth::SchemeConfig *, const char *) STUB
Auth::User::User(Auth::SchemeConfig *, const char *) : ipList(0) {STUB}
Auth::CredentialState Auth::User::credentials() const STUB_RETVAL(credentials_state)
void Auth::User::credentials(CredentialState) STUB
void Auth::User::absorb(Auth::User::Pointer) STUB
Auth::User::~User() STUB_NOP
void Auth::User::clearIp() STUB
void Auth::User::removeIp(Ip::Address) STUB
void Auth::User::addIp(Ip::Address) STUB
void Auth::User::removeIp(const Ip::Address &) STUB
void Auth::User::addIp(const Ip::Address &) STUB
void Auth::User::CredentialsCacheStats(StoreEntry *) STUB

#include "auth/UserRequest.h"
Expand Down
Loading