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::vectorstd::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
Re: [PATCH] OAuth 2.0 Bearer authentication
On 08/09/2014 04:19 AM, Amos Jeffries wrote: 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. It may be acceptable to reuse the old caching code that uses periodic cleanup. However, adding brand new caching classes with periodic cleanup is a rather different matter. An it is a common wrong approach excuse cannot be successfully applied to a completely new cache class IMO. In the proposed patch, the caching with periodic cleanup is done by TokenCache, which is a new type with various management code surrounding it (essentially a new class for the purpose if this discussion). +// 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. Why cannot this code use the existing LruMap class from the start? Yes, LruMap requires some adjustments to handle more class users, but what is worse: Making those adjustments or committing code that introduces a new wrong class? The consistency reason hardly applies to a new cache class. Yes, some existing caches use time-based capacity, but some do not. Why pick the wrong model for the new stuff? The memory calculation is also not as simple as it seems. I did not say it is simple. I said it is fairly straightforward, which is at least a notch above simple :-). I think it reflects the complexity of those changes well. 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 Sounds straightforward to me. Note that: 1) store_ is correctly private and SBuf::length() is not sufficient to account for most of the allocated MemBlob capacity being consumed. I do not understand why exposing the underlying storage capacity information for this purpose is a big deal. 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. Sure, but we do not need a precise number. We only need a good estimate. And if experience shows that using shared MemBlob capacity is not good enough, we can always cow the tokens stored to make sure they own their buffer exclusively. Please note that the cache is responsible only for what it uses. It is not responsible for the memory used by others, even if it is the same memory. If a MemBlob is shared by a cache entry and something else in Squid, removing that cache entry is sufficient as far as cache obligations and accounting are concerned. 3) the whole calculation is hard-coded at compile time in LruMap. So we cannot call the SBuf size methods anyway. I do not understand why making the LRU entry size variable for this purpose is a big deal. 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). That bug is one of the motivations to do the right thing here. I do not see why we should repeat a similar mistake twice. Let's learn from our mistakes. And making LruMap more flexible would actually help with fixing the certificate storage bug as well (although the problem with that bug is far more difficult than adding variable entry size to LruMap). +/* + * 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 =
Re: [PATCH] OAuth 2.0 Bearer authentication
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.
+// 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?
+/*
+ * 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.
+ For Basic and Bearer the default is Squid proxy-caching web server.
Please add a comma after Bearer.
Thank you,
Alex.
(**) I hope we do not need to debate that rule of thumb, but I am ready
to justify it if needed.
[PATCH] OAuth 2.0 Bearer authentication
RFC 6750 OAuth 2.0 Authorization Framework: Bearer Token Usage
The attached patch adds a minimal implementation of Bearer
authentication scheme to Squid. It consists of three components:
1) Squid build system infrastructure for building Bearer authentication
2) A testing fake-auth helper (bearer_fake_auth).
Helper which takes Bearer helper input an always returns OK.
3) Bearer authentication library (module) for Squid.
* implements the logics for squid.conf Bearer auth_param scheme and
necessary configuration options.
* implements the helper management and API for Bearer helpers.
* implements logics for www-auth and proxy-auth header parsing and
generating.
At present no restriction between HTTP and HTTPS is defined by Squid.
Challenges will be made for both. It is left to the client to ensure
adequate security on the connection it sends Bearer tokens.
* implements helper driven TTL for token caching.
Due to significant security risks with Bearer tokens the TTL is not
configurable from squid.conf. Instead the helper is expected to provide
a ttl= parameter from the auth backend explicitly determining the time
in seconds for which each response may be cached and re-used. In absence
of ttl= value the helper response is treated as already expired (a nonce).
A garbage collection TTL cleanup_interval is configurable and removes
cache entries which have been stale for at least 1 hr.
* uses a default token scope of proxy:HTTP for generic HTTP proxies
NOTES:
* At present no web browsers implement Bearer authentication in
response to a proxy-authenticate challenge.
- However some of the common browsers should support Bearer
authentication with reverse proxies over HTTPS (Firefox and IE
apparently, not Chrome).
- command line tools and AJAX / XHR implementations which allow header
customisation can be scripted to support Bearer.
* This is only a minimal implementation, emitting only the realm= and
scope= parameters to clients.
- The key_extras mechanism can be used to pass extension client request
parameters to the Bearer helper.
- Extension parameters in Squid responses is not supported.
* Bearer authentication to cache_peers is not supported explicitly.
- implicit support exists with login=PASSTHRU, which may be used to
relay Bearer tokens for SSO to multiple proxies.
Amos
=== modified file 'configure.ac'
--- configure.ac2014-07-13 08:49:42 +
+++ configure.ac2014-07-30 23:59:58 +
@@ -1765,40 +1765,53 @@
SQUID_YESNO([$enableval],
[unrecognized argument to --enable-auth: $enableval])
])
AC_MSG_NOTICE([Authentication support enabled: ${enable_auth:=yes}])
SQUID_DEFINE_BOOL(USE_AUTH,$enable_auth,[Enable support for authentication])
AM_CONDITIONAL(ENABLE_AUTH, test x$enable_auth != xno)
AUTH_MODULES=
AC_ARG_ENABLE(auth-basic,
AS_HELP_STRING([--enable-auth-basic=list of helpers],
[Enable the basic authentication scheme, and build the specified helpers.
Not providing an explicit list of helpers will attempt build of
all possible helpers. Default is to do so.
To disable the basic authentication scheme, use --disable-auth-basic.
To enable but build no helpers, specify none.
To see available helpers, see the helpers/basic_auth directory. ]),[
#nothing to do really
])
m4_include([helpers/basic_auth/modules.m4])
+AC_ARG_ENABLE(auth-bearer,
+ AS_HELP_STRING([--enable-auth-bearer=list of helpers],
+ [Enable the OAuth 2.0 Bearer authentication scheme, and build the
+ specified helpers.
+ Not providing an explicit list of helpers will attempt build of
+ all possible helpers. Default is to do so.
+ To disable the Bearer authentication scheme, use --disable-auth-bearer.
+ To enable but build no helpers, specify none.
+ To see available helpers, see the helpers/bearer_auth directory. ]),[
+#nothing to do really
+])
+m4_include([helpers/bearer_auth/modules.m4])
+
AC_ARG_ENABLE(auth-ntlm,
AS_HELP_STRING([--enable-auth-ntlm=list of helpers],
[Enable the NTLM authentication scheme, and build the specified helpers.
Not providing an explicit list of helpers will attempt build of
all possible helpers. Default is to do so.
To disable the NTLM authentication scheme, use --disable-auth-ntlm.
To enable but build no helpers, specify none.
To see available helpers, see the helpers/ntlm_auth directory. ]),[
])
m4_include([helpers/ntlm_auth/modules.m4])
AC_ARG_ENABLE(auth-negotiate,
AS_HELP_STRING([--enable-auth-negotiate=list of helpers],
[Enable the Negotiate authentication scheme, and build the specified
helpers.
Not providing an explicit list of helpers will attempt build of
all possible helpers. Default is to do so.
To disable the Negotiate authentication scheme,
use --disable-auth-negotiate.
To enable but build no helpers, specify none.
@@ -3446,40 +3459,41 @@
