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

Validate monitoring password at parse time #47740

Merged
merged 8 commits into from
Nov 13, 2019

Conversation

danhermann
Copy link
Contributor

@danhermann danhermann commented Oct 8, 2019

Provides parse-time validation for AUTH_PASSWORD_SETTING as described in #47711.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (:Core/Features/Monitoring)

@danhermann danhermann changed the title Validate AUTH_PASSWORD_SETTING at parse time Validate monitoring password at parse time Oct 9, 2019
new Setting.Validator<String>() {
@Override
public void validate(String password) {

Copy link
Contributor

@jakelandis jakelandis Oct 14, 2019

Choose a reason for hiding this comment

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

Can you add a comment to why this is empty ?

edit: i mean a comment in the code :)

if (Strings.isNullOrEmpty(username)) {
if (Strings.isNullOrEmpty(password) == false) {
throw new SettingsException(
"[" + AUTH_PASSWORD_SETTING.getConcreteSettingForNamespace(namespace).getKey() + "] without [" +
Copy link
Contributor

Choose a reason for hiding this comment

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

should the test assert this message is in the error too ?

Copy link
Contributor

Choose a reason for hiding this comment

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

NMVD I see it now .. it already did. sorry for the noise.

Copy link
Contributor

@jakelandis jakelandis left a comment

Choose a reason for hiding this comment

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

LGTM (nit request to add comment to empty method.)

assertThat(
e,
hasToString(
containsString("Failed to parse value [_pass] for setting [xpack.monitoring.exporters._http.auth.password]")));
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I just realized this will place a password in the log file. Can you log an enhancement to help prevent this ?

@danhermann
Copy link
Contributor Author

@elasticmachine update branch

@danhermann
Copy link
Contributor Author

@jakelandis, I think all review concerns have been addressed between the additional commits and #48066.

@danhermann
Copy link
Contributor Author

@jakelandis, @rjernst, could you let me know if this one looks good to you?

Copy link
Contributor

@jakelandis jakelandis left a comment

Choose a reason for hiding this comment

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

LGTM

}

@Override
public void validate(String password, Map<Setting<?>, Object> settings) {
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need similar validation in AUTH_USERNAME_SETTING? Otherwise we could have username exist but no password?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rjernst, that validation was added in #47821.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, thanks. i guess this PR is out of date with master?

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM

@danhermann
Copy link
Contributor Author

@elasticmachine update branch

@danhermann danhermann merged commit 9159af5 into elastic:master Nov 13, 2019
@danhermann danhermann deleted the 47711_auth_password branch November 13, 2019 17:42
danhermann added a commit to danhermann/elasticsearch that referenced this pull request Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants