test(k12): compare customization timings at equal length, not across lengths - #69
Merged
Merged
Conversation
…lengths `test_customization_constant_time` measured `""` (0 bytes), `"short"` (5), `"medium_length_customization"` (27) and `[0xAA; 100]`, then asserted all four timings sit within one 60% band. That asserts something false by construction. The customization string is absorbed into the sponge, so its LENGTH changes how much work Kt128 does -- 100 bytes costs strictly more than 0, and crossing a block boundary costs an extra permutation. The assertion only ever held because hashing the 1000-byte message dominated the difference: a test that passes only while the effect it measures is drowned out is not measuring anything. It is also aimed at the wrong property. A customization string is a domain separator -- public input, not key material -- so a length-dependent timing is not a side channel even in principle. The property actually worth asserting is that timing does not depend on the customization's CONTENT at a fixed length, which is what would matter if a customization ever carried secret material. All four inputs are now exactly 32 bytes and differ only in their bytes (all-zero, all-one, alternating, counter-derived), so the comparison is meaningful and the systematic length bias is gone. What this does NOT claim. It does not explain the observed CI failure on PR #57 (serde_json 1.0.150 -> 1.0.151), and it should not be sold as the fix for it. That run failed on 2026-07-20 at merge-base 123fc27, which predates `min_time` (introduced 2026-07-25 in a515797) -- so it was still single-sample wall clock, and the reported outlier was customization 0, the EMPTY string, i.e. the cheapest input timing slowest. That is noise, not length bias. Re-running #57 against current `main`, which now has the repeated-measurement minimum, is what addresses that failure; this commit removes an unsound assertion and restores the margin the length spread was eating, which makes future noise-induced failures less likely rather than impossible. The 60% band is deliberately NOT tightened. All four inputs now do identical work, so the band only absorbs runner noise; narrowing it would trade the bug just fixed for a fresh source of intermittent failures. This remains a coarse wall-clock smoke test, and the doc comment now says so: real leakage assessment belongs to `lib-q-sca-test` and the dedicated "Constant-Time Verification" job, and a pass here is not evidence of constant-timeness. Verified: 5/5 consecutive runs green, `cargo fmt -p lib-q-k12 -- --check` clean, `cargo clippy -p lib-q-k12 --all-targets --all-features -- -D warnings` clean.
🔒 Security Validation ReportGenerated: Tue Jul 28 22:47:45 UTC 2026 📊 Summary
✅ Overall Security Status: PASSEDAll critical security validations passed successfully. 🔍 DetailsThis report covers:
📋 Next Steps✅ Security validation passed. Code is ready for deployment. ✅ Security validation passed! This code meets all security requirements. |
🔍 Pull Request SummaryGenerated: Tue Jul 28 22:52:59 UTC 2026 📋 Validation Results
✅ Overall Status: PASSED🔒 Security Checklist
📝 Review NotesPlease review the security implications of this change carefully. ✅ Automated validation passed! This PR is ready for review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
test_customization_constant_timecompared customization strings of different lengths (0, 5, 27, 100 bytes) against a single 60% timing band.That asserts something false by construction: the customization is absorbed into the sponge, so a longer one costs strictly more work. It only ever passed because hashing the 1000-byte message dominated the difference. It is also aimed at the wrong property - a customization string is a public domain separator, not key material, so length-dependent timing is not a side channel even in principle.
All four inputs are now exactly 32 bytes and differ only in content (all-zero, all-one, alternating, counter-derived), so the comparison isolates the property worth asserting: that timing does not depend on customization content at fixed length.
What this does not claim. It is not the fix for #57's red run. That failed on 2026-07-20 at merge-base 123fc27, which predates
min_time(added 2026-07-25 in a515797), so it was still single-sample wall clock - and the reported outlier was customization 0, the empty string, i.e. the cheapest input timing slowest. That is noise. This change removes an unsound assertion and restores the margin the length spread was eating.The 60% band is deliberately not tightened - all inputs now do identical work, so it only absorbs runner noise.
Verified locally: 5/5 consecutive runs green, fmt clean,
clippy --all-targets --all-features -- -D warningsclean.