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
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,
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
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,
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.
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
sssd-devel@lists.fedorahosted.org