Skip to content

CA-414418: Sessions do not got destroyed after domain user been removed and performance improvement #6609

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

Merged
merged 3 commits into from
Aug 2, 2025
Merged

CA-414418: Sessions do not got destroyed after domain user been removed and performance improvement #6609

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b0e2b5b
CA-414418: Detection of AD account removal does not cause logout
liulinC Jul 31, 2025
92377bf
CA-414418: Perf: save user validate result and apply to sessions
liulinC Jul 31, 2025
2778096
CA-414418: Code refine for comments
liulinC Aug 1, 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
98 changes: 56 additions & 42 deletions ocaml/xapi/xapi_session.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ module Listext = Xapi_stdext_std.Listext
open Client
open Auth_signature
open Extauth
module SessionValidateMap = Map.Make (String)

module AuthFail : sig
(* stats are reset each time you query, so if there hasn't
Expand Down Expand Up @@ -420,7 +421,7 @@ let destroy_db_session ~__context ~self =
(* CP-703: ensure that activate sessions are invalidated in a bounded time *)
(* in response to external authentication/directory services updates, such as *)
(* e.g. group membership changes, or even account disabled *)
let revalidate_external_session ~__context ~session =
let revalidate_external_session ~__context acc session =
Context.with_tracing ~__context __FUNCTION__ @@ fun __context ->
try
(* guard: we only want to revalidate external sessions, where is_local_superuser is false *)
Expand All @@ -430,8 +431,7 @@ let revalidate_external_session ~__context ~session =
(Db.Session.get_is_local_superuser ~__context ~self:session
|| Xapi_database.Db_backend.is_session_registered (Ref.string_of session)
)
then (
(* 1. is the external authentication disabled in the pool? *)
then (* 1. is the external authentication disabled in the pool? *)
let master = Helpers.get_master ~__context in
let auth_type = Db.Host.get_external_auth_type ~__context ~self:master in
if auth_type = "" then (
Expand All @@ -442,53 +442,63 @@ let revalidate_external_session ~__context ~session =
(trackid session)
in
debug "%s" msg ;
destroy_db_session ~__context ~self:session
destroy_db_session ~__context ~self:session ;
acc
) else
(* otherwise, we try to revalidate it against the external authentication service *)
let session_lifespan = 60.0 *. 30.0 in
(* allowed session lifespan = 30 minutes *)
let random_lifespan = Random.float 60.0 *. 10.0 in

(* extra random (up to 10min) lifespan to spread access to external directory *)

(* 2. has the external session expired/does it need revalidation? *)
let session_last_validation_time =
Date.to_unix_time
(Db.Session.get_validation_time ~__context ~self:session)
in
let now = Date.now () in
let session_needs_revalidation =
let session_timed_out =
Date.to_unix_time now
> session_last_validation_time +. session_lifespan +. random_lifespan
in
if session_needs_revalidation then (

(* extra random (up to 10min) lifespan to spread access to external directory *)
let authenticated_user_sid =
Db.Session.get_auth_user_sid ~__context ~self:session
in
let validate_with_memo acc f =
match SessionValidateMap.find_opt authenticated_user_sid acc with
| None ->
f acc
| Some false ->
debug "Destory session %s as previous check for user %s not pass"
(trackid session) authenticated_user_sid ;
destroy_db_session ~__context ~self:session ;
acc
| Some true ->
debug "Skip check session %s as previous check for user %s pass"
(trackid session) authenticated_user_sid ;
acc
in

if session_timed_out then (
(* if so, then:*)
validate_with_memo acc @@ fun acc ->
debug "session %s needs revalidation" (trackid session) ;
let authenticated_user_sid =
Db.Session.get_auth_user_sid ~__context ~self:session
in

(* 2a. revalidate external authentication *)

(* CP-827: if the user was suspended (disabled,expired,locked-out), then we must destroy the session *)
let suspended, _ =
is_subject_suspended ~__context ~cache:true authenticated_user_sid
in
let suspended =
if suspended then
is_subject_suspended ~__context ~cache:false
authenticated_user_sid
|> fst
else
suspended
is_subject_suspended ~__context ~cache:false authenticated_user_sid
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this solves the original bug?
AFAICT the initial query with 'cache:true' found that the account is missing (suspended), and the 2nd query with 'cache:false' said that it is not suspended (so I was guessing that winbind might also be caching something), but I may have misinterpreted the logs.

Copy link
Collaborator Author

@liulinC liulinC Jul 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

No, the initial query with cache:true will always succeed and found the account not suspended.
(As xapi db update thread never update it due to NOT_FOUND exception)
The update db thread can not handle it, refer to my comments in the CA ticket.
And I verified it can resolve the issue.

in
if suspended then (
debug
"Subject (identifier %s) has been suspended, destroying session \
%s"
authenticated_user_sid (trackid session) ;
(* we must destroy the session in this case *)
destroy_db_session ~__context ~self:session
destroy_db_session ~__context ~self:session ;
SessionValidateMap.add authenticated_user_sid false acc
) else
try
(* if the user is not in the external directory service anymore, this call raises Not_found *)
Expand Down Expand Up @@ -525,7 +535,8 @@ let revalidate_external_session ~__context ~session =
in
debug "%s" msg ;
(* we must destroy the session in this case *)
destroy_db_session ~__context ~self:session
destroy_db_session ~__context ~self:session ;
SessionValidateMap.add authenticated_user_sid false acc
) else (
(* non-empty intersection: externally-authenticated subject still has login rights in the pool *)

Expand All @@ -552,7 +563,9 @@ let revalidate_external_session ~__context ~session =
~value:subject_in_intersection ;
debug "updated subject for session %s, sid %s "
(trackid session) authenticated_user_sid
)
) ;
debug "end revalidation of session %s " (trackid session) ;
SessionValidateMap.add authenticated_user_sid true acc
with Not_found ->
(* subject ref for intersection's sid does not exist in our metadata!!! *)
(* this should never happen, it's an internal metadata inconsistency between steps 2b and 2c *)
Expand All @@ -564,7 +577,8 @@ let revalidate_external_session ~__context ~session =
in
debug "%s" msg ;
(* we must destroy the session in this case *)
destroy_db_session ~__context ~self:session
destroy_db_session ~__context ~self:session ;
SessionValidateMap.add authenticated_user_sid false acc
)
with Auth_signature.Subject_cannot_be_resolved | Not_found ->
(* user was not found in external directory in order to obtain group membership *)
Expand All @@ -577,15 +591,18 @@ let revalidate_external_session ~__context ~session =
in
debug "%s" msg ;
(* user is not in the external directory anymore: we must destroy the session in this case *)
destroy_db_session ~__context ~self:session
) ;
debug "end revalidation of session %s " (trackid session)
)
destroy_db_session ~__context ~self:session ;
SessionValidateMap.add authenticated_user_sid false acc
) else
acc
else
acc
with e ->
(*unexpected exception: we absorb it and print out a debug line *)
debug "Unexpected exception while revalidating session %s: %s"
(trackid session)
(ExnHelper.string_of_exn e)
(ExnHelper.string_of_exn e) ;
acc

