Skip to content

Commit

Permalink
ignore default username when no password is set
Browse files Browse the repository at this point in the history
fixes a regression introduced with the api_key support for xpack monitoring and management in elastic#11864 which disabled the possibility to not use any authentication by relying on the default options and only enabling monitoring for example. It now ignores the default username option when no password is explicitly set.
  • Loading branch information
colinsurprenant authored and elasticsearch-bot committed Jul 13, 2020
1 parent 2afe60d commit 87df15d
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 24 deletions.
16 changes: 5 additions & 11 deletions x-pack/lib/helpers/elasticsearch_options.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,12 @@ def es_options_from_settings(feature, settings)
opts['ssl'] = true
end

# the username setting has a default value and should not be included when using another authentication
# the username setting has a default value and should not be included when using another authentication such as cloud_auth or api_key.
# it should also not be included when no password is set.
# it is safe to silently remove here since all authentication verifications have been validated at this point.
if settings.set?("#{prefix}#{feature}.elasticsearch.cloud_auth") || settings.set?("#{prefix}#{feature}.elasticsearch.api_key")
if settings.set?("#{prefix}#{feature}.elasticsearch.cloud_auth") ||
settings.set?("#{prefix}#{feature}.elasticsearch.api_key") ||
(!settings.set?("#{prefix}#{feature}.elasticsearch.password") && !settings.set?("#{prefix}#{feature}.elasticsearch.username"))
opts.delete('user')
end

Expand Down Expand Up @@ -201,15 +204,6 @@ def validate_authentication!(feature, settings, prefix)
authentication_count += 1 if provided_password
authentication_count += 1 if provided_api_key

if authentication_count == 0
# when no explicit authentication is set it is relying on default username
# but without and explicit password set
raise(ArgumentError,
"With the default #{prefix}#{feature}.elasticsearch.username, " +
"#{prefix}#{feature}.elasticsearch.password must be set"
)
end

if authentication_count > 1
raise(ArgumentError, "Multiple authentication options are specified, please only use one of #{prefix}#{feature}.elasticsearch.username/password, #{prefix}#{feature}.elasticsearch.cloud_auth or #{prefix}#{feature}.elasticsearch.api_key")
end
Expand Down
8 changes: 6 additions & 2 deletions x-pack/spec/config_management/elasticsearch_source_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,12 @@
}
end

it "should raise an ArgumentError" do
expect { described_class.new(system_settings) }.to raise_error(ArgumentError)
it "will rely on username and password settings" do
# since cloud_id and cloud_auth are simply containers for host and username/password
# both could be set independently and if cloud_auth is not set then authn will be done
# using the provided username/password settings, which can be set or not if not auth is
# required.
expect { described_class.new(system_settings) }.to_not raise_error
end
end
end
Expand Down
20 changes: 9 additions & 11 deletions x-pack/spec/helpers/elasticsearch_options_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,11 @@
}
end

it "fails without password" do
expect {
test_class.es_options_from_settings_or_modules('monitoring', system_settings)
}.to raise_error(ArgumentError, /password must be set/)
it "ignores the implicit default username when no password is set" do
# when no explicit password is set then the default/implicit username should be ignored
es_options = test_class.es_options_from_settings_or_modules('monitoring', system_settings)
expect(es_options).to_not include("user")
expect(es_options).to_not include("password")
end

context "with cloud_auth" do
Expand Down Expand Up @@ -296,13 +297,10 @@
end

context "when cloud_auth is not set" do

it "raises for invalid configuration" do
# if not other authn is provided it will assume basic auth using the default username
# but the password is missing.
expect {
test_class.es_options_from_settings_or_modules('monitoring', system_settings)
}.to raise_error(ArgumentError, /With the default.*?username,.*?password must be set/)
it "does not use authentication and ignores the default username" do
es_options = test_class.es_options_from_settings_or_modules('monitoring', system_settings)
expect(es_options).to include("cloud_id")
expect(es_options.keys).to_not include("hosts", "user", "password")
end

context 'username and password set' do
Expand Down

0 comments on commit 87df15d

Please sign in to comment.