Validate monitoring password at parse time #47740
Conversation
|
Pinging @elastic/es-core-features (:Core/Features/Monitoring) |
danhermann
changed the title
| new Setting.Validator<String>() { | ||
| @Override | ||
| public void validate(String password) { | ||
|
|
There was a problem hiding this comment.
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 [" + |
There was a problem hiding this comment.
should the test assert this message is in the error too ?
There was a problem hiding this comment.
NMVD I see it now .. it already did. sorry for the noise.
| assertThat( | ||
| e, | ||
| hasToString( | ||
| containsString("Failed to parse value [_pass] for setting [xpack.monitoring.exporters._http.auth.password]"))); |
There was a problem hiding this comment.
Actually, I just realized this will place a password in the log file. Can you log an enhancement to help prevent this ?
|
@elasticmachine update branch |
|
@jakelandis, I think all review concerns have been addressed between the additional commits and #48066. |
|
@jakelandis, @rjernst, could you let me know if this one looks good to you? |
| } | ||
|
|
||
| @Override | ||
| public void validate(String password, Map<Setting<?>, Object> settings) { |
There was a problem hiding this comment.
Don't we need similar validation in AUTH_USERNAME_SETTING? Otherwise we could have username exist but no password?
There was a problem hiding this comment.
Ah ok, thanks. i guess this PR is out of date with master?
|
@elasticmachine update branch |
Provides parse-time validation for
AUTH_PASSWORD_SETTINGas described in #47711.