Skip to content

Allow LDAP connection from file descriptor #337

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

Merged
merged 2 commits into from
Jun 5, 2020

Conversation

tiran
Copy link
Member

@tiran tiran commented May 29, 2020

ldap.initialize() now takes an optional fileno argument to create an
LDAP connection from a connected socket.

set_option() and get_option() now verify that LDAPObject is valid. This
fixes an assertion error and possible segfault after unbind_ext().

@tiran tiran added this to the 3.3 milestone May 29, 2020
@tiran tiran force-pushed the from_fileno branch 5 times, most recently from 35e288f to 8c40832 Compare May 29, 2020 12:28
Comment on lines +37 to +38
* ldap_init_fd() is not a private API but it's not in a public header either
* SSSD has been using the function for a while, so it's probably OK.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any plans to make it public?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why it hasn't landed in upstream headers. The API has been around for a while and it's stable. SSSD uses it heavily.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an RFE open?
I'm worried by the extern declaration: OpenLDAP is free to add a ldap_init_fd function to the public headers, and even make the declaration a bit different. (Would e.g. unsigned int proto be enough to cause python-ldap to fail to build?).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been document with the exact same function signature for 13 years, openldap/openldap@282a3c5 . It's unlikely that the function will disappear or change signature.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.
Could you make sure OpenLDAP devs know, though?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.
Could you make sure OpenLDAP devs know, though?

The statement was false. It is provided in the public openldap.h header file.

Copy link
Member Author

@tiran tiran Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... which is not packaged by most distributions like Debian/Ubuntu or Fedora/RHEL.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... which is not packaged by most distributions like Debian/Ubuntu or Fedora/RHEL.

Then you need to talk to the distributions, not the OpenLDAP developers.

set_option() and get_option() now verify that LDAPObject is valid. This
fixes an assertion error and possible segfault after unbind_ext().

Signed-off-by: Christian Heimes <cheimes@redhat.com>
@tiran tiran force-pushed the from_fileno branch 2 times, most recently from 968f2ae to e6a0a38 Compare June 5, 2020 16:36
@codecov
Copy link

codecov bot commented Jun 5, 2020

Codecov Report

Merging #337 into master will decrease coverage by 0.88%.
The diff coverage is 59.09%.

@@            Coverage Diff             @@
##           master     #337      +/-   ##
==========================================
- Coverage   71.38%   70.49%   -0.89%     
==========================================
  Files          49       49              
  Lines        4837     4871      +34     
  Branches      808      816       +8     
==========================================
- Hits         3453     3434      -19     
- Misses       1050     1084      +34     
- Partials      334      353      +19     
Impacted Files Coverage Δ
Lib/ldap/functions.py 80.00% <ø> (-17.50%) ⬇️
Modules/functions.c 56.47% <44.00%> (-5.20%) ⬇️
Lib/ldap/ldapobject.py 73.97% <60.00%> (-3.94%) ⬇️
Modules/LDAPObject.c 66.43% <75.00%> (-0.10%) ⬇️
Lib/slapdtest/_slapdtest.py 85.05% <100.00%> (+0.05%) ⬆️
Lib/ldap/__init__.py 76.92% <0.00%> (-13.47%) ⬇️
Lib/ldap/sasl.py 80.55% <0.00%> (-5.56%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8fd053...00afc3f. Read the comment docs.

``ldap.initialize()`` now takes an optional fileno argument to create an
LDAP connection from a connected socket.

See: python-ldap#178
Signed-off-by: Christian Heimes <cheimes@redhat.com>
Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, Travis CI is not reporting results. But the build for e59905c passed: https://travis-ci.org/github/python-ldap/python-ldap/builds/695145760

@encukou
Copy link
Member

encukou commented Jun 5, 2020

And e6a0a38 and 00afc3f are identical.

@encukou encukou merged commit cb4eb78 into python-ldap:master Jun 5, 2020
@tiran tiran deleted the from_fileno branch June 5, 2020 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants