fix(utils): length-prefix paths in compute_content_hash; deprecate bare content_sha* recipe keys - #5992
fix(utils): length-prefix paths in compute_content_hash; deprecate bare content_sha* recipe keys#5992pb01ka wants to merge 6 commits into
compute_content_hash; deprecate bare content_sha* recipe keys#5992Conversation
compute_content_hash to prevent hash collisions
|
We require contributors to sign our Contributor License Agreement and we don't have one on file for @pb01ka. In order for us to review and merge your code, please e-sign the Contributor License Agreement PDF. We then need to manually verify your signature, merge the PR (conda/infrastructure#1345), and ping the bot to refresh the PR. |
| by their full path. For each entry in the contents table, compute the hash for the concatenated | ||
| bytes of: | ||
|
|
||
| - The decimal UTF-8 byte length of the path, followed by a UTF-8 encoded `:` separator. |
There was a problem hiding this comment.
UTF-8 byte length as in the number of bytes required to encode the path with UTF-8? And why do we need the colon separator?
There was a problem hiding this comment.
Added an explanation for it.
|
Seems like the following test is failing, This means the change I have made might be backwards incompatible? Is this a genuine concern? cc: @jaimergp |
That's to be decided in the CEP amendment we need to co-submit along with this fix. If it's a problem, we may need to add a |
|
@jaimergp The PR is passing all the tests. Please let me know if the changes look good here. |
| "content_sha256_v1": None, | ||
| "content_sha384_v1": None, | ||
| "content_sha512_v1": None, |
There was a problem hiding this comment.
Not sure about the v1 suffix here. It makes it look like this is actually the successor, not the deprecated version. We need a different term. Maybe just a leading underscore? Or something else?
_content_sha256_legacy_content_sha256_deprecated_content_sha256
Or the other way around, adding something to the new one (which is maybe better for bw compat):
length_prefixed_content_sha256(too long?)delimited_content_sha256normalized_content_sha256(true in both editions, but is maybe more informative)
There was a problem hiding this comment.
Good point on the naming. A couple of thoughts:
-
Leading underscore vs
_v1suffix - semantically these are equivalent: both signal "this is the old/legacy variant." The backward-compatibility story is identical either way - users who have stored hashes computed with the original CEP-19 algorithm (no length-prefixing) still need to opt in to the legacy key explicitly, regardless of whether it's called_content_sha256orcontent_sha256_v1. I slightly prefer_v1because I think it makes the relationship between the two explicit (v1 = original, unversioned = current/improved), but I'm open to switching if the team finds_content_sha256or_legacy_content_sha256clearer. -
Adding something to the new key instead - this is actually the cleanest option for backward compatibility. If we keep the existing
content_sha256name for the legacy algorithm and introducelength_prefixed_content_sha256(ordelimited_/normalized_) for the improved one, existing recipes and tooling that already referencecontent_sha256continue to work with zero changes and no deprecation warnings needed. The new, more secure algorithm is purely opt-in via the new name. The only downside is that the "plain" name now refers to the weaker algorithm, which could surprise new users - but that's a documentation problem, not a compatibility one. Among the suggestions,normalized_content_sha256reads the best to me since length-prefixing is an implementation detail whereas "normalized" conveys intent.
There was a problem hiding this comment.
I think we can find a better adjective than normalized, if it's about intent. Can you propose a few? e.g. I'm thinking of corrected_content_sha256 but I'm worried there's yet another flaw and then we have to come up with something else 😂
There was a problem hiding this comment.
Fair concern - "corrected" (and "normalized") both imply "this is the fixed version," which ages badly if another issue surfaces later. Here are a few alternatives grouped by strategy:
Describe the mechanism, not the quality
delimited_content_sha256- the entries (paths, symlinks) are delimited before hashing, so concatenation collisions can't occur. Specific enough to be meaningful, but makes no claim about overall algorithm quality.prefixed_content_sha256- refers directly to the length-prefixing. Accurate, but does expose the implementation detail (length_prefixed_has similar concerns).
Describe the property
distinct_content_sha256- the inputs are kept distinct; no two different path lists can produce the same hash input. Conveys the intent without implying "fully corrected."unambiguous_content_sha256- similar angle: the serialization is unambiguous. Slightly more self-explanatory but a bit long.
Just version it
content_sha256_v2- the most future-proof option. Makes no quality claim at all, just signals "newer algorithm." If a third revision is ever needed,v3slots in cleanly. The downside is it's less informative on its own, but a short docstring fixes that.
My recommendation: delimited_content_sha256 if you want the name to be self-documenting, content_sha256_v2 if you want it to be future-proof without overloading meaning onto an adjective.
There was a problem hiding this comment.
Already done. We are in good shape as of now.
compute_content_hash to prevent hash collisionscompute_content_hash; deprecate bare content_sha* recipe keys
…revent hash collisions feat(source): add legacy=True / content_sha*_v1 keys for CEP-19 backwards compatibility
…ameter and regression
|
The failing test doesn't seem relevant. AFAICT, they seem to fail due to Github constraints. Please let me know if I am perceiving this wrongly. Other than this the PR is up to date. Please let me know if any other change is needed. |
…ontent_sha* keys - Rename content_sha*_v1 legacy keys to bare content_sha* (deprecated) - Rename bare content_sha* keys (new algorithm) to content_sha*_v2 - Emit PendingDeprecationWarning in compute_content_hash when legacy=True, directing users to migrate to content_sha*_v2 recipe keys - Add test verifying the deprecation warning is raised
|
The failing test is due to network issues I think, FAILED tests/test_post.py::test_pypi_installer_metadata - conda.CondaMultiError: ('Connection broken: IncompleteRead(8124139 bytes read, 4861299 more expected)', IncompleteRead(8124139 bytes read, 4861299 more expected))Please feel free to re-run the failed checks to be sure. Other than this the PR is ready. If you have any reviews please feel free to share. I will also send a draft PR with the CEP linking this PR. It will supersede CEP-19. |
|
The CEP PR is raised at conda/ceps#174. |
|
Thanks @pb01ka, I'll mark this as draft until the CEP is approved. |
Description
Fix a hash-collision bug in
compute_content_hashreported in conda/ceps#150 and implement the v2 algorithm specified in the accompanying CEP (supersedes CEP 19).Bug fix - new v2 algorithm
Root cause. The hash stream for each directory entry was built by concatenating raw bytes in the order
<path><type><content><separator>, with no field-length information. Because filenames can contain the same bytes used as type markers (F,D,L) and the entry separator (-), two structurally different trees could produce an identical byte stream.For example:
testFhello-worldwith contentwwwtest(contenthello) plus a fileworld(contentwww)Both produced stream
testFhello-worldFwww-, yielding the same SHA-256 digest.Fix. Each variable-length field is now prefixed with its decimal byte length followed by
:before being fed to the hasher:<path_bytes><len(path_bytes)>:<path_bytes><target_bytes><len(target_bytes)>:<target_bytes>This makes field boundaries unambiguous and eliminates the collision.
Backwards compatibility and key naming
The algorithm change produces different digests for the same directory contents, so existing stored hashes are not compatible with the new algorithm. Two mechanisms are provided:
Distinct key families. The new fixed algorithm is exposed under the
content_sha256_v2/content_sha384_v2/content_sha512_v2recipe keys. New recipes should use these. The original CEP-19 algorithm is retained under the barecontent_sha256/content_sha384/content_sha512keys for backwards compatibility, but those keys are now deprecated - using them emits aDeprecationWarningat build time directing recipe authors to migrate to the_v2keys.legacyparameter.compute_content_hash()accepts a newlegacy=Truekeyword argument that reproduces the original CEP-19 byte stream exactly, for any code that computes or verifies hashes programmatically against pre-existing stored values. Passinglegacy=Truealso emits aDeprecationWarning.Migration guide
content_sha256content_sha256_v2content_sha384content_sha384_v2content_sha512content_sha512_v2Re-compute your hashes with
compute_content_hash(directory, legacy=False)(the default) and update the recipe keys accordingly.Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?Add / update outdated documentation?(no user-facing docs affected)