rzo1 commented on code in PR #2034:
URL: https://github.com/apache/stormcrawler/pull/2034#discussion_r3797844731
##########
core/src/main/java/org/apache/stormcrawler/protocol/ProtocolResponse.java:
##########
@@ -45,6 +45,12 @@ public class ProtocolResponse {
*/
public static final String PROTOCOL_VERSIONS_KEY = "_protocol_versions_";
+ /**
+ * Key which holds the SSL/TLS cipher suites. For requests sent over
http:// the value may be
+ * null.
+ */
+ public static final String CIPHER_SUITES_KEY = "_cipher_suites_";
Review Comment:
One handshake yields exactly one cipher suite and the WARC header is
singular, so the singular key reads better — and it can't be renamed once it is
in persisted metadata.
```suggestion
/**
* Key which holds the SSL/TLS cipher suite negotiated during the
handshake. Not set if the
* request was sent over an unencrypted connection (http://).
*/
public static final String CIPHER_SUITE_KEY = "_cipher_suite_";
```
##########
core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java:
##########
@@ -686,6 +687,7 @@ public Response intercept(Interceptor.Chain chain) throws
IOException {
.header(ProtocolResponse.RESPONSE_IP_KEY, ipAddress)
.header(ProtocolResponse.REQUEST_TIME_KEY,
Long.toString(startFetchTime))
.header(ProtocolResponse.PROTOCOL_VERSIONS_KEY,
protocols.toString())
+ .header(ProtocolResponse.CIPHER_SUITES_KEY, cipherSuite)
Review Comment:
This throws an NPE for every `http://` fetch when `http.store.headers=true`
(details and stack trace in the review summary) — `Response.Builder.header`
rejects a null value, so the writer's null check is never reached.
I can't offer this as a one-click suggestion because the fix also touches
`return response.newBuilder()` above, which is outside the diff. The chain
needs to become a local builder:
```java
// returns a modified version of the response
final Response.Builder builder =
response.newBuilder()
.header(
ProtocolResponse.REQUEST_HEADERS_KEY,
new String(encodedBytesRequest,
StandardCharsets.ISO_8859_1))
.header(
ProtocolResponse.RESPONSE_HEADERS_KEY,
new String(encodedBytesResponse,
StandardCharsets.ISO_8859_1))
.header(ProtocolResponse.RESPONSE_IP_KEY,
ipAddress)
.header(
ProtocolResponse.REQUEST_TIME_KEY,
Long.toString(startFetchTime))
.header(ProtocolResponse.PROTOCOL_VERSIONS_KEY,
protocols.toString());
// no handshake and no cipher suite for connections over http://
if (cipherSuite != null) {
builder.header(ProtocolResponse.CIPHER_SUITE_KEY,
cipherSuite);
}
return builder.build();
```
##########
core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java:
##########
@@ -669,10 +669,11 @@ public Response intercept(Interceptor.Chain chain) throws
IOException {
.getBytes(StandardCharsets.ISO_8859_1));
final StringBuilder protocols = new
StringBuilder(response.protocol().toString());
+ String cipherSuite = null;
final Handshake handshake = connection.handshake();
if (handshake != null) {
protocols.append(',').append(handshake.tlsVersion());
- protocols.append(',').append(handshake.cipherSuite());
+ cipherSuite = handshake.cipherSuite().toString();
}
Review Comment:
`TlsVersion.toString()` yields the enum name (`TLS_1_3`), which is not one
of the values listed in the field proposal — those are `tls/1.0` .. `tls/1.3`
(and `ssl/3.0`). Mapping it here keeps `_protocol_versions_` in the registered
vocabulary for every consumer, not just the WARC writer.
`CipherSuite.javaName()` is also a bit more explicit than relying on
`toString()`.
```suggestion
final StringBuilder protocols = new
StringBuilder(response.protocol().toString());
final Handshake handshake = connection.handshake();
String cipherSuite = null;
if (handshake != null) {
protocols.append(',').append(getProtocolIdentifier(handshake.tlsVersion()));
cipherSuite = handshake.cipherSuite().javaName();
}
```
with this helper next to `getNormalizedProtocolName(Protocol)` (plus `import
okhttp3.TlsVersion;`):
```java
/**
* Maps a {@link TlsVersion} to the protocol identifier used in the
<code>WARC-Protocol
* </code> header, see the <a
* href="https://github.com/iipc/warc-specifications/issues/42";>WARC
field proposal</a>. The
* enum names of {@link TlsVersion} (e.g. <code>TLS_1_3</code>) are
not part of the
* registered values (e.g. <code>tls/1.3</code>).
*/
private static String getProtocolIdentifier(TlsVersion tlsVersion) {
switch (tlsVersion) {
case SSL_3_0:
return "ssl/3.0";
case TLS_1_0:
return "tls/1.0";
case TLS_1_1:
return "tls/1.1";
case TLS_1_2:
return "tls/1.2";
case TLS_1_3:
return "tls/1.3";
default:
return tlsVersion.javaName().toLowerCase(Locale.ROOT);
}
}
```
##########
external/warc/src/main/java/org/apache/stormcrawler/warc/WARCRecordFormat.java:
##########
@@ -468,7 +468,14 @@ public byte[] format(Tuple tuple) {
metadata.getFirstValue(
ProtocolResponse.PROTOCOL_VERSIONS_KEY,
this.protocolMDprefix);
if (protocolVersions != null) {
- buffer.append("WARC-Protocol:
").append(protocolVersions).append(CRLF);
+ for (String val : StringUtils.split(protocolVersions, ',')) {
+ buffer.append("WARC-Protocol: ").append(val).append(CRLF);
+ }
+ }
+ final String cipherSuites =
+ metadata.getFirstValue(ProtocolResponse.CIPHER_SUITES_KEY,
this.protocolMDprefix);
+ if (cipherSuites != null) {
+ buffer.append("WARC-Cipher-Suite:
").append(cipherSuites).append(CRLF);
}
Review Comment:
Key rename, plus trimming/skipping empty tokens — the metadata value is
user-visible and may well be written by another protocol implementation with
spaces after the commas, which would produce an invalid `WARC-Protocol:
tls/1.3` value.
```suggestion
if (protocolVersions != null) {
// for layered protocols the metadata value holds multiple
comma-separated
// values, the WARC-Protocol header is repeated for every single
value
for (String protocolVersion :
StringUtils.split(protocolVersions, ',')) {
protocolVersion = protocolVersion.trim();
if (!protocolVersion.isEmpty()) {
buffer.append("WARC-Protocol:
").append(protocolVersion).append(CRLF);
}
}
}
final String cipherSuite =
metadata.getFirstValue(ProtocolResponse.CIPHER_SUITE_KEY,
this.protocolMDprefix);
if (cipherSuite != null) {
buffer.append("WARC-Cipher-Suite:
").append(cipherSuite).append(CRLF);
}
```
##########
external/warc/src/test/java/org/apache/stormcrawler/warc/WARCHdfsBoltTest.java:
##########
@@ -130,6 +130,13 @@ void testHttp2() throws IOException {
assertTrue(
response.headers().first("WARC-Protocol").isPresent(),
"WARC response record is expected to include WARC header
\"WARC-Protocol\"");
+ assertEquals(
+ 2,
+ response.headers().all("WARC-Protocol").size(),
+ "WARC response record is expected to include WARC header
\"WARC-Protocol\"");
Review Comment:
Asserting the values makes the repetition explicit, and the count assertion
already subsumes the `isPresent()` one above it. (The message on the count
assertion is also a copy of the previous one.)
```suggestion
assertEquals(
List.of("HTTP/2", "tls/1.3"),
response.headers().all("WARC-Protocol"),
"WARC response record is expected to repeat the WARC header
\"WARC-Protocol\" for every protocol layer");
```
##########
external/warc/src/test/java/org/apache/stormcrawler/warc/WARCHdfsBoltTest.java:
##########
@@ -211,7 +218,9 @@ private Tuple getPage(String httpVersionString) {
+ "Connection: close\r\n\r\n");
metadata.addValue(
protocolMDprefix + ProtocolResponse.PROTOCOL_VERSIONS_KEY,
- httpVersionString + ",TLS_1_3,TLS_AES_256_GCM_SHA384");
+ httpVersionString + ",TLS_1_3");
+ metadata.addValue(
+ protocolMDprefix + ProtocolResponse.CIPHER_SUITES_KEY,
"TLS_AES_256_GCM_SHA384");
Review Comment:
```suggestion
metadata.addValue(
protocolMDprefix + ProtocolResponse.PROTOCOL_VERSIONS_KEY,
httpVersionString + ",tls/1.3");
metadata.addValue(
protocolMDprefix + ProtocolResponse.CIPHER_SUITE_KEY,
"TLS_AES_256_GCM_SHA384");
```
##########
external/warc/src/test/java/org/apache/stormcrawler/warc/WARCRecordFormatTest.java:
##########
@@ -225,9 +225,9 @@ void testReplaceHttpVersion() {
+ "Content-Encoding: gzip\r\n"
+ "Content-Length: 26\r\n"
+ "Connection: close");
+ metadata.addValue(protocolMDprefix +
ProtocolResponse.PROTOCOL_VERSIONS_KEY, "h2,TLS_1_3");
metadata.addValue(
- protocolMDprefix + ProtocolResponse.PROTOCOL_VERSIONS_KEY,
- "h2,TLS_1_3,TLS_AES_256_GCM_SHA384");
+ protocolMDprefix + ProtocolResponse.CIPHER_SUITES_KEY,
"TLS_AES_256_GCM_SHA384");
Review Comment:
```suggestion
metadata.addValue(protocolMDprefix +
ProtocolResponse.PROTOCOL_VERSIONS_KEY, "h2,tls/1.3");
metadata.addValue(
protocolMDprefix + ProtocolResponse.CIPHER_SUITE_KEY,
"TLS_AES_256_GCM_SHA384");
```
##########
external/warc/src/test/java/org/apache/stormcrawler/warc/WARCRecordFormatTest.java:
##########
@@ -246,8 +246,14 @@ void testReplaceHttpVersion() {
statusLine.matches("^HTTP/1\\.[01] .*"),
"WARC response record: HTTP status line must start with
HTTP/1.1 or HTTP/1.0");
assertTrue(
- headersPayload[0].contains("\r\nWARC-Protocol: "),
- "WARC response record is expected to include WARC header
\"WARC-Protocol\"");
+ headersPayload[0].contains("\r\nWARC-Protocol: h2\r\n"),
+ "WARC response record is expected to include a WARC header
\"WARC-Protocol: h2\"");
+ assertTrue(
+ headersPayload[0].contains("\r\nWARC-Protocol: TLS_1_3\r\n"),
+ "WARC response record is expected to include a WARC header
\"WARC-Protocol: TLS_1_3\"");
Review Comment:
```suggestion
assertTrue(
headersPayload[0].contains("\r\nWARC-Protocol: tls/1.3\r\n"),
"WARC response record is expected to include a WARC header
\"WARC-Protocol: tls/1.3\"");
```
--
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]
