URL: https://github.com/SSSD/sssd/pull/269 Author: NWilson Title: #269: Add support for ActiveDirectory's logonHours restrictions Action: opened
PR body: """ This is a straightforward patch for denying access to a user when the user is not permitted to access their account due to logonHours restrictions.
This matches the default behaviour for domain-joined Windows machines. When outside the logonHours, all types of authentication are denied (password/Kerberos/certificate) - so it is appropriate to put this check inside the PAM "account" rules. """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/269/head:pr269 git checkout pr269
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
centos-ci commented: """ Can one of the admins verify this patch? """
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-300793133
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
centos-ci commented: """ Can one of the admins verify this patch? """
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-300793139
URL: https://github.com/SSSD/sssd/pull/269 Author: NWilson Title: #269: Add support for ActiveDirectory's logonHours restrictions Action: edited
Changed field: body Original value: """ This is a straightforward patch for denying access to a user when the user is not permitted to access their account due to logonHours restrictions.
This matches the default behaviour for domain-joined Windows machines. When outside the logonHours, all types of authentication are denied (password/Kerberos/certificate) - so it is appropriate to put this check inside the PAM "account" rules. """
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
fidencio commented: """ ok to test """
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-300797835
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
jhrozek commented: """ Thank you very much for the patch.
I have some suggestions: 1) I would actually suggest to take a look at samba's `logon_hours_ok` function. I haven't checked carefully if the two make any difference function-wise, but my first instinct would be to reuse what other projects already have rather than implementing the functionality from scratch.
2) Even though on general it makes sense to behave as Windows clients do, at least with my RHEL maintainer hat on, I'm not thrilled about adding another login restriction by default. This could bite environments where the logonHours are set, but the admins rely on Linux clients not "seeing" the attribute during upgrade to a new version. So I would suggest to make the new attribute evaluation configurable during configure time -- I guess if the option value was not set, we would just skip this check and the configure time option would just set the correct attribute value so that the option would be enforced. """
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-301736959
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
NWilson commented: """ Thanks, I'll look into those things.
1. Regarding samba's `logon_hours_ok` - it's a static function, so we can't call it directly. It's pretty-much identical to the short method I've written in this PR, only they've added a little bit more debug-level logging. The only way to re-use their method would be to call their `authsam_account_ok` which does quite a bit of other stuff, some of it samba-specific I think. I don't really know the samba codebase, and how much benefit their is to trying to share this bit of functionality. sssd includes its own framework for querying and caching account records, so it's not going to fit in very well. 2. Would it be better to have a runtime switch, rather than a compile-time switch? If RHEL is going to compile this feature out, there's not much benefit to adding it (since our customers will be using stock RHEL). If it were a runtime parameter, it could default to 'on' with a Release Note explaining to customers they can turn it off if they prefer to. """
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-301754453
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
NWilson commented: """ Thanks, I'll look into those things.
1. Regarding samba's `logon_hours_ok` - it's a static function, so we can't call it directly. It's pretty-much identical to the short method I've written in this PR, only they've added a little bit more debug-level logging. The only way to re-use their method would be to call their `authsam_account_ok` which does quite a bit of other stuff, some of it samba-specific I think. I don't really know the samba codebase, and how much benefit their is to trying to share this bit of functionality. sssd includes its own framework for querying and caching account records, so it's not going to fit in very well. 2. Would it be better to have a runtime switch, rather than a compile-time switch? If RHEL is going to compile this feature out, there's not much benefit to adding it (since our customers will be using stock RHEL). If it were a runtime parameter, it could default to 'on' with a Release Note explaining to customers they can turn it off if they prefer to. """
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-301754453
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
jhrozek commented: """ On Tue, May 16, 2017 at 04:33:22AM -0700, Nicholas Wilson wrote:
Thanks, I'll look into those things.
- Regarding samba's `logon_hours_ok` - it's a static function, so we can't call it directly. It's pretty-much identical to the short method I've written in this PR, only they've added a little bit more debug-level logging. The only way to re-use their method would be to call their `authsam_account_ok` which does quite a bit of other stuff, some of it samba-specific I think. I don't really know the samba codebase, and how much benefit their is to trying to share this bit of functionality. sssd includes its own framework for querying and caching account records, so it's not going to fit in very well.
Ah, sorry, I didn't mean to call the function, but more or less copy its contents :-)
(module our differences in debug messages etc..)
But as I said, I didn't study the differences into detail and perhaps you're right. I need to check during a more careful review round..
- Would it be better to have a runtime switch, rather than a compile-time switch? If RHEL is going to compile this feature out, there's not much benefit to adding it (since our customers will be using stock RHEL). If it were a runtime parameter, it could default to 'on' with a Release Note explaining to customers they can turn it off if they prefer to.
Let me explain better what I was proposing. Many of the options can be set (on the source level) to NULL. In the source it's often done to denote an 'unset' option.
I was proposing to have a configure switch (--enable-ad-logon-hours-check maybe?) that, if selected, would set the value of the ldap_user_ad_logon_hours option to the expected attribute value logonHours. But when disabled, this option would set the attribute value to NULL in the src/providers/ldap/ldap_opts.c source. Presumably (but I didn't test this) the check should be skipped with an allowed return code if the attribute value that points to the logon hours attribute is NULL.
So conservative distributions would then configure sssd on the source level with --disable-ad-logon-hours and perhaps Fedora or other distributions where changes are more expected could flip this switch on by default.
Please note that even if the option was set to NULL by default on the source level (so, RHEL), the admin could still opt-in for the feature simply by setting: ldap_user_ad_logon_hours = logonHours in sssd.conf to get this feature. So there is also a runtime switch, the difference is the defaults.
The other way around (opting out) is a bit more clunky, but still doable with: ldap_user_ad_logon_hours = somethingthatdoesntexist
The configure-level switch is by the way what we did when we introduced GPO access control to RHEL. There, RHEL-7 also defaults to "permissive", so it allows access even if GPOs would have denied it, but Fedora (which will eventually become RHEL-8) already defaults to "enforcing" and GPOs are evaluated.
I hope it makes more sense now.
"""
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-301873477
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
jhrozek commented: """ @NWilson would you like me to help move this PR forward by writing a patch on top of yours that allows configure-time selection of the default? """
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-304274934
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
lslebodn commented: """
@NWilson would you like me to help move this PR forward by writing a patch on top of yours that allows configure-time selection of the default?
I would rather prefer to set default value of option to NULL rather then creating configure time option for any new "controversial" feature. + update man page for `sssd-ad`
BTW man `sssd-ldap` says that default value of `ldap_user_ad_logon_hours` is `logonHours` But such attribute is not part of rfc2307 or rfc2307 bis schema. So it should not be set ( == NULL) for standard ldap schemas anyway. """
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-304313809
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
jhrozek commented: """ On Fri, May 26, 2017 at 08:32:47AM -0700, lslebodn wrote:
@NWilson would you like me to help move this PR forward by writing a patch on top of yours that allows configure-time selection of the default?
I would rather prefer to set default value of option to NULL rather then creating configure time option for any new "controversial" feature. + update man page for `sssd-ad`
Why controversial? I think it's the same thing as GPOs -- it's something that Windows would have enforced, so sssd-ad (which in this respects pretents it's a Windows box) honors that.
Or in other words -- if we were writing sssd-ad from scratch and could implement all features at once, this is something we would implement, I think.
BTW man `sssd-ldap` says that default value of `ldap_user_ad_logon_hours` is `logonHours` But such attribute is not part of rfc2307 or rfc2307 bis schema. So it should not be set ( == NULL) for standard ldap schemas anyway.
As pointed out in the review :)
"""
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-304315615
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
NWilson commented: """ Thanks for reminding me, it's on a post-it note on my monitor at work actually, I just haven't had time to look into it this week :( It does need to be done so the PR doesn't get "stuck".
I've noted your comments and was planning to address them. However, if you have time to make the changes I'd be grateful.
I'll find a few hours next week to have a go - it sounds like the configure-time parameter default is the way to go, so I'll go ahead on that basis.
----- Nicholas Wilson: nicholas@nicholaswilson.me.uk 29 Beales Way, CB4 2PW 07845 182898
On 26 May 2017 at 13:51, Jakub Hrozek notifications@github.com wrote:
@NWilson https://github.com/nwilson would you like me to help move this PR forward by writing a patch on top of yours that allows configure-time selection of the default?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SSSD/sssd/pull/269#issuecomment-304274934, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWLl86W6TSTaGNkC_23Fba3kTeH7Vgzks5r9srhgaJpZM4NYBAJ .
"""
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-304371171
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
NWilson commented: """ Thanks for reminding me, it's on a post-it note on my monitor at work actually, I just haven't had time to look into it this week :( It does need to be done so the PR doesn't get "stuck".
I've noted your comments and was planning to address them. However, if you have time to make the changes I'd be grateful.
I'll find a few hours next week to have a go - it sounds like the configure-time parameter default is the way to go, so I'll go ahead on that basis.
----- Nicholas Wilson: nicholas@nicholaswilson.me.uk 29 Beales Way, CB4 2PW 07845 182898
On 26 May 2017 at 13:51, Jakub Hrozek notifications@github.com wrote:
@NWilson https://github.com/nwilson would you like me to help move this PR forward by writing a patch on top of yours that allows configure-time selection of the default?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SSSD/sssd/pull/269#issuecomment-304274934, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWLl86W6TSTaGNkC_23Fba3kTeH7Vgzks5r9srhgaJpZM4NYBAJ .
"""
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-304371171
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
NWilson commented: """ By the way, should I be concerned about the CI failure that the bot reported? """
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-304371721
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
jhrozek commented: """ On Fri, May 26, 2017 at 12:40:56PM -0700, Nicholas Wilson wrote:
By the way, should I be concerned about the CI failure that the bot reported?
At one point, the CI tests were a bit unstable but let me retest the patch..
"""
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-304590669
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
jhrozek commented: """ retest this please """
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-304590696
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
jhrozek commented: """ First, I'm sorry this PR stalled for several months.
I finally had some time to re-check it and the primary thing I would suggest here is to not add the code into the generic LDAP expiration code. I think it belongs more to the `ad_access.c` module, because in general I would prefer to not add more AD-specific access controls into the generic LDAP module and also because IPA has been planning for a long time to add a time-based host access control policy and I think in IPA environments with AD trusted users, the AD logon hours policy should not be evaluated.
So here's what I would propose to do: - move the evaluation function to the `ad_acess.c` module - unset the LDAP attribute mappings for IPA and LDAP case - add a configure option which would be disabled for now, but enabled in a future release that would enable this functionality for the AD provider. (note: Do we need a runtime switch like we have for GPOs here as well?) """
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-327159735
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
jhrozek commented: """ @sumit-bose what do you think? """
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-327159813
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
sumit-bose commented: """ I'm a bit torn apart. On one had I agree that new AD specific functionality should be added to the AD provider. On the other hand the check is similar to ldap_user_ad_account_expires and ldap_user_ad_user_account_control and there is already ldap_user_nds_login_allowed_time_map which is the same functionality with an NDS specific attribute. So putting the code in sdap_access.c is ok imo.
Maybe it can be seen this way. Although the attribute is used by AD it can be easily added to any LDAP server to control time based access for users based on the logic used by AD. This is in contrast to the handling of GPOs which cannot easily added to any other environment than AD.
I think the attribute mapping can be set for LDAP (see paragraph about) but should be unset for IPA.
About enabling and backwards compatibility. What about adding a new account expire policy like e.g. 'ad_x' which includes everything the 'ad' policy checks plus logonHours? I would document that this policy might change in future becasue someone might want to implement support for the userWorkstation attribute (https://msdn.microsoft.com/en-us/library/ms680868(v=vs.85).aspx) as well.
"""
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-328160365
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
simo5 commented: """ Nobody in their right mind should implement the legacy thing there is in AD, so it wouldn't be a bad thing to keep it in the AD provider (it is *very* specific to AD :) """
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-328170680
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
jhrozek commented: """ So I finally had some time to test this PR (sorry again for the long delays..).
There is a trivial fix that needs to be squashed to prevent segfaults for users with no logonHours: ``` - } else if (ad_account_outside_logonhours(logon_hours->data)) { + } else if (logon_hours != NULL + && ad_account_outside_logonhours(logon_hours->data)) { ```
Also, MIN() and MAX() were not defined on my system, but that's a one-liner for each macro.
But, more importantly, the logonHours attribute is not replicated to the Global Catalog, so with our current model of preferring the GC, we don't fetch the attribute. It would be trivial to make the PAM account requests hit the LDAP port, but then with our current LDAP attribute handling, any subsequent GC request would treat the attribute as removed.
I currently don't know how to solve that except disabling the GC support in case someone enabled the logonHours support (which implies adding a runtime option for the logonHours support, defaulting to false).
The comments I had previously about the IPA environments were not valid at least on IPA clients, because those do not receive the attribute through the extdom operation. I haven't tested an IPA server yet. """
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-332320647
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
jhrozek commented: """ We need to first work on https://pagure.io/SSSD/sssd/issue/3538, until then, this PR is unfortunately blocked. """
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-380481010
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
Label: +Blocked
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
alexey-tikhonov commented: """
We need to first work on https://pagure.io/SSSD/sssd/issue/3538, until then, this PR is unfortunately blocked.
This issue was closed as "wontfix" so perhaps this PR should be closed as well. """
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-658771564
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
alexey-tikhonov commented: """
We need to first work on https://pagure.io/SSSD/sssd/issue/3538, until then, this PR is unfortunately blocked.
This ticket was closed as WONTFIX. """
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-629236146
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
thalman commented: """ I'm sorry but we decided not to include this PR. """
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-686523658
URL: https://github.com/SSSD/sssd/pull/269 Title: #269: Add support for ActiveDirectory's logonHours restrictions
thalman commented: """ I'm sorry but we decided not to include this PR. The work on GC (https://pagure.io/SSSD/sssd/issue/3538) has been closed as WONTFIX. """
See the full comment at https://github.com/SSSD/sssd/pull/269#issuecomment-686523658
URL: https://github.com/SSSD/sssd/pull/269 Author: NWilson Title: #269: Add support for ActiveDirectory's logonHours restrictions Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/269/head:pr269 git checkout pr269
sssd-devel@lists.fedorahosted.org
-
alexey-tikhonov -
centos-ci
-
fidencio
-
jhrozek
-
lslebodn
-
NWilson
-
simo5
-
sumit-bose
-
thalman