Only escape login and password for ldap query, not for connection

[arnaud-morvan]

No description provided.

[digitalresistor]

I don't see any functional changes here other than some renaming of variables. What am I missing?

Edit: Another read through the code and I understand what this is doing now, but why do we not need to escape the password before we pass it to a connection?

[arnaud-morvan]

I removed:

login = escape_filter_chars(login_unsafe)
password = escape_filter_chars(password_unsafe)

And only escape them in call to search.execute, not before self.manager.connection(login_dn, password)

Because it create an issue when escaping password before self.manager.connection(login_dn, password).

I can remove the parameters renaming if you think it is more compatible with current master.

[stevepiercy]

@bertjwregeer I see no functional changes in this PR. What does this PR actually change?

@arnaud-morvan can you give an example of the issue? We will need a test case that we can reproduce. Thank you!

[digitalresistor]

@stevepiercy expand out the code in that function so you can see even the non-changed parts. Instead of always escaping the username/password it now only escapes them in one single part of the code.

I don't know enough about the ldap code to know if that is correct or not, as I don't use this module. But it does make a functional change. Not sure I would have changed the variable names as I think that is adding to the confusion.

[stevepiercy]

I did look at the entire code of the class Connector, and I still don't see what functionally changes. I see that the escaping of an unsafe login and password has been moved from outside the with context manager to inside of it. It still always escapes the values.

I also see that variables were renamed, obliterating some useful debugging by removing the unsafe_* variables. See screen shots of running tests on master versus this PR.

master	PR

I prefer to preserve the variable names, as the original names explicitly state what is going on.

[digitalresistor]

I see that the escaping of an unsafe login and password has been moved from outside the with context manager to inside of it. It still always escapes the values.

Yes, for that one part of the code. So since that escaping only happens in one place, now the rest of the function (hidden by the "see more" (which also confused me at first)) still has a use of the value that is unescaped.

[stevepiercy]

@bertjwregeer I was not using GitHub's UI because it hides context and bit me, too. I am using PyCharm and checked out the branch for the PR to run tests on both branches and use a debugger. I did that because you said there is a functional change, but unless I misinterpret the debugger results, there is no functional difference.

Are you talking about the code only within the Connector.authenticate method or something else? I'm looking only at that method.

The only other place I see login used is in the exception's logger message, and that doesn't matter here.

Finally I noticed this in the docstring, so maybe this whole PR is moot.

        ...Applications using pyramid_ldap can preprocess
        the logins to make sure they are formatted correctly for their
        ``ldap.login_filter_tpl`` setting.

[digitalresistor]

@stevepiercy

This line:

https://github.com/Pylons/pyramid_ldap/pull/29/files#diff-c193aae7c4e258554856e771621ec9e969a52dbb58572cfb040e99f4f0499c8aR166

Is no longer using the filtered/escaped username/password instead it is using the original provided values that were passed to the function.

So the change is that there is filtering going on here:

https://github.com/Pylons/pyramid_ldap/pull/29/files#diff-c193aae7c4e258554856e771621ec9e969a52dbb58572cfb040e99f4f0499c8aR158-R160

Which means that this is a functional change.

[stevepiercy]

@bertjwregeer OK, now I see. Thank you! Here's the debug for master and this PR in that try/except.

master

authenticate, __init__.py:170
test_authenticate_escapes_password, tests.py:217
#snip
password = {str} 'bad\\5c\\2a\\28\\29password'
password_unsafe = {str} 'bad\\*()password'

this PR

authenticate, __init__.py:168
test_authenticate_escapes_password, tests.py:216
#snip
password = {str} 'bad\\*()password'

Back to your original question...

but why do we not need to escape the password before we pass it to a connection?

I have the same question. I would think that we want to pass in a safe, escaped password. Passing in an unsafe, unescaped password sounds bad. @arnaud-morvan would you please respond? Thank you!

[arnaud-morvan]

@stevepiercy @bertjwregeer

If I correctly remember, escaping is for characters like parenthesys which have special meaning in filters.
So it make sense to escape them when using search.execute.

But it result in a failed authentication in manager.connection if password or login is escaped and contains such characters.

I can remove the parameters renaming if needed.

[stevepiercy]

For the record, here is the function in python-ldap that escapes characters. Escaping depends on mode, with the default of 0 which escapes \, *, (, ), and \x00

https://github.com/python-ldap/python-ldap/blob/e885b621562a3c987934be3fba3873d21026bf5c/Lib/ldap/filter.py#L17-L46

But it result in a failed authentication in manager.connection if password or login is escaped and contains such characters.

Can you verify in an actual LDAP deployment that this is true, perhaps using the example username BAD\\login and password bad\\*()password? Logs or stacktrace would be helpful. I would like to see what you see.

I can remove the parameters renaming if needed.

I think that reverting the variable renaming would make the code much clearer.