Skip to content

CA-252876: AD group name with parenthesis not work as expect in Xense… #3031

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 1 commit into from
May 22, 2017
Merged

CA-252876: AD group name with parenthesis not work as expect in Xense… #3031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
57 changes: 25 additions & 32 deletions ocaml/xapi/extauth_plugin_ADpbis.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,35 +18,46 @@
module D = Debug.Make(struct let name="extauth_plugin_ADpbis" end)
open D

open Stdext.Xstringext

let match_error_tag (lines:string list) =
let err_catch_list =
let err_catch_list =
[ "DNS_ERROR_BAD_PACKET", Auth_signature.E_LOOKUP;
"LW_ERROR_PASSWORD_MISMATCH", Auth_signature.E_CREDENTIALS;
"LW_ERROR_INVALID_ACCOUNT", Auth_signature.E_INVALID_ACCOUNT;
"LW_ERROR_ACCESS_DENIED", Auth_signature.E_DENIED;
"LW_ERROR_DOMAIN_IS_OFFLINE", Auth_signature.E_UNAVAILABLE;
"LW_ERROR_INVALID_OU", Auth_signature.E_INVALID_OU;
(* More errors to be caught here *)
]
]
in
let split_to_words = fun str ->
let open Stdext.Xstringext in
let seps = ['('; ')'; ' '; '\t'; '.'] in
String.split_f (fun s -> List.exists (fun sep -> sep = s) seps) str
let split_to_words = fun str ->
let seps = ['('; ')'; ' '; '\t'; '.'] in
String.split_f (fun s -> List.exists (fun sep -> sep = s) seps) str
in
let rec has_err lines err_pattern =
match lines with
| [] -> false
| line :: rest ->
try
| line :: rest ->
try
ignore(List.find (fun w -> w = err_pattern) (split_to_words line));
true
with Not_found -> has_err rest err_pattern
in
try
try
let (_, errtag) = List.find (fun (err_pattern, _) -> has_err lines err_pattern) err_catch_list in
errtag
with Not_found -> Auth_signature.E_GENERIC
with Not_found -> Auth_signature.E_GENERIC

let extract_sid_from_group_list = fun group_list ->
List.map (fun (n,v)->
let v = String.replace ")" "" v in
let v = String.replace "sid =" "|" v in
let vs = String.split_f (fun c -> c = '|') v in
let sid = String.trim (List.nth vs 1) in
debug "extract_sid_from_group_list get sid=[%s]" sid;
sid
) (List.filter (fun (n,v)->n="") group_list)

module AuthADlw : Auth_signature.AUTH_MODULE =
struct
Expand All @@ -64,17 +75,6 @@ struct

let splitlines s = String.split_f (fun c -> c = '\n') (String.replace "#012" "\n" s)

let rec string_trim s =
let l = String.length s in
if l = 0 then
s
else if s.[0] = ' ' || s.[0] = '\t' || s.[0] = '\n' || s.[0] = '\r' then
string_trim (String.sub s 1 (l-1))
else if s.[l-1] = ' ' || s.[l-1] = '\t' || s.[l-1] = '\n' || s.[l-1] = '\r' then
string_trim (String.sub s 0 (l-1))
else
s

let pbis_common_with_password (password:string) (pbis_cmd:string) (pbis_args:string list) =
let debug_cmd = pbis_cmd ^ " " ^ (List.fold_left (fun p pp -> p^" "^pp) " " pbis_args) in
try
Expand Down Expand Up @@ -241,8 +241,8 @@ struct
debug "parse %s: currkey=[%s] line=[%s]" debug_cmd currkey line;
if List.length slices > 1 then
begin
let key = string_trim (List.hd slices) in
let value = string_trim (List.nth slices 1) in
let key = String.trim (List.hd slices) in
let value = String.trim (List.nth slices 1) in
debug "parse %s: key=[%s] value=[%s] currkey=[%s]" debug_cmd key value currkey;
if String.length value > 0 then
(acc @ [(key, value)], "")
Expand All @@ -251,7 +251,7 @@ struct
end
else
let key = currkey in
let value = string_trim line in
let value = String.trim line in
debug "parse %s: key=[%s] value=[%s] currkey=[%s]" debug_cmd key value currkey;
(acc @ [(key, value)], currkey)
) in
Expand Down Expand Up @@ -353,14 +353,7 @@ struct
And pbis_common will return subject_attrs as
[("Number of groups found for user 'test@testdomain'", "2"), ("", line1), ("", line2) ... ("", lineN)]
*)
List.map (fun (n,v)->
let v = String.replace ")" "|" v in
let v = String.replace "sid =" "|" v in
let vs = String.split_f (fun c -> c = '|') v in
let sid = string_trim (List.nth vs 1) in
debug "pbis_get_group_sids_byname %s get sid=[%s]" _subject_name sid;
sid
) (List.filter (fun (n,v)->n="") subject_attrs)
extract_sid_from_group_list subject_attrs

