-
Notifications
You must be signed in to change notification settings - Fork 284
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.
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
CA-356901: Perform ldap query if winbind failed to resovle subject #4478
Conversation
5968aee
to
0d006c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - even though this is making an extra request sometimes, it'll only happen when users enter an incorrect user name (which should be rare)
There is a typo in the title here and several places in the commit message: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably need a generic function to escape arguments that we pass to ldapquery that can be user controlled.
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 <[email protected]>
0d006c2
to
6ff5967
Compare
6ff5967
to
15c0337
Compare
15c0337
to
2a5a95e
Compare
2a5a95e
to
87adc86
Compare
updated, @lindig @edwintorok please help to review at your convenient, thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not an expert on winbind, so this will have to rely on testing to prove that this works correctly and securely. I can't spot any immediate issues in terms of OCaml language.
let escape str = | ||
List.fold_left | ||
(fun acc element -> | ||
let reg = fst element in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can also be written as let reg, value = element
, or fun acc (reg, value)
, but is fine like this too.
@@ -200,12 +263,15 @@ module Ldap = struct | |||
; password_expired= logand user_account_control passw_expire_bit <> 0l | |||
} | |||
|
|||
let query_user sid domain_netbios kdc = | |||
let env_of_krb5 domain_netbios = | |||
let domain_krb5_cfg = | |||
Filename.concat domain_krb5_dir | |||
(Printf.sprintf "krb5.conf.%s" domain_netbios) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this created by winbind itself? I can't find any code in xapi that would create this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, winbind create it, we just re-use it (for the KDCs configured)
Might be useful to add a comment to |
There are some concerns on whether the login process here is secure (but that is unrelated to this PR). Could a user set up their own domain server then attempt to login with [email protected], which would successfully authenticate them and when asked to map the domain to a KDC it would map it to an attacker controlled one, and map the username to a SID or the SID to a name it would map it to a SID that exists in our AD hierarchy, and map the name to [email protected] (assuming that the pool is joined to trusteddomain.example.com). |
The faked DC will not serve as normal DC, as during ldap query, net command will authenticate with domain controller by kerberos protocol, I do not think the fake server can hold the machine password for the machine account created during domain join. There is an xenrt automation test for this by @xihuany |
If you use ldap query injection and successfully become another user, you still needs to provide the password for that user. |
Signed-off-by: Lin Liu <[email protected]>
87adc86
to
f1a520d
Compare
The security concen is addressed in PSEC-8206, if still any further concern, let's talk there as it should not block this PR. |
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 resovle it.
This commit fix this issue by perform ldap query on winbin fail
Signed-off-by: Lin Liu [email protected]