Re: [PATCH] OAuth 2.0 Bearer authentication

On 5/08/2014 3:22 a.m., Alex Rousskov wrote:
> On 07/31/2014 03:29 AM, Amos Jeffries wrote:
>
>> A garbage collection TTL "cleanup_interval" is configurable and removes
>> cache entries which have been stale for at least 1 hr.
>
> While some old code still uses periodic cleanup, I think we should avoid
> adding more code like that. Periodic cleanup leads to cache overflows,
> memory usage surprises, and hot idle. Please remove old unneeded cache
> entries when adding a new cache entry instead of checking the cache
> state every hour.

I agree. However this is a common algorithm for all of Squid
authentication types. Updating it should be done as a separate action
and cover more than just this auth scheme. In particular the core cache
code is shared by Basic and Digest.

>> + // only clear tokens out of cache after 1 hour stale
>> + // could make this configurable
>> + time_t stalenessLimit = current_time.tv_sec - 60*60;
>
> Cache size should be(**) regulated in terms of memory usage (bytes), not
> entry age (seconds). I believe estimating memory footprint of a cache
> entry would be fairly straightforward in this case. Would you be willing
> to convert the code to use size limits?

I agree with the idea. If/when we can move to LruMap or equivalent that
will happen. For now this is consistent with the other auth schemes, and
sufficient as a temporary state.

The memory calculation is also not as simple as it seems. The SBuf
content consists of the majority of memory usage and is dynamic at
run-time. What is actually needed to account the memory size of a
TokenCache node is:

 sizeof(TokenPointer)
  +
 sizeof(Token)
  +
 (*X)->b64encoded.store_->capacity

Note that:
 1) store_ is correctly private and SBuf::length() is not sufficient to
account for most of the allocated MemBlob capacity being consumed.
  The Token may be holding 64KB I/O buffer MemBlob for access to 10
bytes of token string.

 2) the store_->capacity may be shared by Token, so freeing just this
entry may not reduce memory usage by amount it "uses". Accounting for it
may have caused the cache size to be over-estimated.

 3) the whole calculation is hard-coded at compile time in LruMap. So we
cannot call the SBuf size methods anyway.

I notice that the current use of LruMap for storing
CertValidationResponse objects is also suffering from this problem. It
fails to account for the data size used by a whole vector of error
strings, and all of a X509 certificate memory allocation (we know those
are huge with lots of linked details).
  Instead it assumes the cert is sizeof(X509*) and the string content is
sizeof(std::vector<std::string>) which is a KB or more away from reality
per-entry.

>> + /*
>> + * For big caches we could consider stepping through
>> + * 100/200 entries at a time. Lets see how it flies
>> + * first.
>> + */
>> + Auth::Bearer::TokenCache *cache = &Auth::Bearer::Token::Cache;
>> + for (Auth::Bearer::TokenCache::iterator itr = cache->begin(); itr != cache->end();) {
>
> I do not think we should do linear search like that. It is going to be
> too slow for large caches and adding arbitrary "maximum number of
> iterations" limits just adds more tuning headaches. Why not use an LRU
> cache instead? IIRC, we have even added LruMap class for that recently.
>

No less efficient than the linear walk the LruMap does. A full scan is
required for the time-based removal since entries are mapped by their
key value not by expiry time.

LruMap also uses raw pointers for storing its entries. These Token
objects are ref-counted and if we store as raw pointers they will either
leak or be freed while still linked by the cache.
 The only way around that seems to be storing Pointers to the list and
making every access a double de-reference. Or re-implementing the entire
LruMap template using Pointer and a dynamic size() calculation.

The existing cache model used by both Basic and Digest already is
sufficient for the needs of this feature. Note that the periodic cleanup
is an optimization for large caches, to reduce the impact of this linear
search for stale objects instead of

>
>> + For Basic and Bearer the default is "Squid proxy-caching web server".
>
> Please add a comma after Bearer.

Done.

Amos
Received on Sat Aug 09 2014 - 10:19:54 MDT