Skip to content

Commit 5313809

Browse files
authored
CA-414418: Sessions do not got destroyed after domain user been removed and performance improvement (#6609)
``` commit 92377bf (HEAD -> private/linl/CA-414418, origin/private/linl/CA-414418) Author: Lin Liu <[email protected]> Date: Thu Jul 31 07:25:30 2025 +0000 CA-414418: Perf: save user validate result and apply to sessions For all sessions created by external/AD users, session revalidate will check whether the users are still acitve, and kick off the session accordingly. However, xapi check the user for every session. The problem here is lots of session are created by only a few users. (for the case of CVAD and ControlUP). This would cause lot of duplicated check for the same user again and again, which is slow and waste lots of resources. To fix the issue, [(user_sid, check_result)] is defined for every round of check. The check result is saved so later check for the session with same user can just be applied. Signed-off-by: Lin Liu <[email protected]> commit b0e2b5b Author: Lin Liu <[email protected]> Date: Thu Jul 31 06:17:32 2025 +0000 CA-414418: Detection of AD account removal does not cause logout For performance, during revalidate existing sessions, xapi query subject details from xapi db first, if the subject is suspend, then goes to AD, to make sure unblocked user can login. There is a backend thread to update xapi db subject information from AD. However, it can not handle the case that the subject is removed. (and should not remove the subject for user until user remove it explictly). Thus, the subject information is not updated and keep alive. In this case, subject revalidate always got session not suspend from xapi db. The issue is fixed by query subject information from AD direclty, and session revalidate thread handle the removed subject properly to kick off the sessions For performance, there is a follow up commit to resovle that Signed-off-by: Lin Liu <[email protected]> ```
2 parents 09953c3 + 2778096 commit 5313809

File tree

1 file changed

+56
-42
lines changed

1 file changed

+56
-42
lines changed

ocaml/xapi/xapi_session.ml

Lines changed: 56 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ module Listext = Xapi_stdext_std.Listext
3232
open Client
3333
open Auth_signature
3434
open Extauth
35+
module SessionValidateMap = Map.Make (String)
3536

3637
module AuthFail : sig
3738
(* stats are reset each time you query, so if there hasn't
@@ -420,7 +421,7 @@ let destroy_db_session ~__context ~self =
420421
(* CP-703: ensure that activate sessions are invalidated in a bounded time *)
421422
(* in response to external authentication/directory services updates, such as *)
422423
(* e.g. group membership changes, or even account disabled *)
423-
let revalidate_external_session ~__context ~session =
424+
let revalidate_external_session ~__context acc session =
424425
Context.with_tracing ~__context __FUNCTION__ @@ fun __context ->
425426
try
426427
(* guard: we only want to revalidate external sessions, where is_local_superuser is false *)
@@ -430,8 +431,7 @@ let revalidate_external_session ~__context ~session =
430431
(Db.Session.get_is_local_superuser ~__context ~self:session
431432
|| Xapi_database.Db_backend.is_session_registered (Ref.string_of session)
432433
)
433-
then (
434-
(* 1. is the external authentication disabled in the pool? *)
434+
then (* 1. is the external authentication disabled in the pool? *)
435435
let master = Helpers.get_master ~__context in
436436
let auth_type = Db.Host.get_external_auth_type ~__context ~self:master in
437437
if auth_type = "" then (
@@ -442,53 +442,63 @@ let revalidate_external_session ~__context ~session =
442442
(trackid session)
443443
in
444444
debug "%s" msg ;
445-
destroy_db_session ~__context ~self:session
445+
destroy_db_session ~__context ~self:session ;
446+
acc
446447
) else
447448
(* otherwise, we try to revalidate it against the external authentication service *)
448449
let session_lifespan = 60.0 *. 30.0 in
449450
(* allowed session lifespan = 30 minutes *)
450451
let random_lifespan = Random.float 60.0 *. 10.0 in
451452

452-
(* extra random (up to 10min) lifespan to spread access to external directory *)
453-
454453
(* 2. has the external session expired/does it need revalidation? *)
455454
let session_last_validation_time =
456455
Date.to_unix_time
457456
(Db.Session.get_validation_time ~__context ~self:session)
458457
in
459458
let now = Date.now () in
460-
let session_needs_revalidation =
459+
let session_timed_out =
461460
Date.to_unix_time now
462461
> session_last_validation_time +. session_lifespan +. random_lifespan
463462
in
464-
if session_needs_revalidation then (
463+
464+
(* extra random (up to 10min) lifespan to spread access to external directory *)
465+
let authenticated_user_sid =
466+
Db.Session.get_auth_user_sid ~__context ~self:session
467+
in
468+
let validate_with_memo acc f =
469+
match SessionValidateMap.find_opt authenticated_user_sid acc with
470+
| None ->
471+
f acc
472+
| Some false ->
473+
debug "Destory session %s as previous check for user %s not pass"
474+
(trackid session) authenticated_user_sid ;
475+
destroy_db_session ~__context ~self:session ;
476+
acc
477+
| Some true ->
478+
debug "Skip check session %s as previous check for user %s pass"
479+
(trackid session) authenticated_user_sid ;
480+
acc
481+
in
482+
483+
if session_timed_out then (
465484
(* if so, then:*)
485+
validate_with_memo acc @@ fun acc ->
466486
debug "session %s needs revalidation" (trackid session) ;
467-
let authenticated_user_sid =
468-
Db.Session.get_auth_user_sid ~__context ~self:session
469-
in
470487

471488
(* 2a. revalidate external authentication *)
472489

473490
(* CP-827: if the user was suspended (disabled,expired,locked-out), then we must destroy the session *)
474491
let suspended, _ =
475-
is_subject_suspended ~__context ~cache:true authenticated_user_sid
476-
in
477-
let suspended =
478-
if suspended then
479-
is_subject_suspended ~__context ~cache:false
480-
authenticated_user_sid
481-
|> fst
482-
else
483-
suspended
492+
is_subject_suspended ~__context ~cache:false authenticated_user_sid
484493
in
485494
if suspended then (
486495
debug
487496
"Subject (identifier %s) has been suspended, destroying session \
488497
%s"
489498
authenticated_user_sid (trackid session) ;
490499
(* we must destroy the session in this case *)
491-
destroy_db_session ~__context ~self:session
500+
destroy_db_session ~__context ~self:session ;
501+
SessionValidateMap.add authenticated_user_sid false acc
492502
) else
493503
try
494504
(* if the user is not in the external directory service anymore, this call raises Not_found *)
@@ -525,7 +535,8 @@ let revalidate_external_session ~__context ~session =
525535
in
526536
debug "%s" msg ;
527537
(* we must destroy the session in this case *)
528-
destroy_db_session ~__context ~self:session
538+
destroy_db_session ~__context ~self:session ;
539+
SessionValidateMap.add authenticated_user_sid false acc
529540
) else (
530541
(* non-empty intersection: externally-authenticated subject still has login rights in the pool *)
531542

@@ -552,7 +563,9 @@ let revalidate_external_session ~__context ~session =
552563
~value:subject_in_intersection ;
553564
debug "updated subject for session %s, sid %s "
554565
(trackid session) authenticated_user_sid
555-
)
566+
) ;
567+
debug "end revalidation of session %s " (trackid session) ;
568+
SessionValidateMap.add authenticated_user_sid true acc
556569
with Not_found ->
557570
(* subject ref for intersection's sid does not exist in our metadata!!! *)
558571
(* this should never happen, it's an internal metadata inconsistency between steps 2b and 2c *)
@@ -564,7 +577,8 @@ let revalidate_external_session ~__context ~session =
564577
in
565578
debug "%s" msg ;
566579
(* we must destroy the session in this case *)
567-
destroy_db_session ~__context ~self:session
580+
destroy_db_session ~__context ~self:session ;
581+
SessionValidateMap.add authenticated_user_sid false acc
568582
)
569583
with Auth_signature.Subject_cannot_be_resolved | Not_found ->
570584
(* user was not found in external directory in order to obtain group membership *)
@@ -577,15 +591,18 @@ let revalidate_external_session ~__context ~session =
577591
in
578592
debug "%s" msg ;
579593
(* user is not in the external directory anymore: we must destroy the session in this case *)
580-
destroy_db_session ~__context ~self:session
581-
) ;
582-
debug "end revalidation of session %s " (trackid session)
583-
)
594+
destroy_db_session ~__context ~self:session ;
595+
SessionValidateMap.add authenticated_user_sid false acc
596+
) else
597+
acc
598+
else
599+
acc
584600
with e ->
585601
(*unexpected exception: we absorb it and print out a debug line *)
586602
debug "Unexpected exception while revalidating session %s: %s"
587603
(trackid session)
588-
(ExnHelper.string_of_exn e)
604+
(ExnHelper.string_of_exn e) ;
605+
acc
589606

590607
(* CP-703: ensure that activate sessions are invalidated in a bounded time *)
591608
(* in response to external authentication/directory services updates, such as *)
@@ -595,21 +612,18 @@ let revalidate_all_sessions ~__context =
595612
try
596613
debug "revalidating all external sessions in the local host" ;
597614
(* obtain all sessions in the pool *)
598-
let sessions = Db.Session.get_all ~__context in
615+
Db.Session.get_all ~__context
599616
(* filter out those sessions where is_local_superuser or client_certificate is true *)
600617
(* we only want to revalidate the sessions created using the external authentication service *)
601-
let external_sessions =
602-
List.filter
603-
(fun session ->
604-
(not (Db.Session.get_is_local_superuser ~__context ~self:session))
605-
&& not (Db.Session.get_client_certificate ~__context ~self:session)
606-
)
607-
sessions
608-
in
609-
(* revalidate each external session *)
610-
List.iter
611-
(fun session -> revalidate_external_session ~__context ~session)
612-
external_sessions
618+
|> List.filter (fun session ->
619+
(not (Db.Session.get_is_local_superuser ~__context ~self:session))
620+
&& not (Db.Session.get_client_certificate ~__context ~self:session)
621+
)
622+
|> (* revalidate each external session *)
623+
List.fold_left
624+
(revalidate_external_session ~__context)
625+
SessionValidateMap.empty
626+
|> ignore
613627
with e ->
614628
(*unexpected exception: we absorb it and print out a debug line *)
615629
debug "Unexpected exception while revalidating external sessions: %s"

0 commit comments

Comments
 (0)