Skip to content
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

Multi-threaded libwbclient API should be supported #46

Closed
kvv81 opened this issue Jan 8, 2021 · 4 comments · Fixed by #47
Closed

Multi-threaded libwbclient API should be supported #46

kvv81 opened this issue Jan 8, 2021 · 4 comments · Fixed by #47

Comments

@kvv81
Copy link

kvv81 commented Jan 8, 2021

We use gssntlmssp library with support of winbind external server from multi-threaded application.
We see sporadic unexpected authentication failures during stress-test (when few requests are done in parallel) while everything is ok for non-parallel flow. Root-cause of the problem is unexpected interleaved data received by winbind, in this case request is dropped. We are getting an error for wbcAuthenticateUserEx call:

wbc_status = wbcAuthenticateUserEx(&wbc_params, &wbc_info, &wbc_err);

For details of this call from gssntlmssp library, see winbind_srv_auth function here:
https://github.com/gssapi/gss-ntlmssp/blob/main/src/winbind.c

From winbind side:

[2021/01/06 08:29:19.646294, 10, pid=256853, effective(0, 0), real(0, 0), class=winbind] ../source3/winbindd/winbindd.c:763(process_request_send)
  process_request_send: process_request: request fn NTLMAUTH
...
[2021/01/06 08:29:19.646523,  0, pid=256853, effective(0, 0), real(0, 0), class=winbind] ../source3/winbindd/winbindd.c:1005(winbind_client_activity)
  winbind_client_activity[256138:PAM_AUTH_CRAP]:unexpected data from client - removing client
[2021/01/06 08:29:19.646616,  1, pid=256853, effective(0, 0), real(0, 0), class=winbind] ../source3/winbindd/winbindd_dual.c:337(wb_child_request_cleanup)
  wb_child_request_cleanup: keep orphaned subreq[0x55564bdc1b00]

We have got this feedback from Samba developers (Volker Lendecke):
... one guess would be that the code using the gss-ntlmssp library is
multi-threaded. While the gss-ntlmssp library possibly is thread-safe
in general, its use of libwbclient is definitely not. Directly using
wbcAuthenticateUserEx() and other needs to be protected by a mutex, or
alternatively the library must create a wbcContext using wbcCtxCreate
in thread-local storage and then call wbcCtxAuthenticateUserEx(). The
wbcCtx*() calls are designed to be callable in a multi-threaded
environment, the wbcAuthenticateUserEx call is definitely not.

We need to have an option for using multi-threaded API of libwbclient from gssntlmssp.
One can use some compile-time option to specify the intended API or optionally we can just refactor the code to always use MT-safe calls.

gssntlmssp has few other winbind client calls - wbcInterfaceDetails and wbcCredentialCache - see src/winbind.c of gssntlmssp.
We haven't seen such races with them yet, but probably that's due very short time of request handling (local requests, no need to talk with DC). Probably all libwbclient calls should use the same approach.

@kvv81
Copy link
Author

kvv81 commented Jan 8, 2021

Correction of the comment from Volker Lendecke:
while looking at the code in more detail, in theory there
should already be a mutex around the wbcAuthenticateUserEx call, it's
in the wb_global_ctx_mutex in nsswitch/wb_common.c. So I think the
blame is not in gss-ntlmssp but on libwbclient.

In theory if libwbclient is built in an
environment that has pthread support available and correctly detected,
this should have been protected already, albeit serialized.

You should contact the provider of your
libwbclient.so binary as to why this mutex protection fails in your
case. Samba does have code to serialize the calls through the global
wbcCtx.

This part of issue should be handled separately from gssntlmssp scope;
however wbcAuthenticateUserEx still can't provide parallel operations for high-performance systems (requests are serialized internally by libwbclient) so the issue should be probably re-phrased to new feature support.

@simo5
Copy link
Collaborator

simo5 commented Jan 8, 2021

Yeah sounds like a RFE for winbindd really.
I do not see any actionable item for GSSNTLMSSP here, so closing.
Let me know if I am missing something.

@simo5 simo5 closed this as completed Jan 8, 2021
@simo5
Copy link
Collaborator

simo5 commented Jan 21, 2021

After some more background discussion, it turns out I can indeed use a diferent API that will be better suited for multi-thread cases.
Reopening.

@simo5
Copy link
Collaborator

simo5 commented Jan 21, 2021

@kvv81 in #47 there is a possible implementation for this.
The good thing is that there is no need for a new API from Winbindd, all we need is already available.
The bad news is that the implementation I have right now may be an issue with applications that have many threads.

An alternative could be to cretae a wbcCtx explicitly within a GSSAPI ctx evry time one is needed, and then destroy as the GSSAPI ctx is destroyed.
however this may incur more overhad than the use of the current big mutex, as it ends up openinf sockets and closing them down for each authentication.

So let me know what you think would work best for your use case, either here, or on the PR.

@simo5 simo5 closed this as completed in #47 Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants