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

External Authentication Caching #5943

Merged
merged 9 commits into from
Aug 27, 2024

Conversation

contificate
Copy link
Contributor

@contificate contificate commented Aug 19, 2024

This change introduces the ability to cache valid results obtained from external authentication services (such as winbind for active directory). Currently, it is a per-host feature. In future, it may be promoted to a pool-wide feature.


The cache is configured by 3 entries in xapi.conf, for example:

enable-external-authentication-cache = true
external-authentication-expiry = 30
external-authentication-cache-size = 10

This configuration would create a cache of capacity 10 and gives each cached entry a TTL of 30 seconds. By default, the cache is disabled. The data structure underlying the cache is a bounded priority search queue: if you attempt to insert a fresh entry when the cache is at capacity, the entry that is soonest to expire is evicted to make space for the new entry.


This has been tested using real AD services internally. Important behaviour such as bruteforce protection on invalid credentials is retained by this feature, as the cache only stores authenticated credentials (and falls back to the "slow path" - querying the external auth plugin - when no valid cache entry exists).



The above interaction shows the authentication, without caching, taking just over a second. Then, subsequent logins taking roughly half a second. The interaction then delays for 15 seconds (the TTL of the cache being used) and shows that it regresses to taking around original amount of time (as it no longer uses the cached result, as it has expired).


The above interaction demonstrates that, even when a valid credential for a user is cached, the presence of the cache won't undermine the default bruteforce protection behaviour of sleeping for roughly 4-5 seconds.

Copy link
Member

@psafont psafont left a comment

Choose a reason for hiding this comment

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

The key for the authentication cache should be Mtime.Span.t instead of Mtime.Span. It removes edge-cases from the code.

ocaml/tests/test_bounded_psq.ml Outdated Show resolved Hide resolved
ocaml/xapi/helpers.ml Outdated Show resolved Hide resolved
ocaml/xapi/xapi_globs.ml Outdated Show resolved Hide resolved
ocaml/xapi/xapi_globs.ml Outdated Show resolved Hide resolved
Factors out logic for transforming custom specifications into standard
specifications in order to permit custom argument specifications to be provided
alongside descriptions. Introduces a new list of options for this purpose.

Signed-off-by: Colin James <[email protected]>
In preparation for memoization, we wrap a region of
"login_with_password" (the part calling out to external authentication code)
within a nested function ("query_external_auth").

We introduce a new record type "external_auth_result" that represents the set
of downward exposed variables that that are bound within the code our nested
function has incorporated. This record becomes the result of
"query_external_auth". Consequently, the function is immediately invoked and
its result is matched upon to bring all of the fields into scope. The future
memoization code will treat a call to this function as the "slow path".

Signed-off-by: Colin James <[email protected]>
Implements a wrapper for Psq that provides maximum capacity behaviour.

Signed-off-by: Colin James <[email protected]>
Adds basic functional testing for a bounded priority search queue.

Signed-off-by: Colin James <[email protected]>
Provides a simple authentication cache built using a bounded priority search
queue.

The structure of the cache is effectively a (bounded) priority search queue,
mapping usernames to entries - where entries have an explicit time-to-live and
can store arbitrary data (optionally secured with a cryptographic hash).

Also provides a configurable expiry span in the form of a global in Xapi_globs,
"external_authentication_expiry". This time span is added to the current
time - upon entering data into the cache - to provide an expiration time. Its
default value is 5 minutes. The input is specified in seconds.

The underlying priority search queue treats the minimum entry as being the
entry with highest priority. Therefore, as entries are compared by their
expiration time, the entry that is soonest to expire is the one that is evicted
when attempting to add a new entry when the cache is at capacity.

Signed-off-by: Colin James <[email protected]>
@liulinC
Copy link
Collaborator

liulinC commented Aug 21, 2024

@contificate Given this is a big update, could you please run XenRT suite: Authentication Sanity Test before merge.
Note: There are some failures due to XenRT itself, you can ignore such cases for now.

ocaml/xapi/xapi_session.ml Outdated Show resolved Hide resolved
ocaml/xapi/xapi_globs.ml Outdated Show resolved Hide resolved
ocaml/xapi/xapi_globs.ml Outdated Show resolved Hide resolved
Copy link
Member

@psafont psafont left a comment

