On Tue, 2011-11-08 at 14:53 +0100, Pavel Zuna wrote: > This patch adds the whole Winbind provider. We agreed with Summit, that it > would > be better to submit as a single patch as splitting it wouldn't make review > any > easier. > > It has the ID and AUTH providers. ID can do user, groups, initgroups and > enumeration of them. AUTH can do authentication and password change operation. > > To build the provider, you need to use the > --enable-experimental-winbind-provider configure flag. samba-winbind-devel > package is required. > > SSSD configuration option for the Winbind provider can be found in > /etc/sssd/sssd.api.d/sssd-winbind.conf. The correspond pretty much to Winbind > options normally found in smb.conf. > > Both NT and AD domains have to be joined first using samba net utility: > net ads join -S server -U user ... > > A man page (man sssd-winbind) is coming soon. > > Big thanks to Summit for helping me out with testing this a reviewing commit > in > my private repo periodically. Thanks!
Pavel and I did an extensive review of the WInbind ID provider today. See attached text file for full details.
(09:25:03 AM) sgallagh: pzuna: Ok, shall we do a pair review of the Winbind provider? (09:25:08 AM) pzuna: sgallagh: sure (09:26:21 AM) sgallagh: pzuna: Ok, let's start with the ID provider. We'll walk through a user lookup first. (09:27:02 AM) sgallagh: I just need a moment while I bring up the source. (09:27:50 AM) sgallagh: Ok, so there's nothing terribly surprising about the winbind_account_info_handler() here, so let's start with winbind_users_get_send() (09:29:20 AM) sgallagh: So far this looks good (Sorry for the play-by-play, it's been almost a week since I did the quick review, so I forget where exactly the issues started) (09:29:31 AM) pzuna: no problem :) (09:31:07 AM) sgallagh: pzuna: I don't see the value of returning a winbind_id_data in winbind_users_query_recv() (09:32:03 AM) sgallagh: That's the only piece of relevant data there. Why bother creating the union? (09:32:28 AM) pzuna: sgallagh: it's so that we can have common handling for groups and users (09:32:38 AM) sgallagh: If you have a reason to use the union in the consumer, create it there and just assign the pwd part to it. (09:32:48 AM) pzuna: the whole thing could be a lot more generic than it is currently (09:33:04 AM) sgallagh: So in winbind_users_get_done(), call winbind_users_query_recv(subreq, state, &id.pwd); (09:33:22 AM) pzuna: ok (09:33:23 AM) sgallagh: The recv() functions shouldn't be generic without a good reason (09:33:36 AM) sgallagh: They should return EXACTLY what was requested. No more and no less. (09:34:03 AM) sgallagh: pzuna: Also, your winbind_id_lists are a memory nightmare :( (09:34:51 AM) sgallagh: pzuna: winbind_id_list_append() creates memory in a completely wrong way (09:35:29 AM) sgallagh: By iterating through 'list', you're actually changing the starting point of the list every time you append to it (09:35:57 AM) sgallagh: pzuna: For another thing, it's not safe to allocate memory on the first item in the list, because if you decide to remove and free that item, you're freeing the entire list. (09:36:16 AM) sgallagh: Instead, you need to pass in a memctx and make sure that the context is a superset of the list. (09:36:46 AM) pzuna: ok, although I don't know how I'm changing the starting point (09:37:06 AM) sgallagh: pzuna: you're passing in a pointer to list (09:37:16 AM) sgallagh: You're changing the address of that pointer in the while() loop (09:37:33 AM) pzuna: yes, but that pointer variable is local to the winbind_id_list_append function (09:37:41 AM) pzuna: so nothing changes outside of it (09:38:09 AM) sgallagh: ah, you're right. Still, not pretty :) (09:38:17 AM) sgallagh: And the real problem here is the memory assignment (09:39:34 AM) pzuna: yeah, it's probably not prettiest, I'll add another helper variable... I'm just used to use as little as possible since I was once programming on machines with limited stack space :) (09:40:40 AM) sgallagh: pzuna: I'm not a huge fan of unions in general (09:40:46 AM) sgallagh: They tend to confuse what belongs where (09:42:13 AM) sgallagh: pzuna: I guess what I'm asking is why you're not just making winbind_id_data a struct that contains prev and next pointers (09:42:23 AM) slep left the room (quit: Ping timeout: 258 seconds). (09:42:29 AM) sgallagh: Which should be allocated on 'state' (09:42:40 AM) sgallagh: Then you can use the DLIST_* functions from util.h to manipulate them (09:43:03 AM) pzuna: ok, I'll rework the whole id_list part (09:43:06 AM) pzuna: to use DLIST (09:43:17 AM) pzuna: it will be more consistent with the rest of the code base as well (09:43:31 AM) sgallagh: yes (09:44:22 AM) sgallagh: The other benefit you'll get from this is that you won't have to pass a special container type into store_users. (09:44:42 AM) sgallagh: Because it will just be the pointer to the first winbind_id_data (and then will read through any ->next pointers) (09:45:33 AM) sgallagh: pzuna: Make sure you allocate the memory for these pointers on a common parent context and NOT on the first pointer in the list though (09:45:47 AM) sgallagh: As I said above, this means it would be unsafe to remove the first entry, since that would free the rest. (09:45:57 AM) pzuna: ok (09:45:58 AM) sgallagh: So go one level up (probably to 'state') for a memctx (09:46:42 AM) boxybrown left the room (quit: Ping timeout: 240 seconds). (09:47:03 AM) sgallagh: pzuna: Ok, I have a few comments on winbind_id_list_sysdb_store_users() next (09:48:03 AM) sgallagh: Change the order of things around a bit with the transaction commit (09:48:35 AM) sgallagh: You want to be able to check whether the _commit() failed. If it did, you need to call _cancel() to ensure that we're not holding an extra transaction ID (09:48:51 AM) sgallagh: Otherwise, we'll never write to the disk again until SSSD is restarted. (09:49:03 AM) pzuna: ok (09:51:43 AM) sgallagh: pzuna: This is my usual construct: http://www.fpaste.org/IZwO/ (09:52:05 AM) sgallagh: (I typed that by hand, so there may be minor errors, like the DEBUG level) (09:52:14 AM) pzuna: ok (09:52:26 AM) pzuna: btw think we could continue in an hour or so? (09:53:44 AM) sgallagh: pzuna: Ok, please ping me. (09:53:51 AM) pzuna: will do, thanks! (11:52:40 AM) pzuna: sgallagh: ping, sorry it took a bit longer than expected (12:11:47 PM) sgallagh: pzuna: I'm back from lunch now. (12:12:31 PM) pzuna: sgallagh: ok, want to pick up where we stopped? (12:13:11 PM) sgallagh: pzuna: Sure, just a moment (12:16:17 PM) sgallagh: pzuna: Ok, we last left off with talking about DLIST, right? (12:17:12 PM) pzuna: zes (12:17:19 PM) pzuna: yes* (czech keyboard :)) (12:17:24 PM) sgallagh: :) (12:18:45 PM) sgallagh: Ok, just reading through the rest of the user lookup (12:19:20 PM) sgallagh: pzuna: In winbind_account_info_done(), you need to call talloc_zfree(req) after the _recv() functions. (12:19:42 PM) sgallagh: (Yes, it will eventually get cleaned up when the breq is freed, but it's best to maintain isolation) (12:20:42 PM) sgallagh: Ok, let's walk through user enum next (12:21:44 PM) sgallagh: pzuna: Is wbcSetpwent() a call that potentially blocks? (12:21:52 PM) pzuna: sgallagh: can I free it after calling breq->fn(...)? (12:21:57 PM) pzuna: sgallagh: or do I have to do it before? (12:22:22 PM) sgallagh: pzuna: You should always free the req as the first thing you do after calling the _recv() function (12:22:33 PM) pzuna: sgallagh: to be honest, I'm not sure (12:22:37 PM) pzuna: ok (12:23:02 PM) sgallagh: pzuna: That helps force a coding style that ensures you pay attention to your memory hierarchy (12:23:27 PM) sgallagh: Because you need to make sure you've done approprate talloc_steal() of any values being returned in the recv() function (12:23:35 PM) sgallagh: (None in this case, since it's just dp_error) (12:24:01 PM) sgallagh: pzuna: What I mean is: once the SIGCHLD work is ready, are you going to wrap that call in a fork()? (12:25:04 PM) sgallagh: If so, then wbcSetpwent and wbcEndpwent should be separate steps in the request (12:25:12 PM) pzuna: it will be wrapped in fork when the prefork work is readyy (12:25:13 PM) pzuna: ready* (12:25:17 PM) pzuna: all requests will be (12:25:32 PM) sgallagh: Ok, then we need to break this up into three steps (12:25:58 PM) sgallagh: s/steps/subreqs/ (12:26:12 PM) sgallagh: As each one completes, move on to the next one. (12:26:25 PM) sgallagh: Unless you're going to capture all three in a single child process (12:26:31 PM) sgallagh: (which I suppose isn't a bad idea0 (12:26:55 PM) pzuna: they are going to be in a single child, they're all part of a single request (12:27:47 PM) sgallagh: ok, then we'll worry about re-arranging this later (12:29:02 PM) sgallagh: pzuna: Could you please rearrange the code so that winbind_users_fetch_send() follows winbind_users_enum_send() (before winbind_users_enum_step()) (12:29:06 PM) sgallagh: It's easier to read. (12:29:28 PM) sgallagh: Actually, scratch that. (12:29:46 PM) sgallagh: On further thought, there's probably no reason to be separating this into a tevent_req loop at all. (12:29:59 PM) sgallagh: Oh wait (12:30:02 PM) sgallagh: Now I remember (12:30:18 PM) sgallagh: We did this so that it wouldn't block other requests. (12:30:24 PM) sgallagh: (Sorry, thinking as I go) (12:30:39 PM) sgallagh: That construct will disappear when we do the fork() stuff (12:31:26 PM) pzuna: :) (12:33:11 PM) sgallagh: pzuna: As previously discussed with the direct lookup, please change the winbind_users_fetch_recv() to just return a pointer to a struct passwd instead of the union (12:34:10 PM) pzuna: ok (12:34:50 PM) sgallagh: General rule: only return absolutely relevant data. Leave it up to the consumer to stick it into another variable if warranted. (12:35:12 PM) sgallagh: It ends up reducing clutter and multiple levels of indirection (12:37:33 PM) sgallagh: pzuna: Also, I'm thinking about how you're handling the recv() function for the enumeration, and I don't like it. (12:38:02 PM) mkosek1 left the room (quit: Ping timeout: 258 seconds). (12:38:23 PM) pzuna: sgallagh: what exactly you don't like about it? :) (12:38:25 PM) sgallagh: It's kind of a violation of compartmentalization to pass back the wbc_status (12:38:44 PM) pzuna: that's a pretty long word :D (12:38:55 PM) sgallagh: :-D (12:39:06 PM) sgallagh: I mean that it's not relevant to the calling function directly (12:39:21 PM) sgallagh: It would be better to just use the return errno code for making the decisions (12:39:48 PM) sgallagh: i.e. map WBC_ERR_NO_MEMORY to ENOMEM and WBC_ERR_DOMAIN_NOT_FOUND to ENOENT (12:40:33 PM) pzuna: sgallagh: that's what I'm doing in users_enum_step (12:40:39 PM) sgallagh: Then you can consider success or failure based entirely on (EOK || ENOENT) instead of testing both the pwd return for NULL *and* checking wbc_error (12:40:50 PM) pzuna: sgallagh: and DOMAIN_NOT_FOUND is the end of the list, to it's EOK (12:40:57 PM) sgallagh: I know... I'm telling you it's ugly that way :) (12:41:44 PM) pzuna: ok (12:41:49 PM) sgallagh: the _step() can be reduced to (if (ret != EOK && ret == ENOENT) goto done; (12:42:01 PM) sgallagh: And then loop again if it's !ENOENT (12:42:04 PM) pzuna: yeah, you're right just realized how you ment it (12:42:45 PM) sgallagh: Actually, you could use EAGAIN (loop) and EOK (finished) (12:42:52 PM) sgallagh: That might be more clear. (12:43:48 PM) pzuna: ok (12:44:55 PM) sgallagh: Ok, moving on to winbind_users_enum_recv() (12:45:24 PM) sgallagh: As discussed with direct lookups before, drop the id_list and just return a DLIST of winbind_id_data (12:45:43 PM) sgallagh: Ok, I think that's it for user enum (12:49:17 PM) sgallagh: pzuna: Ready to start down the 'group' paths? :) (12:49:35 PM) pzuna: sgallagh: /ready :) (12:49:49 PM) pzuna: sgallagh: but I suspect it will be the same things there (12:49:54 PM) sgallagh: Probably (12:50:00 PM) sgallagh: But I want to walk through the whole thing, just in case. (12:50:11 PM) sgallagh: initgroups might be more interesting, I admit (12:50:46 PM) sgallagh: actually, let's just jump to the store groups part (12:51:27 PM) sgallagh: Actually, here's a question: (12:51:45 PM) sgallagh: Under what circumstances could iter->id.grp be NULL if iter isn't NULL? (12:51:53 PM) sgallagh: pzuna: ^^ (12:53:28 PM) pzuna: sgallagh: well that's because of bad design in the way winbind_id_list work (12:53:42 PM) sgallagh: ok, so that will go away when id_list does? (12:53:45 PM) sgallagh: Good :) (12:53:55 PM) pzuna: yes :) (12:54:50 PM) sgallagh: Ok, I just need a minute to read through winbind_id_list_sysdb_populate_users() (12:55:47 PM) sgallagh: pzuna: Your transaction management in winbind_id_list_sysdb_populate_users() is wrong (12:55:58 PM) sgallagh: It's not safe if you ever passed in in_transaction == false (12:56:24 PM) sgallagh: It's probably best to just always start and commit/cancel a transaction in this function (12:56:30 PM) sgallagh: Use the same construct I mentioned earlier. (12:56:42 PM) sgallagh: Nested transactions are safe in LDB, so that's going to be a better move. (12:57:27 PM) pzuna: ok, I didn't know that (12:58:08 PM) sgallagh: pzuna: You can nest transactions safely and they won't write to the disk until all transactions are committed (12:58:40 PM) sgallagh: pzuna: I just noticed something about the user lookups (12:58:57 PM) sgallagh: You're not handling deleted users anywhere (12:59:44 PM) sgallagh: winbind_users_get_recv() needs to be able to handle ENOENT if the user is not present in AD (01:00:10 PM) sgallagh: If that happens, we need to delete the user from the SYSDB (01:00:19 PM) sgallagh: This is a necessary security precaution (01:01:05 PM) pzuna: sgallagh: only in the case of fetching a single user if I understand correctly (01:01:07 PM) sgallagh: Correction, that needs to be done in winbind_users_get_done() (01:01:31 PM) sgallagh: pzuna: In the case of enumeration it's trickier :) (01:02:17 PM) sgallagh: pzuna: Because you now need to compare the complete lists on both AD and sysdb (01:02:29 PM) sgallagh: AD's list is *always* authoritative. (01:02:46 PM) sgallagh: So if it missing any entries present in sysdb, they must be removed. (01:02:53 PM) sgallagh: s/it/it is/ (01:07:47 PM) pzuna: sgallagh: isn't it easier to just flush all users in sysdb and add the new ones? (01:08:04 PM) sgallagh: pzuna: Way too expensive in a large system (01:08:16 PM) pzuna: sgallagh: I thought it would be the other way around :) (01:08:21 PM) sgallagh: Since most if not all users/groups will have member/memberOf linkages that need to be processed (01:08:47 PM) sgallagh: Chances are, there will only be one or two entries that disappear at a time. (01:14:05 PM) sgallagh: pzuna: Sorry, got pulled into another discussion (01:14:11 PM) sgallagh: I'm back now (01:14:32 PM) sgallagh: So that's going to require some thought. (01:15:57 PM) sgallagh: It's been two years since I looked at how the LDAP provider handles that, so I'm not sure how we handle it there (01:16:05 PM) sgallagh: Probably worth doing a little digging. (01:17:42 PM) pzuna: I'll take a look and try to figure something out (01:17:46 PM) sgallagh: Ok (01:17:54 PM) sgallagh: Put that on the back-burner for now (01:18:17 PM) sgallagh: I can live without that for a first pass (though it has to be fixed before we exit "experimental")\ (01:18:43 PM) sgallagh: pzuna: We'll open a bug on that (01:19:13 PM) sgallagh: Other than that, the rest of my comments on the group lookups are roughly the same as the user lookups (01:19:26 PM) sgallagh: Let's take a look at initgroups (01:22:13 PM) sgallagh: pzuna: Please change winbind_groups_by_user_step() so that you're not storing groups until you've retrieved all of the values. (01:22:28 PM) sgallagh: Otherwise, you're doing a disk-write for every group that a user is a member of. OUCH (01:23:13 PM) sgallagh: Also, as part of the initgroups request, you need to retrieve the actual user. (01:24:41 PM) sgallagh: So you probably want to start by doing a winbind_users_query_send() and then do the wbcGetGroups(), loop through the winbind_groups_query_send() and then at the end, save it all in a single transaction. (01:26:20 PM) pzuna: sgallagh: actually in groups_by_user_step it only stores groups when done... I know it's not completely clear at first sight (01:26:44 PM) pzuna: sgallagh: I should probably rearrange that function... it only stores when index reaches count, otherwise it calls send again (01:26:45 PM) sgallagh: pzuna: Comments might help :) (01:27:05 PM) sgallagh: But as I said, you still need to do the user lookup too (01:27:14 PM) sgallagh: That way the sysdb can connect the groups to the user properly (01:27:20 PM) pzuna: definitely, but I've seen next to zero comments in SSSD sources, so I thought there's a policy against them :) (01:27:31 PM) sgallagh: I deserved that :) (01:27:45 PM) pzuna: :) (01:28:40 PM) sgallagh: ok, that's pretty much the entire ID provder there. (01:29:03 PM) sgallagh: Do we want to move on to auth, or are you sufficiently overwhelmed for today? (01:30:20 PM) pzuna: that will do for now I guess, we can do auth later (01:30:40 PM) pzuna: it will probably take at least tomorrow to get all of this done (right) :) (01:30:52 PM) sgallagh: pzuna: Ok, I hope you took good notes (or at least logged the conversation) ;-) (01:30:55 PM) pzuna: it's getting a bit late here :) (01:31:02 PM) pzuna: logged ;-) (01:31:04 PM) sgallagh: Understood (01:31:24 PM) pzuna: ok thanks for the comments and advice (01:31:27 PM) sgallagh: pzuna: If you need to ask questions, please don't hesitate (01:31:37 PM) pzuna: ok I won't :) (01:31:38 PM) sgallagh: pzuna: No problem. Sorry it took so long to find the time to do so (01:31:45 PM) pzuna: no problem
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
