-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Add deprecation warnings for ssl config fallback #36847
Changes from 9 commits
a3c95b4
610f6f4
692c4a4
122053e
a67ba76
a04f694
21ace21
07ed085
7697ef3
6484b1a
3b3eada
930bfd6
21acf04
6a50e7a
6e7fca0
e3bf2b6
eaf88c5
9df9495
fe4d0bd
ff93bd3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,16 +7,20 @@ | |
|
||
import org.apache.http.conn.ssl.NoopHostnameVerifier; | ||
import org.apache.http.nio.conn.ssl.SSLIOSessionStrategy; | ||
import org.apache.logging.log4j.LogManager; | ||
import org.apache.logging.log4j.Logger; | ||
import org.apache.lucene.util.SetOnce; | ||
import org.elasticsearch.ElasticsearchException; | ||
import org.elasticsearch.common.CheckedSupplier; | ||
import org.elasticsearch.common.Strings; | ||
import org.elasticsearch.common.component.AbstractComponent; | ||
import org.elasticsearch.common.logging.DeprecationLogger; | ||
import org.elasticsearch.common.settings.Settings; | ||
import org.elasticsearch.env.Environment; | ||
import org.elasticsearch.xpack.core.XPackSettings; | ||
import org.elasticsearch.xpack.core.common.socket.SocketAccess; | ||
import org.elasticsearch.xpack.core.security.SecurityField; | ||
import org.elasticsearch.xpack.core.security.authc.ldap.LdapRealmSettings; | ||
import org.elasticsearch.xpack.core.ssl.cert.CertificateInfo; | ||
|
||
import javax.net.ssl.HostnameVerifier; | ||
|
@@ -60,7 +64,8 @@ | |
*/ | ||
public class SSLService extends AbstractComponent { | ||
|
||
private final Settings settings; | ||
private static final Logger logger = LogManager.getLogger(SSLService.class); | ||
private static final DeprecationLogger deprecationLogger = new DeprecationLogger(logger); | ||
|
||
/** | ||
* This is a mapping from "context name" (in general use, the name of a setting key) | ||
|
@@ -82,6 +87,7 @@ public class SSLService extends AbstractComponent { | |
private final SSLConfiguration globalSSLConfiguration; | ||
private final SetOnce<SSLConfiguration> transportSSLConfiguration = new SetOnce<>(); | ||
private final Environment env; | ||
private final Settings settings; | ||
|
||
/** | ||
* Create a new SSLService that parses the settings for the ssl contexts that need to be created, creates them, and then caches them | ||
|
@@ -118,6 +124,13 @@ Map<SSLConfiguration, SSLContextHolder> loadSSLConfigurations() { | |
return Collections.emptyMap(); | ||
} | ||
|
||
@Override | ||
SSLConfiguration sslConfiguration(Settings settings) { | ||
SSLConfiguration sslConfiguration = super.sslConfiguration(settings); | ||
SSLService.this.checkSSLConfigurationForFallback("monitoring exporter", settings, sslConfiguration); | ||
jaymode marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return sslConfiguration; | ||
} | ||
|
||
/** | ||
* Returns the existing {@link SSLContextHolder} for the configuration | ||
* @throws IllegalArgumentException if not found | ||
|
@@ -399,22 +412,35 @@ Map<SSLConfiguration, SSLContextHolder> loadSSLConfigurations() { | |
|
||
sslSettingsMap.forEach((key, sslSettings) -> { | ||
if (sslSettings.isEmpty()) { | ||
if (shouldCheckForFallbackDeprecation(key)) { | ||
checkSSLConfigurationForFallback(key, sslSettings, new SSLConfiguration(sslSettings, globalSSLConfiguration)); | ||
} | ||
storeSslConfiguration(key, globalSSLConfiguration); | ||
} else { | ||
final SSLConfiguration configuration = new SSLConfiguration(sslSettings, globalSSLConfiguration); | ||
if (shouldCheckForFallbackDeprecation(key)) { | ||
checkSSLConfigurationForFallback(key, sslSettings, configuration); | ||
} | ||
storeSslConfiguration(key, configuration); | ||
sslContextHolders.computeIfAbsent(configuration, this::createSslContext); | ||
} | ||
}); | ||
|
||
final Settings transportSSLSettings = settings.getByPrefix(XPackSettings.TRANSPORT_SSL_PREFIX); | ||
final SSLConfiguration transportSSLConfiguration = new SSLConfiguration(transportSSLSettings, globalSSLConfiguration); | ||
final boolean transportSSLEnabled = XPackSettings.TRANSPORT_SSL_ENABLED.get(settings); | ||
if (transportSSLEnabled) { | ||
checkSSLConfigurationForFallback(XPackSettings.TRANSPORT_SSL_PREFIX, transportSSLSettings, transportSSLConfiguration); | ||
} | ||
this.transportSSLConfiguration.set(transportSSLConfiguration); | ||
storeSslConfiguration(XPackSettings.TRANSPORT_SSL_PREFIX, transportSSLConfiguration); | ||
Map<String, Settings> profileSettings = getTransportProfileSSLSettings(settings); | ||
sslContextHolders.computeIfAbsent(transportSSLConfiguration, this::createSslContext); | ||
profileSettings.forEach((key, profileSetting) -> { | ||
final SSLConfiguration configuration = new SSLConfiguration(profileSetting, transportSSLConfiguration); | ||
if (transportSSLEnabled && key.equals("transport.profiles.default.xpack.security.ssl") == false) { | ||
checkSSLConfigurationForFallback(key, profileSetting, configuration); | ||
} | ||
storeSslConfiguration(key, configuration); | ||
sslContextHolders.computeIfAbsent(configuration, this::createSslContext); | ||
}); | ||
|
@@ -429,6 +455,58 @@ private void storeSslConfiguration(String key, SSLConfiguration configuration) { | |
sslConfigurations.put(key, configuration); | ||
} | ||
|
||
private boolean shouldCheckForFallbackDeprecation(String name) { | ||
if (name.startsWith("xpack.security.authc.realms.")) { | ||
// try to see if this is actually using TLS | ||
Settings realm = settings.getByPrefix(name.substring(0, name.indexOf(".ssl"))); | ||
String type = realm.get("type"); | ||
// only check the types we know use ssl. custom realms may but we don't want to cause confusion | ||
if (LdapRealmSettings.LDAP_TYPE.equals(type) || LdapRealmSettings.AD_TYPE.equals(type)) { | ||
List<String> urls = realm.getAsList("url"); | ||
if (urls.isEmpty() == false && urls.stream().anyMatch(s -> s.startsWith("ldaps://"))) { | ||
return true; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SAML uses ssl if the metadata path points to a |
||
} | ||
} else if (name.startsWith("xpack.monitoring.exporters.")) { | ||
Settings exporterSettings = settings.getByPrefix(name.substring(0, name.indexOf(".ssl"))); | ||
List<String> hosts = exporterSettings.getAsList("host"); | ||
if (hosts.stream().anyMatch(s -> s.startsWith("https"))) { | ||
return true; | ||
} | ||
} else if (name.equals(XPackSettings.HTTP_SSL_PREFIX) && XPackSettings.HTTP_SSL_ENABLED.get(settings)) { | ||
return true; | ||
} else if (name.equals("xpack.http.ssl") && XPackSettings.WATCHER_ENABLED.get(settings)) { | ||
return true; | ||
} | ||
return false; | ||
} | ||
|
||
private void checkSSLConfigurationForFallback(String name, Settings settings, SSLConfiguration config) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it might be nice to print the key missing from the actual config that falls back to the default value here instead of a descriptive name, but I'm not sure if it can be reliably computed and if it would actually make the deprecation log better, so I just bring it up for consideration There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that it would be nice, but I think the effort to compute this would probably outweigh the benefits |
||
final SSLConfiguration noFallBackConfig = new SSLConfiguration(settings); | ||
if (config.equals(noFallBackConfig) == false) { | ||
List<String> fallbackReliers = new ArrayList<>(); | ||
if (config.keyConfig().equals(noFallBackConfig.keyConfig()) == false) { | ||
fallbackReliers.add("key configuration"); | ||
} | ||
if (config.trustConfig().equals(noFallBackConfig.trustConfig()) == false) { | ||
fallbackReliers.add("trust configuration"); | ||
} | ||
if (config.cipherSuites().equals(noFallBackConfig.cipherSuites()) == false) { | ||
fallbackReliers.add("enabled cipher suites"); | ||
} | ||
if (config.sslClientAuth() != noFallBackConfig.sslClientAuth()) { | ||
fallbackReliers.add("client authentication"); | ||
} | ||
if (config.supportedProtocols().equals(noFallBackConfig.supportedProtocols()) == false) { | ||
fallbackReliers.add("supported protocols"); | ||
} | ||
if (config.verificationMode() != noFallBackConfig.verificationMode()) { | ||
fallbackReliers.add("certificate verification mode"); | ||
} | ||
deprecationLogger.deprecated("SSL configuration [{}] relies upon fallback to another configuration for {}, which is " + | ||
"deprecated.", name, fallbackReliers); | ||
} | ||
} | ||
|
||
/** | ||
* Returns information about each certificate that is referenced by any SSL configuration. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the legacy (non-secure) password always be deprecated? It looks like this just got missed/lost at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is deprecated always. What you are seeing is the fallback setting being configured, which has its own properties; the properties here are the ones for the secure setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course. It really is a bit of a mess isn't it.