#1998 WARC writer: WARC-Protocol header to follow WARC field proposals by sebastian-nagel · Pull Request #2034 · apache/stormcrawler

sebastian-nagel · 2026-07-29T17:34:42Z

This PR addresses #1998.

OkHttp protocol:

add protocol response header key _cipher_suites_ to hold the SSL/TLS Cipher suite separate from _protocol_versions_.

WARC writer:

add WARC header WARC-Cipher-Suite
split multiple values in WARC-Protocol header and repeat header

Example WARC header with this change applied:

WARC/1.0
WARC-Record-ID: <urn:uuid:64d3f1a3-2293-4581-b676-a4d715b65ebc>
Content-Length: 30510
WARC-Date: 2026-07-29T17:20:01Z
WARC-Type: response
WARC-IP-Address: 2620:cb:2000::1
WARC-Target-URI: https://commoncrawl.org/blog/common-crawl-foundation-at-lrec-2026
Content-Type: application/http; msgtype=response
WARC-Protocol: h2
WARC-Protocol: TLS_1_3
WARC-Cipher-Suite: TLS_AES_128_GCM_SHA256
WARC-Payload-Digest: sha1:72ZYLF3J57X4GOO6UNSPQHE366QPTWVL
WARC-Block-Digest: sha1:M34FZULWWPMGUNTPJZLUGUPQ7TE5G5JS

HTTP/1.1 200 
date: Wed, 29 Jul 2026 17:20:01 GMT
...

(fixes apache#1998) OkHttp protocol: add protocol response header key `_cipher_suites_` to hold the SSL/TLS Cipher suite separate from `_protocol_versions_`. WARC writer: 1. add WARC header `WARC-Cipher-Suite` 2. split multiple values in `WARC-Protocol` header and repeat header

jnioche · 2026-07-30T09:39:45Z

                    .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)


what happens if cipherSuite is null?

WARC writer checks for null anyway, because another protocol implementation could have been used which does not collect the necessary information. If it's not available, it does not add the WARC-Cipher-Suite header.

But I can change the code, so that no header is added, if no Cipher suite was used (connection over http://).

Doesn't Response.Builder.header() reject a null value though? If the handshake is null (plain http) I don't see how this survives the interceptor - have you tried a plain http:// fetch with http.store.headers=true? That's the WARC setup, so worth checking before the writer's null check even comes into play. Maybe worth a small test on the interceptor too, I don't think anything covers it right now?

dpol1

Shape looks right to me, few open questions in the threads. Also, branch is from before the Storm 3 / Java 25 upgrade, could you rebase? Would like a green run on the actual base.

dpol1 · 2026-08-12T14:49:42Z

                    .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)


Doesn't Response.Builder.header() reject a null value though? If the handshake is null (plain http) I don't see how this survives the interceptor - have you tried a plain http:// fetch with http.store.headers=true? That's the WARC setup, so worth checking before the writer's null check even comes into play. Maybe worth a small test on the interceptor too, I don't think anything covers it right now?

dpol1 · 2026-08-12T14:53:03Z

