rzo1 commented on code in PR #2126:
URL: https://github.com/apache/stormcrawler/pull/2126#discussion_r3944167776
##########
core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java:
##########
@@ -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;
Review Comment:
`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.
##########
core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java:
##########
@@ -285,6 +347,15 @@ public EventListener create(Call call) {
}
private void addCookiesToRequest(Builder rb, String url, Metadata md) {
+ if (!sendCredentials) {
Review Comment:
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:
```java
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.)
##########
core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java:
##########
@@ -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 =
Review Comment:
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.
--
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]
