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 Watcher Account "unsecure" settings #36736

Merged
23 changes: 23 additions & 0 deletions docs/reference/migration/migrate_7_0/settings.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -116,3 +116,26 @@ xpack.security.authc.realms:
Any realm specific secure settings that have been stored in the elasticsearch
keystore (such as ldap bind passwords, or passwords for ssl keys) must be updated
in a similar way.

[float]
[[watcher-notifications-account-settings]]
==== Watcher notifications account settings

The following settings have been removed in favor of the secure variants.
The <<secure-settings, secure settings>> have to be defined inside each cluster
node's keystore, i.e., they are not to be specified via the cluster settings API.

- `xpack.notification.email.account.<id>.smtp.password`, instead use
`xpack.notification.email.account.<id>.smtp.secure_password`
- `xpack.notification.hipchat.account.<id>.auth_token`, instead use
`xpack.notification.hipchat.account.<id>.secure_auth_token`
- `xpack.notification.jira.account.<id>.url`, instead use
`xpack.notification.jira.account.<id>.secure_url`
- `xpack.notification.jira.account.<id>.user`, instead use
`xpack.notification.jira.account.<id>.secure_user`
- `xpack.notification.jira.account.<id>.password`, instead use
`xpack.notification.jira.account.<id>.secure_password`
- `xpack.notification.pagerduty.account.<id>.service_api_key`, instead use
`xpack.notification.pagerduty.account.<id>.secure_service_api_key`
- `xpack.notification.slack.account.<id>.url`, instead use
`xpack.notification.slack.account.<id>.secure_url`
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@
public class Account {

static final String SMTP_PROTOCOL = "smtp";
private static final String SMTP_PASSWORD = "password";
private static final Setting<SecureString> SECURE_PASSWORD_SETTING = SecureSetting.secureString("secure_" + SMTP_PASSWORD, null);
private static final Setting<SecureString> SECURE_PASSWORD_SETTING = SecureSetting.secureString("secure_password", null);

static {
SecurityManager sm = System.getSecurityManager();
Expand Down Expand Up @@ -213,7 +212,7 @@ static class Smtp {

port = settings.getAsInt("port", settings.getAsInt("localport", settings.getAsInt("local_port", 25)));
user = settings.get("user", settings.get("from", null));
password = getSecureSetting(SMTP_PASSWORD, settings, SECURE_PASSWORD_SETTING);
password = getSecureSetting(settings, SECURE_PASSWORD_SETTING);
//password = passStr != null ? passStr.toCharArray() : null;
properties = loadSmtpProperties(settings);
}
Expand All @@ -225,17 +224,12 @@ static class Smtp {
* Note: if your setting was not previously secure, than the string reference that is in the setting object is still
* insecure. This is only constructing a new SecureString with the char[] of the insecure setting.
*/
private static SecureString getSecureSetting(String settingName, Settings settings, Setting<SecureString> secureSetting) {
String value = settings.get(settingName);
if (value == null) {
SecureString secureString = secureSetting.get(settings);
if (secureString != null && secureString.length() > 0) {
return secureString;
} else {
return null;
}
private static SecureString getSecureSetting(Settings settings, Setting<SecureString> secureSetting) {
SecureString secureString = secureSetting.get(settings);
if (secureString != null && secureString.length() > 0) {
return secureString;
} else {
return new SecureString(value.toCharArray());
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems a bit different from the other getSecureSettings that are in the other *Account classes, and somewhat trappy. Why would we be returning null instead of throwing up like we do in the others? I ask because I hope we can consolidate the functionality into only Account, if thats at all possible, instead of having a method that does this validation in every *Account class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hub-cap There is no Account base class. Also I tried to touch as less as possible to make reviewing easier. IMO we should be using the SecureSetting from the corresponding Service (EmailService in this case). But I prefer to leave this for another time. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds great to me

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,6 @@ public class EmailService extends NotificationService<Account> {
Setting.affixKeySetting("xpack.notification.email.account.", "smtp.user",
(key) -> Setting.simpleString(key, Property.Dynamic, Property.NodeScope));

private static final Setting.AffixSetting<String> SETTING_SMTP_PASSWORD =
Setting.affixKeySetting("xpack.notification.email.account.", "smtp.password",
(key) -> Setting.simpleString(key, Property.Dynamic, Property.NodeScope, Property.Filtered));

private static final Setting.AffixSetting<SecureString> SETTING_SECURE_PASSWORD =
Setting.affixKeySetting("xpack.notification.email.account.", "smtp.secure_password",
(key) -> SecureSetting.secureString(key, null));
Expand Down Expand Up @@ -122,7 +118,6 @@ public EmailService(Settings settings, @Nullable CryptoService cryptoService, Cl
clusterSettings.addAffixUpdateConsumer(SETTING_SMTP_HOST, (s, o) -> {}, (s, o) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_SMTP_PORT, (s, o) -> {}, (s, o) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_SMTP_USER, (s, o) -> {}, (s, o) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_SMTP_PASSWORD, (s, o) -> {}, (s, o) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_SMTP_TIMEOUT, (s, o) -> {}, (s, o) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_SMTP_CONNECTION_TIMEOUT, (s, o) -> {}, (s, o) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_SMTP_WRITE_TIMEOUT, (s, o) -> {}, (s, o) -> {});
Expand Down Expand Up @@ -182,7 +177,7 @@ public Email email() {

private static List<Setting<?>> getDynamicSettings() {
return Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_PROFILE, SETTING_EMAIL_DEFAULTS, SETTING_SMTP_AUTH, SETTING_SMTP_HOST,
SETTING_SMTP_PASSWORD, SETTING_SMTP_PORT, SETTING_SMTP_STARTTLS_ENABLE, SETTING_SMTP_USER, SETTING_SMTP_STARTTLS_REQUIRED,
SETTING_SMTP_PORT, SETTING_SMTP_STARTTLS_ENABLE, SETTING_SMTP_USER, SETTING_SMTP_STARTTLS_REQUIRED,
SETTING_SMTP_TIMEOUT, SETTING_SMTP_CONNECTION_TIMEOUT, SETTING_SMTP_WRITE_TIMEOUT, SETTING_SMTP_LOCAL_ADDRESS,
SETTING_SMTP_LOCAL_PORT, SETTING_SMTP_SEND_PARTIAL, SETTING_SMTP_WAIT_ON_QUIT, SETTING_SMTP_SSL_TRUST_ADDRESS);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@

public abstract class HipChatAccount {

public static final String AUTH_TOKEN_SETTING = "auth_token";
public static final String ROOM_SETTING = HipChatMessage.Field.ROOM.getPreferredName();
public static final String DEFAULT_ROOM_SETTING = "message_defaults." + HipChatMessage.Field.ROOM.getPreferredName();
public static final String DEFAULT_USER_SETTING = "message_defaults." + HipChatMessage.Field.USER.getPreferredName();
Expand All @@ -32,7 +31,7 @@ public abstract class HipChatAccount {
public static final String DEFAULT_COLOR_SETTING = "message_defaults." + HipChatMessage.Field.COLOR.getPreferredName();
public static final String DEFAULT_NOTIFY_SETTING = "message_defaults." + HipChatMessage.Field.NOTIFY.getPreferredName();

private static final Setting<SecureString> SECURE_AUTH_TOKEN_SETTING = SecureSetting.secureString("secure_" + AUTH_TOKEN_SETTING, null);
private static final Setting<SecureString> SECURE_AUTH_TOKEN_SETTING = SecureSetting.secureString("secure_auth_token", null);

protected final Logger logger;
protected final String name;
Expand All @@ -52,16 +51,11 @@ protected HipChatAccount(String name, Profile profile, Settings settings, HipCha
}

private static String getAuthToken(String name, Settings settings) {
String authToken = settings.get(AUTH_TOKEN_SETTING);
if (authToken == null || authToken.length() == 0) {
SecureString secureString = SECURE_AUTH_TOKEN_SETTING.get(settings);
if (secureString == null || secureString.length() < 1) {
throw new SettingsException("hipchat account [" + name + "] missing required [" + AUTH_TOKEN_SETTING + "] setting");
}
authToken = secureString.toString();
SecureString secureString = SECURE_AUTH_TOKEN_SETTING.get(settings);
if (secureString == null || secureString.length() < 1) {
throw new SettingsException("hipchat account [" + name + "] missing required [" + SECURE_AUTH_TOKEN_SETTING + "] setting");
}

return authToken;
return secureString.toString();
}

public abstract String type();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,6 @@ public class HipChatService extends NotificationService<HipChatAccount> {
static final Setting<Integer> SETTING_DEFAULT_PORT =
Setting.intSetting("xpack.notification.hipchat.port", 443, Setting.Property.Dynamic, Setting.Property.NodeScope);

private static final Setting.AffixSetting<String> SETTING_AUTH_TOKEN =
Setting.affixKeySetting("xpack.notification.hipchat.account.", "auth_token",
(key) -> Setting.simpleString(key, Setting.Property.Dynamic, Setting.Property.NodeScope, Setting.Property.Filtered,
Setting.Property.Deprecated));

private static final Setting.AffixSetting<SecureString> SETTING_AUTH_TOKEN_SECURE =
Setting.affixKeySetting("xpack.notification.hipchat.account.", "secure_auth_token",
(key) -> SecureSetting.secureString(key, null));
Expand Down Expand Up @@ -75,7 +70,6 @@ public HipChatService(Settings settings, HttpClient httpClient, ClusterSettings
clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {});
clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_HOST, (s) -> {});
clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_PORT, (s) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_AUTH_TOKEN, (s, o) -> {}, (s, o) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_PROFILE, (s, o) -> {}, (s, o) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_ROOM, (s, o) -> {}, (s, o) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_HOST, (s, o) -> {}, (s, o) -> {});
Expand All @@ -101,7 +95,7 @@ protected HipChatAccount createAccount(String name, Settings accountSettings) {
}

private static List<Setting<?>> getDynamicSettings() {
return Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_AUTH_TOKEN, SETTING_PROFILE, SETTING_ROOM, SETTING_MESSAGE_DEFAULTS,
return Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_PROFILE, SETTING_ROOM, SETTING_MESSAGE_DEFAULTS,
SETTING_DEFAULT_HOST, SETTING_DEFAULT_PORT, SETTING_HOST, SETTING_PORT);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
package org.elasticsearch.xpack.watcher.notification.jira;

import org.elasticsearch.common.Booleans;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.settings.SecureSetting;
import org.elasticsearch.common.settings.SecureString;
Expand Down Expand Up @@ -42,15 +41,12 @@ public class JiraAccount {
**/
public static final String DEFAULT_PATH = "/rest/api/2/issue";

static final String USER_SETTING = "user";
static final String PASSWORD_SETTING = "password";
static final String URL_SETTING = "url";
static final String ISSUE_DEFAULTS_SETTING = "issue_defaults";
static final String ALLOW_HTTP_SETTING = "allow_http";

private static final Setting<SecureString> SECURE_USER_SETTING = SecureSetting.secureString("secure_" + USER_SETTING, null);
private static final Setting<SecureString> SECURE_PASSWORD_SETTING = SecureSetting.secureString("secure_" + PASSWORD_SETTING, null);
private static final Setting<SecureString> SECURE_URL_SETTING = SecureSetting.secureString("secure_" + URL_SETTING, null);
private static final Setting<SecureString> SECURE_USER_SETTING = SecureSetting.secureString("secure_user", null);
private static final Setting<SecureString> SECURE_PASSWORD_SETTING = SecureSetting.secureString("secure_password", null);
private static final Setting<SecureString> SECURE_URL_SETTING = SecureSetting.secureString("secure_url", null);

private final HttpClient httpClient;
private final String name;
Expand All @@ -62,7 +58,7 @@ public class JiraAccount {
public JiraAccount(String name, Settings settings, HttpClient httpClient) {
this.httpClient = httpClient;
this.name = name;
String url = getSetting(name, URL_SETTING, settings, SECURE_URL_SETTING);
String url = getSetting(name, settings, SECURE_URL_SETTING);
try {
URI uri = new URI(url);
Scheme protocol = Scheme.parse(uri.getScheme());
Expand All @@ -71,16 +67,11 @@ public JiraAccount(String name, Settings settings, HttpClient httpClient) {
}
this.url = uri;
} catch (URISyntaxException | IllegalArgumentException e) {
throw new SettingsException("invalid jira [" + name + "] account settings. invalid [" + URL_SETTING + "] setting", e);
}
this.user = getSetting(name, USER_SETTING, settings, SECURE_USER_SETTING);
if (Strings.isEmpty(this.user)) {
throw requiredSettingException(name, USER_SETTING);
}
this.password = getSetting(name, PASSWORD_SETTING, settings, SECURE_PASSWORD_SETTING);
if (Strings.isEmpty(this.password)) {
throw requiredSettingException(name, PASSWORD_SETTING);
throw new SettingsException(
"invalid jira [" + name + "] account settings. invalid [" + SECURE_URL_SETTING.getKey() + "] setting", e);
}
this.user = getSetting(name, settings, SECURE_USER_SETTING);
this.password = getSetting(name, settings, SECURE_PASSWORD_SETTING);
try (XContentBuilder builder = XContentBuilder.builder(XContentType.JSON.xContent())) {
builder.startObject();
settings.getAsSettings(ISSUE_DEFAULTS_SETTING).toXContent(builder, ToXContent.EMPTY_PARAMS);
Expand All @@ -95,17 +86,12 @@ public JiraAccount(String name, Settings settings, HttpClient httpClient) {
}
}

private static String getSetting(String accountName, String settingName, Settings settings, Setting<SecureString> secureSetting) {
String value = settings.get(settingName);
if (value == null) {
SecureString secureString = secureSetting.get(settings);
if (secureString == null || secureString.length() < 1) {
throw requiredSettingException(accountName, settingName);
}
value = secureString.toString();
private static String getSetting(String accountName, Settings settings, Setting<SecureString> secureSetting) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like something we could put in Account

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, but I think that's a job for another PR (see prev comment)

SecureString secureString = secureSetting.get(settings);
if (secureString == null || secureString.length() < 1) {
throw requiredSettingException(accountName, secureSetting.getKey());
}

return value;
return secureString.toString();
}

public String getName() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,18 +32,6 @@ public class JiraService extends NotificationService<JiraAccount> {
Setting.affixKeySetting("xpack.notification.jira.account.", "allow_http",
(key) -> Setting.boolSetting(key, false, Property.Dynamic, Property.NodeScope));

private static final Setting.AffixSetting<String> SETTING_URL =
Setting.affixKeySetting("xpack.notification.jira.account.", "url",
(key) -> Setting.simpleString(key, Property.Dynamic, Property.NodeScope, Property.Filtered));

private static final Setting.AffixSetting<String> SETTING_USER =
Setting.affixKeySetting("xpack.notification.jira.account.", "user",
(key) -> Setting.simpleString(key, Property.Dynamic, Property.NodeScope, Property.Filtered));

private static final Setting.AffixSetting<String> SETTING_PASSWORD =
Setting.affixKeySetting("xpack.notification.jira.account.", "password",
(key) -> Setting.simpleString(key, Property.Dynamic, Property.NodeScope, Property.Filtered, Property.Deprecated));

private static final Setting.AffixSetting<SecureString> SETTING_SECURE_USER =
Setting.affixKeySetting("xpack.notification.jira.account.", "secure_user",
(key) -> SecureSetting.secureString(key, null));
Expand All @@ -68,9 +56,6 @@ public JiraService(Settings settings, HttpClient httpClient, ClusterSettings clu
// ensure logging of setting changes
clusterSettings.addSettingsUpdateConsumer(SETTING_DEFAULT_ACCOUNT, (s) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_ALLOW_HTTP, (s, o) -> {}, (s, o) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_URL, (s, o) -> {}, (s, o) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_USER, (s, o) -> {}, (s, o) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_PASSWORD, (s, o) -> {}, (s, o) -> {});
clusterSettings.addAffixUpdateConsumer(SETTING_DEFAULTS, (s, o) -> {}, (s, o) -> {});
// do an initial load
reload(settings);
Expand All @@ -82,7 +67,7 @@ protected JiraAccount createAccount(String name, Settings settings) {
}

private static List<Setting<?>> getDynamicSettings() {
return Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_ALLOW_HTTP, SETTING_URL, SETTING_USER, SETTING_PASSWORD, SETTING_DEFAULTS);
return Arrays.asList(SETTING_DEFAULT_ACCOUNT, SETTING_ALLOW_HTTP, SETTING_DEFAULTS);
}

private static List<Setting<?>> getSecureSettings() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,17 @@

public class PagerDutyAccount {

private static final String SERVICE_KEY_SETTING = "service_api_key";
private static final String TRIGGER_DEFAULTS_SETTING = "event_defaults";
private static final Setting<SecureString> SECURE_SERVICE_API_KEY_SETTING =
SecureSetting.secureString("secure_" + SERVICE_KEY_SETTING, null);
private static final Setting<SecureString> SECURE_SERVICE_API_KEY_SETTING = SecureSetting.secureString("secure_service_api_key", null);

private final String name;
private final String serviceKey;
private final HttpClient httpClient;
private final IncidentEventDefaults eventDefaults;

PagerDutyAccount(String name, Settings accountSettings, Settings serviceSettings, HttpClient httpClient) {
PagerDutyAccount(String name, Settings accountSettings, HttpClient httpClient) {
this.name = name;
this.serviceKey = getServiceKey(name, accountSettings, serviceSettings);
this.serviceKey = getServiceKey(name, accountSettings);
this.httpClient = httpClient;

this.eventDefaults = new IncidentEventDefaults(accountSettings.getAsSettings(TRIGGER_DEFAULTS_SETTING));
Expand All @@ -51,17 +49,12 @@ public SentEvent send(IncidentEvent event, Payload payload, String watchId) thro
return SentEvent.responded(event, request, response);
}

private static String getServiceKey(String name, Settings accountSettings, Settings serviceSettings) {
String serviceKey = accountSettings.get(SERVICE_KEY_SETTING, serviceSettings.get(SERVICE_KEY_SETTING, null));
if (serviceKey == null) {
SecureString secureString = SECURE_SERVICE_API_KEY_SETTING.get(accountSettings);
if (secureString == null || secureString.length() < 1) {
throw new SettingsException("invalid pagerduty account [" + name + "]. missing required [" + SERVICE_KEY_SETTING +
"] setting");
}
serviceKey = secureString.toString();
private static String getServiceKey(String name, Settings accountSettings) {
SecureString secureString = SECURE_SERVICE_API_KEY_SETTING.get(accountSettings);
if (secureString == null || secureString.length() < 1) {
throw new SettingsException(
"invalid pagerduty account [" + name + "]. missing required [" + SECURE_SERVICE_API_KEY_SETTING.getKey() + "] setting");
}

return serviceKey;
return secureString.toString();
}
}
Loading