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

Remove deprecated SSL settings and simplify SSL configuration #213

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
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
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
## 5.0.0 ##
- SSL settings that were marked deprecated in version `4.17.0` are now marked obsolete, and will prevent the plugin from starting.
- These settings are:
- `ssl`, which should bre replaced by `ssl_enabled`
- `ca_file`, which should bre replaced by `ssl_certificate_authorities`
- `ssl_certificate_verification`, which should bre replaced by `ssl_verification_mode`
- [#213](https://github.com/logstash-plugins/logstash-input-elasticsearch/pull/213)

## 4.20.4
- Fix issue where the `index` parameter was being ignored when using `response_type => aggregations` [#209](https://github.com/logstash-plugins/logstash-input-elasticsearch/pull/209)

Expand Down
49 changes: 14 additions & 35 deletions docs/index.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,9 @@ TIP: Set the `target` option to avoid potential schema conflicts.

This plugin supports the following configuration options plus the <<plugins-{type}s-{plugin}-common-options>> and the <<plugins-{type}s-{plugin}-deprecated-options>> described later.

NOTE: As of version `5.0.0` of this plugin, a number of previously deprecated settings related to SSL have been removed.
Please check out <<plugins-{type}s-{plugin}-obsolete-options>> for details.

[cols="<,<,<",options="header",]
|=======================================================================
|Setting |Input type|Required
Expand Down Expand Up @@ -478,6 +481,8 @@ Enable SSL/TLS secured communication to Elasticsearch cluster.
Leaving this unspecified will use whatever scheme is specified in the URLs listed in <<plugins-{type}s-{plugin}-hosts>> or extracted from the <<plugins-{type}s-{plugin}-cloud_id>>.
If no explicit protocol is specified plain HTTP will be used.

When not explicitly set, SSL will be automatically enabled if any of the specified hosts use HTTPS.

[id="plugins-{type}s-{plugin}-ssl_key"]
===== `ssl_key`
* Value type is <<path,path>>
Expand Down Expand Up @@ -608,47 +613,21 @@ option when authenticating to the Elasticsearch server. If set to an
empty string authentication will be disabled.


[id="plugins-{type}s-{plugin}-deprecated-options"]
==== Elasticsearch Input deprecated configuration options
[id="plugins-{type}s-{plugin}-obsolete-options"]
==== Elasticsearch Input Obsolete Configuration Options

This plugin supports the following deprecated configurations.
WARNING: As of version `5.0.0` of this plugin, some configuration options have been replaced.
The plugin will fail to start if it contains any of these obsolete options.

WARNING: Deprecated options are subject to removal in future releases.

[cols="<,<,<",options="header",]
[cols="<,<",options="header",]
|=======================================================================
|Setting|Input type|Replaced by
| <<plugins-{type}s-{plugin}-ca_file>> |a valid filesystem path|<<plugins-{type}s-{plugin}-ssl_certificate_authorities>>
| <<plugins-{type}s-{plugin}-ssl>> |<<boolean,boolean>>|<<plugins-{type}s-{plugin}-ssl_enabled>>
| <<plugins-{type}s-{plugin}-ssl_certificate_verification>> |<<boolean,boolean>>|<<plugins-{type}s-{plugin}-ssl_verification_mode>>
|Setting|Replaced by
| ca_file | <<plugins-{type}s-{plugin}-ssl_certificate_authorities>>
| ssl | <<plugins-{type}s-{plugin}-ssl_enabled>>
| ssl_certificate_verification | <<plugins-{type}s-{plugin}-ssl_verification_mode>>
|=======================================================================

[id="plugins-{type}s-{plugin}-ca_file"]
===== `ca_file`
deprecated[4.17.0, Replaced by <<plugins-{type}s-{plugin}-ssl_certificate_authorities>>]

* Value type is <<path,path>>
* There is no default value for this setting.

SSL Certificate Authority file in PEM encoded format, must also include any chain certificates as necessary.

[id="plugins-{type}s-{plugin}-ssl"]
===== `ssl`
deprecated[4.17.0, Replaced by <<plugins-{type}s-{plugin}-ssl_enabled>>]

* Value type is <<boolean,boolean>>
* Default value is `false`

If enabled, SSL will be used when communicating with the Elasticsearch
server (i.e. HTTPS will be used instead of plain HTTP).


[id="plugins-{type}s-{plugin}-ssl_certificate_verification"]
===== `ssl_certificate_verification`
deprecated[4.17.0, Replaced by <<plugins-{type}s-{plugin}-ssl_verification_mode>>]

* Value type is <<boolean,boolean>>
* Default value is `true`

Option to validate the server's certificate. Disabling this severely compromises security.
When certificate validation is disabled, this plugin implicitly trusts the machine
Expand Down
54 changes: 9 additions & 45 deletions lib/logstash/inputs/elasticsearch.rb
Original file line number Diff line number Diff line change
Expand Up @@ -198,23 +198,12 @@ class LogStash::Inputs::Elasticsearch < LogStash::Inputs::Base
# Set the address of a forward HTTP proxy.
config :proxy, :validate => :uri_or_empty

# SSL
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than remove, let's mark as obsolete. We typically go from deprecated -> obsolete -> final removal

Marking a setting as obsolete triggers a specific error message in Logstash that prevents Logstash from starting, but gives an informative error message to the user:

https://github.com/elastic/logstash/blob/2a23680cfd12438dc1a4429a1717a31e06e47816/logstash-core/locales/en.yml#L146

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, I totally missed that. Will get this fixed up!

config :ssl, :validate => :boolean, :default => false, :deprecated => "Set 'ssl_enabled' instead."

# SSL Certificate Authority file in PEM encoded format, must also include any chain certificates as necessary
config :ca_file, :validate => :path, :deprecated => "Set 'ssl_certificate_authorities' instead."

# OpenSSL-style X.509 certificate certificate to authenticate the client
config :ssl_certificate, :validate => :path

# SSL Certificate Authority files in PEM encoded format, must also include any chain certificates as necessary
config :ssl_certificate_authorities, :validate => :path, :list => true

# Option to validate the server's certificate. Disabling this severely compromises security.
# For more information on the importance of certificate verification please read
# https://www.cs.utexas.edu/~shmat/shmat_ccs12.pdf
config :ssl_certificate_verification, :validate => :boolean, :default => true, :deprecated => "Set 'ssl_verification_mode' instead."

# The list of cipher suites to use, listed by priorities.
# Supported cipher suites vary depending on which version of Java is used.
config :ssl_cipher_suites, :validate => :string, :list => true
Expand Down Expand Up @@ -242,7 +231,6 @@ class LogStash::Inputs::Elasticsearch < LogStash::Inputs::Base
config :ssl_truststore_password, :validate => :password

# The JKS truststore to validate the server's certificate.
# Use either `:ssl_truststore_path` or `:ssl_certificate_authorities`
config :ssl_truststore_path, :validate => :path

# The format of the truststore file. It must be either jks or pkcs12
Expand All @@ -264,6 +252,11 @@ class LogStash::Inputs::Elasticsearch < LogStash::Inputs::Base
# If set, the _source of each hit will be added nested under the target instead of at the top-level
config :target, :validate => :field_reference

# Obsolete Settings
config :ssl, :obsolete => "Set 'ssl_enabled' instead."
config :ca_file, :obsolete => "Set 'ssl_certificate_authorities' instead."
config :ssl_certificate_verification, :obsolete => "Set 'ssl_verification_mode' instead."

# config :ca_trusted_fingerprint, :validate => :sha_256_hex
include LogStash::PluginMixins::CATrustedFingerprintSupport

Expand Down Expand Up @@ -406,8 +399,6 @@ def setup_client_ssl
ssl_options[:ssl] = true if @ssl_enabled

unless @ssl_enabled
# Keep it backward compatible with the deprecated `ssl` option
ssl_options[:trust_strategy] = trust_strategy_for_ca_trusted_fingerprint if original_params.include?('ssl')
return ssl_options
end

Expand Down Expand Up @@ -471,38 +462,11 @@ def setup_client_ssl
end

def setup_ssl_params!
@ssl_enabled = normalize_config(:ssl_enabled) do |normalize|
normalize.with_deprecated_alias(:ssl)
end

# Infer the value if neither the deprecate `ssl` and `ssl_enabled` were set
infer_ssl_enabled_from_hosts

@ssl_certificate_authorities = normalize_config(:ssl_certificate_authorities) do |normalize|
normalize.with_deprecated_mapping(:ca_file) do |ca_file|
[ca_file]
end
# Only infer ssl_enabled if it wasn't explicitly set
unless original_params.include?('ssl_enabled')
@ssl_enabled = effectively_ssl?
params['ssl_enabled'] = @ssl_enabled
end

@ssl_verification_mode = normalize_config(:ssl_verification_mode) do |normalize|
normalize.with_deprecated_mapping(:ssl_certificate_verification) do |ssl_certificate_verification|
if ssl_certificate_verification == true
"full"
else
"none"
end
end
end

params['ssl_enabled'] = @ssl_enabled
params['ssl_certificate_authorities'] = @ssl_certificate_authorities unless @ssl_certificate_authorities.nil?
params['ssl_verification_mode'] = @ssl_verification_mode unless @ssl_verification_mode.nil?
end

def infer_ssl_enabled_from_hosts
return if original_params.include?('ssl') || original_params.include?('ssl_enabled')

@ssl_enabled = params['ssl_enabled'] = effectively_ssl?
end

def setup_hosts
Expand Down
2 changes: 1 addition & 1 deletion logstash-input-elasticsearch.gemspec
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
Gem::Specification.new do |s|

s.name = 'logstash-input-elasticsearch'
s.version = '4.20.4'
s.version = '5.0.0'
s.licenses = ['Apache License (2.0)']
s.summary = "Reads query results from an Elasticsearch cluster"
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
13 changes: 13 additions & 0 deletions spec/inputs/elasticsearch_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,19 @@
end
end

describe 'handling obsolete settings' do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[{:name => 'ssl', :replacement => 'ssl_enabled', :sample_value => true},
{:name => 'ca_file', :replacement => 'ssl_certificate_authorities', :sample_value => 'spec/fixtures/test_certs/ca.crt'},
{:name => 'ssl_certificate_verification', :replacement => 'ssl_verification_mode', :sample_value => false }].each do | obsolete_setting|
context "with obsolete #{obsolete_setting[:name]}" do
let (:config) { {obsolete_setting[:name] => obsolete_setting[:sample_value]} }
it "should raise a config error with the appropriate message" do
expect { plugin.register }.to raise_error LogStash::ConfigurationError, /The setting `#{obsolete_setting[:name]}` in plugin `elasticsearch` is obsolete and is no longer available. Set '#{obsolete_setting[:replacement]}' instead/i
end
end
end
end

context "against not authentic Elasticsearch" do
before(:each) do
Elasticsearch::Client.send(:define_method, :ping) { raise Elasticsearch::UnsupportedProductError.new("Fake error") } # define error ping method
Expand Down