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

adding 2-factor auth capability to ldap eauth module - #42280 #42426

Merged
merged 8 commits into from Jul 31, 2017

Conversation

Projects
None yet
5 participants
@michaelgibson
Copy link
Contributor

commented Jul 20, 2017

What does this PR do?

Not sure if this is the best approach for this but it seems to suit my purposes.
Allows for 2 factor capability in ldap eauth module by honoring the bind credentials specified in the configuration.
See #42280

What issues does this PR fix or reference?

#42280

Previous Behavior

Bind credentials(binddn/bindpw) are not used for authorization requests.
Only for initially getting user information.

New Behavior

Bind credentials, when specified, are used for all non-user authentication requests.

Tests written?

Yes/No

Please review Salt's Contributing Guide for best practices.

@salt-jenkins

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

@michaelgibson, thanks for your PR! By analyzing the history of the files in this pull request, we identified @cro, @KrisSaxton and @eliasp to be potential reviewers.

@gtmanfred gtmanfred requested a review from cro Jul 20, 2017

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Jul 25, 2017

@cro Can you look at this?

@cro
Copy link
Member

left a comment

Hey @michaelgibson, I'm reviewing your PR and not understanding how the 2FA part works. Do you prepend/append the token onto the password?

@cro

This comment has been minimized.

Copy link
Member

commented Jul 26, 2017

OK, nevermind, I didn't read the associated issue carefully enough. Let me continue discussion over there.

@cro

This comment has been minimized.

Copy link
Member

commented Jul 27, 2017

I opened a PR against your branch with some documentation.

https://github.com/michaelgibson/salt/pull/1

Feel free to merge, then rebase this PR (since it is out-of-date WRT develop) and we'll take another look.

michaelgibson added some commits Jul 27, 2017

Merge pull request #1 from cro/issue-42280
Add different bindpw behavior to the Oxygen release notes.
@cro

cro approved these changes Jul 27, 2017

Copy link
Member

left a comment

This looks OK as long as all the tests pass.

@cachedout cachedout merged commit b5304e7 into saltstack:develop Jul 31, 2017

6 of 8 checks passed

default Build finished.
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #8717 — FAILURE
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #15912 — SUCCESS
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #653 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #13305 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #680 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #13253 — SUCCESS
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #13402 — SUCCESS
Details

@amendlik amendlik referenced this pull request Jan 9, 2018

Merged

LDAP group membership #45347

@cro cro referenced this pull request Aug 13, 2018

Merged

[2018.3] fix to auth/ldap.py #48901

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.