+            String cipherSuite = null;
            final Handshake handshake = connection.handshake();
            if (handshake != null) {
                protocols.append(',').append(handshake.tlsVersion());


What does this write, TLS_1_3 or tls/1.3? The registry only lists the tls/x.y forms - is the enum name close enough for the proposal, or do we want a mapping here?

dpol1 · 2026-08-12T14:53:08Z

+     * 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_";


Why plural? One handshake gives one suite and the WARC header is singular. Once this key ships in persisted metadata we can't rename it - is _cipher_suite_ better while we still can?

rzo1

Picking up the open threads — I rebased the branch onto current main locally (Storm 2.8.9 / Java 25, Selenium module removal): it applies cleanly, no conflicts, and WARCRecordFormatTest, WARCHdfsBoltTest and the okhttp protocol tests are green with the changes suggested below.

Three points from the threads, with concrete suggestions inline:

1. Null cipher suite is a real NPE, not just a writer-side concern. @dpol1 is right. Response.Builder.header(String, String) is Kotlin non-null, so a plain http:// fetch with http.store.headers=true fails before the WARC writer's null check ever runs:

java.lang.NullPointerException: Parameter specified as non-null is null:
    method okhttp3.Response$Builder.header, parameter value
	at org.apache.stormcrawler.protocol.okhttp.HttpProtocol$HTTPHeadersInterceptor.intercept(HttpProtocol.java:690)

I reproduced this against the Jetty test server on the current head of this branch. The header has to be skipped instead of set to null (see the comment on HttpProtocol.java).

2. TLS_1_3 vs tls/1.3. The enum name is not a registered value — the proposal only lists the tls/x.y forms — so a mapping seems worth having, otherwise the whole point of following the field proposal is lost for the TLS layer. Suggestion inline.

3. _cipher_suites_ -> _cipher_suite_. Agreed, singular, and better to rename now than after it ships in persisted metadata.

I also added a regression test for the plain-HTTP case @dpol1 asked about, since nothing covers the interceptor today — feel free to take it as is (core/src/test/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocolHeadersTest.java):

/** Tests the protocol metadata collected by the response interceptor. */
class HttpProtocolHeadersTest extends AbstractProtocolTest {

    /**
     * Over an unencrypted connection there is no handshake, hence no TLS version and no cipher
     * suite. The cipher suite header must be skipped entirely: OkHttp's {@code
     * Response.Builder.header(...)} does not accept a null value.
     */
    @Test
    void plainHttpRequestStoresProtocolVersionButNoCipherSuite() throws Exception {
        HttpProtocol protocol = new HttpProtocol();
        Config conf = protocolConfig();
        conf.put("http.store.headers", true);
        protocol.configure(conf);

        ProtocolResponse response =
                protocol.getProtocolOutput("http://localhost:" + HTTP_PORT, Metadata.empty);

        assertEquals(200, response.getStatusCode());
        Metadata metadata = response.getMetadata();
        assertEquals(
                "http/1.1",
                metadata.getFirstValue(ProtocolResponse.PROTOCOL_VERSIONS_KEY),
                "The protocol version is expected to be stored for plain HTTP requests");
        assertNull(
                metadata.getFirstValue(ProtocolResponse.CIPHER_SUITE_KEY),
                "No cipher suite is expected without a TLS handshake");
    }

    private Config protocolConfig() {
        Config conf = new Config();
        conf.put("http.agent.name", "test");
        conf.put("http.agent.version", "1.0");
        conf.put("http.agent.description", "test");
        conf.put("http.agent.url", "http://test.example.com");
        conf.put("http.agent.email", "test@example.com");
        return conf;
    }
}

(It fails with the NPE above on the current head of this branch and passes with the suggested interceptor change.) Nice work otherwise — the repeated WARC-Protocol header and the separate cipher suite field are the right shape.

rzo1 · 2026-08-17T18:29:47Z

+    /**
+     * 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_";


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.

Suggested change

/**

* 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_";

/**

* 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_";

rzo1 · 2026-08-17T18:29:47Z

            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();
            }


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().

Suggested change

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();

}

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;):

/** * 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); } }

rzo1 · 2026-08-17T18:29:47Z

                    .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)


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:

// 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();

rzo1 · 2026-08-17T18:29:47Z

        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);
        }


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.

Suggested change

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);

}

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);

}

rzo1 · 2026-08-17T18:29:47Z

        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\"");


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.)

Suggested change

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\"");

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");

rzo1 · 2026-08-17T18:29:47Z

        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");


Suggested change

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");

metadata.addValue(

protocolMDprefix + ProtocolResponse.PROTOCOL_VERSIONS_KEY,

httpVersionString + ",tls/1.3");

metadata.addValue(

protocolMDprefix + ProtocolResponse.CIPHER_SUITE_KEY, "TLS_AES_256_GCM_SHA384");

rzo1 · 2026-08-17T18:29:47Z

+        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");


Suggested change

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");

metadata.addValue(protocolMDprefix + ProtocolResponse.PROTOCOL_VERSIONS_KEY, "h2,tls/1.3");

metadata.addValue(

protocolMDprefix + ProtocolResponse.CIPHER_SUITE_KEY, "TLS_AES_256_GCM_SHA384");

rzo1 · 2026-08-17T18:29:47Z

+        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\"");


Suggested change

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\"");

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\"");

jnioche reviewed Jul 30, 2026

View reviewed changes

dpol1 reviewed Aug 12, 2026

View reviewed changes

rzo1 reviewed Aug 17, 2026

View reviewed changes

-    /**
-     * 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_";
+    /**
+     * 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_";

-        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);
-        }
+        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);
+        }

Uh oh!

Conversation

sebastian-nagel commented Jul 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpol1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rzo1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants