Remove use of deprecated http_instance method, SSL verification parameterized #18
Conversation
…rent connection method, as well as creating an SSL context. Using this new interface, SSL verification can be enabled/disabled with an optional parameter.
|
Compared to #16, this PR also allows SSL verification enable/disabling. |
| @@ -40,6 +43,24 @@ def lookup(path, vault_url = nil) | |||
|
|
|||
| private | |||
|
|
|||
| def create_ssl_context(verify_ssl) | |||
| ssl_provider = Puppet::SSL::SSLProvider.new | |||
| default_ssl_context = ssl_provider.load_context() | |||
There was a problem hiding this comment.
In another project I used Puppet.lookup(:ssl_context). Looking at https://github.com/puppetlabs/puppet/blob/efa75f0fb0b5d4877750c3eadddd70bcc4ee081d/lib/puppet.rb#L210-L223 it looks like that looks like it also handles the password and might be a better base.
| @@ -19,7 +20,9 @@ def lookup(path, vault_url = nil) | |||
| raise Puppet::Error, "Unable to parse a hostname from #{vault_url}" unless uri.hostname | |||
|
|
|||
| use_ssl = uri.scheme == 'https' | |||
| connection = Puppet::Network::HttpPool.http_instance(uri.host, uri.port, use_ssl) | |||
| ssl_context = create_ssl_context(verify_ssl) | |||
There was a problem hiding this comment.
I think it should be nil when there's no ssl
| ssl_context = create_ssl_context(verify_ssl) | |
| ssl_context = use_ssl ? create_ssl_context(verify_ssl) : nil |
|
@BnMcG I'd like to get this over the line. Could you look at the failing tests? |
|
Sure, I'll try and take a look this week. |
|
@BnMcG Can you upgrade to at least Puppet Agent 6.15 and see if http_instance still throws a deprecation warning? |
|
I'm closing this since this was implemented in #50 |
Pull Request (PR) description
This PR removes the use of the deprecated http_instance method and replaces it with the suggested connection method instead.
Using the new connection interface, it's possible to specify an SSL context, and therefore whether or not to verify SSL connections. I've implemented this as an optional parameter that defaults to true (ie: the default behaviour should remain unchanged).
By parameterizing SSL verification, it's possible to connect to established Vault instances over HTTPS, that are outside of the PKI chain established by the Puppet server. Given that the plugin currently allows the option of HTTP connections, it doesn't seem less secure to also optionally disable SSL verification.
I'm not very familiar with Ruby development, so I've not modified any of the existing tests, but if there aren't any objections to this new approach, I'm happy to try and write tests covering the new functionality, and reflecting the change in Puppet API interface use. Anecdotally, I have tested this with my own Vault (1.1.0) server and Puppet setup (6.4.0) and it works under normal use.
This Pull Request (PR) fixes the following issues
n/a