Credentials are sent over connections the okhttp protocol never authenticated, and the trust-all path uses an "SSL" context with an always-true hostname verifier

[rzo1]

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.

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.

/*
 * 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.

[abhinav-phi]

Hi! I'd like to work on this.
Plan: I'll ensure okhttp credentials are only sent over connections that passed TLS/host verification, fix the trust handling, and add tests for the credential-leak scenario.
/assign take