7:53 a.m.
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
9:53 a.m.
Hi,
On 2011-11-08 15:53, Pavel Zuna wrote:
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.
I looked at the configuration options provided and I think some
additions might be helpful.
+char const *winbind_option_names[WINBIND_OPTS_COUNT] = {
In general, how do you setup "shared" things like logging level which is
already done in sssd.conf but there seems to be no corresponding Winbind
option available? For example, if one sets SSSD logging level to 9 will
Winbind then be equally verbose in its logging?
+ "template shell", /* WINBIND_OPT_TEMPLATE_SHELL */
Will this play nicely with existing sssd.conf options like vetoed_shells
and shell_fallback? If "template shell" is being provided do you think
"template homedir" would be useful, too?
It should be possible to adjust "client ldap sasl wrapping", "ldap
ssl",
and "ldap ssl ads". "ldap debug level" probably not needed.
I would be nice if one could set "winbind normalize names = yes".
Not sure would "winbind use default domain = yes" be useful / possible.
Same for "create krb5 conf = no".
Cheers,
--
Marko Myllynen
10:43 a.m.
On Tue, Nov 08, 2011 at 05:53:03PM +0200, Marko Myllynen wrote:
Hi,
On 2011-11-08 15:53, Pavel Zuna wrote:
> 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.
I looked at the configuration options provided and I think some
additions might be helpful.
+char const *winbind_option_names[WINBIND_OPTS_COUNT] = {
In general, how do you setup "shared" things like logging level which is
already done in sssd.conf but there seems to be no corresponding Winbind
option available? For example, if one sets SSSD logging level to 9 will
Winbind then be equally verbose in its logging?
+ "template shell", /* WINBIND_OPT_TEMPLATE_SHELL */
Will this play nicely with existing sssd.conf options like vetoed_shells
and shell_fallback? If "template shell" is being provided do you think
"template homedir" would be useful, too?
It should be possible to adjust "client ldap sasl wrapping", "ldap
ssl",
and "ldap ssl ads". "ldap debug level" probably not needed.
Are you sure the ldap options are used by winbind?
I would be nice if one could set "winbind normalize names = yes".
Not sure would "winbind use default domain = yes" be useful / possible.
Same for "create krb5 conf = no".
I think most, if not all, comments are valid. But the idea was to have a
basic sssd winbind provider as an experimental feature in the sssd
master branch and add more features over time. Would you mind to open an
enhancement ticket in trac and add your comments there?
Thank you.
bye,
Sumit
6:15 a.m.
Hi,
>> 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.
>
> I looked at the configuration options provided and I think some
> additions might be helpful.
>
> It should be possible to adjust "client ldap sasl wrapping", "ldap
ssl",
> and "ldap ssl ads". "ldap debug level" probably not needed.
Are you sure the ldap options are used by winbind?
yes, with ldap ssl = start tls / ldap ssl ads = yes winbindd will try to
use StartTLS. client ldap sasl wrapping affects to all users of the the
libads code in Samba (which are net client utility mostly).
> Not sure would "winbind use default domain = yes" be
useful / possible.
> Same for "create krb5 conf = no".
I think most, if not all, comments are valid. But the idea was to have a
basic sssd winbind provider as an experimental feature in the sssd
master branch and add more features over time. Would you mind to open an
enhancement ticket in trac and add your comments there?
Sure. I created two tickets, the first one should be pretty
straightforward, it's just about adding configuration options not
needing any additional SSSD/Winbind logic:
https://fedorahosted.org/sssd/ticket/1084
And the other for options which might require some additional logic
(e.g., template shell vs allowed_shells/vetoed_shells/shell_fallback):
https://fedorahosted.org/sssd/ticket/1085
FWIW, those who are testing the Winbind backend and joining a machine to
a 2008R2 domain should be probably aware that currently Samba does not
provide AES keys causing issues with krb5, I've recently filed these BZs
about the issue:
https://bugzilla.redhat.com/show_bug.cgi?id=748407
https://bugzilla.redhat.com/show_bug.cgi?id=748528
Cheers,
--
Marko Myllynen
1:09 p.m.
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.
8:01 a.m.
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
4557
days inactive
4572
days old
sssd-devel@lists.fedorahosted.org
5 comments
4 participants
participants (4)
-
Marko Myllynen -
Pavel Zuna -
Stephen Gallagher -
Sumit Bose