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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
