Skip to content

Commit

Permalink
Fix smtp.ssl.trust setting for watcher email (#56090)
Browse files Browse the repository at this point in the history
The ssl.trust setting for Watcher provides a list of hostnames that
should be automatically trusted for SSL hostname verification. It was
accidentally broken when we added the full ssl.* settings for email
notifications (see #45272)

This commit corrects this, so the setting is once again respected,
as long as none of the other ssl settings are configured for email
notifications.

Resolves: #52153
  • Loading branch information
tvernum authored May 28, 2020
1 parent 77ca5dc commit de687ce
Show file tree
Hide file tree
Showing 5 changed files with 73 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public final class SSLConfiguration {
private final List<String> supportedProtocols;
private final SSLClientAuth sslClientAuth;
private final VerificationMode verificationMode;
private final boolean explicitlyConfigured;

/**
* Creates a new SSLConfiguration from the given settings. There is no fallback configuration when invoking this constructor so
Expand All @@ -52,6 +53,7 @@ public final class SSLConfiguration {
this.supportedProtocols = getListOrDefault(SETTINGS_PARSER.supportedProtocols, settings, XPackSettings.DEFAULT_SUPPORTED_PROTOCOLS);
this.sslClientAuth = SETTINGS_PARSER.clientAuth.get(settings).orElse(XPackSettings.CLIENT_AUTH_DEFAULT);
this.verificationMode = SETTINGS_PARSER.verificationMode.get(settings).orElse(XPackSettings.VERIFICATION_MODE_DEFAULT);
this.explicitlyConfigured = settings.isEmpty() == false;
}

/**
Expand Down Expand Up @@ -108,6 +110,10 @@ List<Path> filesToMonitor(@Nullable Environment environment) {
return paths;
}

public boolean isExplicitlyConfigured() {
return explicitlyConfigured;
}

@Override
public String toString() {
return "SSLConfiguration{" +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@
import java.security.PrivilegedExceptionAction;
import java.util.Properties;
import java.util.Set;
import java.util.stream.Collectors;

import static org.elasticsearch.xpack.core.watcher.WatcherField.EMAIL_NOTIFICATION_SSL_PREFIX;

public class Account {

Expand Down Expand Up @@ -187,7 +190,7 @@ static class Config {
final Smtp smtp;
final EmailDefaults defaults;

Config(String name, Settings settings, @Nullable SSLSocketFactory sslSocketFactory) {
Config(String name, Settings settings, @Nullable SSLSocketFactory sslSocketFactory, Logger logger) {
this.name = name;
profile = Profile.resolve(settings.get("profile"), Profile.STANDARD);
defaults = new EmailDefaults(name, settings.getAsSettings("email_defaults"));
Expand All @@ -197,6 +200,15 @@ static class Config {
throw new SettingsException(msg);
}
if (sslSocketFactory != null) {
String sslKeys = smtp.properties.keySet().stream()
.map(String::valueOf)
.filter(key -> key.startsWith("mail.smtp.ssl."))
.collect(Collectors.joining(","));
if (sslKeys.isEmpty() == false) {
logger.warn("The SMTP SSL settings [{}] that are configured for Account [{}]" +
" will be ignored due to the notification SSL settings in [{}]",
sslKeys, name, EMAIL_NOTIFICATION_SSL_PREFIX);
}
smtp.setSocketFactory(sslSocketFactory);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,14 +141,14 @@ public EmailService(Settings settings, @Nullable CryptoService cryptoService, SS

@Override
protected Account createAccount(String name, Settings accountSettings) {
Account.Config config = new Account.Config(name, accountSettings, getSmtpSslSocketFactory());
Account.Config config = new Account.Config(name, accountSettings, getSmtpSslSocketFactory(), logger);
return new Account(config, cryptoService, logger);
}

@Nullable
private SSLSocketFactory getSmtpSslSocketFactory() {
final SSLConfiguration sslConfiguration = sslService.getSSLConfiguration(EMAIL_NOTIFICATION_SSL_PREFIX);
if (sslConfiguration == null) {
if (sslConfiguration == null || sslConfiguration.isExplicitlyConfigured() == false) {
return null;
}
return sslService.sslSocketFactory(sslConfiguration);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,51 @@ public void testCanSendMessageToSmtpServerByDisablingVerification() throws Excep
}
}

public void testCanSendMessageToSmtpServerUsingSmtpSslTrust() throws Exception {
assumeFalse("Can't run in a FIPS JVM with verification mode None", inFipsJvm());
List<MimeMessage> messages = new ArrayList<>();
server.addListener(messages::add);
try {
final Settings.Builder settings = Settings.builder()
.put("xpack.notification.email.account.test.smtp.ssl.trust", "localhost");
final MockSecureSettings secureSettings = new MockSecureSettings();
ExecutableEmailAction emailAction = buildEmailAction(settings, secureSettings);

WatchExecutionContext ctx = WatcherTestUtils.createWatchExecutionContext();
emailAction.execute("my_action_id", ctx, Payload.EMPTY);

assertThat(messages, hasSize(1));
} finally {
server.clearListeners();
}
}

/**
* This orderining could be considered to be backwards (the global "notification" settings take precedence
* over the account level "smtp.ssl.trust" setting) but smtp.ssl.trust was ignored for a period of time (see #52153)
* so this is the least breaking way to resolve that.
*/
public void testNotificationSslSettingsOverrideSmtpSslTrust() throws Exception {
List<MimeMessage> messages = new ArrayList<>();
server.addListener(messages::add);
try {
final Settings.Builder settings = Settings.builder()
.put("xpack.notification.email.account.test.smtp.ssl.trust", "localhost")
.put("xpack.notification.email.ssl.verification_mode", "full");
final MockSecureSettings secureSettings = new MockSecureSettings();
ExecutableEmailAction emailAction = buildEmailAction(settings, secureSettings);

WatchExecutionContext ctx = WatcherTestUtils.createWatchExecutionContext();
final MessagingException exception = expectThrows(MessagingException.class,
() -> emailAction.execute("my_action_id", ctx, Payload.EMPTY));

final List<Throwable> allCauses = getAllCauses(exception);
assertThat(allCauses, Matchers.hasItem(Matchers.instanceOf(SSLException.class)));
} finally {
server.clearListeners();
}
}

private ExecutableEmailAction buildEmailAction(Settings.Builder baseSettings, MockSecureSettings secureSettings) {
secureSettings.setString("xpack.notification.email.account.test.smtp.secure_password", EmailServer.PASSWORD);
Settings settings = baseSettings
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ public void testConfig() throws Exception {

Settings settings = builder.build();

Account.Config config = new Account.Config(accountName, settings, null);
Account.Config config = new Account.Config(accountName, settings, null, logger);

assertThat(config.profile, is(profile));
assertThat(config.defaults, equalTo(emailDefaults));
Expand All @@ -165,7 +165,7 @@ public void testSend() throws Exception {
.put("smtp.port", server.port())
.put("smtp.user", EmailServer.USERNAME)
.setSecureSettings(secureSettings)
.build(), null), null, logger);
.build(), null, logger), null, logger);

Email email = Email.builder()
.id("_id")
Expand Down Expand Up @@ -202,7 +202,7 @@ public void testSendCCAndBCC() throws Exception {
.put("smtp.port", server.port())
.put("smtp.user", EmailServer.USERNAME)
.setSecureSettings(secureSettings)
.build(), null), null, logger);
.build(), null, logger), null, logger);

