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 ssl_enabled option #44

Merged
merged 1 commit into from
Jun 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
## 7.4.0
- Adds new `ssl_enabled` setting for enabling/disabling the SSL configurations [#44](https://github.com/logstash-plugins/logstash-mixin-http_client/pull/44)

## 7.3.0
- Adds standardized SSL settings and deprecates their non-standard counterparts. Deprecated settings will continue to work, and will provide pipeline maintainers with guidance toward using their standardized counterparts [#42](https://github.com/logstash-plugins/logstash-mixin-http_client/pull/42)
- Adds new `ssl_truststore_path`, `ssl_truststore_password`, and `ssl_truststore_type` settings for configuring SSL-trust using a PKCS-12 or JKS trust store, deprecating their `truststore`, `truststore_password`, and `truststore_type` counterparts.
Expand Down
10 changes: 10 additions & 0 deletions lib/logstash/plugin_mixins/http_client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ def self.included(base)
# See https://hc.apache.org/httpcomponents-client-ga/httpclient/apidocs/org/apache/http/impl/conn/PoolingHttpClientConnectionManager.html#setValidateAfterInactivity(int)[these docs for more info]
base.config :validate_after_inactivity, :validate => :number, :default => 200

# Enable/disable the SSL configurations
base.config :ssl_enabled, :validate => :boolean, :default => true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

es-output and es-input have ssl_enabled with no default value. It bases on the host prefix, https or http, to enable ssl feature, which is kind of setting to auto.

Would default to true changes the behaviour of existing plugins?
Could the mixin default to auto and preserve the current smart ssl feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kaisecheng! This mixin already assumes ssl_enabled => true, adding all the configured ssl_* settings to the manticore's client, without giving users an option to disable them (other than comment out the SSL configuration). That said, it shouldn't change any existing behaviour.

Currently, this mixin is being used by the HTTP filter, output and poller plugins, and won't affect ES and other plugins that does not use it under the hood.

The ideal behaviour of the ssl_enabled flag is under discussing, there are different behaviours for this option depending on the plugin, and that will be standardised on the second phase of the SSL Standardisation project. Adding this option only makes it compliant with the defined common settings.

Could the mixin default to auto and preserve the current smart ssl feature?

For the existing plugin using this mixin, no. The client itself does not know how the plugin will use it, it doesn't know the URLs it will request, for example. it only adds the configs and builds the client, hading it over to the plugins to execute the requests and add any other smart features.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, I mixed the http plugin with es plugin 🤦 The settings make perfect sense


# If you need to use a custom X.509 CA (.pem certs) specify the path to that here
base.config :ssl_certificate_authorities, :validate => :path, :list => :true

Expand Down Expand Up @@ -188,6 +191,13 @@ def client_config
def ssl_options

options = {}

unless @ssl_enabled
ignored_ssl_settings = original_params.select { |k| k != 'ssl_enabled' && k.start_with?('ssl_') }
self.logger.warn("Configured SSL settings are not used when `ssl_enabled` is set to `false`: #{ignored_ssl_settings.keys}") if ignored_ssl_settings.any?
return options
end

if @ssl_certificate_authorities&.any?
raise LogStash::ConfigurationError, 'Multiple values on `ssl_certificate_authorities` are not supported by this plugin' if @ssl_certificate_authorities.size > 1

Expand Down
2 changes: 1 addition & 1 deletion logstash-mixin-http_client.gemspec
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Gem::Specification.new do |s|
s.name = 'logstash-mixin-http_client'
s.version = '7.3.0'
s.version = '7.4.0'
s.licenses = ['Apache License (2.0)']
s.summary = "AWS mixins to provide a unified interface for Amazon Webservice"
s.description = "This gem is a Logstash plugin required to be installed on top of the Logstash core pipeline using $LS_HOME/bin/logstash-plugin install gemname. This gem is not a stand-alone program"
Expand Down
26 changes: 26 additions & 0 deletions spec/plugin_mixin/http_client_ssl_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,32 @@
end
end
end

describe 'with ssl_enabled' do
context 'set to false' do
let(:basic_config) { super().merge('ssl_enabled' => false) }
let(:plugin) { plugin_class.new(basic_config) }

it 'should not configure the client :ssl' do
expect(plugin.client_config[:ssl]).to eq({})
end

context 'and another ssl_* config set' do
let(:basic_config) { super().merge('ssl_verification_mode' => 'none') }
let(:logger_mock) { double('logger') }

before(:each) do
allow(plugin).to receive(:logger).and_return(logger_mock)
end

it 'should log a warn message' do
allow(logger_mock).to receive(:warn)
plugin.client_config
expect(logger_mock).to have_received(:warn).with('Configured SSL settings are not used when `ssl_enabled` is set to `false`: ["ssl_verification_mode"]')
end
end
end
end
end
end

Expand Down