Skip to content

Commit 97ef7ed

Browse files
yaauiejsvd
andauthored
params: don't require explicit 'ssl=>true' when cloud_id or all-HTTPS hosts are present (#1066)
* params: don't require explicit 'ssl=>true' when cloud_id or all-HTTPS hosts are present * Apply suggestions from code review Co-authored-by: João Duarte <jsvd@users.noreply.github.com> * snip merge artifact * pr feedback: remove misleading line Co-authored-by: João Duarte <jsvd@users.noreply.github.com>
1 parent 99ba76f commit 97ef7ed

File tree

5 files changed

+96
-18
lines changed

5 files changed

+96
-18
lines changed

CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
## 11.11.0
2+
- When using an `api_key` along with either `cloud_id` or https `hosts`, you no longer need to also specify `ssl => true` [#1065](https://github.com/logstash-plugins/logstash-output-elasticsearch/issues/1065)
3+
14
## 11.10.0
25
- Feature: expose `dlq_routed` document metric to track the documents routed into DLQ [#1090](https://github.com/logstash-plugins/logstash-output-elasticsearch/pull/1090)
36

docs/index.asciidoc

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -406,8 +406,8 @@ For more details on actions, check out the {ref}/docs-bulk.html[Elasticsearch bu
406406
* Value type is <<password,password>>
407407
* There is no default value for this setting.
408408

409-
Authenticate using Elasticsearch API key. Note that this option also requires
410-
enabling the `ssl` option.
409+
Authenticate using Elasticsearch API key.
410+
Note that this option also requires SSL/TLS, which can be enabled by supplying a <<plugins-{type}s-{plugin}-cloud_id>>, a list of HTTPS <<plugins-{type}s-{plugin}-hosts>>, or by setting <<plugins-{type}s-{plugin}-ssl,`ssl => true`>>.
411411

412412
Format is `id:api_key` where `id` and `api_key` are as returned by the
413413
Elasticsearch {ref}/security-api-create-api-key.html[Create API key API].
@@ -1040,11 +1040,9 @@ do not use full URL here, only paths, e.g. "/sniff/_nodes/http"
10401040
* Value type is <<boolean,boolean>>
10411041
* There is no default value for this setting.
10421042

1043-
Enable SSL/TLS secured communication to Elasticsearch cluster. Leaving this
1044-
unspecified will use whatever scheme is specified in the URLs listed in 'hosts'.
1045-
If no explicit protocol is specified plain HTTP will be used. If SSL is
1046-
explicitly disabled here the plugin will refuse to start if an HTTPS URL is
1047-
given in 'hosts'
1043+
Enable SSL/TLS secured communication to Elasticsearch cluster.
1044+
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>>.
1045+
If no explicit protocol is specified plain HTTP will be used.
10481046

10491047
[id="plugins-{type}s-{plugin}-ssl_certificate_verification"]
10501048
===== `ssl_certificate_verification`

lib/logstash/plugin_mixins/elasticsearch/common.rb

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,14 @@ def build_client(license_checker = nil)
2323
# because they must be executed prior to building the client and logstash
2424
# monitoring and management rely on directly calling build_client
2525
# see https://github.com/logstash-plugins/logstash-output-elasticsearch/pull/934#pullrequestreview-396203307
26-
validate_authentication
2726
fill_hosts_from_cloud_id
27+
validate_authentication
28+
2829
setup_hosts
2930

31+
32+
params['ssl'] = effectively_ssl? unless params.include?('ssl')
33+
3034
# inject the TrustStrategy from CATrustedFingerprintSupport
3135
if trust_strategy_for_ca_trusted_fingerprint
3236
params["ssl_trust_strategy"] = trust_strategy_for_ca_trusted_fingerprint
@@ -49,7 +53,7 @@ def validate_authentication
4953
raise LogStash::ConfigurationError, 'Multiple authentication options are specified, please only use one of user/password, cloud_auth or api_key'
5054
end
5155

52-
if @api_key && @api_key.value && @ssl != true
56+
if @api_key && @api_key.value && !effectively_ssl?
5357
raise(LogStash::ConfigurationError, "Using api_key authentication requires SSL/TLS secured communication using the `ssl => true` option")
5458
end
5559

@@ -69,6 +73,15 @@ def setup_hosts
6973
end
7074
end
7175

76+
def effectively_ssl?
77+
return @ssl unless @ssl.nil?
78+
79+
hosts = Array(@hosts)
80+
return false if hosts.nil? || hosts.empty?
81+
82+
hosts.all? { |host| host && host.scheme == "https" }
83+
end
84+
7285
def hosts_default?(hosts)
7386
# NOTE: would be nice if pipeline allowed us a clean way to detect a config default :
7487
hosts.is_a?(Array) && hosts.size == 1 && hosts.first.equal?(LogStash::PluginMixins::ElasticSearch::APIConfigs::DEFAULT_HOST)
@@ -208,12 +221,12 @@ def next_sleep_interval(current_interval)
208221

209222
def handle_dlq_response(message, action, status, response)
210223
_, action_params = action.event, [action[0], action[1], action[2]]
211-
224+
212225
# TODO: Change this to send a map with { :status => status, :action => action } in the future
213226
detailed_message = "#{message} status: #{status}, action: #{action_params}, response: #{response}"
214-
227+
215228
log_level = dig_value(response, 'index', 'error', 'type') == 'invalid_index_name_exception' ? :error : :warn
216-
229+
217230
handle_dlq_status(action.event, log_level, detailed_message)
218231
end
219232

logstash-output-elasticsearch.gemspec

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
Gem::Specification.new do |s|
22
s.name = 'logstash-output-elasticsearch'
3-
s.version = '11.10.0'
3+
s.version = '11.11.0'
44
s.licenses = ['apache-2.0']
55
s.summary = "Stores logs in Elasticsearch"
66
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"

spec/unit/outputs/elasticsearch_spec.rb

Lines changed: 69 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,17 @@
1717
allow_any_instance_of(LogStash::Outputs::ElasticSearch::HttpClient::Pool).to receive(:start)
1818
end
1919

20+
let(:spy_http_client_builder!) do
21+
allow(described_class::HttpClientBuilder).to receive(:build).with(any_args).and_call_original
22+
end
23+
2024
let(:after_successful_connection_thread_mock) do
2125
double('after_successful_connection_thread', value: true)
2226
end
2327

2428
before(:each) do
2529
if do_register
30+
spy_http_client_builder!
2631
stub_http_client_pool!
2732

2833
allow(subject).to receive(:finish_register) # stub-out thread completion (to avoid error log entries)
@@ -1003,29 +1008,88 @@
10031008
let(:api_key) { "some_id:some_api_key" }
10041009
let(:base64_api_key) { "ApiKey c29tZV9pZDpzb21lX2FwaV9rZXk=" }
10051010

1006-
context "when set without ssl" do
1011+
shared_examples 'secure api-key authenticated client' do
1012+
let(:do_register) { true }
1013+
1014+
it 'adds the appropriate Authorization header to the manticore client' do
1015+
expect(manticore_options[:headers]).to eq({ "Authorization" => base64_api_key })
1016+
end
1017+
it 'is provides ssl=>true to the http client builder' do; aggregate_failures do
1018+
expect(described_class::HttpClientBuilder).to have_received(:build).with(anything, anything, hash_including('ssl'=>true))
1019+
end; end
1020+
end
1021+
1022+
context "when set without ssl => true" do
10071023
let(:do_register) { false } # this is what we want to test, so we disable the before(:each) call
10081024
let(:options) { { "api_key" => api_key } }
10091025

10101026
it "should raise a configuration error" do
10111027
expect { subject.register }.to raise_error LogStash::ConfigurationError, /requires SSL\/TLS/
10121028
end
1029+
1030+
context 'with cloud_id' do
1031+
let(:sample_cloud_id) { 'sample:dXMtY2VudHJhbDEuZ2NwLmNsb3VkLmVzLmlvJGFjMzFlYmI5MDI0MTc3MzE1NzA0M2MzNGZkMjZmZDQ2OjkyNDMkYTRjMDYyMzBlNDhjOGZjZTdiZTg4YTA3NGEzYmIzZTA6OTI0NA==' }
1032+
let(:options) { super().merge('cloud_id' => sample_cloud_id) }
1033+
1034+
it_behaves_like 'secure api-key authenticated client'
1035+
end
10131036
end
10141037

1015-
context "when set without ssl but with a https host" do
1038+
context "when set without ssl specified but with an https host" do
10161039
let(:do_register) { false } # this is what we want to test, so we disable the before(:each) call
10171040
let(:options) { { "hosts" => ["https://some.host.com"], "api_key" => api_key } }
10181041

1042+
it_behaves_like 'secure api-key authenticated client'
1043+
end
1044+
1045+
context "when set without ssl specified but with an http host`" do
1046+
let(:do_register) { false } # this is what we want to test, so we disable the before(:each) call
1047+
let(:options) { { "hosts" => ["http://some.host.com"], "api_key" => api_key } }
1048+
1049+
it "should raise a configuration error" do
1050+
expect { subject.register }.to raise_error LogStash::ConfigurationError, /requires SSL\/TLS/
1051+
end
1052+
end
1053+
1054+
context "when set with `ssl => false`" do
1055+
let(:do_register) { false } # this is what we want to test, so we disable the before(:each) call
1056+
let(:options) { { "ssl" => "false", "api_key" => api_key } }
1057+
10191058
it "should raise a configuration error" do
10201059
expect { subject.register }.to raise_error LogStash::ConfigurationError, /requires SSL\/TLS/
10211060
end
10221061
end
10231062

10241063
context "when set" do
1025-
let(:options) { { "ssl" => true, "api_key" => ::LogStash::Util::Password.new(api_key) } }
1064+
let(:options) { { "api_key" => ::LogStash::Util::Password.new(api_key) } }
10261065

1027-
it "should use the custom headers in the adapter options" do
1028-
expect(manticore_options[:headers]).to eq({ "Authorization" => base64_api_key })
1066+
context "with ssl => true" do
1067+
let(:options) { super().merge("ssl" => true) }
1068+
it_behaves_like 'secure api-key authenticated client'
1069+
end
1070+
1071+
context "with ssl => false" do
1072+
let(:options) { super().merge("ssl" => "false") }
1073+
1074+
let(:do_register) { false } # this is what we want to test, so we disable the before(:each) call
1075+
it "should raise a configuration error" do
1076+
expect { subject.register }.to raise_error LogStash::ConfigurationError, /requires SSL\/TLS/
1077+
end
1078+
end
1079+
1080+
context "without ssl specified" do
1081+
context "with an https host" do
1082+
let(:options) { super().merge("hosts" => ["https://some.host.com"]) }
1083+
it_behaves_like 'secure api-key authenticated client'
1084+
end
1085+
context "with an http host`" do
1086+
let(:do_register) { false } # this is what we want to test, so we disable the before(:each) call
1087+
let(:options) { { "hosts" => ["http://some.host.com"], "api_key" => api_key } }
1088+
1089+
it "should raise a configuration error" do
1090+
expect { subject.register }.to raise_error LogStash::ConfigurationError, /requires SSL\/TLS/
1091+
end
1092+
end
10291093
end
10301094
end
10311095

0 commit comments

Comments
 (0)