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

Adds support for ppolicy overlay. #31

Merged
merged 1 commit into from
Aug 2, 2016
Merged

Conversation

tcsalameh
Copy link
Contributor

This adds support for the ppolicy overlay, by adding the necessary
changes to the cn=config database. The user will be required to activate
the ppolicy schema, and any password enforcement will rely on a user
defined default password object at cn=passwordDefault,.

This adds support for the ppolicy overlay, by adding the necessary
changes to the cn=config database. The user will be required to activate
the ppolicy schema, and any password enforcement will rely on a user
defined default password object at cn=passwordDefault,<olcSuffix>.
@coveralls
Copy link

coveralls commented Jul 29, 2016

Coverage Status

Coverage decreased (-0.4%) to 53.165% when pulling ea42419 on tcsalameh:ppolicy-config into 99b89af on bodgit:master.

@bodgit bodgit added this to the v1.2.0 milestone Jul 30, 2016
if $ppolicy {
validate_re($pp_hash_cleartext, '^TRUE$|^FALSE$')
validate_re($pp_use_lockout, '^TRUE$|^FALSE$')
validate_re($pp_forward_updates, '^TRUE$|^FALSE$')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer these to be exposed as native booleans and do the conversion of true -> 'TRUE' & false -> 'FALSE' within the module, (maybe add a function to do that?). I'd also rather not abbreviate the ppolicy to pp in the parameter names, yeah they're long and wordy but it matches the other parameters. I also see 'pp' and think 'pretty print' 😄

@bodgit
Copy link
Owner

bodgit commented Aug 1, 2016

The one nit with this is in the case of the server being a read-only replica, in order to increase lockout/bind failed counters in the read-write upstream directory the chain overlay needs to be enabled and configured in addition to setting the update_ref parameter. At the moment I just make the clients chase referrals themselves.

@bodgit bodgit merged commit ea42419 into bodgit:master Aug 2, 2016
@bodgit
Copy link
Owner

bodgit commented Aug 2, 2016

I've merged this (with some minor alterations) now that I've added preliminary chain overlay support so it can work in a replicated setup. I'm not 100% convinced yet that all of these overlays are in the right order; the OpenLDAP documentation is confusing as %$*! in this regard.

Thanks for your contributions, I've been meaning to add unique and ppolicy support for a while. I'd just like to sort #22 and possibly #19 then I'll cut a v1.2.0 release.

@tcsalameh
Copy link
Contributor Author

Thank you for merging these changes. The overlays have been working fine on our setup with this ordering, but I agree that the OpenLDAP documentation is really confusing on that topic.

We'd started working on some of these changes too, and I noticed that there's a puppet library function - bool2str - that converts a boolean to two strings. So adding another function to do this may not be necessary; something like:
bool2str($::openldap::server::ppolicy_forward_updates, 'TRUE', 'FALSE')
should also work for the conversions on lines 445-447 in config.pp.

Also, thank you for adding preliminary support for the chain overlay (another area with confusing documentation!). We had started working on this, but were running into some issues, and we'll go from what you've done so far.

@bodgit
Copy link
Owner

bodgit commented Aug 2, 2016

We'd started working on some of these changes too, and I noticed that there's a puppet library function - bool2str - that converts a boolean to two strings. So adding another function to do this may not be necessary; something like:
bool2str($::openldap::server::ppolicy_forward_updates, 'TRUE', 'FALSE')
should also work for the conversions on lines 445-447 in config.pp.

I knew there was a str2bool function but I didn't realise there's a companion bool2str, in which case I'll just refactor to use that if it works and rip out the function I added.

Also, thank you for adding preliminary support for the chain overlay (another area with confusing documentation!). We had started working on this, but were running into some issues, and we'll go from what you've done so far.

It took me most of last night to work out how to get chaining to work, as you say the documentation is confusing and I could only find examples using the old slapd.conf syntax. It turned out I had the right syntax and overlay objects but I had them configured on the *db database with the other overlays when it should apparently be configured on the frontend database. I'll probably add a ppolicy + chain example to the README.

bodgit added a commit that referenced this pull request Aug 12, 2016
The user is required to load the schema but it's not needed to be able
to enable the overlay so just document that its required. Requiring the
schema meant it has to be included before the server which causes an
error that surfaced when testing with Puppet 4.6.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants