From 1809cd243214a1280a89d134ef9cc2434dc2e001 Mon Sep 17 00:00:00 2001 From: Lin Liu Date: Fri, 23 Jul 2021 08:37:18 +0000 Subject: [PATCH 1/2] CA-356901: Perform ldap query if winbind failed to resolve subject name to subject identifier winbind has cache timeout set to 60s, so winbind sync its cached data with domain controller every 60 seconds. If a user is newly created in DC within 60s, winbind failed to resolve it. This commit fix this issue by perform ldap query on winbind fail Signed-off-by: Lin Liu --- ocaml/xapi/extauth_plugin_ADwinbind.ml | 133 ++++++++++++++++++------- 1 file changed, 98 insertions(+), 35 deletions(-) diff --git a/ocaml/xapi/extauth_plugin_ADwinbind.ml b/ocaml/xapi/extauth_plugin_ADwinbind.ml index 45028ee502d..9ba93557714 100644 --- a/ocaml/xapi/extauth_plugin_ADwinbind.ml +++ b/ocaml/xapi/extauth_plugin_ADwinbind.ml @@ -62,6 +62,14 @@ let domain_krb5_dir = Filename.concat Xapi_globs.samba_dir "lock/smb_krb5" let debug_level () = !Xapi_globs.winbind_debug_level |> string_of_int +type domain_info = { + service_name: string + ; workgroup: string option + (* For upgrade case, the legacy db does not contain workgroup *) + ; netbios_name: string option + (* Persist netbios_name to support hostname change *) +} + let hd msg = function | [] -> error "%s" msg ; @@ -81,6 +89,21 @@ let ntlm_auth uname passwd : (unit, exn) result = Ok () with _ -> Error (auth_ex uname) +let get_domain_info_from_db () = + (fun __context -> + let host = Helpers.get_localhost ~__context in + let service_name = + Db.Host.get_external_auth_service_name ~__context ~self:host + in + let workgroup, netbios_name = + Db.Host.get_external_auth_configuration ~__context ~self:host |> fun l -> + (List.assoc_opt "workgroup" l, List.assoc_opt "netbios_name" l) + in + {service_name; workgroup; netbios_name} + ) + |> Server_helpers.exec_with_new_task + "retrieving external auth domain workgroup" + module Ldap = struct type user = { name: string @@ -200,12 +223,15 @@ module Ldap = struct ; password_expired= logand user_account_control passw_expire_bit <> 0l } - let query_user sid domain_netbios kdc = + let env_of_lookup domain_netbios = let domain_krb5_cfg = Filename.concat domain_krb5_dir (Printf.sprintf "krb5.conf.%s" domain_netbios) in - let env = [|Printf.sprintf "KRB5_CONFIG=%s" domain_krb5_cfg|] in + [|Printf.sprintf "KRB5_CONFIG=%s" domain_krb5_cfg|] + + let query_user sid domain_netbios kdc = + let env = env_of_lookup domain_netbios in let* stdout = try (* Query KDC instead of use domain here @@ -227,9 +253,38 @@ module Ldap = struct args in Ok stdout - with _ -> Error (generic_ex "ldap query failed") + with _ -> Error (generic_ex "ldap query user info from sid failed") in parse_user stdout generic_ex "%s" + + let query_sid ~name ~kdc ~domain_netbios = + let key = "objectSid" in + let env = env_of_lookup domain_netbios in + let query = Printf.sprintf "(|(sAMAccountName=%s)(name=%s))" name name in + let args = + [ + "ads" + ; "search" + ; "-d" + ; debug_level () + ; "--server" + ; kdc + ; "--machine-pass" + ; query + ; key + ] + in + try + Helpers.call_script ~env !Xapi_globs.net_cmd args + |> Xapi_cmd_result.of_output ~sep:':' ~key + |> fun x -> Ok x + with + | Forkhelpers.Spawn_internal_error (_, stdout, _) -> + Error (generic_ex "Ldap query sid failed: %s" stdout) + | Not_found -> + Error (generic_ex "%s not found in ldap result" key) + | _ -> + Error (generic_ex "Failed to lookup sid from username %s" name) end type domain_name_type = Name | NetbiosName @@ -345,6 +400,25 @@ module Wbinfo = struct | _ -> Error (generic_ex "Invalid domain user name %s" uname) + let domain_and_user_of_uname uname = + let open Astring.String in + match String.split_on_char '\\' uname with + | [netbios; user] -> + let* domain = + domain_name_of ~target_name_type:Name ~from_name:netbios + in + Ok (domain, user) + | _ -> ( + match String.split_on_char '@' uname with + | [user; domain] -> + Ok (domain, user) + | _ -> + if is_infix ~affix:"@" uname || is_infix ~affix:{|\|} uname then + Error (generic_ex "Invalid domain user name %s" uname) + else + Ok ((get_domain_info_from_db ()).service_name, uname) + ) + let all_domain_netbios () = (* * List all domains (trusted and own domain) @@ -445,14 +519,6 @@ module Wbinfo = struct parse_uid_info stdout fun () -> parsing_ex args end -type domain_info = { - service_name: string - ; workgroup: string option - (* For upgrade case, the legacy db does not contain workgroup *) - ; netbios_name: string option - (* Persist netbios_name to support hostname change *) -} - module Migrate_from_pbis = struct (* upgrade-pbis-to-winbind handles most of the migration from PBIS database * to winbind database @@ -532,21 +598,6 @@ module Migrate_from_pbis = struct netbios_name end -let get_domain_info_from_db () = - (fun __context -> - let host = Helpers.get_localhost ~__context in - let service_name = - Db.Host.get_external_auth_service_name ~__context ~self:host - in - let workgroup, netbios_name = - Db.Host.get_external_auth_configuration ~__context ~self:host |> fun l -> - (List.assoc_opt "workgroup" l, List.assoc_opt "netbios_name" l) - in - {service_name; workgroup; netbios_name} - ) - |> Server_helpers.exec_with_new_task - "retrieving external auth domain workgroup" - let kdcs_of_domain domain = try Helpers.call_script ~log_output:On_failure net_cmd @@ -921,13 +972,32 @@ let build_dns_hostname_option ~config_params = | _ -> [] +let closest_kdc_of_domain domain = + match ClosestKdc.from_db domain with + | Some kdc -> + kdc + | None -> + (* Just pick the first KDC in the list *) + kdc_of_domain domain + module AuthADWinbind : Auth_signature.AUTH_MODULE = struct let get_subject_identifier' subject_name = let* domain = try Ok (get_domain_info_from_db ()).service_name with e -> Error e in let subject_name = domainify_uname ~domain subject_name in - Wbinfo.sid_of_name subject_name + match Wbinfo.sid_of_name subject_name with + | Ok sid -> + Ok sid + | Error e -> + debug "Failed to query sid from cache, error: %s, retry ldap" + (ExnHelper.string_of_exn e) ; + let* domain, user = Wbinfo.domain_and_user_of_uname subject_name in + let* domain_netbios = + Wbinfo.domain_name_of ~target_name_type:NetbiosName ~from_name:domain + in + let kdc = closest_kdc_of_domain domain in + Ldap.query_sid ~name:user ~kdc ~domain_netbios (* subject_id get_subject_identifier(string subject_name) @@ -1004,14 +1074,7 @@ module AuthADWinbind : Auth_signature.AUTH_MODULE = struct let query_subject_information_user (uid : int) (sid : string) = let* {user_name; gecos; gid} = Wbinfo.uid_info_of_uid uid in let* domain_netbios, domain = Wbinfo.domain_of_uname user_name in - let closest_kdc = - match ClosestKdc.from_db domain with - | Some kdc -> - kdc - | None -> - (* Just pick the first KDC in the list *) - kdc_of_domain domain - in + let closest_kdc = closest_kdc_of_domain domain in let* { name ; upn From f1a520db1ce1fbe707844dcac87ea3ee2ffbd4da Mon Sep 17 00:00:00 2001 From: Lin Liu Date: Mon, 26 Jul 2021 10:22:17 +0000 Subject: [PATCH 2/2] CA-356901: Escape subject before perform ldap query Signed-off-by: Lin Liu --- ocaml/tests/test_extauth_plugin_ADwinbind.ml | 23 ++++ ocaml/xapi/extauth_plugin_ADwinbind.ml | 120 ++++++++++++------- ocaml/xapi/extauth_plugin_ADwinbind.mli | 2 + 3 files changed, 104 insertions(+), 41 deletions(-) diff --git a/ocaml/tests/test_extauth_plugin_ADwinbind.ml b/ocaml/tests/test_extauth_plugin_ADwinbind.ml index 9e924371aa2..cabc51a8e26 100644 --- a/ocaml/tests/test_extauth_plugin_ADwinbind.ml +++ b/ocaml/tests/test_extauth_plugin_ADwinbind.ml @@ -112,6 +112,28 @@ let test_domainify_uname = |> List.map @@ fun (inp, exp) -> (Printf.sprintf "%s -> %s" inp exp, `Quick, check inp exp) +let test_ldap_escape = + let open Extauth_plugin_ADwinbind.Ldap in + let check str exp () = + let msg = Printf.sprintf "%s -> %s" str exp in + let escaped = escape str in + Alcotest.(check string) msg exp escaped + in + let matrix = + [ + ({|user|}, {|user|}) + ; ({|(user)|}, {|\28user\29|}) + ; ({|(user|}, {|\28user|}) + ; ({|user)|}, {|user\29|}) + ; ({|us\er)|}, {|us\5der\29|}) + ; ({|user)1|}, {|user\291|}) + ; ({|user*|}, {|user\2a|}) + ] + in + matrix + |> List.map @@ fun (inp, exp) -> + (Printf.sprintf "%s -> %s" inp exp, `Quick, check inp exp) + let test_parse_wbinfo_uid_info = let open Extauth_plugin_ADwinbind.Wbinfo in let string_of_result x = @@ -424,6 +446,7 @@ let tests = ; ("ADwinbind:test_range", Range.tests) ; ("ADwinbind:test_parse_value_from_pbis", ParseValueFromPbis.tests) ; ("ADwinbind:test_domainify_uname", test_domainify_uname) + ; ("ADwinbind:test_ldap_escape", test_ldap_escape) ; ("ADwinbind:test_parse_wbinfo_uid_info", test_parse_wbinfo_uid_info) ; ("ADwinbind:test_parse_ldap_stdout", test_parse_ldap_stdout) ; ( "ADwinbind:test_wbinfo_exception_of_stderr" diff --git a/ocaml/xapi/extauth_plugin_ADwinbind.ml b/ocaml/xapi/extauth_plugin_ADwinbind.ml index 9ba93557714..ed74502c323 100644 --- a/ocaml/xapi/extauth_plugin_ADwinbind.ml +++ b/ocaml/xapi/extauth_plugin_ADwinbind.ml @@ -90,21 +90,61 @@ let ntlm_auth uname passwd : (unit, exn) result = with _ -> Error (auth_ex uname) let get_domain_info_from_db () = - (fun __context -> - let host = Helpers.get_localhost ~__context in - let service_name = - Db.Host.get_external_auth_service_name ~__context ~self:host - in - let workgroup, netbios_name = - Db.Host.get_external_auth_configuration ~__context ~self:host |> fun l -> - (List.assoc_opt "workgroup" l, List.assoc_opt "netbios_name" l) - in - {service_name; workgroup; netbios_name} - ) - |> Server_helpers.exec_with_new_task - "retrieving external auth domain workgroup" + Server_helpers.exec_with_new_task "retrieving external auth domain workgroup" + @@ fun __context -> + let host = Helpers.get_localhost ~__context in + let service_name = + Db.Host.get_external_auth_service_name ~__context ~self:host + in + let workgroup, netbios_name = + Db.Host.get_external_auth_configuration ~__context ~self:host + |> fun config -> + (List.assoc_opt "workgroup" config, List.assoc_opt "netbios_name" config) + in + {service_name; workgroup; netbios_name} module Ldap = struct + module Escape = struct + (* + * Escape characters according to + * https://docs.microsoft.com/en-gb/windows/win32/adsi/search-filter-syntax?redirectedfrom=MSDN#special-characters + * *) + + let reg_star = {|*|} |> Re.str |> Re.compile + + let reg_left_bracket = {|(|} |> Re.str |> Re.compile + + let reg_right_bracket = {|)|} |> Re.str |> Re.compile + + let reg_backward_slash = {|\|} |> Re.str |> Re.compile + + let reg_null = "\000" |> Re.str |> Re.compile + + let reg_slash = {|/|} |> Re.str |> Re.compile + + let escape_map = + [ + (* backward slash goes first as others will include backward slash*) + (reg_backward_slash, {|\5d|}) + ; (reg_star, {|\2a|}) + ; (reg_left_bracket, {|\28|}) + ; (reg_right_bracket, {|\29|}) + ; (reg_null, {|\00|}) + ; (reg_slash, {|\2f|}) + ] + + let escape str = + List.fold_left + (fun acc element -> + let reg = fst element in + let value = snd element in + Re.replace_string reg ~by:value acc + ) + str escape_map + end + + let escape str = Escape.escape str + type user = { name: string ; display_name: string @@ -223,7 +263,7 @@ module Ldap = struct ; password_expired= logand user_account_control passw_expire_bit <> 0l } - let env_of_lookup domain_netbios = + let env_of_krb5 domain_netbios = let domain_krb5_cfg = Filename.concat domain_krb5_dir (Printf.sprintf "krb5.conf.%s" domain_netbios) @@ -231,7 +271,7 @@ module Ldap = struct [|Printf.sprintf "KRB5_CONFIG=%s" domain_krb5_cfg|] let query_user sid domain_netbios kdc = - let env = env_of_lookup domain_netbios in + let env = env_of_krb5 domain_netbios in let* stdout = try (* Query KDC instead of use domain here @@ -259,7 +299,9 @@ module Ldap = struct let query_sid ~name ~kdc ~domain_netbios = let key = "objectSid" in - let env = env_of_lookup domain_netbios in + let env = env_of_krb5 domain_netbios in + let name = escape name in + (* Escape name to avoid injection detection *) let query = Printf.sprintf "(|(sAMAccountName=%s)(name=%s))" name name in let args = [ @@ -676,11 +718,9 @@ let from_config ~name ~err_msg ~config_params = let all_number_re = Re.Perl.re {|^\d+$|} |> Re.Perl.compile let get_localhost_name () = - (fun __context -> - Helpers.get_localhost ~__context |> fun host -> - Db.Host.get_hostname ~__context ~self:host - ) - |> Server_helpers.exec_with_new_task "retrieving hostname" + Server_helpers.exec_with_new_task "retrieving hostname" @@ fun __context -> + Helpers.get_localhost ~__context |> fun host -> + Db.Host.get_hostname ~__context ~self:host let assert_hostname_valid ~hostname = let all_numbers = Re.matches all_number_re hostname <> [] in @@ -716,13 +756,12 @@ let persist_extauth_config ~domain ~user ~ou_conf ~workgroup ~netbios_name = ] @ ou_conf in - (fun __context -> - Helpers.get_localhost ~__context |> fun self -> - Db.Host.set_external_auth_configuration ~__context ~self ~value ; - Db.Host.get_name_label ~__context ~self - |> debug "update external_auth_configuration for host %s" - ) - |> Server_helpers.exec_with_new_task "update external_auth_configuration" + Server_helpers.exec_with_new_task "update external_auth_configuration" + @@ fun __context -> + Helpers.get_localhost ~__context |> fun self -> + Db.Host.set_external_auth_configuration ~__context ~self ~value ; + Db.Host.get_name_label ~__context ~self + |> debug "update external_auth_configuration for host %s" let disable_machine_account ~service_name = function | Some u, Some p -> ( @@ -884,21 +923,19 @@ module ClosestKdc = struct Error e let update_db ~domain ~kdc = - (fun __context -> - let self = Helpers.get_localhost ~__context in - Db.Host.get_external_auth_configuration ~__context ~self |> fun value -> - (domain, kdc) :: List.remove_assoc domain value |> fun value -> - Db.Host.set_external_auth_configuration ~__context ~self ~value - ) - |> Server_helpers.exec_with_new_task "update domain closest kdc" + Server_helpers.exec_with_new_task "update domain closest kdc" + @@ fun __context -> + let self = Helpers.get_localhost ~__context in + Db.Host.get_external_auth_configuration ~__context ~self |> fun value -> + (domain, kdc) :: List.remove_assoc domain value |> fun value -> + Db.Host.set_external_auth_configuration ~__context ~self ~value let from_db domain = - (fun __context -> - let self = Helpers.get_localhost ~__context in - Db.Host.get_external_auth_configuration ~__context ~self - |> List.assoc_opt domain - ) - |> Server_helpers.exec_with_new_task "query domain closest kdc" + Server_helpers.exec_with_new_task "query domain closest kdc" + @@ fun __context -> + let self = Helpers.get_localhost ~__context in + Db.Host.get_external_auth_configuration ~__context ~self + |> List.assoc_opt domain let lookup domain = try @@ -982,6 +1019,7 @@ let closest_kdc_of_domain domain = module AuthADWinbind : Auth_signature.AUTH_MODULE = struct let get_subject_identifier' subject_name = + (* Called in the login path with a yet unauthenticated user *) let* domain = try Ok (get_domain_info_from_db ()).service_name with e -> Error e in diff --git a/ocaml/xapi/extauth_plugin_ADwinbind.mli b/ocaml/xapi/extauth_plugin_ADwinbind.mli index 61f69d70c1b..cc9590a0c60 100644 --- a/ocaml/xapi/extauth_plugin_ADwinbind.mli +++ b/ocaml/xapi/extauth_plugin_ADwinbind.mli @@ -61,6 +61,8 @@ module Ldap : sig val string_of_user : user -> string val parse_user : string -> (user, string) result + + val escape : string -> string end module Migrate_from_pbis : sig