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

Fix ldap eauth #45811

Merged
merged 10 commits into from Feb 15, 2018

Conversation

Projects
None yet
5 participants
@gtmanfred
Copy link
Contributor

commented Jan 31, 2018

What does this PR do?

This pulls in a fix from @amendlik for checking the group authentication, but also cleans up the unicode/str/bytes stuff that was not working in this external authentication

Tests written?

No (but this has been tested manually against freeipa)

Commits signed with GPG?

Yes

@gtmanfred gtmanfred requested a review from saltstack/team-core as a code owner Jan 31, 2018

@DmitryKuzmenko
Copy link
Contributor

left a comment

Commented inline.

@@ -321,7 +326,7 @@ def groups(username, **kwargs):
_config('persontype'))
user_dn_results = bind.search_s(_config('basedn'),
ldap.SCOPE_SUBTREE,
get_user_dn_search, ['distinguishedName'])
get_user_dn_search, [str('distinguishedName')])

This comment has been minimized.

Copy link
@DmitryKuzmenko

DmitryKuzmenko Feb 1, 2018

Contributor

Here is a missing # future lint: disable=blacklisted-function lint directive.

@@ -336,7 +341,7 @@ def groups(username, **kwargs):
search_results = bind.search_s(_config('basedn'),
ldap.SCOPE_SUBTREE,
ldap_search_string,
[_config('accountattributename'), 'cn'])
[salt.utils.stringutils.to_str(_config('accountattributename')), str('cn')])

This comment has been minimized.

Copy link
@DmitryKuzmenko

DmitryKuzmenko Feb 1, 2018

Contributor

Same here # future lint: disable=blacklisted-function

@@ -352,11 +357,11 @@ def groups(username, **kwargs):
search_results = bind.search_s(search_base,
ldap.SCOPE_SUBTREE,
search_string,
[_config('accountattributename'), 'cn'])
[salt.utils.stringutils.to_str(_config('accountattributename')), str('cn')])

This comment has been minimized.

Copy link
@DmitryKuzmenko

DmitryKuzmenko Feb 1, 2018

Contributor

Same here: # future lint: disable=blacklisted-function


for entry, result in search_results:
for user in result[_config('accountattributename')]:
if username == user.split(',')[0].split('=')[-1]:
if username == salt.utils.data.decode(user).split(',')[0].split('=')[-1]:

This comment has been minimized.

Copy link
@DmitryKuzmenko

DmitryKuzmenko Feb 1, 2018

Contributor

If I'm not wrong salt.utils.stringutils.to_unicode shall be used here instead of salt.utils.data.decode.
Probably @terminalmage will correct me.

@@ -374,12 +379,14 @@ def groups(username, **kwargs):
search_results = bind.search_s(search_base,
ldap.SCOPE_SUBTREE,
search_string,
[_config('accountattributename'), 'cn', _config('groupattribute')])
[salt.utils.stringutils.to_str(_config('accountattributename')),
str('cn'),

This comment has been minimized.

Copy link
@DmitryKuzmenko

DmitryKuzmenko Feb 1, 2018

Contributor

# future lint: disable=blacklisted-function

for _, entry in search_results:
if username in entry[_config('accountattributename')]:
group_list.append(entry['cn'][0])
for user, entry in search_results:
if username == user.split(',')[0].split('=')[-1]:
if username == salt.utils.data.decode(user).split(',')[0].split('=')[-1]:

This comment has been minimized.

Copy link
@DmitryKuzmenko

DmitryKuzmenko Feb 1, 2018

Contributor

salt.utils.stringutils.to_unicode

@@ -435,7 +442,7 @@ def __expand_ldap_entries(entries, opts=None):
search_results = bind.search_s(search_base,
ldap.SCOPE_SUBTREE,
search_string,
['cn'])
[str('cn')])

This comment has been minimized.

Copy link
@DmitryKuzmenko

DmitryKuzmenko Feb 1, 2018

Contributor

# future lint: disable=blacklisted-function

@gtmanfred

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2018

Issues are fixed, let me just setup freeipa again, and make sure i didn't break anything

@gtmanfred

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2018

👍 still works with python3 and python2 with the changes

@DmitryKuzmenko
Copy link
Contributor

left a comment

LGTM

@rallytime rallytime requested a review from thatch45 Feb 2, 2018

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2018

