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

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

Merged
merged 2 commits into from Aug 14, 2018

Conversation

Projects
None yet
7 participants
@garethgreenaway
Member

garethgreenaway commented Aug 2, 2018

What does this PR do?

Fixing issue when a valid token is generated by the salt-api even when invalid user credentials are passed. This change verifies that the binddn credentials are valid, then verifies that the username & password (if not None) are also valid.

What issues does this PR fix or reference?

#48665

Previous Behavior

When binddn credentials are included in the configuration, a valid token is generated by the salt-api regardless of whether the username & password is valid.

New Behavior

If binddn credentials are configured, first the binddn credentials are verified and if they are valid, then the username & password credentials are verified.

Tests written?

No.

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@garethgreenaway garethgreenaway requested a review from saltstack/team-core as a code owner Aug 2, 2018

@salt-jenkins salt-jenkins requested a review from saltstack/team-suse Aug 2, 2018

@garethgreenaway garethgreenaway requested a review from cro Aug 2, 2018

@rallytime rallytime requested review from dwoz and DmitryKuzmenko Aug 3, 2018

@dwoz

dwoz approved these changes Aug 5, 2018

@@ -283,9 +283,15 @@ def auth(username, password):
log.error('LDAP authentication requires python-ldap module')
return False
# If bind credentials are configured, use them instead of user's
# If bind credentials are configured, verify that we can a valid bind

This comment has been minimized.

@cro

cro Aug 10, 2018

Member

Minor typo here.

if bind and username and password:
bind = _bind(username, password,
anonymous=_config('auth_by_group_membership_only', mandatory=False)
and _config('anonymous', mandatory=False))

This comment has been minimized.

@cro

cro Aug 10, 2018

Member

So are we sure this won't invalidate a one-time-use token?

This comment has been minimized.

@garethgreenaway

garethgreenaway Aug 10, 2018

Member

Is this the scenario that we had discussed with two-factor authentication or was there another scenario that you're concerned about?

This comment has been minimized.

@cro

cro Aug 10, 2018

Member

Same scenario, yes. The OP's auth environment generates OTPs that are literally only valid once.

This comment has been minimized.

@garethgreenaway

garethgreenaway Aug 11, 2018

Member

@cro So far my manually testing using both CURL commands against the API and using salt commands with external authentication, function the same as they do without this change (with a valid login). I can't see anywhere in the OP's auth environment that indicates that it's generating OTPs, can you point that out?

This comment has been minimized.

@cro

cro Aug 13, 2018

Member

I originally got that information from the discussion on #42280 and #42426 and the docs that I wrote (https://github.com/michaelgibson/salt/pull/1/files). So the idea was that the user would be presenting a single-use password the first time, but from then on out we would just re-authenticate with the bind username and password. Since this was happening on the CLI there wasn't a danger that another user could hijack the session after the first auth exchange. Whether or not the OTP was used vs the bindpw was determined by the presence of show_jid in the payload since it was not present the first time, but all subsequent payloads contained it.

garethgreenaway added some commits Aug 2, 2018

Fixing issue when a valid token is generated even when invalid user c…
…redentials are passed. This change verifies that the binddn credentials are valid, then verifies that the username & password (if not None) are also valid.

@garethgreenaway garethgreenaway force-pushed the garethgreenaway:48665_auth_ldap_valid_token_failed_auth branch from 51377ef to d4e4f2e Aug 10, 2018

@cachedout cachedout merged commit e78fc0e into saltstack:2018.3 Aug 14, 2018

5 of 8 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has failed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has failed
Details
WIP ready for review
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment