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

feature: bind user for active_directory auth #6255

Merged
merged 7 commits into from Mar 29, 2017

Conversation

Projects
None yet
5 participants
@murrant
Member

murrant commented Mar 23, 2017

Optional, allows the use of "remember me", API, and user alerting.

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

  • Have you signed the Contributors agreement - please do NOT submit a pull request unless you have (signing the agreement in the same pull request is fine). Your commit message for signing the agreement must appear as per the docs.
  • Have you followed our code guidelines?

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

fixes: #6134

murrant added some commits Mar 23, 2017

feature: bind user for active_directory auth
Optional, allows the use of "remember me", API, and alerting.
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Mar 23, 2017

Auto-Deploy finished, Test PR at http://6255.ci.librenms.org or https://6255.ci.librenms.org

@FTBZ

This comment has been minimized.

Show comment
Hide comment
@FTBZ

FTBZ Mar 24, 2017

Contributor

I don't know if it's possible, but when the bind user isn't working. It's showing a floating "Invalid credentials" on all pages and allow login of standard AD account (but without access to anything).

A better message should be useful I think.

screen shot 2017-03-24 at 07 38 14

Contributor

FTBZ commented Mar 24, 2017

I don't know if it's possible, but when the bind user isn't working. It's showing a floating "Invalid credentials" on all pages and allow login of standard AD account (but without access to anything).

A better message should be useful I think.

screen shot 2017-03-24 at 07 38 14

@FTBZ

This comment has been minimized.

Show comment
Hide comment
@FTBZ

FTBZ Mar 24, 2017

Contributor

Noticed just a second annoying thing, the base DN cannot be specified for the bind user? Because our standard users and our technical users like a "bind user" isn't on the same Active Directory OU.

Contributor

FTBZ commented Mar 24, 2017

Noticed just a second annoying thing, the base DN cannot be specified for the bind user? Because our standard users and our technical users like a "bind user" isn't on the same Active Directory OU.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Mar 24, 2017

Member

Thanks for testing FTBZ, I'll see what I can do. I think I need an additional check in reauthenticate to make sure the bind user bind was successful.

I'm not sure about the bind user DN, one way to fix that is to set your base DN higher up the tree.

Maybe, it would make sense to support full DN for the bind user.

Member

murrant commented Mar 24, 2017

Thanks for testing FTBZ, I'll see what I can do. I think I need an additional check in reauthenticate to make sure the bind user bind was successful.

I'm not sure about the bind user DN, one way to fix that is to set your base DN higher up the tree.

Maybe, it would make sense to support full DN for the bind user.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Mar 24, 2017

Member

Actually the bind user doen't use base dn, it uses ${config['auth_ad_binduser']}@${config['auth_ad_domain']}

Member

murrant commented Mar 24, 2017

Actually the bind user doen't use base dn, it uses ${config['auth_ad_binduser']}@${config['auth_ad_domain']}

Make sure the ldapbind credentials are correct on reauth.
Do not send output if they are incorrect (use d_echo) this breaks ajax calls, etc.
Add scripts/auth_test.php, to make it easier to debug authentication.
@FTBZ

This comment has been minimized.

Show comment
Hide comment
@FTBZ

FTBZ Mar 27, 2017

Contributor

Ref. to #6134 for tracking. My tests was concluent, the problem seems to be fixed.

Contributor

FTBZ commented Mar 27, 2017

Ref. to #6134 for tracking. My tests was concluent, the problem seems to be fixed.

Refine auth_test.php a bit more
A few small cleanups in other places of the auth
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Mar 27, 2017

Auto-Deploy finished, Test PR at http://6255.ci.librenms.org or https://6255.ci.librenms.org

Add auth_test.php to docs
Some more improvements in the auth_test.php output.
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Mar 27, 2017

Auto-Deploy finished, Test PR at http://6255.ci.librenms.org or https://6255.ci.librenms.org

Show outdated Hide outdated doc/Extensions/Authentication.md
#### Testing authentication
You can test authentication with this script:
```shell
./scripts/auth_test

This comment has been minimized.

@laf

laf Mar 27, 2017

Member

scripts/auth_test.php?

@laf

laf Mar 27, 2017

Member

scripts/auth_test.php?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Mar 27, 2017

Member

@FTBZ Would you mind retesting this PR once @murrant is done?

Member

laf commented Mar 27, 2017

@FTBZ Would you mind retesting this PR once @murrant is done?

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

LibreNMS-CI commented Mar 27, 2017

Auto-Deploy finished, Test PR at http://6255.ci.librenms.org or https://6255.ci.librenms.org

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Mar 27, 2017

The inspection completed: 12 new issues, 6 updated code elements

scrutinizer-notifier commented Mar 27, 2017

The inspection completed: 12 new issues, 6 updated code elements

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Mar 27, 2017

Member

All, done I think I gave it a pretty good round of testing myself. I would like an additional confirmation from @FTBZ

I also learned that apparently Firefox does some sort of session caching across browser closes...

Member

murrant commented Mar 27, 2017

All, done I think I gave it a pretty good round of testing myself. I would like an additional confirmation from @FTBZ

I also learned that apparently Firefox does some sort of session caching across browser closes...

@FTBZ

This comment has been minimized.

Show comment
Hide comment
@FTBZ

FTBZ Mar 28, 2017

Contributor
  • The floating text message has disappeared and was replaced by the red box on the login screen. It's better, but it's strange to get this kind of message before trying to authenticate. The documentation needs to notify about this if it's not possible to replace the message with the exact error.

screen shot 2017-03-28 at 07 55 15

  • I confirm that having two different OU for users and bind user has no impact
  • Notification email is working fine, now you can use "Issue alerts to admins" with AD account
  • API Access token can be created for AD user now, wasn't possible before without editing the DB

I've a Dashboard problem linked to AD account probably, but the patch returns me a lot of errors when trying to be applied to my production system.

Contributor

FTBZ commented Mar 28, 2017

  • The floating text message has disappeared and was replaced by the red box on the login screen. It's better, but it's strange to get this kind of message before trying to authenticate. The documentation needs to notify about this if it's not possible to replace the message with the exact error.

screen shot 2017-03-28 at 07 55 15

  • I confirm that having two different OU for users and bind user has no impact
  • Notification email is working fine, now you can use "Issue alerts to admins" with AD account
  • API Access token can be created for AD user now, wasn't possible before without editing the DB

I've a Dashboard problem linked to AD account probably, but the patch returns me a lot of errors when trying to be applied to my production system.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Mar 28, 2017

Member

Thanks for the update these are all existing issues.

I just made a PR for the dashboard and twofactor bugs. #6286.

The error message can possibly be avoided... might look in to it sometime in a separate PR. It will give verbose output if it is enabled.

Member

murrant commented Mar 28, 2017

Thanks for the update these are all existing issues.

I just made a PR for the dashboard and twofactor bugs. #6286.

The error message can possibly be avoided... might look in to it sometime in a separate PR. It will give verbose output if it is enabled.

@murrant murrant merged commit 1ea7af4 into librenms:master Mar 29, 2017

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@murrant murrant deleted the murrant:ldap-bind branch Mar 29, 2017

@boudreau boudreau referenced this pull request Mar 31, 2017

Closed

API token not working #6307

4 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment