[WIP] add URL support to constructor, some smarter TLS defaults

[tmaher]

fixes #246 (hi @danleyden), relevant RFCs are 2255 (obsolete) and 4516. This is in-progress because I haven't updated the class docs or tests yet. Before I do, I wanted to open the PR and make sure I'm generally on the right track. Example:

Net::LDAP.new(url: 'ldaps://ldap01.example.com:9636/dc=example,dc=com',
              auth: {method: :simple, username: <bind_dn>, password: <bind_pw>})
#
# is the same as
#
Net::LDAP.new(host: 'ldap01.example.com',
              port: 9636,
              base: 'dc=example,dc=com',
              encryption: {method: :simple_tls,
                           tls_options: OpenSSL::SSL::SSLContext::DEFAULT_PARAMS },
              auth: {method: :simple, username: <bind_dn>, password: <bind_pw>})

but wait, there's more!

Net::LDAP.new(url: 'ldap://ldap01.example.com',
              encryption: true)
#
# is the same as
#
Net::LDAP.new(host: 'ldap01.example.com',
              encryption: {method: :start_tls,
                           tls_options: :default })
#
# or if you're not into the whole brevity thing
#
Net::LDAP.new(host: 'ldap01.example.com',
              encryption: {method: :start_tls
                           tls_options: OpenSSL::SSL::SSLContext::DEFAULT_PARAMS })

TLS changes

• If you set encryption: true (previously a noop), the internal @encryption hash gets populated with the OpenSSL defaults. Those defaults set VERIFY_PEER (cert validation!) and use the default OS-provided CA certificate store. Yay easy cert validation!
• To be explicit: the new encryption: true argument does NOT require the use of url. If you don't set an url, method: :start_tls vs :simple_tls is set by looking at the port. 636 is :simple_tls, everything else is :start_tls.
• tls_options: :default is a shortcut for tls_options: OpenSSL::SSL::SSLContext::DEFAULT_PARAMS. That string is too long.

More rambling

Aside from search base DN, the LDAP URL RFCs also allow you to specify a constrained list of attributes to return on search results, a scope (sub/one/all), and a search filter. A default scope for searches seems like a reasonable thing for Net::LDAP instances to track, similar to base DN, but it doesn't currently so I didn't add it. I could see an argument for writing a Net::LDAP#search_by_url, or extending the existing Net::LDAP#search, but I think this gem has higher priorities.

Credentials: The RFCs don't say anything about accepting username/pw befor the hostname (e.g. http://user:[email protected]/), and it's in formal ABNF and everything, so I've avoided handling those. I don't particularly want to encourage people to put passwords in URLs.

You'll note from the rubocop config change that rubocop started complaining about Net::LDAP's length. I think we can shrink that considerably by moving a ton of constants into their own class/module. That's getting to be a more major change, though, so next time.

/cc @jch

[tmaher]

General note: My intention here is for this to be a noop for existing code. Specifically, this PR should NOT change the cert validation behavior introduced in #279. If you don't specify an URL and don't set encryption: true, no cert validation by default. If you use an ldaps:// URL, or encryption: true, or tls_options: :default, you get cert validation.

[danleyden]

@tmaher not looked at the code, but the examples look to be exactly what I was meaning.

One further improvement that would add to my happiness would be supporting multiple urls. Currently, I believe that net-ldap supports providing multiple ldap servers through providing hosts rather than host.

Would it be possible to support multiple URLs? I suspect that there would likely need to be some error checking to detect if more than the host/port/protocol combination were different and raise appropriate errors.

Of course, not having double checked the code, that might already be there.

Thanks - this will help us get rid of a load of nasty code in our app

[jch]

Heh someday, it'd be nice to slim down this list of acceptable arg types.

[tmaher]

Down the road, I'm thinking of splitting tls_options out as a top-level argument passed into Net::LDAP.new. @encryption should then be able to be just a symbol. Having it be a hash with two entries makes it more complicated to invoke correctly.

[jch]

Having it be a hash with two entries makes it more complicated to invoke correctly

👍

[jch]

If you set encryption: true (previously a noop), the internal @encryption hash gets populated with the OpenSSL defaults. Those defaults set VERIFY_PEER (cert validation!) and use the default OS-provided CA certificate store. Yay easy cert validation!

Maybe I missed this in #279, but was there a breaking change in the encryption: param behavior in either this or the other PR? Something we should note in the changelog before we release.

A default scope for searches seems like a reasonable thing for Net::LDAP instances to track, similar to base DN, but it doesn't currently so I didn't add it

👍 Sounds reasonable

I could see an argument for writing a Net::LDAP#search_by_url, or extending the existing Net::LDAP#search, but I think this gem has higher priorities

Agreed. Even though the spec allows it, I'd rather not introduce a new public interface for search.

The RFCs don't say anything about accepting username/pw befor the hostname (e.g. http://user:[email protected]/), and it's in formal ABNF and everything, so I've avoided handling those. I don't particularly want to encourage people to put passwords in URLs.

👍 I can imagine some complaints about this, perhaps it'd be worth documenting

You'll note from the rubocop config change that rubocop started complaining about Net::LDAP's length. I think we can shrink that considerably by moving a ton of constants into their own class/module. That's getting to be a more major change, though, so next time.

Thanks for being mindful of keeping pull requests focused. It makes them easier to review

Would it be possible to support multiple URLs? I suspect that there would likely need to be some error checking to detect if more than the host/port/protocol combination were different and raise appropriate errors.

I would prefer this to go into a separate PR if you do decide to implement it.

@tmaher could you add some tests for this?