CA-199656: Fix race in registering sessions with alternate databases

ocaml/database/db_backend.ml

@@ -29,6 +29,8 @@ let display_sql_writelog_val = ref true (* compute/write sql-writelog debug stri
 
 let master_database = ref (Db_cache_types.Database.make Schema.empty)
 
+let __test_set_master_database db = master_database := db
+
 let make () = Db_ref.in_memory (ref master_database)
 
 (* !!! Right now this is called at cache population time. It would probably be preferable to call it on flush time instead, so we
@@ -69,9 +71,12 @@ let blow_away_non_persistent_fields (schema: Schema.t) db =
 let db_registration_mutex = Mutex.create ()
 let foreign_databases: ((API.ref_session, Db_ref.t) Hashtbl.t) = Hashtbl.create 5
 
-let register_session_with_database session db_ref =
+let create_registered_session create_session db_ref =
 	Mutex.execute db_registration_mutex
-		(fun () -> Hashtbl.replace foreign_databases session db_ref)
+		(fun () ->
+			let session = create_session () in
+			Hashtbl.replace foreign_databases session db_ref;
+			session)
 
 let unregister_session session =
 	Mutex.execute db_registration_mutex

ocaml/database/db_backend.mli

@@ -0,0 +1,32 @@
+(*
+ * Copyright (C) Citrix Systems Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *)
+
+val db_FLUSH_TIMER : float
+
+(** A setter for the master database. Only used by the unit testing code. *)
+val __test_set_master_database : Db_cache_types.Database.t -> unit
+
+val make : unit -> Db_ref.t
+
+val blow_away_non_persistent_fields :
+	Schema.t -> Db_cache_types.Database.t -> Db_cache_types.Database.t
+
+val create_registered_session :
+	(unit -> API.ref_session) -> Db_ref.t -> API.ref_session
+
+val unregister_session : API.ref_session -> unit
+
+val is_session_registered : API.ref_session -> bool
+
+val get_registered_database : API.ref_session -> Db_ref.t option

ocaml/test/mock.ml

@@ -25,7 +25,8 @@ module Database = struct
 	;;
 
 	let make_global ~conn ~reuse () =
-		Db_backend.master_database := Db_cache_types.Database.make Schema.empty;
+		Db_backend.__test_set_master_database
+			(Db_cache_types.Database.make Schema.empty);
 		let db = Db_backend.make () in
 		Db_cache_impl.make
 			db (if reuse then conn else []) (Datamodel_schema.of_datamodel ());

ocaml/xapi/xapi_session.ml

@@ -27,6 +27,8 @@ open Extauth
 
 let local_superuser = "root"
 
+let xapi_internal_originator = "xapi"
+
 let serialize_auth = Mutex.create()
 
 let wipe_string_contents str = for i = 0 to String.length str - 1 do str.[i] <- '\000' done
@@ -265,39 +267,48 @@ let revalidate_all_sessions ~__context =
 	)with e -> (*unexpected exception: we absorb it and print out a debug line *)
 		debug "Unexpected exception while revalidating external sessions: %s" (ExnHelper.string_of_exn e)
 
-let login_no_password_common ~__context ~uname ~originator ~host ~pool ~is_local_superuser ~subject ~auth_user_sid ~auth_user_name ~rbac_permissions =
-	let session_id = Ref.make () in
-	let uuid = Uuid.to_string (Uuid.make_uuid ()) in
-	let user = Ref.null in (* always return a null reference to the deprecated user object *)
-	let parent = try Context.get_session_id __context with _ -> Ref.null in
-	  (*match uname with   (* the user object is deprecated in favor of subject *)
-	      Some uname -> Helpers.get_user ~__context uname
-	    | None -> Ref.null in*)
-	(* This info line is important for tracking, auditability and client accountability purposes on XenServer *)
-	(* Never print the session id nor uuid: they are secret values that should be known only to the user that *)
-	(* has just logged in. Instead, we print a non-invertible hash as the tracking id for the session id *)
-	(* see also task creation in context.ml *)
-	(* CP-982: promote tracking debug line to info status *)
-	(* CP-982: create tracking id in log files to link username to actions *)
-	info "Session.create %s pool=%b uname=%s originator=%s is_local_superuser=%b auth_user_sid=%s parent=%s"
-		(trackid session_id) pool (match uname with None->""|Some u->u) originator is_local_superuser auth_user_sid (trackid parent);
-	Db.Session.create ~__context ~ref:session_id ~uuid
-	                  ~this_user:user ~this_host:host ~pool:pool
-	                  ~last_active:(Date.of_float (Unix.time ())) ~other_config:[] 
-	                  ~subject:subject ~is_local_superuser:is_local_superuser
-	                  ~auth_user_sid ~validation_time:(Date.of_float (Unix.time ()))
-	                  ~auth_user_name ~rbac_permissions ~parent ~originator;
+let login_no_password_common ~__context ~uname ~originator ~host ~pool ~is_local_superuser ~subject ~auth_user_sid ~auth_user_name ~rbac_permissions ~db_ref =
+	let create_session () =
+		let session_id = Ref.make () in
+		let uuid = Uuid.to_string (Uuid.make_uuid ()) in
+		let user = Ref.null in (* always return a null reference to the deprecated user object *)
+		let parent = try Context.get_session_id __context with _ -> Ref.null in
+			(*match uname with   (* the user object is deprecated in favor of subject *)
+					Some uname -> Helpers.get_user ~__context uname
+				| None -> Ref.null in*)
+		(* This info line is important for tracking, auditability and client accountability purposes on XenServer *)
+		(* Never print the session id nor uuid: they are secret values that should be known only to the user that *)
+		(* has just logged in. Instead, we print a non-invertible hash as the tracking id for the session id *)
+		(* see also task creation in context.ml *)
+		(* CP-982: promote tracking debug line to info status *)
+		(* CP-982: create tracking id in log files to link username to actions *)
+		info "Session.create %s pool=%b uname=%s originator=%s is_local_superuser=%b auth_user_sid=%s parent=%s"
+			(trackid session_id) pool (match uname with None->""|Some u->u) originator is_local_superuser auth_user_sid (trackid parent);
+		Db.Session.create ~__context ~ref:session_id ~uuid
+			~this_user:user ~this_host:host ~pool:pool
+			~last_active:(Date.of_float (Unix.time ())) ~other_config:[] 
+			~subject:subject ~is_local_superuser:is_local_superuser
+			~auth_user_sid ~validation_time:(Date.of_float (Unix.time ()))
+			~auth_user_name ~rbac_permissions ~parent ~originator;
+		session_id
+	in
+	let session_id = match db_ref with
+	| Some db_ref -> Db_backend.create_registered_session create_session db_ref
+	| None -> create_session ()
+	in
 	Rbac_audit.session_create ~__context ~session_id ~uname;
 	(* At this point, the session is created, but with an incorrect time *)
 	(* Force the time to be updated by calling an API function with this session *)
 	let rpc = Helpers.make_rpc ~__context in
-	ignore(Client.Session.get_uuid rpc session_id session_id);
+	ignore(Client.Pool.get_all rpc session_id);
 	session_id
 
 (* XXX: only used internally by the code which grants the guest access to the API.
    Needs to be protected by a proper access control system *)
 let login_no_password  ~__context ~uname ~host ~pool ~is_local_superuser ~subject ~auth_user_sid ~auth_user_name ~rbac_permissions =
-	login_no_password_common ~__context ~uname ~originator:"" ~host ~pool ~is_local_superuser ~subject ~auth_user_sid ~auth_user_name ~rbac_permissions
+	login_no_password_common ~__context ~uname
+		~originator:xapi_internal_originator ~host ~pool ~is_local_superuser
+		~subject ~auth_user_sid ~auth_user_name ~rbac_permissions ~db_ref:None
 
 (** Cause the master to update the session last_active every 30s or so *)
 let consider_touching_session rpc session_id = 
@@ -363,7 +374,7 @@ let login_with_password ~__context ~uname ~pwd ~version ~originator = wipe_param
 		(* we trust requests from local unix filename sockets, so no need to authenticate them before login *)
 		login_no_password_common ~__context ~uname:(Some uname) ~originator ~host:(Helpers.get_localhost ~__context) 
 			~pool:false ~is_local_superuser:true ~subject:(Ref.null)
-			~auth_user_sid:"" ~auth_user_name:uname ~rbac_permissions:[]
+			~auth_user_sid:"" ~auth_user_name:uname ~rbac_permissions:[] ~db_ref:None
 	end 
 	else
 	let () =
@@ -378,7 +389,7 @@ let login_with_password ~__context ~uname ~pwd ~version ~originator = wipe_param
 			debug "Success: local auth, user %s from %s" uname (Context.get_origin __context);
 			login_no_password_common ~__context ~uname:(Some uname) ~originator ~host:(Helpers.get_localhost ~__context) 
 				~pool:false ~is_local_superuser:true ~subject:(Ref.null) ~auth_user_sid:"" ~auth_user_name:uname
-				~rbac_permissions:[]
+				~rbac_permissions:[] ~db_ref:None
 		end
 	in	
 	let thread_delay_and_raise_error ?(error=Api_errors.session_authentication_failed) uname msg =
@@ -550,7 +561,7 @@ let login_with_password ~__context ~uname ~pwd ~version ~originator = wipe_param
 							) in 
 						login_no_password_common ~__context ~uname:(Some uname) ~originator ~host:(Helpers.get_localhost ~__context) 
 							~pool:false ~is_local_superuser:false ~subject:subject ~auth_user_sid:subject_identifier ~auth_user_name:subject_name