let pbis_get_sid_bygid gid =

Expand Down
38 changes: 37 additions & 1 deletion ocaml/xapi/test_extauth_plugin_ADpbis.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
open OUnit
open Test_highlevel

module PbisAuthErrorsCatch= Generic.Make(struct
module PbisAuthErrorsCatch = Generic.Make(struct
module Io = struct
type input_t = string list
type output_t = Auth_signature.auth_service_error_tag
Expand Down Expand Up @@ -55,9 +55,45 @@ module PbisAuthErrorsCatch= Generic.Make(struct
]
end)

module PbisExtractSid = Generic.Make(struct
module Io = struct
type input_t = (string * string) list
type output_t = string list

let string_of_input_t = Test_printers.(list (pair string string))
let string_of_output_t = Test_printers.(list string)
end

let transform = Extauth_plugin_ADpbis.extract_sid_from_group_list

let tests = [
[(" ", " ")], [];

[("Exception","Remote connection shutdown!")], [];

[("Number of groups found for user 'testAD@BLE'", "0");
("Error", "No record found")],
[];

[("Number of groups found for user 'admin@NVC'", "1");
("", "Group[1 of 1] name = NVC\\testg(ab) (gid = 564135020, sid = S-1-5-21-1171552557-368733809-2946345504-1132)")],
["S-1-5-21-1171552557-368733809-2946345504-1132"];

[("Number of groups found for user 'cnk3@UN'", "1");
("", "Group[1 of 1] name = UN\\KnmOJ (gid = 492513842, sid = S-1-5-31-5921451325-154521381-3135732118-4527)")],
["S-1-5-31-5921451325-154521381-3135732118-4527"];

[("Number of groups found for user 'test@testdomain'", "2");
("", "Group[1 of 2] name = testdomain\\dnsadmins (gid = 580912206, sid = S-1-5-21-791009147-1041474540-2433379237-1102)");
("", "Group[2 of 2] name = testdomain\\domain+users (gid = 580911617, sid = S-1-5-21-791009147-1041474540-2433379237-513)")],
["S-1-5-21-791009147-1041474540-2433379237-1102"; "S-1-5-21-791009147-1041474540-2433379237-513"];
]
end)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test for a couple of malformed entries as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mseri
Thanks for your comment.
Updated. Are you happy to review again?

Copy link
Contributor

@mseri mseri May 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, can you add a test over [("blah", "1"); ("", "0 | 1 | 2 | 3")] as well?

Copy link
Contributor

@mseri mseri May 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Is it supposed to get '1' really? I would imagine that that should be considered broken input... and get [] or an error.

Copy link
Contributor Author

@fillzero fillzero May 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@mseri , here is the output using utop:

utop # extract_sid_from_group_list [("blah", "1"); ("", "0 | 1 | 2 | 3")];;
- : bytes list = ["1"]

And log from make test

2017-05-19T14:57:55+00:00 607ae1f1e5f0#00 I: Test base_suite:0:test_extauth_ADpbis:1:test_pbis_extract_sid:1:[("blah", "1"); ("", "0 | 1 | 2 | 3")] -> ["1"] is successful.

Copy link
Contributor

@mseri mseri May 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that is exactly my point. It should not. You should extract the sid, and there is no sid in that string. Also I believe you would get something like:

(*Note: I did not test the following, I might be wrong *)
extract_sid_from_group_list [("Number of groups found for user 'cnk3@UN'", "1");
 ("", "Group[1 of 1] name = a|b|c|d\\UN\\KnmOJ (gid = 492513842, sid = S-1-5-31-5921451325-154521381-3135732118-4527)")];;

["b"];

as well, which would be clearly wrong.


let test =
"test_extauth_ADpbis" >:::
[
"test_pbis_auth_errors_catch" >::: PbisAuthErrorsCatch.tests;
"test_pbis_extract_sid" >::: PbisExtractSid.tests;
]