[prev in list] [next in list] [prev in thread] [next in thread] 

List:       sssd-devel
Subject:    [SSSD] [PATCH] Add Winbind provider.
From:       sgallagh () redhat ! com (Stephen Gallagher)
Date:       2011-11-23 14:01:26
Message-ID: 1322056886.2263.4.camel () sgallagh520 ! ipa ! sgallagh ! bos ! redhat ! com
[Download RAW message or body]

On Tue, 2011-11-15 at 14:09 -0500, Stephen Gallagher wrote:
> 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.

Just to complete the review, I also did a review of the authentication
and password-change code today.

(08:07:17 AM) sgallagh: pzuna: Ok, so I'm looking at the auth/chpass
provider now.
(08:07:39 AM) sgallagh: pzuna: What does it mean if
wbcAuthenticateUser() returns WBC_ERR_PWD_CHANGE_FAILED?
(08:08:41 AM) pzuna: sgallagh: hmm don't remember
(08:08:59 AM) sgallagh: You have it returning PAM_CRED_ERR
(08:09:29 AM) pzuna: yeah I know, it's probably something I added when
redoing the wbc_status processing
08:10
(08:10:41 AM) sgallagh: pzuna: I'm trying to figure out if it means
anything interesting, like "password-change required"
(08:11:40 AM) pzuna: sgallagh: I'm looking at wbinfo sources right now
to see if there are any clues
(08:14:56 AM) pzuna: looks like it's never going to return this error
code, so we can delete the case
08:15
(08:15:12 AM) sgallagh: What does it return if the password is in the
grace period?
(08:15:23 AM) sgallagh: Are we handling that?
(08:16:02 AM) sgallagh: Or does it even present that interface?
(08:18:15 AM) pzuna: looking at wbinfo sources I don't think it does,
better ask someone from the samba team
(08:18:33 AM) sgallagh: pzuna: Ok, not a problem for now.
(08:19:07 AM) sgallagh: I was just trying to determine if that error
code was a confusing way of doing that
08:20
(08:20:47 AM) sgallagh: pzuna: You're getting confused with tevent_req
error codes and results of the auth.
(08:21:30 AM) sgallagh: You should only call tevent_req_error() if
something happened that prevents you from getting a definitive answer
from winbind
(08:21:55 AM) sgallagh: Also, you're mixing error codes in
winbind_pam_authenticate_send()
(08:22:07 AM) sgallagh: Sometimes you're returning PAM error codes,
sometimes errno.
(08:22:11 AM) sgallagh: That's never safe.
(08:22:41 AM) sgallagh: What you want to do is add a new variable to
winbind_pam_authenticate_state to store the PAM error code.
(08:23:31 AM) sgallagh: The return value of the tevent_req should
indicate whether the request ITSELF succeeded.
(08:24:20 AM) sgallagh: pzuna: So your _recv() function should have a
separate return variable for the pam_result.
08:25
(08:25:30 AM) sgallagh: pzuna: Also, don't use the same variable (ret)
when dealing with functions returning different error code types. It
gets confusing.
(08:25:41 AM) sgallagh: pzuna: Are you still there?
(08:26:07 AM) pzuna: yes
(08:27:52 AM) sgallagh: Ok, you were quiet :)
(08:29:36 AM) sgallagh: pzuna: Do you understand the problem there?
(08:29:53 AM) pzuna: I do, already reworking it as we talk :)
08:30
(08:30:00 AM) sgallagh: ok, great
(08:30:32 AM) sgallagh: I realize there are some places where we're not
maintaining that strict separation between tevent_req failure and
internal results.
(08:30:47 AM) sgallagh: (Such as places where we allow lookups to return
ENOENT)
(08:31:19 AM) sgallagh: But in general, the goal is that tevent_req
errors should indicate errors with the request itself, and if the
results are failures, that should be reported separately.
08:35
(08:38:54 AM) sgallagh: pzuna: Why are you using talloc_zero_size() and
memcpy in winbind_pam_chauthtok_send()?
(08:39:01 AM) sgallagh: What's wrong with talloc_memdup()?
(08:39:58 AM) pzuna: sgallagh: I'm doing the same thing in
pam_authententicate_send as welll
08:40
(08:40:18 AM) pzuna: no reason, I can use talloc_memdup if it's the
preferred way
(08:40:21 AM) sgallagh: So you are. Same question :)
(08:40:44 AM) sgallagh: Actually, given that password is a char *, why
not use talloc_strdup()?
(08:41:15 AM) pzuna: sgallagh: it's unsigned char and not sure if it's
zero terminated
(08:41:34 AM) sgallagh: Then it should be uint8_t instead of unsigned
char
(08:42:23 AM) pzuna: unsigned char is coming from PAM, I don't know if
it's zero terminated or not... so I thought it's safer to use memcpy
(08:42:36 AM) sgallagh: You're right.
(08:42:51 AM) sgallagh: (also, unsigned char == uint8_t)
(08:43:01 AM) sgallagh: But it's more obvious if you treat it as a byte
array
(08:43:46 AM) sgallagh: Would you mind changing "password" to be uint8_t
* and using talloc_array() instead of talloc_zero_size()?
(08:44:11 AM) pzuna: I wouldn't, but why not use memdup?
(08:44:13 AM) sgallagh: It's better with talloc to stay type-safe where
possible
(08:44:24 AM) sgallagh: Because with memdup we can't add the
null-terminator that you need :)
(08:44:41 AM) simo: sgallagh, why talloc_array ?
(08:44:46 AM) simo: use talloc_memdup()
08:45
(08:45:04 AM) pzuna: sgallagh: true
(08:45:23 AM) sgallagh: simo: Can't use talloc_memdup, as just expressed
(08:45:33 AM) sgallagh: Current state has no null-termination guarantee
(08:45:35 AM) simo: is this an actual password ?
(08:45:38 AM) sgallagh: Yes
(08:45:41 AM) simo: sgallagh, are you sure ?
(08:45:49 AM) simo: what interface doesn't have null guarantee ?
(08:45:53 AM) sgallagh: PAM
(08:45:57 AM) sgallagh: the authtok
(08:45:58 AM) simo: really ?
(08:46:09 AM) sgallagh: It gives us a size and an array of unsigned char
(08:48:51 AM) sgallagh: simo: And I prefer to use talloc_array() over
talloc_size where possible, because it maintains the possibility of
doing type-checking if we want/need to
08:50
(08:53:42 AM) sgallagh: pzuna: You have the same problem with mixing
error codes in the password-change code
(08:54:19 AM) pzuna: sgallagh: I know :)
(08:54:21 AM) sgallagh: simo: Regarding the authtok, it's possible to
send a binary blob with no null-termination into the authtok field. Such
as smart-card data
(08:54:33 AM) sgallagh: pzuna: Just being thorough in my review :
(08:54:34 AM) sgallagh: :)
(08:54:39 AM) pzuna: :)
08:55
(08:56:15 AM) simo: sgallagh, ack
(08:59:31 AM) sgallagh: pzuna: I think that's all I have for the PAM
stuff.
(08:59:36 AM) sgallagh: Looks pretty good
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
Url : https://fedorahosted.org/pipermail/sssd-devel/attachments/20111123/793a652e/attachment.bin 

[prev in list] [next in list] [prev in thread] [next in thread]