[PATCH] Add new account expired rule to LDAP access provider

[Sumit Bose]

Hi,

these two patches aim to fix trac ticket #672. While the first patch
only makes a utility function public the second adds some new layers to
the LDAP access provider. I've tried to make the changes in a way to
make it easy to add new rules (#670) and new expire policies (#673,
#674, #690).

I would like to ask the reviewers to check if the new code is really as
flexible as I think and if it enough to evaluate the shadow expire
attribute here. TIA

bye,
Sumit

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/25/2010 08:25 AM, Sumit Bose wrote:
>  Hi,
>  
>  these two patches aim to fix trac ticket #672. While the first patch
>  only makes a utility function public the second adds some new layers to
>  the LDAP access provider. I've tried to make the changes in a way to
>  make it easy to add new rules (#670) and new expire policies (#673,
>  #674, #690).
>  
>  I would like to ask the reviewers to check if the new code is really as
>  flexible as I think and if it enough to evaluate the shadow expire
>  attribute here. TIA
>  
>  bye,
>  Sumit
>   

Patch 0001: Ack.

Patch 0002: Nack.
The manpage entry for ldap_account_expire_policy should list the valid
values for this option. The patch comment notes that this is only
"shadow" at present.

sssm_ldap_access_init() should throw an error (or at least warn and
ignore it) if we get a duplicate access rule. e.g.:
ldap_access_order = filter, filter

Please rename sdap_access_decide_offline(), sdap_access_retry(),
sdap_access_connect_done() and sdap_access_get_access_done() to be clear
that they apply to the access_filter.

I'm not sure we want to deny on a missing password expiration attribute.
Some users might be lacking this attribute with the intent that it means
"no expiration". Maybe we should add a strictness option here.

next_access_rule() should probably return on PAM_ACCT_EXPIRED, not just
PAM_PERM_DENIED.


- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkz3poUACgkQeiVVYja6o6OzvQCfesHt9x72o974xPOquRY0NtKt
> 1VAAn2/yojzemyFBhnv9Zbm3HLQk6rr1
> =SinO
> -----END PGP SIGNATURE-----

[Sumit Bose]

On Thu, Dec 02, 2010 at 09:00:37AM -0500, Stephen Gallagher wrote:
>  -----BEGIN PGP SIGNED MESSAGE-----
>  Hash: SHA1
>  
>  On 11/25/2010 08:25 AM, Sumit Bose wrote:
>  > Hi,
>  > 
>  > these two patches aim to fix trac ticket #672. While the first patch
>  > only makes a utility function public the second adds some new layers to
>  > the LDAP access provider. I've tried to make the changes in a way to
>  > make it easy to add new rules (#670) and new expire policies (#673,
>  > #674, #690).
>  > 
>  > I would like to ask the reviewers to check if the new code is really as
>  > flexible as I think and if it enough to evaluate the shadow expire
>  > attribute here. TIA
>  > 
>  > bye,
>  > Sumit
>  > 
>  
>  
>  Patch 0001: Ack.
>  
>  Patch 0002: Nack.
>  The manpage entry for ldap_account_expire_policy should list the valid
>  values for this option. The patch comment notes that this is only
>  "shadow" at present. 
done

>  
>  sssm_ldap_access_init() should throw an error (or at least warn and
>  ignore it) if we get a duplicate access rule. e.g.:
>  ldap_access_order = filter, filter 
I prefer the error, because it might not be clear which order was
intended.

>  
>  Please rename sdap_access_decide_offline(), sdap_access_retry(),
>  sdap_access_connect_done() and sdap_access_get_access_done() to be clear
>  that they apply to the access_filter. 
done

>  
>  I'm not sure we want to deny on a missing password expiration attribute.
>  Some users might be lacking this attribute with the intent that it means
>  "no expiration". Maybe we should add a strictness option here. 
I checked you pam_ldap handles this. A missing attribute or a value of 0
means "no expiration".

>  
>  next_access_rule() should probably return on PAM_ACCT_EXPIRED, not just
>  PAM_PERM_DENIED. 
done

Thanks for the review, new version attached.

bye,
Sumit

>  
>  
>  - -- 
>  Stephen Gallagher
>  RHCE 804006346421761
>  
>  Delivering value year after year.
>  Red Hat ranks #1 in value among software vendors.
>  http://www.redhat.com/promo/vendor/
> >  -----BEGIN PGP SIGNATURE-----
> >  Version: GnuPG v1.4.11 (GNU/Linux)
> >  Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
> >  
> >  iEYEARECAAYFAkz3poUACgkQeiVVYja6o6OzvQCfesHt9x72o974xPOquRY0NtKt
> >  1VAAn2/yojzemyFBhnv9Zbm3HLQk6rr1
> >  =SinO
> >  -----END PGP SIGNATURE----- 
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/sssd-devel

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/03/2010 06:56 AM, Sumit Bose wrote:
>  On Thu, Dec 02, 2010 at 09:00:37AM -0500, Stephen Gallagher wrote:
>  On 11/25/2010 08:25 AM, Sumit Bose wrote:
> >>> Hi,
> >>>
> >>> these two patches aim to fix trac ticket #672. While the first patch
> >>> only makes a utility function public the second adds some new layers to
> >>> the LDAP access provider. I've tried to make the changes in a way to
> >>> make it easy to add new rules (#670) and new expire policies (#673,
> >>> #674, #690).
> >>>
> >>> I would like to ask the reviewers to check if the new code is really as
> >>> flexible as I think and if it enough to evaluate the shadow expire
> >>> attribute here. TIA
> >>>
> >>> bye,
> >>> Sumit
> >>>
>  
>  
>  Patch 0001: Ack.
>  
>  Patch 0002: Nack.
>  The manpage entry for ldap_account_expire_policy should list the valid
>  values for this option. The patch comment notes that this is only
>  "shadow" at present.
>  
> > done
>  
>  
>  sssm_ldap_access_init() should throw an error (or at least warn and
>  ignore it) if we get a duplicate access rule. e.g.:
>  ldap_access_order = filter, filter
>  
> > I prefer the error, because it might not be clear which order was
> > intended.
>  
>  
>  Please rename sdap_access_decide_offline(), sdap_access_retry(),
>  sdap_access_connect_done() and sdap_access_get_access_done() to be clear
>  that they apply to the access_filter.
>  
> > done
>  
>  
>  I'm not sure we want to deny on a missing password expiration attribute.
>  Some users might be lacking this attribute with the intent that it means
>  "no expiration". Maybe we should add a strictness option here.
>  
> > I checked you pam_ldap handles this. A missing attribute or a value of 0
> > means "no expiration".
>  
>  
>  next_access_rule() should probably return on PAM_ACCT_EXPIRED, not just
>  PAM_PERM_DENIED.
>  
> > done
>  
> > Thanks for the review, new version attached. 


Nack.

Typo in sssd-ldap.5.xml (tat)

typo in ldap_init.c (dublicate)


check_list_for_duplicates() could be made more scalable by doing (in
pseudocode):

diff_string_lists(list, NULL, &nodupes, NULL, NULL); /* Pass in one
string, and get the list of entries that were only in that string */
if (array_len(nodupes) < array_len(list)) {
    /* list had duplicates */
}

Internally, this just plays some tricks with a hash addition so that if
the same key is used, it will be dropped. So the resulting array
returned to *_list1_only will contain no duplicates. If the number of
entries in the _list1_only array is fewer than the number of entries in
the input list, duplicates were dropped.

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkz47IgACgkQeiVVYja6o6PovwCfW8a0+X3KKPIJ1rRMm52yov0i
> xzMAniR20aYjU5uJqHKlpF8AOe2Zx4eZ
> =RXH6
> -----END PGP SIGNATURE-----

[Sumit Bose]

On Fri, Dec 03, 2010 at 08:11:39AM -0500, Stephen Gallagher wrote:
>  -----BEGIN PGP SIGNED MESSAGE-----
>  Hash: SHA1
>  
>  On 12/03/2010 06:56 AM, Sumit Bose wrote:
>  > On Thu, Dec 02, 2010 at 09:00:37AM -0500, Stephen Gallagher wrote:
>  > On 11/25/2010 08:25 AM, Sumit Bose wrote:
>  >>>> Hi,
>  >>>>
>  >>>> these two patches aim to fix trac ticket #672. While the first patch
>  >>>> only makes a utility function public the second adds some new layers to
>  >>>> the LDAP access provider. I've tried to make the changes in a way
> to
>  >>>> make it easy to add new rules (#670) and new expire policies (#673,
>  >>>> #674, #690).
>  >>>>
>  >>>> I would like to ask the reviewers to check if the new code is really as
>  >>>> flexible as I think and if it enough to evaluate the shadow expire
>  >>>> attribute here. TIA
>  >>>>
>  >>>> bye,
>  >>>> Sumit
>  >>>>
>  > 
>  > 
>  > Patch 0001: Ack.
>  > 
>  > Patch 0002: Nack.
>  > The manpage entry for ldap_account_expire_policy should list the valid
>  > values for this option. The patch comment notes that this is only
>  > "shadow" at present.
>  > 
>  >> done
>  > 
>  > 
>  > sssm_ldap_access_init() should throw an error (or at least warn and
>  > ignore it) if we get a duplicate access rule. e.g.:
>  > ldap_access_order = filter, filter
>  > 
>  >> I prefer the error, because it might not be clear which order was
>  >> intended.
>  > 
>  > 
>  > Please rename sdap_access_decide_offline(), sdap_access_retry(),
>  > sdap_access_connect_done() and sdap_access_get_access_done() to be clear
>  > that they apply to the access_filter.
>  > 
>  >> done
>  > 
>  > 
>  > I'm not sure we want to deny on a missing password expiration attribute.
>  > Some users might be lacking this attribute with the intent that it means
>  > "no expiration". Maybe we should add a strictness option here.
>  > 
>  >> I checked you pam_ldap handles this. A missing attribute or a value of 0
>  >> means "no expiration".
>  > 
>  > 
>  > next_access_rule() should probably return on PAM_ACCT_EXPIRED, not just
>  > PAM_PERM_DENIED.
>  > 
>  >> done
>  > 
>  >> Thanks for the review, new version attached.
>  
>  
>  
>  Nack.
>  
>  Typo in sssd-ldap.5.xml (tat)
>  
>  typo in ldap_init.c (dublicate) 
fixed

>  
>  
>  check_list_for_duplicates() could be made more scalable by doing (in
>  pseudocode):
>  
>  diff_string_lists(list, NULL, &nodupes, NULL, NULL); /* Pass in one
>  string, and get the list of entries that were only in that string */
>  if (array_len(nodupes) < array_len(list)) {
>      /* list had duplicates */
>  }
>  
>  Internally, this just plays some tricks with a hash addition so that if
>  the same key is used, it will be dropped. So the resulting array
>  returned to *_list1_only will contain no duplicates. If the number of
>  entries in the _list1_only array is fewer than the number of entries in
>  the input list, duplicates were dropped. 
We discussed this on irc and agreed that the order list is too short to
justify the use of a hash. Additionally it is not necessary here to get
a new list without duplicates, because it is sufficient to return an
error after the first duplicate is found. To avoid an unintended use of
this call I moved it frim util.c to ldap_init.c

New version attached.

bye,
Sumit


>  
>  - -- 
>  Stephen Gallagher
>  RHCE 804006346421761
>  
>  Delivering value year after year.
>  Red Hat ranks #1 in value among software vendors.
>  http://www.redhat.com/promo/vendor/
> >  -----BEGIN PGP SIGNATURE-----
> >  Version: GnuPG v1.4.11 (GNU/Linux)
> >  Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
> >  
> >  iEYEARECAAYFAkz47IgACgkQeiVVYja6o6PovwCfW8a0+X3KKPIJ1rRMm52yov0i
> >  xzMAniR20aYjU5uJqHKlpF8AOe2Zx4eZ
> >  =RXH6
> >  -----END PGP SIGNATURE----- 
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/sssd-devel

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/03/2010 04:22 PM, Sumit Bose wrote:
> > We discussed this on irc and agreed that the order list is too
> short to
> > justify the use of a hash. Additionally it is not necessary here to get
> > a new list without duplicates, because it is sufficient to return an
> > error after the first duplicate is found. To avoid an unintended use of
> > this call I moved it frim util.c to ldap_init.c
>  
> > New version attached.
>   
Ack to both patches.

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkz86coACgkQeiVVYja6o6NJGgCeLFH7uvHFZrZ4Bbbi3CZVpAYN
> KAEAnA4UL1WMoKsV0n4eieOYYd+pWfto
> =yngT
> -----END PGP SIGNATURE-----

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/06/2010 08:48 AM, Stephen Gallagher wrote:
>  On 12/03/2010 04:22 PM, Sumit Bose wrote:
> >> We discussed this on irc and agreed that the order list is too short to
> >> justify the use of a hash. Additionally it is not necessary here to get
> >> a new list without duplicates, because it is sufficient to return an
> >> error after the first duplicate is found. To avoid an unintended use of
> >> this call I moved it frim util.c to ldap_init.c
>  
> >> New version attached.
>  
>  
>  Ack to both patches.
>   
Pushed to master.

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkz885AACgkQeiVVYja6o6OhdgCcD6tw0vmEZKcjWPqhGsV9donD
> uD4An3XPGSY30daiDqJlyCUUQck+z8yH
> =VqF9
> -----END PGP SIGNATURE-----