Bug 590062

Created attachment 413256 [details]
account policy plugin 1.0 (inactivity limits)

The source in this tarball is for account policy plug-in 1.0, which currently only supports one policy: account inactivity limits.  It also records bind timestamp to support the account inactivity limit.  This is made available under GPLv2.

*** Bug 580956 has been marked as a duplicate of this bug. ***

Are there any test scripts for this plugin?

The tests I used were non-portable unit tests, not integrated to tetframework.  Also, getting approval for sharing them would meet with difficulty today.

I wrote this plug-in in 2006 in response to customer needs.  Since then there has been discussion of account inactivity on the ldapext mailing list, particularly around the middle of 2010.  The 10th iteration of Prasanta's password policy draft has password idling added:  http://tools.ietf.org/html/draft-behera-ldap-password-policy-10

Since my plug-in predates the draft there are some differences:
1) I define as account policy rather than password policy.  More account policies would be added to the plug-in, for example for account expiration.  If I had thought of it as a password policy I would have implemented in the frontend (libslapd) along with the current password policy (which ought to be moved out to a plug-in.  Also, updated for compatibility with the new PWP drafts - many things have changed, e.g. password expiration time is now computed at authentication relative to password change timestamp, rather than our absolute expiration timestamp).
2) I solved my provisioning problem where a new user doesn't have a last authentication time by reverting to createTimestamp when lastLoginTimestamp is missing.  I don't think this situation is addressed in the draft.
3) The schema differs, mine is defined in account policy terms under HP's OID, but otherwise I felt they're pretty similar.

HP's account inactivity policy schema:
( 2.16.840.1.113719.1.1.4.1.35 NAME 'lastLoginTime'
  DESC 'Last login time'
  SYNTAX 1.3.6.1.4.1.1466.115.121.1.24
  SINGLE-VALUE
  USAGE directoryOperation
  X-ORIGIN 'Account Policy Plugin' )

( 1.3.6.1.4.1.11.1.3.2.1.3 NAME 'accountInactivityLimit'
  DESC 'Account inactivity limit'
  SYNTAX 1.3.6.1.4.1.1466.115.121.1.15
  SINGLE-VALUE
  X-ORIGIN 'Account Policy Plugin' )

Password policy draft 10's password idling schema:
( 1.3.6.1.4.1.42.2.27.8.1.29 NAME 'pwdLastSuccess'
  DESC 'The timestamp of the last successful authentication'
  EQUALITY generalizedTimeMatch
  ORDERING generalizedTimeOrderingMatch
  SYNTAX 1.3.6.1.4.1.1466.115.121.1.24
  SINGLE-VALUE
  NO-USER-MODIFICATION
  USAGE directoryOperation )

( 1.3.6.1.4.1.42.2.27.8.1.26 NAME 'pwdMaxIdle'
  EQUALITY integerMatch
  SYNTAX 1.3.6.1.4.1.1466.115.121.1.27
  SINGLE-VALUE )

Ok.  Which would you prefer?  Get the current account policy plugin into 389 now, or revise it to make it compatible with the current password policy I-D?

I would go with the current implementation for now.

There's no telling how long it will take for this part of the I-D to stabilize.  Reviewing an ldapext exchange from 8/10/2010 indicates more changes coming: the current I-D idle password policy might start disabling individual unused passwords while inactive account disabling is shoved off to a future account policy draft.  I take this to mean that my account policy implementation can co-exist with what's in the password policy I-D since.

By the way, I was already thinking that someone would have other ideas for where the login timestamp should be stored so in the plug-in's configuration entry one can specify what attribute name to use.

Created attachment 450337 [details]
0001-add-the-account-policy-plugin-and-related-server-cod.patch

This differs from the previous patch in a couple of ways:
1) adds a configure switch --enable-acctpolicy (on by default) so that it is possible to disable building the plugin and schema
2) fixes some minor problems in the schema and config entries
3) fixes some minor memory leaks and compiler warnings

