Encryption Bug

[tonobo]

Hey, 0.12.1 is incompatible with 0.13.0. I get the following trace, with encryption enabled.

"no implicit conversion of Symbol into Integer"

net-ldap (0.13.0) lib/net/ldap/connection.rb:118:in []' net-ldap (0.13.0) lib/net/ldap/connection.rb:118:insetup_encryption'
net-ldap (0.13.0) lib/net/ldap/connection.rb:30:in prepare_socket' net-ldap (0.13.0) lib/net/ldap/connection.rb:44:inblock in open_connection'
net-ldap (0.13.0) lib/net/ldap/connection.rb:42:in each' net-ldap (0.13.0) lib/net/ldap/connection.rb:42:inopen_connection'
net-ldap (0.13.0) lib/net/ldap/connection.rb:19:in initialize' net-ldap (0.13.0) lib/net/ldap.rb:1254:innew'
net-ldap (0.13.0) lib/net/ldap.rb:1254:in new_connection' net-ldap (0.13.0) lib/net/ldap.rb:849:inblock in bind'
net-ldap (0.13.0) lib/net/ldap/instrumentation.rb:19:in instrument' net-ldap (0.13.0) lib/net/ldap.rb:843:inbind'

[jch]

@timmyarch could you include the snippet of code you're calling with?

[tonobo]

@jch this will do the trick ;) - the bind will raise

ldap = Net::LDAP.new(
:host => LDAP_HOST,
:port => LDAP_PORT,
:base => LDAP_BASE,
:encryption => :simple_tls,
:auth => { :method => :simple, :username => user_dn, :password => password }
)
ldap.bind

[jch]

@timmyarch The constructor way of initializing encryption requires a hash (#239). This is different than old way of setting it through the #encryption method, but it does standardize the param to always be a hash. Your example should work with:

ldap = Net::LDAP.new(:encryption => { :simple_tls }, ...)
ldap.bind

@satoryu I noticed the "Documentation" link on https://rubygems.org/gems/net-ldap/versions/0.13.0 links to a really old version of net-ldap (0.0.4). I tried to file a bug on rubydoc.info, but got a 500. Any ideas?

[tonobo]

@jch encryption accepts an array? Are there multiple types of encryption?

[jch]

@timmyarch there are, docs are unfortunately linking to the wrong version, but you can see it in the source here:

ruby-net-ldap/lib/net/ldap.rb

Lines 479 to 505 in b32f4e7

	# * :encryption => specifies the encryption to be used in communicating
	# with the LDAP server. The value must be a Hash containing additional
	# parameters, which consists of two keys:
	# method: - :simple_tls or :start_tls
	# options: - Hash of options for that method
	# The :simple_tls encryption method encrypts <i>all</i> communications
	# with the LDAP server. It completely establishes SSL/TLS encryption with
	# the LDAP server before any LDAP-protocol data is exchanged. There is no
	# plaintext negotiation and no special encryption-request controls are
	# sent to the server. <i>The :simple_tls option is the simplest, easiest
	# way to encrypt communications between Net::LDAP and LDAP servers.</i>
	# It's intended for cases where you have an implicit level of trust in the
	# authenticity of the LDAP server. No validation of the LDAP server's SSL
	# certificate is performed. This means that :simple_tls will not produce
	# errors if the LDAP server's encryption certificate is not signed by a
	# well-known Certification Authority. If you get communications or
	# protocol errors when using this option, check with your LDAP server
	# administrator. Pay particular attention to the TCP port you are
	# connecting to. It's impossible for an LDAP server to support plaintext
	# LDAP communications and <i>simple TLS</i> connections on the same port.
	# The standard TCP port for unencrypted LDAP connections is 389, but the
	# standard port for simple-TLS encrypted connections is 636. Be sure you
	# are using the correct port.
	#
	# The :start_tls like the :simple_tls encryption method also encrypts all
	# communcations with the LDAP server. With the exception that it operates
	# over the standard TCP port.

[satoryu]

@jch the current link is for ruby-net-ldap not net-ldap. replace it with http://www.rubydoc.info/gems/net-ldap/Net/LDAP .

[jch]

@satoryu thanks. It's closer at 0.12.1, but still not quite there.

[tonobo]

Ah nice thanks, this will help. But it will be fine to get a pre parameter validation. To prevent such fails. Normally it should be always backwards compatible after minor Version bump.

[christopher-b]

To clarify @jch's example above, either of these should work:

ldap = Net::LDAP.new(:encryption => { :method => :simple_tls }, ...)
ldap = Net::LDAP.new(:encryption => [ :simple_tls ], ...)

[jch]

But it will be fine to get a pre parameter validation. To prevent such fails.

@satoryu would you be interested in adding a check and raising ArgumentError if someone passes in a non-hash?

Normally it should be always backwards compatible after minor Version bump.

@timmyarch I agree with you that this was confusing behavior to a user, but this is actually a new interface so there is no backwards compatibility to consider. The old way of setting encryption was to set it after initialization:

ldap = Net::LDAP.new(...)
ldap.encryption(...)

This makes me want to revisit releasing a major version so we can have more stability in the API since there are many production environments that rely on this gem. I'll open up a separate issue for that.

[satoryu]

👌 I'll sent a PR.

[cthielen]

I want to second this API breakage lament.

We use net-ldap heavily in production and also have been passing encryption as a simple symbol, not a hash, and ended up with minor production downtime.

[jch]

@cthielen Thanks for reporting in.

How did you previously set up encryption? This introduces a new way of setting encryption through the constructor, which was not previously available. If you set it up via #encryption, the API is deprecated, but continues to work as previously. There shouldn't be an API breakage.

[sgringwe]

Also was affected by this with same configuration as @timmyarch. We mock most of our ldap code in tests so we didn't catch it.

Regardless @jch, not too big of a deal and I appreciate the time you spend on this library!

[ryanshow]

@jch the Net::LDAP constructor has accepted an :encryption parameter since at least v0.2, see here:

https://github.com/ruby-ldap/ruby-net-ldap/blob/v0.2/lib/net/ldap.rb#L382

It was modified in def2c46 to pass-through the given parameter instead of running it through the encryption method to normalize it into a hash, thus breaking the existing API for anyone who was passing in a symbol.

This breaks, amongst other things, omniauth-ldap which is supplying the encryption parameter to the constructor. See https://github.com/intridea/omniauth-ldap/blob/9d36cdb9f3d4da040ab6f7aff54450392b78f5eb/lib/omniauth-ldap/adaptor.rb#L52

[jch]

@ryanshow @cthielen I'm sorry, I must've missed that during code review. I believe @satoryu expressed interest in working on this, but would either of you be interested in opening a PR to remedy this?

[jch]

v0.14.0 should fix this #265

[satoryu]

@ryanshow @timmyarch @cthielen I'm sorry. As @ryanshow 's work #264 , you would be able to specify encryption method as Symbol.

@jch I was working on implementing a validation for encryption option which raises an exception if a given encryption option is not a Hash. I think it is not needed since normalization has been adopted. what do you think?

[tonobo]

@satoryu Thanks a lot! 👍

@jch, @satoryu The greatest way in my opinion would be the implement a generic parameter validation.