[eduardomourar]

Related issue

Related #380

Description

Hashing was hard-coded to MD5, truncated to 32 uppercase hex characters. Callers that need an untruncated, lowercase digest to line up with a hash computed elsewhere (e.g. AWS CDK's SHA256 asset hashes) had no way to get one from core and had to reimplement hashing/bundler-output-capturing themselves.

Add HashAlgorithm (MD5 | SHA256) to AssetOptions, threaded through AssetHash.of, AssetStaging and TerraformAsset. MD5 keeps the existing truncated/uppercased behavior; SHA256 is returned full-length and lowercase.

Checklist

• I have updated the PR title to match CDKTN's style guide
• I have run the linter on my code locally
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation if applicable
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works if applicable
• New and existing unit tests pass locally with my changes

[sakul-learning]

Review summary

No blocking correctness or security issues found. The algorithm is propagated consistently through AssetHash, AssetStaging, the source-hash cache key, bundler-output hashing, and TerraformAsset; the default MD5 behavior remains compatible. I left two non-blocking inline comments:

• AssetOptions.hashAlgorithm: narrow the external-hash interoperability claim
• SHA-256 tests: add a known-answer assertion and one focused OUTPUT-path case

Verification at head 42ad876215c18f6b077691d2671711af89381739:

• pnpm exec jest packages/cdktn/test/validations.test.ts --runInBand — 37 tests and 8 snapshots passed
• pnpm exec nx build cdktn — jsii succeeded with 0 errors and 0 warnings
• pnpm exec nx test cdktn --runInBand — 48 suites, 693 tests, and 300 snapshots passed

[sakul-learning]

AssetOptions.hashAlgorithm changes the digest primitive and rendering, but not the bytes CDKTN feeds into that digest. For example, AssetHash.of() applies CDKTN’s canonical entry framing, so a file containing hello hashes to 27076d39dabf3e0cb647460e8ada94593e92938c2d7af0d6252b49d431f363e3, not the raw file SHA-256 2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824. The legacy/canonical and source/output paths also frame different inputs. Could this wording say that SHA-256 selects CDKTN’s digest algorithm and output format, and reserve “line up with one computed elsewhere” for a specifically documented compatible input/framing path?

[jsteinich]

Adding to this, since the claim is specifically about AWS CDK: I checked it against aws-cdk's actual core/lib/fs/fingerprint.ts rather than a raw file digest, and the two streams are structurally unrelated:

	AWS CDK	cdktn canonical
prefix	hashes options.extra, options.follow, options.ignoreMode first	nothing
per file	\x01 file:<path> \x02 <size>:<sha256(content)> \x03 — a nested content hash	F <mode> <relPath>\0<len>\0<raw bytes>
framing	SOH / SOT / ETX	null bytes, includes file mode

So it is not only that the framing differs from a raw digest — a cdktn SHA256 hash cannot equal a CDK hash for any tree. The matching 64-char lowercase format makes that easy to miss, since the values look interchangeable.

Two things beyond the doc wording: the PR description makes the same claim ("line up with … AWS CDK's SHA256 asset hashes"), so it is worth correcting there too. And if CDK-identical hashes are genuinely the goal — e.g. for TerraConstructs-ported constructs that compare them — that needs CDK's fingerprint algorithm rather than its digest, which per the layering in #380 would sit in a compat package rather than core.

[so0k]

yeah I don't think we can match AWSCDK hash, but I'm curious if setting the algorithm to SHA256 allows preserving the asset keys from TerraConstructs - I don't think that's the end goal anyway as the CDKTN Asset pipeline is more careful and works differently from TerraConstructs so this would be fine as a breaking change when adopted into TerraConstructs

[sakul-learning]

This verifies the 64-character lowercase shape, but it would also pass if the implementation accidentally used another 256-bit digest. The related assertions in canonical-asset-hash.test.ts likewise check shape and inequality rather than a known SHA-256 value. Could one of these cases assert an independently computed known answer, and could one focused AssetStaging case exercise the newly threaded OUTPUT path? Both test files run under the normal nx test cdktn target, so that small addition would protect the public algorithm contract without requiring a large matrix.

[so0k]

Non-blocking test review. The new tests cover the SHA256 format for AssetHash.of, the pass-through from TerraformAsset, and both extraHash folds. A few production paths this diff changes have no test yet; listing them in rough order of user impact. All of them fit in the unit tier.

1. Hash cache key (asset-staging.ts, algorithm: this.hashAlgorithm in cacheKey). AssetStaging keeps one hash cache per app. Before hashing a source directory it builds a key from the inputs - if algorithm wer not part of the key, 2 assets in the same app pointing at the same directory but differing on algorithm would produce the same key. If algorithm field were dropped, the second asset in an App would get the first one's cached hash, in the wrong algorithm and format. No current test can see it, because the stack() helper builds a fresh App per call and the cache is per root. A case with two AssetStaging over the same source in one stack (default and SHA256), asserting each format, would own it.
2. OUTPUT hashing with a bundler (algorithm passed to hashPath(produced, ...)). To ensure the one place where the hash is computed over a bundler's built output instead of the source directory. No test combines a bundler, AssetHashType.OUTPUT and SHA256. This test catches regressions if algorithm were dropped from the second hashPath call path (where assetHashType: AssetHashType.Output and a bundler is set.
3. Both sides of canonicalAssetHashes. hashPath picks canonicalHashPath or legacyHashPath from the flag, and this diff threads algorithm through both; features.ts asks for a test of the disabled and the enabled case. Every new TerraformAsset test runs with the flag enabled, so the legacy branch is never exercised with SHA256. That branch is what any project without the flag in its cdktf.json runs: those created before 0.24.0, or not created by cdktn init. A test with the flag off catches algorithm not reaching legacyHashPath, or one of its createHash calls staying on MD5, which would hand those projects a truncated MD5 while they asked for SHA256. A known-answer case does it and also proves the digest really is SHA-256: the legacy scheme hashes file bytes only, so with the flag off a directory holding one file containing hello hashes to plain sha256("hello") = 2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824 (checked locally against this head).
4. Default value pins, one per scheme. The default path was rewritten: hashPath now resolves the algorithm and formats through formatDigest, and the extraHash/salt/bundlerKey folds do the same. The promise is that hashes computed without hashAlgorithm do not move, because the hash is part of the staged asset path, so a moved hash re-stages and redeploys the asset. The unit tests only assert the 32-uppercase-hex shape, which still passes if every value changes (for example a different truncation, or a changed input to the digest). A literal pin on each scheme catches that, and two populations depend on it:
   • Legacy (flag off). The same one-file fixture without hashAlgorithm gives 5D41402ABC4B2A76B9719D911017C592, which is plain md5("hello") truncated and uppercased, so the literal is an independent oracle and not a value recomputed by hashPath.
   • Canonical (flag on). Projects initialised with cdktn init since 0.24.0 have the flag on, and nothing pins a canonical value: the canonical tests are relational, and the integration literals in test/typescript/modules run with an empty context, so they are legacy too. The framing is cdktn's own, so there is no external oracle; the pin would be a value recorded on main and asserted unchanged on this branch. The canonical scheme hashes file permission bits, so the fixture needs an explicit chmod and a POSIX gate.
5. Out-of-range hashAlgorithm. This one is a production gap first. From a non-TypeScript caller, "sha1" is accepted and returns a truncated uppercase SHA-1, and an unknown name throws Node's raw Digest method not supported. The sibling assetHashType has an explicit guard (assetHashTypeUnknown) and a test; the same treatment here would be consistent.

Inline comments below on two tests that look redundant and one that could be narrowed.

[so0k]

Non-blocking: I think this test can be dropped. It compares a 64-character digest (uppercased) with a 32-character one, so the inequality holds for any two values of those shapes. The only regression it catches is the option being ignored by AssetHash.of, and the previous test (produces a full-length lowercase digest) already fails on that.

[so0k]

Non-blocking: could this be narrowed to the one new contract? The last assertion (the extraHash fold keeps the 64-lowercase-hex format) is the part nothing else covers, since AssetHash.of has its own fold. The content-change half and the withExtra !== original check repeat changes when content changes and the extraHash test above on the same walk, and the name hides the assertion that matters. Something like extraHash folding keeps the SHA256 format with just the format assertion, or a two-row table with the test above (with and without extraHash), would say it directly.

[so0k]

Non-blocking: same as the AssetHash.of one, I think this can be dropped. A 64-character value never equals a 32-character one, so it only fails if TerraformAsset does not forward the option, which the test just above already catches. It also would not notice the default flipping to SHA256: the uppercased SHA256 hash would still differ from the lowercase default.

[eduardomourar]

As discussed, we will migrate TerraConstructs to MD5 (TerraConstructs/base#166), so there is no immediate need for this functionality. So putting this into draft.