Created attachment 450495 [details]
0002-fix-pblock-memory-leak.patch

Created attachment 450496 [details]
0003-do-not-register-pre-post-op-plugins-if-disabled.patch

I'm reviewing the patches.  Please help me understanding the spec...

1. Why bitwise is enabled together when acctpolicy is?  (The Account Policy Plugin it self does not depends on the bitwise plugin...)
+if enable_acctpolicy
+LIBACCTPOLICY_PLUGIN = libacctpolicy-plugin.la
+LIBACCTPOLICY_SCHEMA = $(srcdir)/ldap/schema/60acctpolicy.ldif
+enable_bitwise = 1 <=== ???
+endif

2. Can there be multiple policies in a backend?  If yes, what happens if they are nested and conflicted?
+dn: cn=AccountPolicy,dc=example,dc=com
+objectClass: top
+objectClass: ldapsubentry
+objectClass: extensibleObject
+objectClass: accountpolicy
+# 86400 seconds per day * 30 days = 2592000 seconds
+accountInactivityLimit: 2592000
+cn: AccountPolicy

+dn: cn=AccountPolicy,ou=people,dc=example,dc=com
+objectClass: top
+objectClass: ldapsubentry
+objectClass: extensibleObject
+objectClass: accountpolicy
+# 86400 seconds per day * 3 days = 259200 seconds
+accountInactivityLimit: 259200
+cn: AccountPolicy

3. Found one typo in a comment. ;)
+++ b/ldap/schema/60acctpolicy.ldif
+## acctPolicySubentry is an an account policy pointer (DN syntax)
                          ^^^^^

It'd be nice if we could have a design doc on fedora wiki...
http://directory.fedoraproject.org

(In reply to comment #11)
> I'm reviewing the patches.  Please help me understanding the spec...
> 
> 1. Why bitwise is enabled together when acctpolicy is?  (The Account Policy
> Plugin it self does not depends on the bitwise plugin...)
> +if enable_acctpolicy
> +LIBACCTPOLICY_PLUGIN = libacctpolicy-plugin.la
> +LIBACCTPOLICY_SCHEMA = $(srcdir)/ldap/schema/60acctpolicy.ldif
> +enable_bitwise = 1 <=== ???
> +endif

Arg - copy/paste error - thanks

> 
> 2. Can there be multiple policies in a backend?  If yes, what happens if they
> are nested and conflicted?
> +dn: cn=AccountPolicy,dc=example,dc=com
> +objectClass: top
> +objectClass: ldapsubentry
> +objectClass: extensibleObject
> +objectClass: accountpolicy
> +# 86400 seconds per day * 30 days = 2592000 seconds
> +accountInactivityLimit: 2592000
> +cn: AccountPolicy
> 
> +dn: cn=AccountPolicy,ou=people,dc=example,dc=com
> +objectClass: top
> +objectClass: ldapsubentry
> +objectClass: extensibleObject
> +objectClass: accountpolicy
> +# 86400 seconds per day * 3 days = 259200 seconds
> +accountInactivityLimit: 259200
> +cn: AccountPolicy

It depends on how the operational attribute acctPolicySubentry is populated in the user's entry.  dn: uid=scarter can have
acctPolicySubentry: cn=AccountPolicy,ou=people,dc=example,dc=com
or
acctPolicySubentry: cn=AccountPolicy,dc=example,dc=com

If an admin sets it directly.  If acctPolicySubentry is provided by CoS, it is up to CoS locality or priority to determine which value is set.  If there really is more than one value, the acct policy code just picks the first one, which means it is non-deterministic, and the admin will have to enable plugin debugging in order to see which one it picks.

> 
> 3. Found one typo in a comment. ;)
> +++ b/ldap/schema/60acctpolicy.ldif
> +## acctPolicySubentry is an an account policy pointer (DN syntax)

Thanks.

>                           ^^^^^
> 
> It'd be nice if we could have a design doc on fedora wiki...
> http://directory.fedoraproject.org

