#1998 WARC writer: WARC-Protocol header to follow WARC field proposals - #2034
#1998 WARC writer: WARC-Protocol header to follow WARC field proposals#2034sebastian-nagel wants to merge 1 commit into
Conversation
(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
| .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) |
There was a problem hiding this comment.
what happens if cipherSuite is null?
There was a problem hiding this comment.
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://).
There was a problem hiding this 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?
dpol1
left a comment
dpol1
left a comment
There was a problem hiding this comment.
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.
| .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) |
There was a problem hiding this 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?
| String cipherSuite = null; | ||
| final Handshake handshake = connection.handshake(); | ||
| if (handshake != null) { | ||
| protocols.append(',').append(handshake.tlsVersion()); |
There was a problem hiding this 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?
| * 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_"; |
There was a problem hiding this 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?
3 participants
This PR addresses #1998.
OkHttp protocol:
_cipher_suites_to hold the SSL/TLS Cipher suite separate from_protocol_versions_.WARC writer:
WARC-Cipher-SuiteWARC-Protocolheader and repeat headerExample WARC header with this change applied: