search

gssapi / gss-ntlmssp

A complete implementation of the MS-NLMP documents as a GSSAPI mechanism
ISC License
30 stars 26 forks source link

Multi-threaded libwbclient API should be supported #46

Closed kvv81 closed 3 years ago

kvv81 commented 3 years ago

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 commented 3 years ago

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/wbcommon.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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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.