Re: [PR] #1998 WARC writer: WARC-Protocol header to follow WARC field proposals [stormcrawler]

["dpol1 (via GitHub)" <gi...@apache.org>]

dpol1 commented on code in PR #2034:
URL: https://github.com/apache/stormcrawler/pull/2034#discussion_r3767537924


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

Review Comment:
   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?



##########
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:
   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?



##########
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:
   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?



-- 
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: dev-unsubscribe@stormcrawler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org