Ulf did one a long time ago.
http://directory.fedoraproject.org/wiki/Account_Policy_Design

(In reply to comment #12)
> It depends on how the operational attribute acctPolicySubentry is populated in
> the user's entry.  dn: uid=scarter can have
> acctPolicySubentry: cn=AccountPolicy,ou=people,dc=example,dc=com
> or
> acctPolicySubentry: cn=AccountPolicy,dc=example,dc=com
> 
> If an admin sets it directly.  If acctPolicySubentry is provided by CoS, it is
> up to CoS locality or priority to determine which value is set.  If there
> really is more than one value, the acct policy code just picks the first one,
> which means it is non-deterministic, and the admin will have to enable plugin
> debugging in order to see which one it picks.
> 
> Ulf did one a long time ago.
> http://directory.fedoraproject.org/wiki/Account_Policy_Design

Thank you, Rich and Ulf!

Other than a few items above, I don't see any defects.  Shall I mark "reviewed" on the 3 patches?  Or are you planning to attach a new one?

(In reply to comment #13)
> (In reply to comment #12)
> > It depends on how the operational attribute acctPolicySubentry is populated in
> > the user's entry.  dn: uid=scarter can have
> > acctPolicySubentry: cn=AccountPolicy,ou=people,dc=example,dc=com
> > or
> > acctPolicySubentry: cn=AccountPolicy,dc=example,dc=com
> > 
> > If an admin sets it directly.  If acctPolicySubentry is provided by CoS, it is
> > up to CoS locality or priority to determine which value is set.  If there
> > really is more than one value, the acct policy code just picks the first one,
> > which means it is non-deterministic, and the admin will have to enable plugin
> > debugging in order to see which one it picks.
Also, acctPolicySubentry is defined to be single-valued by schema.

> > 
> > Ulf did one a long time ago.
> > http://directory.fedoraproject.org/wiki/Account_Policy_Design
I just updated it with the new OIDs for my schema (HP's tree), removed some stale info on alternate implementation methods, and added a note to the outstanding issues regarding developments in the password policy drafts.

> 
> Thank you, Rich and Ulf!
> 
> Other than a few items above, I don't see any defects.  Shall I mark "reviewed"
> on the 3 patches?  Or are you planning to attach a new one?

Thanks.  I have a couple more patches.

Created attachment 450574 [details]
0001-add-support-for-global-inactivity-limit.patch

Created attachment 450575 [details]
0001-fix-typos-in-Makefile.am-acctpolicy-schema.patch

This addresses Noriko's review comments

These are all of the patches.  Do they look ok?  If so, I'll push them for 1.2.7

commit d04ffbe70416df4b1b63e7d4e21dbcbb428afe49
Author: Rich Megginson <rmeggins>
Date:   Wed Sep 29 14:19:04 2010 -0600

    fix typos in Makefile.am, acctpolicy schema

commit 037623905acf1379c964821dbb00f82f2ef1ac95
Author: Rich Megginson <rmeggins>
Date:   Wed Sep 29 14:15:08 2010 -0600

    add support for global inactivity limit

commit 4ddeb0bd1e5b79360b1850ea454c08d9c2706ffa
Author: Rich Megginson <rmeggins>
Date:   Wed Sep 29 08:43:12 2010 -0600

    do not register pre/post op plugins if disabled

commit 5f01b7913cc0c19bf8757351506f73adb1c9e58b
Author: Rich Megginson <rmeggins>
Date:   Wed Sep 29 08:42:03 2010 -0600

    fix pblock memory leak

commit 32e2b04dd1d98d96d90fdfaa3841524b3003dcdb
Author: Rich Megginson <rmeggins>
Date:   Fri Sep 17 08:18:29 2010 -0600

    add the account policy plugin and related server code, schema, and config

As per Rich's comments (https://bugzilla.redhat.com/show_bug.cgi?id=498097)
marking the bug as Sanity Only Verified.