Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for ActiveDirectory's logonHours restrictions #269

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@NWilson
Copy link

commented May 11, 2017

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.

Tested on an AD-joined machine (Ubuntu 160.4) with sssd compiled from source. Tested with an account that has loginHours set to deny & to permit.

@centos-ci

This comment has been minimized.

Copy link
Collaborator

commented May 11, 2017

Can one of the admins verify this patch?

1 similar comment
@centos-ci

This comment has been minimized.

Copy link
Collaborator

commented May 11, 2017

Can one of the admins verify this patch?

@fidencio

This comment has been minimized.

Copy link
Contributor

commented May 11, 2017

ok to test

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented May 16, 2017

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.

@NWilson

This comment has been minimized.

Copy link
Author

commented May 16, 2017

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 there 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. Would you really want to migrate the entire access-control mechanism over to being handled by samba?
  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.
@@ -172,6 +172,7 @@ struct sdap_attr_map rfc2307_user_map[] = {
{ "ldap_user_authorized_service", "authorizedService", SYSDB_AUTHORIZED_SERVICE, NULL },
{ "ldap_user_ad_account_expires", "accountExpires", SYSDB_AD_ACCOUNT_EXPIRES, NULL},
{ "ldap_user_ad_user_account_control", "userAccountControl", SYSDB_AD_USER_ACCOUNT_CONTROL, NULL},
{ "ldap_user_ad_logon_hours", "logonHours", SYSDB_AD_LOGON_HOURS, NULL},

This comment has been minimized.

Copy link
@jhrozek

jhrozek May 16, 2017

Contributor

The generic RFC2307 and RFC2307bis maps should default the attribute value to NULL. Only the AD LDAP map should set the attribute value to logonHours.

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented May 16, 2017

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented May 26, 2017

@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?

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented May 26, 2017

@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.

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented May 26, 2017

@NWilson

This comment has been minimized.

Copy link
Author

commented May 26, 2017

@NWilson

This comment has been minimized.

Copy link
Author

commented May 26, 2017

By the way, should I be concerned about the CI failure that the bot reported?

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

retest this please

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2017

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?)
@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2017

@sumit-bose what do you think?

@sumit-bose

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2017

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.

@simo5

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2017

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 :)

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2017

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.

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2018

We need to first work on https://pagure.io/SSSD/sssd/issue/3538, until then, this PR is unfortunately blocked.

@jhrozek jhrozek added Blocked and removed Changes requested labels Apr 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.