Choose a reason for hiding this comment

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

Only some minor nits and comments remain. The general approach looks generally good.

@contificate
Copy link
Contributor Author

@contificate Given this is a big update, could you please run XenRT suite: Authentication Sanity Test before merge. Note: There are some failures due to XenRT itself, you can ignore such cases for now.

I've started suite run (203318). Not sure how long it will take to give any useful results.

@lindig
Copy link
Contributor

lindig commented Aug 21, 2024

From a user's perspective,

external-authentication-expiry-ms = 30000

do we need millisecond precision here? And would it not be better to use a float of seconds - which could still provide this precision if needed?

@contificate
Copy link
Contributor Author

From a user's perspective,

external-authentication-expiry-ms = 30000

do we need millisecond precision here? And would it not be better to use a float of seconds - which could still provide this precision if needed?

I ought to fix the PR's description but, as of recent changes, it's now provided as seconds (with sub-second precision possible, as I believe ShortDurationFromSeconds accepts a float).


type secret = string

type t = digest * salt * secret
Copy link
Contributor

Choose a reason for hiding this comment

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

These are all strings - so easy to confuse. Would a record help to avoid that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I avoided this for stylistic reasons but it could be done that way, in principle.

val create : digest -> salt -> secret -> t
val read : t -> digest * salt * secret

I wanted it to be the case that these injection and projection functions that Secrets are forced to provide evidently don't talk about the key (as that should only be used ephemerally). The users of Secret have t as opaque, so one could use whatever they want for that, provided they provide suitable implementations of the above functions. OCaml lacks anonymous record types (as SML has), which would permit a nice pattern like:

val read : t -> { digest: digest; salt: salt; secret: secret; }

and then order-independent projections at usage sites using record patterns (as you suggest), whilst keeping the components transparent in the signature.


All that said, I could define a record type in the signature and use it there, but then it'd be val read : t -> components and marginally less evident. If people are keen on that, I'd be happy to do that change.

Copy link
Member

@robhoes robhoes left a comment

Choose a reason for hiding this comment

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

This is a great example of how to build up a relatively complex change (especially in a sensitive area of the code) in a well-documented set of commits, which is easy to follow and verify. Thanks!

@minglumlu
Copy link
Member

Yeah. And thanks for for this excellent real world example of OCaml modules and functors. I learned a lot from the reviewing.

@contificate
Copy link
Contributor Author

@contificate Given this is a big update, could you please run XenRT suite: Authentication Sanity Test before merge. Note: There are some failures due to XenRT itself, you can ignore such cases for now.

I've started suite run (203318). Not sure how long it will take to give any useful results.

The job run looks pretty good. All green except one blocked and another a known issue (to my understanding). The job was run with the cache being enabled (hardcoded). A later XenRT test is in discussions to exercise more extreme conditions.

- Adds mirage-crypto-rng and mirage-crypto-rng.unix libraries to xapi's dune
  file. These are used to provide random number generation used for generating
  salts.
- Introduces a cache implementation that secures authenticated sessions
  (results) using SHA-512 hashes (computed using libcrypt's crypt_r function).

Signed-off-by: Colin James <[email protected]>
Adds configurable options "enable-external-authentication-cache" and
"external-authentication-cache-size" to Xapi_globs, which control whether the
cache is enabled and the maximum capacity of the cache, respectively.

Also adds "external-authentication-expiry" option, which permits
configuration of the time-to-live assigned to each cache entry. The global that
it manipulates in Xapi_globs was introduced earlier.

In future, these configuration options may become a pool-wide feature.

Signed-off-by: Colin James <[email protected]>
This change provides the workings required to allow the previously functionalised
section of login_with_password to be bypassed by consulting an authentication
cache.

Before attempting to execute that section of login_with_password (which calls
out to external authentication plugins), a cache is consulted. If a
previously-computed result resides within the cache, that is provided. If there
is no such result present in the cache, or external authentication caching is
disabled, the previous section of code (the "slow path") is invoked.

Signed-off-by: Colin James <[email protected]>
@contificate
Copy link
Contributor Author

Reintroduced dependency upon mirage-crypto-rng.unix, in order to initialise the default generator - for usage in creating salts, rather than maintaining our own generator locally.

@contificate contificate merged commit 6b2d4c7 into xapi-project:master Aug 27, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants