Skip to content

OKHttp: authenticated TLS by default, credentials withheld from unauthenticated servers #2126

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.

Sign up for GitHub

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

Open
abhinav-phi wants to merge 2 commits into
base: main
Choose a base branch
from
Open

OKHttp: authenticated TLS by default, credentials withheld from unauthenticated servers #2126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
10c7ba3
OKHttp: authenticated TLS by default, credentials withheld from unaut…
abhinav-phi Sep 5, 2026
89b6519
OkHttpTrustEverythingTest: serve TLS with WireMock
abhinav-phi Sep 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions archetype/src/main/resources/archetype-resources/crawler-conf.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,12 @@ config:
http.protocol.implementation: "org.apache.stormcrawler.protocol.okhttp.HttpProtocol"
https.protocol.implementation: "org.apache.stormcrawler.protocol.okhttp.HttpProtocol"

# Accept any TLS certificate, e.g. self-signed ones? The servers are not
# authenticated then; credentials (basic auth, credential headers, cookies)
# are withheld over such connections unless
# http.credentials.allow.insecure is set to true.
http.trust.everything: false

# The maximum number of bytes for returned HTTP response bodies.
# The fetched page will be trimmed to 65KB in this case
# Set -1 to disable the limit.
Expand Down
130 changes: 111 additions & 19 deletions core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,11 @@
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import javax.net.ssl.HostnameVerifier;
import javax.net.ssl.SSLContext;
import javax.net.ssl.SSLSession;
import javax.net.ssl.SSLSocketFactory;
import javax.net.ssl.TrustManager;
import javax.net.ssl.X509TrustManager;
Expand Down Expand Up @@ -108,6 +107,23 @@ public class HttpProtocol extends AbstractHttpProtocol {
// makes sure that a missing cookie origin is reported once and not for every url
private final AtomicBoolean missingCookieOriginLogged = new AtomicBoolean();

// makes sure that withheld cookies are reported once and not for every url
private final AtomicBoolean withheldCookiesLogged = new AtomicBoolean();

// whether credentials (basic auth, credential headers, cookies) may be sent:
// false when the servers are not authenticated and
// http.credentials.allow.insecure is not enabled
private boolean sendCredentials = true;

/** Header names carrying credentials, see {@link #isCredentialHeader(String)}. */
private static final Set<String> CREDENTIAL_HEADERS =

@rzo1 rzo1 Sep 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a denylist, and it is missing entries: x-auth-token, api-key, authentication, x-amz-security-token, and anything site-specific the operator configured in http.custom.headers.

A denylist that misses one entry is worse than none, because it reads as complete.

Two options:

  • drop all custom headers when !sendCredentials except an explicit http.headers.insecure.allow list, or
  • keep the denylist but make it configurable through a http.credentials.headers key defaulting to this set.

The second is the smaller change.

Set.of(
HttpHeaders.AUTHORIZATION.toLowerCase(Locale.ROOT),
HttpHeaders.PROXY_AUTHORIZATION.toLowerCase(Locale.ROOT),
// the cookie header is not a constant in HttpHeaders
"cookie",
"x-api-key");

private OkHttpClient.Builder builder;

private static final TrustManager[] trustAllCerts =
Expand All @@ -130,11 +146,12 @@ public java.security.cert.X509Certificate[] getAcceptedIssuers() {
}
};

private static final SSLContext trustAllSslContext;
// package-private so that the OkHttpTrustEverythingTest can check the protocol
static final SSLContext trustAllSslContext;