@amendlik - We're pulling in your changes from #45372 back into the oxygen.rc1 branch instead of your fix in develop. Can you take a look here?

@cachedout
Copy link
Collaborator

left a comment

I am extremely hesitant to merge this without test coverage.

@gtmanfred gtmanfred force-pushed the gtmanfred:oxygen.rc1 branch from f642c21 to ffab15b Feb 7, 2018

@gtmanfred

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2018

tests added.

@rallytime

This comment has been minimized.

@rallytime rallytime requested a review from cachedout Feb 8, 2018

@gtmanfred gtmanfred force-pushed the gtmanfred:oxygen.rc1 branch from eb588a9 to 58567e8 Feb 8, 2018

@gtmanfred

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2018

Needs saltstack/salt-jenkins#837 for these tests to actually be run

@gtmanfred

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2018

re-run py

@gtmanfred

This comment has been minimized.

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2018

re-run lint

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2018

@gtmanfred This has a merge conflict now.

@gtmanfred

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2018

fixed

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2018

re-run lint

amendlik and others added some commits Jan 5, 2018

gtmanfred added some commits Jan 31, 2018

@gtmanfred gtmanfred force-pushed the gtmanfred:oxygen.rc1 branch from 318970a to 472ed2e Feb 12, 2018

@gtmanfred

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2018

Well that didn't fix it, i am unsure of what

22:13:18 Traceback (most recent call last):
22:13:18   File "/var/jenkins/.pyenv/versions/2.7.13/bin/pylint", line 11, in <module>
22:13:18     sys.exit(run_pylint())
22:13:18   File "/root/.pyenv/versions/2.7.13/lib/python2.7/site-packages/pylint/__init__.py", line 17, in run_pylint
22:13:18     Run(sys.argv[1:])
22:13:18   File "/root/.pyenv/versions/2.7.13/lib/python2.7/site-packages/pylint/lint.py", line 1333, in __init__
22:13:18     linter.check(args)
22:13:18   File "/root/.pyenv/versions/2.7.13/lib/python2.7/site-packages/pylint/lint.py", line 755, in check
22:13:18     self._do_check(files_or_modules)
22:13:18   File "/root/.pyenv/versions/2.7.13/lib/python2.7/site-packages/pylint/lint.py", line 886, in _do_check
22:13:18     self.check_astroid_module(ast_node, walker, rawcheckers, tokencheckers)
22:13:18   File "/root/.pyenv/versions/2.7.13/lib/python2.7/site-packages/pylint/lint.py", line 967, in check_astroid_module
22:13:18     walker.walk(ast_node)
22:13:18   File "/root/.pyenv/versions/2.7.13/lib/python2.7/site-packages/pylint/utils.py", line 959, in walk
22:13:18     self.walk(child)
22:13:18   File "/root/.pyenv/versions/2.7.13/lib/python2.7/site-packages/pylint/utils.py", line 959, in walk
22:13:18     self.walk(child)
22:13:18   File "/root/.pyenv/versions/2.7.13/lib/python2.7/site-packages/pylint/utils.py", line 959, in walk
22:13:18     self.walk(child)
22:13:18   File "/root/.pyenv/versions/2.7.13/lib/python2.7/site-packages/pylint/utils.py", line 956, in walk
22:13:18     cb(astroid)
22:13:18   File "/root/.pyenv/versions/2.7.13/lib/python2.7/site-packages/saltpylint/blacklist.py", line 321, in visit_assign
22:13:18     self.imported_salt_modules[node_left.value.expr.name])
22:13:18 AttributeError: 'Attribute' object has no attribute 'name'
22:13:20 + EC2=1
``` is doing

@rallytime rallytime merged commit 0055399 into saltstack:oxygen.rc1 Feb 15, 2018

6 of 9 checks passed

default Pull Requests » Salt PR - Main Build - PY2/PY3 #6069
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #6804 — ABORTED
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #19859 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #22283 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #14639 — SUCCESS
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #2247 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #19248 — SUCCESS
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #16295 — SUCCESS
Details

@gtmanfred gtmanfred deleted the gtmanfred:oxygen.rc1 branch Feb 23, 2018

amendlik added a commit to amendlik/salt that referenced this pull request Dec 31, 2018

Allow unauthenticated bind for listing LDAP groups
This fix was originally made in PR saltstack#45347, then pulled into another
branch under PR saltstack#45811, but was clobbered by c3f1587. This commit
just restores the fix.
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.