elasticsearchmachine
commented
1 year ago Pinging @elastic/es-security (Team:Security)
Closed slobodanadamovic closed 1 year ago
elasticsearchmachine
commented
1 year ago Pinging @elastic/es-security (Team:Security)
slobodanadamovic
commented
1 year ago There seems to be an issue with host name which is passed to WaitForHttpResource when it is created.
The problem is that LocalClusterHandle at line 151 calls node.getHttpAddress() which returns IPv6 address with port (e.g. [::1]:64922). This causes an issue downstream when creating SNIHostName.
The getHttpAddress simply takes the first address from the list see:
https://github.com/elastic/elasticsearch/blob/b45737e43d76d96d112d8e5f1baa94aeabba8f0d/test/test-clusters/src/main/java/org/elasticsearch/test/cluster/local/LocalClusterFactory.java#L147
in this concrete case the list had 2 addresses:
[::1]:64922 and 127.0.0.1:64923Note: Using the IPv6 address causes the issue, while using IPv4 doesn't.
I've raised a PR which, as a workaround, sets localhost for http.host setting, but I believe we need a general solution in WaitForHttpResource.
elasticsearchmachine
commented
1 year ago Pinging @elastic/es-delivery (Team:Delivery)
ywangd
commented
1 year ago @slobodanadamovic Thanks for looking into it. I invested sometime trying to understand the failure as well. It was puzzling to me because the http.ports file does not change, i.e. the IPv6 address worked previously before the refactoring of using the new inline test cluster. I was curious why it is not working now.
It turned out to be related to FIPS. As a side effect of moving test cluster setup to JUnit, the client (WaitForHttpResource) code now runs with the same JVM options as the ES server. In this case, it runs with FIPS enabled. With FIPS enabled, SSL factory and socket have different implementations than the JDK ones which trigger SNI configuration within HttpsClient.
I think ideally we do not want the test cluster setup client to run with FIPS since it can potentially create unnecessary maintainence burden. I am not sure how feasible it is to separate them though. This behaviour can probably be argued to be "more correct" since actual rest tests do run in FIPS mode and they are also mostly client code (different client implementations). @mark-vieira
If we accept this new behaviour, one possible workaround is to disable hostname verification for the setup client, e.g. something like the following works:
diff --git a/test/test-clusters/src/main/java/org/elasticsearch/test/cluster/local/WaitForHttpResource.java b/test/test-clusters/src/main/java/org/elasticsearch/test/cluster/local/WaitForHttpResource.java
index edab2cdf1e7..8ee9169f8c9 100644
--- a/test/test-clusters/src/main/java/org/elasticsearch/test/cluster/local/WaitForHttpResource.java
+++ b/test/test-clusters/src/main/java/org/elasticsearch/test/cluster/local/WaitForHttpResource.java
@@ -140,8 +140,9 @@ public class WaitForHttpResource {
private void configureSslContext(HttpURLConnection connection, SSLContext ssl) {
if (ssl != null) {
- if (connection instanceof HttpsURLConnection) {
- ((HttpsURLConnection) connection).setSSLSocketFactory(ssl.getSocketFactory());
+ if (connection instanceof final HttpsURLConnection httpsURLConnection) {
+ httpsURLConnection.setSSLSocketFactory(ssl.getSocketFactory());
+ httpsURLConnection.setHostnameVerifier((hostname, sslSession) -> true);
} else {
throw new IllegalStateException("SSL trust has been configured, but [" + url + "] is not a 'https' URL");
}Obviously there are other alternatives. I have no strong opinion on how it should be fixed. But it would be better if individual test class does not have to deal with it as suggested by Slobodan.
slobodanadamovic
commented
1 year ago @ywangd Thanks for checking this as well. I missed the fact that the issue happens only when FIPS is enabled.
TIL
With FIPS enabled, SSL factory and socket have different implementations than the JDK ones which trigger SNI configuration within HttpsClient.
benwtrent
commented
1 year ago @slobodanadamovic this is also failing on 8.6 if you want to backport your fix: https://gradle-enterprise.elastic.co/s/vng6xiitobh3c
mark-vieira
commented
1 year ago I actually now see that we aren't properly setting up test clusters using the new framework for FIPS at all. This is an oversight and I'll get that fixed along with a proper fix for this that doesn't require hard-coding http.host.
Build scan: https://gradle-enterprise.elastic.co/s/ugoqwcksbynes/tests/:x-pack:plugin:security:qa:service-account:javaRestTest/org.elasticsearch.xpack.security.authc.service.ServiceAccountIT
Reproduction line:
Applicable branches: main
Reproduces locally?: Didn't try
Failure history: https://gradle-enterprise.elastic.co/scans/tests?tests.container=org.elasticsearch.xpack.security.authc.service.ServiceAccountIT&tests.test=classMethod
Failure excerpt:
Edit: Added reproduction line.