(* CP-703: ensure that activate sessions are invalidated in a bounded time *)
(* in response to external authentication/directory services updates, such as *)
Expand All @@ -595,21 +612,18 @@ let revalidate_all_sessions ~__context =
try
debug "revalidating all external sessions in the local host" ;
(* obtain all sessions in the pool *)
let sessions = Db.Session.get_all ~__context in
Db.Session.get_all ~__context
(* filter out those sessions where is_local_superuser or client_certificate is true *)
(* we only want to revalidate the sessions created using the external authentication service *)
let external_sessions =
List.filter
(fun session ->
(not (Db.Session.get_is_local_superuser ~__context ~self:session))
&& not (Db.Session.get_client_certificate ~__context ~self:session)
)
sessions
in
(* revalidate each external session *)
List.iter
(fun session -> revalidate_external_session ~__context ~session)
external_sessions
|> List.filter (fun session ->
(not (Db.Session.get_is_local_superuser ~__context ~self:session))
&& not (Db.Session.get_client_certificate ~__context ~self:session)
)
|> (* revalidate each external session *)
List.fold_left
(revalidate_external_session ~__context)
SessionValidateMap.empty
|> ignore
with e ->
(*unexpected exception: we absorb it and print out a debug line *)
debug "Unexpected exception while revalidating external sessions: %s"
Expand Down
Loading