-							~rbac_permissions
+							~rbac_permissions ~db_ref:None
 					end
 				(* we only reach this point if for some reason a function above forgot to catch a possible exception in the Auth_signature module*)
 				with
@@ -731,22 +742,21 @@ let get_top ~__context ~self =
   | ancestry -> List.nth ancestry ((List.length ancestry)-1)
 
 (* This function should only be called from inside XAPI. *)
-let create_readonly_session ~__context ~uname =
+let create_readonly_session ~__context ~uname ~db_ref =
 	debug "Creating readonly session.";
 	let role = List.hd (Xapi_role.get_by_name_label ~__context ~label:Datamodel.role_read_only) in
 	let rbac_permissions = Xapi_role.get_permissions_name_label ~__context ~self:role in
 	let master = Helpers.get_master ~__context in
-	login_no_password ~__context ~uname:(Some uname) ~host:master ~pool:false
+	login_no_password_common ~__context ~uname:(Some uname)
+		~originator:xapi_internal_originator ~host:master ~pool:false
 		~is_local_superuser:false ~subject:Ref.null ~auth_user_sid:"readonly-sid"
-		~auth_user_name:uname ~rbac_permissions
+		~auth_user_name:uname ~rbac_permissions ~db_ref
 
 (* Create a database reference from a DB dump, and register it with a new readonly session. *)
 let create_from_db_file ~__context ~filename =
