rzo1 opened a new issue, #2079:
URL: https://github.com/apache/stormcrawler/issues/2079
## What happens
`HttpProtocol.configure()` reads `http.trust.everything` with a default of
`true` and then installs a no-op `X509TrustManager` and a `HostnameVerifier`
that returns `true` for every name. The socket factory comes from a static
`SSLContext.getInstance("SSL")`. Accepting certificates the crawler cannot
validate is a reasonable default for a general web crawl and is documented in
`configuration.adoc:261`, but three things do not follow from it. The
`Authorization` header built from `http.basicauth.user` and
`http.basicauth.password`, any credential in `http.custom.headers`, and
replayed cookies are all sent over that unvalidated connection; the hostname is
not checked even when the certificate chain would otherwise be usable; and
"SSL" is the wrong protocol string for a context that should negotiate TLS.
## Where
`core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java:132`,
`:216-227` and `:243-252` on main. Config keys: `http.trust.everything`,
`http.basicauth.user`, `http.basicauth.password`, `http.custom.headers`,
`http.use.cookies`.
```java
trustAllSslContext = SSLContext.getInstance("SSL");
...
if (ConfUtils.getBoolean(conf, "http.trust.everything", true)) {
builder.sslSocketFactory(trustAllSslSocketFactory, (X509TrustManager)
trustAllCerts[0]);
builder.hostnameVerifier(
new HostnameVerifier() {
@Override
public boolean verify(String hostname, SSLSession session) {
return true;
}
});
}
```
## Why it matters
An operator who configures `http.basicauth.*` for an authenticated crawl has
the credential sent on connections where the server was not authenticated at
all, so anyone able to answer for that name receives it. The always-true
verifier means a certificate issued for one name is accepted for any other,
which is a second, independent loss of server identity. The key is set in no
shipped YAML and nothing is logged when the trust-all manager is installed, so
operators inherit the setting rather than choosing it.
`SSLContext.getInstance("SSL")` currently still negotiates TLS on supported
JREs, but the string asks for the wrong thing.
## Reproduction
Save as
`core/src/test/java/org/apache/stormcrawler/protocol/OkHttpTrustEverythingTest.java`.
```java
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to you under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.stormcrawler.protocol;
import java.lang.reflect.Field;
import javax.net.ssl.HostnameVerifier;
import javax.net.ssl.SSLContext;
import okhttp3.OkHttpClient;
import org.apache.storm.Config;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
/** Checks the TLS setup of the okhttp protocol implementation. */
class OkHttpTrustEverythingTest {
private static Object getField(Class<?> clazz, Object instance, String
name) throws Exception {
Field f = clazz.getDeclaredField(name);
f.setAccessible(true);
return f.get(instance);
}
@Test
void trustAllContextUsesTls() throws Exception {
SSLContext ctx =
(SSLContext)
getField(
org.apache.stormcrawler.protocol.okhttp.HttpProtocol.class,
null,
"trustAllSslContext");
Assertions.assertEquals(
"TLS", ctx.getProtocol(), "the trust-all SSLContext should
be a TLS context");
}
@Test
void hostnameIsStillVerified() throws Exception {
Config conf = new Config();
conf.put("http.agent.name", "this_is_only_a_test");
org.apache.stormcrawler.protocol.okhttp.HttpProtocol protocol =
new org.apache.stormcrawler.protocol.okhttp.HttpProtocol();
protocol.configure(conf);
OkHttpClient client =
(OkHttpClient)
getField(
org.apache.stormcrawler.protocol.okhttp.HttpProtocol.class,
protocol,
"client");
HostnameVerifier verifier = client.hostnameVerifier();
boolean accepted;
try {
// a verifier that actually checks the name needs the session and
// fails or throws when it is absent; the always-true one does
not
accepted = verifier.verify("host.invalid", null);
} catch (RuntimeException e) {
accepted = false;
}
Assertions.assertFalse(
accepted, "the hostname verifier should not accept any name
unconditionally");
protocol.cleanup();
}
}
```
Run it:
```
mvn -pl core test -Dtest=OkHttpTrustEverythingTest
```
Both tests fail on main; they assert the intended behaviour and become
regression tests after the fix.
```
[ERROR] OkHttpTrustEverythingTest.hostnameIsStillVerified:71 the hostname
verifier should not accept any name unconditionally ==> expected: <false> but
was: <true>
[ERROR] OkHttpTrustEverythingTest.trustAllContextUsesTls:45 the trust-all
SSLContext should be a TLS context ==> expected: <TLS> but was: <SSL>
```
The credential leg has no unit test here because it needs a TLS endpoint
with an untrusted certificate. Manually: point a crawl with
`http.basicauth.user` set at an HTTPS server presenting a self-signed
certificate and observe the `Authorization` header arriving.
## Suggested fix
In `HttpProtocol.configure()`, change the static initialiser to
`SSLContext.getInstance("TLS")` and stop installing the always-true
`HostnameVerifier`; certificate trust and hostname checking are separate
decisions, so give the hostname check its own key if anyone needs to switch it
off. Do not add credential headers (`Authorization` from `http.basicauth.*`,
and custom headers marked as credentials) when the trust-all factory is in use,
or require the operator to opt in per configuration; either way log a WARN when
the trust-all manager is installed. Surface `http.trust.everything` in
`crawler-default.yaml` and the archetype `crawler-conf.yaml` files so the
setting is visible. Flipping the default to `false` belongs in a major release,
with a release note for intranet crawls that rely on self-signed certificates.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
