Implement custom tls options #161
Conversation
|
👍 |
| @@ -17,6 +17,8 @@ def initialize(server) | |||
| raise Net::LDAP::LdapError, "Server #{server[:host]} refused connection on port #{server[:port]}." | |||
| rescue Errno::EHOSTUNREACH => error | |||
| raise Net::LDAP::LdapError, "Host #{server[:host]} was unreachable (#{error.message})" | |||
| rescue Errno::ETIMEDOUT | |||
| raise Net::LDAP::LdapError, "Connection to #{server[:host]} timed out." | |||
There was a problem hiding this comment.
Nice addition, but could you file this in a separate PR? I'd like to keep this focused to the SSL changes.
There was a problem hiding this comment.
Alright, will do. Would you like a rebase, or a new commit that reverts this?
There was a problem hiding this comment.
I'm not picky about pristine git history. Your choice ;)
This allows users of the API to either specify a simple CA-File for certificate verification, or a custom SSLContext in order for a more fine-grained control of the TLS options they want to use. As of now, no additional tests were added, but the existing tests were changed to reflect the changes in internal methods, so that they can still pass
By default, we don't set any parameters (empty or no :tls_options). If the hash is non-empty, we pass it directly to set_params and use the resulting context for creating the TLS socket.
|
Looking good. I wish we had integration tests setup around SSL, but that's outside the scope of this PR. If anyone's looking for a side project, the OpenLDAP integration test README would be a good starting point. cc @mtodd |
|
@jch I've actually written integration tests for TLS verification in an internal library using ruby-ldapserver. It's fairly easy to do: |
|
@tarcieri neat. While it'd be convenient to have a ruby test server, I'm not interested in adding that as one of the integration test targets because it is not a common production server (correct me if I'm wrong). I don't think it would be hard to have TLS integration tests in this projects. The steps, roughly, would be:
|
|
@jch for the purposes of testing verification of TLS certs, you don't even need an LDAP server, you could just as easily use a minimal TCP server w\ |
I trust ruby's stdlib implementations, so I'm not interested in standalone testing of TLS cert verification. The integration test with OpenLDAP (and other servers) would be more of a sanity check to make sure we don't release something incompatible with those specific servers. |
|
Aah, I see. As a user of OpenLDAP, I agree that would be cool 😉 |
Implement custom tls options
This allows users of the API to either specify a simple CA-File for certificate verification, or a custom SSLContext in order for a more fine-grained control of the TLS options they want to use.
As of now, no additional tests were added, but the existing tests were changed to reflect the changes in internal methods, so that they still pass.
I have some tests available, but they were designed for another monkeypatch around this, so I'll have to take some time to adjust them for this code before I can publish them.
This pull request also contains the test fix from #160.