static {
try {
trustAllSslContext = SSLContext.getInstance("SSL");
trustAllSslContext = SSLContext.getInstance("TLS");
trustAllSslContext.init(null, trustAllCerts, new java.security.SecureRandom());
} catch (Exception e) {
throw new RuntimeException(e);
Expand All @@ -158,6 +175,37 @@ public void configure(Config conf) {
this.partialContentAsTrimmed =
ConfUtils.getBoolean(conf, "http.content.partial.as.trimmed", false);

/*
* certificate trust and hostname verification are separate decisions:
* accepting any certificate does not imply accepting any name
*/
final boolean trustEverything = ConfUtils.getBoolean(conf, "http.trust.everything", false);
final boolean verifyHostnames = ConfUtils.getBoolean(conf, "http.verify.hostnames", true);
// credentials are withheld over connections whose server certificate
// was not validated, unless explicitly opted in
final boolean insecureCredentialsAllowed =
ConfUtils.getBoolean(conf, "http.credentials.allow.insecure", false);
this.sendCredentials = !trustEverything || insecureCredentialsAllowed;

@rzo1 rzo1 Sep 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sendCredentials is decided once at configure time from http.trust.everything, so cleartext HTTP is not covered.

A plain http:// request authenticates the server not at all, which is weaker than a self-signed certificate, and it still gets the Authorization header, the cookies and the API-key headers. The PR title says "credentials withheld from unauthenticated servers"; cleartext is the clearest case of one.

Suggest deciding per request on request.url().isHttps() && !trustEverything, with http.credentials.allow.insecure covering both cases. If that is too much for this PR, open a follow-up and say so in the description, because the title currently promises more than the code does.


if (trustEverything) {
LOG.warn(
"http.trust.everything is enabled: TLS certificate chains are accepted without "
+ "validation, the identity of the servers is not authenticated. Anybody "
+ "able to answer for the host name receives everything sent to them.");
}
if (!verifyHostnames) {
LOG.warn(
"http.verify.hostnames is disabled: the certificates are not checked against "
+ "the host name either, the identity of the servers is not authenticated.");
}
if (trustEverything && !sendCredentials) {
LOG.warn(
"Credentials configured with http.basicauth.*, credential headers in "
+ "http.custom.headers and cookies are withheld because the servers "
+ "are not authenticated. Set http.credentials.allow.insecure to true "
+ "to send them anyway.");
}

builder =
new OkHttpClient.Builder()
.retryOnConnectionFailure(
Expand Down Expand Up @@ -223,15 +271,33 @@ public void configure(Config conf) {
// use a basic auth?
if (StringUtils.isNotBlank(basicAuthUser)) {
final String basicAuthPass = ConfUtils.getString(conf, "http.basicauth.password", "");
final String encoding =
Base64.getEncoder()
.encodeToString(
(basicAuthUser + ":" + basicAuthPass)
.getBytes(StandardCharsets.UTF_8));
customRequestHeaders.add(new KeyValue(HttpHeaders.AUTHORIZATION, "Basic " + encoding));
if (sendCredentials) {
final String encoding =
Base64.getEncoder()
.encodeToString(
(basicAuthUser + ":" + basicAuthPass)
.getBytes(StandardCharsets.UTF_8));
customRequestHeaders.add(
new KeyValue(HttpHeaders.AUTHORIZATION, "Basic " + encoding));
} else {
LOG.warn(
"Basic authentication configured with http.basicauth.user is withheld "
+ "because the servers are not authenticated (http.trust.everything). "
+ "Set http.credentials.allow.insecure to true to send it anyway.");
}
}

customHeaders.forEach(customRequestHeaders::add);
for (KeyValue customHeader : customHeaders) {
if (!sendCredentials && isCredentialHeader(customHeader.getKey())) {
LOG.warn(
"Custom header {} is withheld because the servers are not authenticated "
+ "(http.trust.everything). Set http.credentials.allow.insecure to "
+ "true to send it anyway.",
customHeader.getKey());
continue;
}
customRequestHeaders.add(customHeader);
}

// optionally block connections to forbidden IP address ranges
// (e.g. localhost/loopback, private/site-local addresses), see
Expand All @@ -245,15 +311,11 @@ public void configure(Config conf) {
builder.addNetworkInterceptor(new HTTPHeadersInterceptor());
}

if (ConfUtils.getBoolean(conf, "http.trust.everything", true)) {
if (trustEverything) {
builder.sslSocketFactory(trustAllSslSocketFactory, (X509TrustManager) trustAllCerts[0]);
builder.hostnameVerifier(
new HostnameVerifier() {
@Override
public boolean verify(String hostname, SSLSession session) {
return true;
}
});
}
if (!verifyHostnames) {
builder.hostnameVerifier((hostname, session) -> true);
}

builder.eventListenerFactory(
Expand Down Expand Up @@ -285,6 +347,15 @@ public EventListener create(Call call) {
}

private void addCookiesToRequest(Builder rb, String url, Metadata md) {
if (!sendCredentials) {

@rzo1 rzo1 Sep 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This early return sits above the "are there any cookies" check on line 361, so every crawl with http.use.cookies: true and http.trust.everything: true logs the warning once even when no cookie was ever set.

Move the guard below the emptiness check:

private void addCookiesToRequest(Builder rb, String url, Metadata md) {
    final String[] cookieStrings =
            md.getValues(RESPONSE_COOKIES_HEADER, protocolMetadataPrefix);
    if (cookieStrings == null || cookieStrings.length == 0) {
        return;
    }
    if (!sendCredentials) {
        if (withheldCookiesLogged.compareAndSet(false, true)) {
            LOG.warn(
                    "Cookies are withheld because the servers are not authenticated "
                            + "(http.trust.everything). Set http.credentials.allow.insecure to "
                            + "true to send them anyway.");
        }
        return;
    }
    ...

(Not offered as a committable suggestion because the replacement spans past the end of the hunk.)

if (withheldCookiesLogged.compareAndSet(false, true)) {
LOG.warn(
"Cookies are withheld because the servers are not authenticated "
+ "(http.trust.everything). Set http.credentials.allow.insecure to "
+ "true to send them anyway.");
}
return;
}
final String[] cookieStrings =
md.getValues(RESPONSE_COOKIES_HEADER, protocolMetadataPrefix);
if (cookieStrings == null || cookieStrings.length == 0) {
Expand Down Expand Up @@ -337,12 +408,33 @@ private URL getCookieOrigin(Metadata md, String url) {
}
}

/**
* Returns true when the header carries credentials which must not be disclosed to servers that
* were not authenticated. Covers the standard credential headers plus any header commonly used
* for API keys; cookie headers are handled separately in {@link #addCookiesToRequest}.
*/
private static boolean isCredentialHeader(String name) {
if (name == null) {
return false;
}
final String normalised = name.trim().toLowerCase(Locale.ROOT);
return CREDENTIAL_HEADERS.contains(normalised);
}

protected void addHeadersToRequest(Builder rb, Metadata md) {
final String[] headerStrings = md.getValues(SET_HEADER_BY_REQUEST, protocolMetadataPrefix);

if (headerStrings != null && headerStrings.length > 0) {
for (String hs : headerStrings) {
KeyValue h = KeyValue.build(hs);
if (!sendCredentials && isCredentialHeader(h.getKey())) {
LOG.warn(
"Header {} set by request is withheld because the servers are not "
+ "authenticated (http.trust.everything). Set "
+ "http.credentials.allow.insecure to true to send it anyway.",
h.getKey());
continue;
}
rb.addHeader(h.getKey(), h.getValue());
}
}
Expand Down
24 changes: 24 additions & 0 deletions core/src/main/resources/crawler-default.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,30 @@ config:
# Follow redirect HTTP responses:
http.allow.redirects: false

# Accept any TLS certificate, including self-signed, expired or otherwise
# invalid ones (okhttp protocol only)? The certificate chains are accepted
# without validation, i.e. the servers are not authenticated: anyone able to
# answer for the host name receives everything sent to them. Needed for
# crawling hosts with unvalidatable certificates, e.g. self-signed ones, but
# also disables protection against man-in-the-middle attacks. Credentials
# (basic auth, credential headers, cookies) are withheld over such
# connections unless http.credentials.allow.insecure is set to true.
http.trust.everything: false

# Check that the certificate presented by the server matches the host name
# contacted (okhttp protocol only)? Disabling this is independent of
# http.trust.everything: a valid certificate for a different name is then
# accepted. Hostname verification does not need to be disabled for hosts
# with self-signed certificates when http.trust.everything is enabled.
http.verify.hostnames: true

# Send credentials (basic auth, credential headers, cookies) even over
# connections whose server certificate was not validated, i.e. when
# http.trust.everything is true (okhttp protocol only)? Without it,
# credentials are withheld so that they are not disclosed to servers which
# were never authenticated.
http.credentials.allow.insecure: false

# IP address filtering (okhttp protocol only). Optionally limit or block
# connections to IP address ranges once the host name has been resolved. This
# prevents information leakage to a public index when a DNS entry points to a
Expand Down
Loading