-	let session_id = create_readonly_session ~__context ~uname:"db-from-file" in
 	let db =
 		(Db_xml.From.file (Datamodel_schema.of_datamodel ()) filename)
 		|> Db_upgrade.generic_database_upgrade
 	in
-	let db_ref = Db_ref.in_memory (ref (ref db)) in
-	Db_backend.register_session_with_database session_id db_ref;
-	session_id
+	let db_ref = Some (Db_ref.in_memory (ref (ref db))) in
+	create_readonly_session ~__context ~uname:"db-from-file" ~db_ref

ocaml/xapi/xapi_session.mli

@@ -35,5 +35,6 @@ val get_group_subject_identifier_from_session: __context:Context.t -> session:[
 val get_all_subject_identifiers: __context:Context.t -> string list
 val logout_subject_identifier: __context:Context.t -> subject_identifier:string -> unit
 val get_top: __context:Context.t -> self:API.ref_session -> API.ref_session
-val create_readonly_session: __context:Context.t -> uname:string -> API.ref_session
+val create_readonly_session:
+  __context:Context.t -> uname:string -> db_ref:Db_ref.t option -> API.ref_session
 val create_from_db_file: __context:Context.t -> filename:string -> API.ref_session

ocaml/xapi/xapi_vdi.ml

@@ -750,12 +750,12 @@ let open_database ~__context ~self =
 		raise (Api_errors.Server_error(Api_errors.vdi_incompatible_type,
 			[Ref.string_of self; Record_util.vdi_type_to_string vdi_type]));
 	try
-		let db_ref = Xapi_vdi_helpers.database_ref_of_vdi ~__context ~vdi:self in
+		let db_ref =
+			Some (Xapi_vdi_helpers.database_ref_of_vdi ~__context ~vdi:self) in
 		(* Create a new session to query the database, and associate it with the db ref *)
 		debug "%s" "Creating readonly session";
-		let read_only_session = Xapi_session.create_readonly_session ~__context ~uname:"disaster-recovery" in
-		Db_backend.register_session_with_database read_only_session db_ref;
-		read_only_session
+		Xapi_session.create_readonly_session ~__context
+			~uname:"disaster-recovery" ~db_ref
 	with e ->
 		let error = Printexc.to_string e in
 		let reason = match e with