Make the WARC digest algorithm configurable - #2110
Conversation
dpol1
left a comment
dpol1
left a comment
There was a problem hiding this comment.
LGTM - default stays byte-identical sha1, all record formats share the configured algorithm, bad names fail at startup. Left one question inline about the sha256 padding.
| */ | ||
| public String getDigest(byte[] bytes) { | ||
| MessageDigest md = DigestUtils.getDigest(digestJCAName); | ||
| return digestPrefix + base32.encodeAsString(md.digest(bytes)); |
There was a problem hiding this comment.
One thing worth deciding before merge: Base32.encodeAsString pads, so sha256 digests end in ====, which strictly speaking falls outside WARC 1.1's token grammar for digest values. jwarc copes, but would a strict CDX tool? Stripping the trailing = here would be cheap now, less so once archives exist. What do you think?
There was a problem hiding this comment.
Agreed — good catch, thanks for flagging it before merge.
The spec backs this up: in ISO 28500 WARC 1.1 the digest fields are labelled-digest = algorithm ":" digest-value with digest-value = token, and the spec note explicitly says "The grammar for digest-value disallows the character = which is used for padding in Base32". So sha256:...==== is formally invalid, and as you say it's cheap to fix now and much less so once padded digests are in existing archives.
Commit a6e24d0 strips the trailing padding: both getDigest methods now route the Base32 output through a small helper that drops any trailing =. Two notes on the impact:
- SHA-1 is unaffected: a 20-byte digest is exactly 32 Base32 characters, so the default output is byte-for-byte identical to before. Only SHA-256 (52 characters + 4
=) changes. - Unpadded Base32 is also what the rest of the ecosystem writes (e.g. heritrix's WARC writer), so strict CDX tooling gets what it expects, while lenient readers like jwarc are fine either way.
The test expectations were updated to the unpadded values and there is now an explicit assertion that the produced digest value never contains = for either algorithm; README and the configuration docs mention the unpadded encoding too.
|
Reformat! |
The WARC digest fields define the digest value as a token, which does not allow the character "=" used for Base32 padding (ISO 28500 WARC 1.1). Strip the trailing padding so that e.g. SHA-256 digests conform; SHA-1 digests are 32 characters without padding and are unaffected. Suggested in review of apache#2110.
|
@abhinav-phi reformat! |
|
Sorry about that — my local check was missing the format validation, only checkstyle was run. Fixed in commit 5e709f5: the touched files were reformatted with |
1 similar comment
|
Sorry about that — my local check was missing the format validation, only checkstyle was run. Fixed in commit 5e709f5: the touched files were reformatted with |
|
rebase too :-) u have to resolve conflicts. |
WARCRecordFormat hard coded the algorithm for the WARC-Payload-Digest and WARC-Block-Digest fields to SHA-1 with no way for an operator to choose another algorithm. Add the configuration key warc.digest.algorithm which accepts sha1 (the default, kept for compatibility with CDX indexes and other downstream tooling) and sha256, and thread it through WARCHdfsBolt so that response, request, resource and metadata records all use it. The static getDigestSha1 helpers are kept and deprecated in favour of the new getDigest instance methods. apache#2106
The WARC digest fields define the digest value as a token, which does not allow the character "=" used for Base32 padding (ISO 28500 WARC 1.1). Strip the trailing padding so that e.g. SHA-256 digests conform; SHA-1 digests are 32 characters without padding and are unaffected. Suggested in review of apache#2110.
The validate-code-format check requires google-java-format (AOSP style); run git-code-format:format-code on the touched files. No functional changes.
5e709f5 to
2a6aec0
Compare
|
Follow-up on the reformat: I applied google-java-format ( The new head is 2a6aec0. GitHub has put the CI workflow run into "action_required" and it needs a maintainer's approval to start (force-push reset the approval): https://github.com/apache/stormcrawler/actions/runs/33533471476 — could someone please approve it? |
| * @deprecated use {@link #getDigest(byte[])} instead; the algorithm is set by {@link | ||
| * #DIGEST_ALGORITHM_PARAM} and no longer fixed to SHA-1 | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
We are going to do a new major release anyway, so if this is deprecated it can also be removed imho.
There was a problem hiding this comment.
Removed in 727ec1d — agreed, no point deprecating across a major release.
| * #DIGEST_ALGORITHM_PARAM} and no longer fixed to SHA-1 | ||
| */ | ||
| @Deprecated | ||
| public static String getDigestSha1(byte[] bytes1, byte[] bytes2) { |
There was a problem hiding this comment.
We are going to do a new major release anyway, so if this is deprecated it can also be removed imho.
what do others think? @jnioche ?
There was a problem hiding this comment.
We are going to do a new major release anyway, so if this is deprecated it can also be removed imho.
what do others think? @jnioche ?
perfect time to remove it indeed
| * "sha1:..." | ||
| */ | ||
| public String getDigest(byte[] bytes) { | ||
| MessageDigest md = DigestUtils.getDigest(digestJCAName); |
There was a problem hiding this comment.
Done in 727ec1d: getDigest(byte[]) now rejects null input via Objects.requireNonNull (it only ever receives arrays here — format() guards the content == null case — but the method is public API). The algorithm itself is validated once at construction time, which keeps the per-call path free of it.
| * @throws IllegalArgumentException if the value is not a supported algorithm | ||
| */ | ||
| private static String getDigestJCAName(String digestAlgorithm) { | ||
| if (digestAlgorithm == null) { |
There was a problem hiding this comment.
Shouldnt we treat digestAlgorithmn.isBlank() as SHA-1 too ?
There was a problem hiding this comment.
Done in 727ec1d: null or blank now selects SHA-1, same as an unset value.
| * Resolve the configured digest algorithm to the JCA name of the message digest. The value is | ||
| * matched case-insensitively and an optional hyphen is ignored, i.e. "sha256", | ||
| * "SHA-256" etc. are all accepted. | ||
| * |
There was a problem hiding this comment.
documentation for the param? what happens if it is null -> return sha1
There was a problem hiding this comment.
Done in 727ec1d: the javadoc now documents the parameter, including the null/blank → SHA-1 behaviour.
| this(protocolMDprefix, DIGEST_ALGORITHM_SHA1); | ||
| } | ||
|
|
||
| public WARCRecordFormat(String protocolMDprefix, String digestAlgorithm) { |
|
|
||
| private final String digestNoContent; | ||
|
|
||
| public WARCRecordFormat(String protocolMDprefix) { |
| * "sha1:..." | ||
| */ | ||
| public String getDigest(byte[] bytes1, byte[] bytes2) { | ||
| MessageDigest md = DigestUtils.getDigest(digestJCAName); |
There was a problem hiding this comment.
Done in 727ec1d: same as above, both arrays are null-checked.
| super(protocolMDprefix); | ||
| } | ||
|
|
||
| public WARCRequestRecordFormat(String protocolMDprefix, String digestAlgorithm) { |
There was a problem hiding this comment.
Done in 727ec1d — javadoc added (and matching ones on MetadataRecordFormat).
- remove the deprecated static getDigestSha1 methods: the next release is a major one (4.0.0), so the deprecated API can go straight away - validate the inputs of the getDigest methods (requireNonNull) - treat a null or blank warc.digest.algorithm value as the default (SHA-1), like an unset one - add javadoc to the constructors of the record formats and document the parameters, including the null/blank behaviour Suggested in review of apache#2110 by rzo1.
|
@rzo1 pushed 727ec1d addressing all nine review comments: the deprecated static getDigestSha1 methods are removed, getDigest validates its inputs, null/blank algorithm values fall back to SHA-1 like an unset one, and the constructors of all three record formats now carry javadoc. Local verify is green (30 tests, format validation, checkstyle). The CI run needs another maintainer approval after the push: https://github.com/apache/stormcrawler/actions/runs/33665614229 — could you (or @jnioche / @dpol1) trigger it? Thanks! |
4 participants
This PR addresses #2106 and fixes it.
Problem
WARCRecordFormat.getDigestSha1()computed a SHA-1 digest over the bytes and returned it assha1:<base32>. That fixed value was used for theWARC-Payload-DigestandWARC-Block-Digestheaders of response, resource, request and metadata records. There was no configuration key for the algorithm, so an operator who wants SHA-256 digests in their archives — which WARC 1.1 allows — could not have them without rewriting the records afterwards.SHA-1 digests are the convention across the WARC ecosystem and identify content for deduplication rather than authenticate it, so this is not urgent — but it was still a fixed choice in a place where the digested bytes come from the network. Operators whose own policy rules out SHA-1, or whose downstream tooling wants SHA-256, had no option.
Solution
Add a configuration key
warc.digest.algorithmand thread the configured algorithm through all record formats:sha1andsha256. The value is matched case-insensitively, an optional hyphen is ignored (sha256,SHA-256,SHA256are all accepted) and surrounding whitespace is trimmed.sha1remains the default: CDX indexes and revisit tooling downstream assumesha1:base32, and changing the default would break them. Deciding on a different default can be done separately from adding the option.md5) raises anIllegalArgumentExceptionwith a descriptive message when the bolt is prepared, instead of silently producing digests with a different algorithm than the one configured.=padding character, so digest values are written as unpadded Base32 (e.g. suggested in review). SHA-1 digests are exactly 32 characters without padding and are unaffected; SHA-256 digests would otherwise end in====.Changes
external/warc/src/main/java/org/apache/stormcrawler/warc/WARCRecordFormat.javaDIGEST_ALGORITHM_PARAM(warc.digest.algorithm) and the supported valuesDIGEST_ALGORITHM_SHA1/DIGEST_ALGORITHM_SHA256.WARCRecordFormat(String protocolMDprefix, String digestAlgorithm); the existing single-argument constructor is unchanged in behaviour and keeps defaulting to SHA-1.getDigest(byte[])andgetDigest(byte[], byte[])emit the matchingsha1:/sha256:prefix. The digest used for records without content (digestNoContent) is now derived from the configured algorithm instead of a static SHA-1 constant.getDigestSha1(byte[])andgetDigestSha1(byte[], byte[])are removed (suggested in review): 4.0.0 is a major release, so the deprecated API can go straight away. Use the instancegetDigestmethods instead.external/warc/src/main/java/org/apache/stormcrawler/warc/WARCRequestRecordFormat.javaandMetadataRecordFormat.javagetDigestinstance methods so request and metadata records use the same algorithm as response and resource records.external/warc/src/main/java/org/apache/stormcrawler/warc/WARCHdfsBolt.javawarc.digest.algorithmfrom the topology configuration (defaultsha1) and passes it to every record format it instantiates, so a single setting governs the whole WARC output.Documentation
external/warc/README.md: documents the new key, the accepted values and the compatibility considerations around SHA-1.docs/src/main/asciidoc/configuration.adoc: addswarc.digest.algorithmto the WARC section of the configuration reference.Note on #2034
As requested in the issue, I checked PR #2034 ("WARC writer: WARC-Protocol header to follow WARC field proposals"): it is about the
WARC-Protocol/ cipher-suite headers and does not touch digest computation, so this is not a duplicate. It modifies the same file, so whichever merges second may need a trivial rebase.Testing
WARCDigestAlgorithmTest(11 tests):nullalgorithm value;getDigest(byte[])andgetDigest(byte[], byte[]), including empty content;SHA256,SHA-256,sha256,SHA-1);IllegalArgumentExceptionfor unsupported values (md5,sha512) on all three record format classes;"", whitespace) algorithm values select the default SHA-1 likenull;=), for both SHA-1 and SHA-256;getDigestrejects null byte arrays (NullPointerException);WARC-Payload-Digest/WARC-Block-Digestcarry thesha256:prefix and the correct digest value, computed independently of the code under test.WARCRecordFormatTestSHA-1 digest tests now exercise the instance methods, covering the default algorithm (plain, empty, two byte arrays, robots.txt digests).WARCHdfsBoltTest.testDigestAlgorithmConfigprepares the bolt withwarc.digest.algorithm: sha256and verifies that the written records (warcinfo, request, response) usesha256:digests.mvn -pl external/warc test: 23 tests run, 0 failures, 0 errors;checkstyle:checkreports 0 violations.