Email email = Email.builder()
.id("_id")
Expand Down Expand Up @@ -240,7 +240,7 @@ public void testSendAuthentication() throws Exception {
Account account = new Account(new Account.Config("default", Settings.builder()
.put("smtp.host", "localhost")
.put("smtp.port", server.port())
.build(), null), null, logger);
.build(), null, logger), null, logger);

Email email = Email.builder()
.id("_id")
Expand All @@ -264,7 +264,7 @@ public void testDefaultAccountTimeout() {
Account account = new Account(new Account.Config("default", Settings.builder()
.put("smtp.host", "localhost")
.put("smtp.port", server.port())
.build(), null), null, logger);
.build(), null, logger), null, logger);

Properties mailProperties = account.getConfig().smtp.properties;
assertThat(mailProperties.get("mail.smtp.connectiontimeout"), is(String.valueOf(TimeValue.timeValueMinutes(2).millis())));
Expand All @@ -279,7 +279,7 @@ public void testAccountTimeoutsCanBeConfigureAsTimeValue() {
.put("smtp.connection_timeout", TimeValue.timeValueMinutes(4))
.put("smtp.write_timeout", TimeValue.timeValueMinutes(6))
.put("smtp.timeout", TimeValue.timeValueMinutes(8))
.build(), null), null, logger);
.build(), null, logger), null, logger);

Properties mailProperties = account.getConfig().smtp.properties;

Expand All @@ -294,7 +294,7 @@ public void testAccountTimeoutsConfiguredAsNumberAreRejected() {
.put("smtp.host", "localhost")
.put("smtp.port", server.port())
.put("smtp.connection_timeout", 4000)
.build(), null), null, logger);
.build(), null, logger), null, logger);
});
}

Expand Down

0 comments on commit de687ce

Please sign in to comment.