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

Add support to use SNI #406

Merged
merged 1 commit into from
Sep 2, 2022
Merged

Conversation

jpdasma
Copy link
Contributor

@jpdasma jpdasma commented Jun 7, 2022

Related to #405

@jpdasma
Copy link
Contributor Author

jpdasma commented Jun 7, 2022

I'm thinking if it's better to have an option to use SNI or just always enable it?

The default openssl binary uses SNI by default.

@jpdasma
Copy link
Contributor Author

jpdasma commented Jun 9, 2022

@HarlemSquirrel sorry for the direct ping, but can you share your insights regarding this? This patched work for my test, but I'm not sure if it's a good idea to just force the use of SNI for every TLS connection.

@HarlemSquirrel
Copy link
Member

Thank you for opening this issue and pull request!

This seems reasonable to me but we need to test with a few different LDAP servers and I don't have access to any that aren't public now. I was able to successfully connect to this public LDAP server:

ldap = Net::LDAP.new host: 'directory.cornell.edu', port: 636, encryption: :simple_tls, auth: { method: :anonymous }
entry = ldap.search_root_dse
entry.namingcontexts.last
# => "o=cornell university,c=us"

Looks like we also need some test updates.

@jpdasma
Copy link
Contributor Author

jpdasma commented Jun 9, 2022

I'll fix the broken tests first.

@jpdasma jpdasma changed the title WIP: Add support to use SNI Add support to use SNI Jun 10, 2022
@jpdasma
Copy link
Contributor Author

jpdasma commented Jun 10, 2022

The failing test should be fixed now. I also tested with db.debian.org:636.

From some of my readings, SNI is an optional TLS extension, so it should be safe to be enabled by default.

Although we might still want to test it to other LDAP server with TLS.

@jpdasma
Copy link
Contributor Author

jpdasma commented Sep 2, 2022

@HarlemSquirrel based on my tests, I think this should be safe to merge. I didn't encountered any issues with OpenLDAP and Windows LDAP.

@HarlemSquirrel HarlemSquirrel merged commit 3f94287 into ruby-ldap:master Sep 2, 2022
@HarlemSquirrel
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants