Luis/utils/ct unsigned masks#63
Open
laruizlo wants to merge 5 commits into
Open
Conversation
Groundwork for the RSA bigint layer: identical mask constructors for u64/u32 limbs so dual-width code is written once. - Register u32 (and i32 for symmetry) in supported_mask_type!. - Macro-generated impl for u64/u32 parity: from_bool, from_bool_var, from_lsb, from_msb, is_zero, is_not_zero, is_equal, select, mov, swap, to_bool_var (const fn except mov). - Unsigned constructions use two's-complement mask identities, not the i64 sign tricks, which are invalid for full-range unsigned values. - Ordering comparisons deliberately omitted: callers derive lt from subtraction borrow chains via from_msb. - Condition<u64>::select becomes const; is_true kept for backward compatibility; Condition<i64> untouched. - Tests: parity suites for both widths with boundary coverage, mask-canonicality asserts, and a borrow-adaptor cross-check against the < operator. Non-goals deferred: generic impl<T> refactor, Condition<u8>, is_in_list CT research question. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s_bit_set - is_negative and from_msb rustdoc now point at each other as the signed/unsigned spellings of the top-bit mask; is_bit_set and from_lsb likewise for the bit-0 case. - New is_bit_set(value, bit) on Condition<u64>/Condition<u32> for shape parity with the signed impl, delegating to from_lsb; index type follows the u32 shift-count convention of core. - Real doc text on the previously empty is_bit_set/is_negative doc comments. - Tests: is_bit_set agrees with from_lsb at bit 0 and from_msb at the top bit, and each single-bit value reports exactly its own bit, both widths. - Comment style fix in the i32 registration note. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Remove Condition<u64>::is_true, which duplicated to_bool_var under a different name and the only &self receiver in the API; its sole callers were this crate's tests. - One accessor across all widths keeps the is_* prefix meaning "data in, mask out", preserves the from_bool_var/to_bool_var boundary pair, and keeps the _var suffix warning that converting to bool exits constant-time discipline. - Migrated the u64 test call sites accordingly; no production callers existed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- i64 mask-canonicality test: select(-1, 0) must return exactly -1 or 0, catching the delete-unary-minus mutants in from_bool_var and is_bit_set that truthiness checks let survive (same shape as the unsigned suites' assert_canonical). - ct_eq_zero_bytes had no tests; added zero/nonzero coverage at first, last, and high-bit positions, killing all four of its mutants including the or-assign to and-assign fold corruption. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Formatting-only sweep of pre-existing drift in 21 files (trailing whitespace, blank-line collapses, rewraps); zero code changes (git diff -w shows only 4 blank-line deletions). Needed because rust-style.yml runs cargo fmt --all --check on every PR, which gates on files this branch never touched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
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.
Extend
ct.rsConditionto unsigned widths (u64 parity, new u32)Why
Groundwork for the upcoming RSA implementation. The RSA bigint layer uses
u64limbs on 64-bit targets andu32limbs elsewhere, and needs identical constant-time mask constructors for both widths so the dual-width code is written once. TodayConditionsupport is uneven:Condition<i64>has the full constructor set,Condition<u64>has onlyfrom_bool/select/is_true, andCondition<u32>does not exist.This PR is deliberately separate from the RSA work so the shared security-critical
utilscode gets its own review, and the RSA PRs stay pure-RSA.What
u32(andi32for width symmetry) insupported_mask_type!. Registration alone gives the new widths the existing generic boolean operator impls (& | ^ !).Condition<u64>andCondition<u32>to constructor parity where meaning is width-independent:TRUE/FALSEfrom_bool,from_bool_var: mask from a boolean (wrapping-sub construction)from_lsb: mask from bit 0 (parity/oddness tests)from_msb: mask from the top bit (adaptor for borrow/carry words coming out of wrapping subtraction chains:from_msb(borrow)is theltmask, no post-processing)is_zero,is_not_zero,is_equalselect,mov,swap,to_bool_varconst fnexceptmovCondition<i64>is untouched.Condition<u64>::selectbecomesconst(previously a plain fn, additive change);is_trueis kept for backward compatibility with a doc note pointing atto_bool_var.IMPORTANT / API removal: commit
0202982removesCondition<u64>::is_true(). It duplicatedto_bool_var()under a second name and was the API's only&selfreceiver; the standardized accessor across allConditionwidths is nowto_bool_var(). A workspace-wide search found no production callers (its only uses were this crate's own tests, migrated in the same commit), so nothing downstream breaks, but any out-of-tree or in-flight code callingis_true()must switch toto_bool_var()if the current PR is accepted as is. The removal is deliberately isolated in its own commit: if reviewers prefer to keepis_true, dropping that single commit restores it without affecting the rest of the PR.Design notes for review
is_negativevia arithmetic shift,is_ltvia the sign ofx - y) whose overflow reasoning is invalid for full-range unsigned values. The unsigned versions use the standard two's-complement mask identities:wrapping_subfrom a 0/1 bit, and MSB-extraction ofx | x.wrapping_neg()for the zero test.ltfrom their subtraction borrow chain and convert withfrom_msb; a word-level unsignedis_ltwould invite exactly the misuse the i64 trick warns about.const fn, matching the i64 style;black_boxhygiene stays where it already lives (the byte-level helpers), sinceblack_boxis not const-compatible and the existing i64 mask constructors don't use it either.Tests
Macro-generated test suites keep u64 and u32 at exact behavioural parity:
0, 1, MAX, MAX-1, 1 << (BITS-1)for every constructor, the values where leaked signed-representation reasoning would produce wrong masks.select(MAX, 0)must return exactlyMAXor0, proving the mask is all-ones/all-zeros through the public API (kills wrong-mask mutants).from_msbof a widening-subtraction borrow word agrees with the<operator over all boundary pairs.Verified additionally against a dual-width bigint prototype implementing the RSA phase-1 predicate/conditional set (
ct_is_zero,ct_eq,ct_ltvia sbb chain,is_odd,bit(i), select/assign/swap, conditional modular correction, masked table scan) — compiles and passes written-once againstCondition<Word>in both the native-u64 and forced-u32 lanes.Consumers
hex,base64,mlkem,mlkem-lowmemoryuse exclusivelyCondition<i64>constructors, untouched code paths.bouncycastle-utilstests: 69/69 pass. Quality stats: unwrap/Err()counts flat (491/260 core-code baseline).Pre-push verification:
cargo test --workspacepassedcargo mutantsovercrypto/utilscrypto/utils/src/secret.rs:293:9:replace<impl Drop for Secret<T>>::dropwith()Non-goals, considered and deferred
impl<T>" TODO — worthwhile, but a refactor of shared security-critical code beyond what this feature needs.Condition<u8>for the hex/base64 TODO — same direction as this work; can follow once the shape here is agreed.is_in_listcompiler-CT research question — the new code sidesteps it by using only the mask shapes.Observed during review, out of scope for this PR
While aligning the unsigned API with the existing
Condition<i64>surface, a few pre-existing inconsistencies were noted. They are listed here so reviewers know they were seen and deliberately left alone; each is small and could be a standalone cleanup:is_bit_set(value: i64, bit: i64)types its bit index asi64. The new unsigned variant usesu32, following core's shift-count convention. The signed one should eventually migrate, which is a breaking signature change.or_halvesis public but is an implementation detail. It returns a rawi64rather than aCondition, exists only to build is_zero, and has no callers outside this file. It should become private.is_lte,is_gte, andis_within_rangeare notconstbecause they use the non-constNotoperator onCondition. The unsignedis_zerosidesteps this by complementing the inner value instead; the same trick would make the signed trioconstwith no behavior change.negate's doc comment is a leftover bug-investigation narrative. It walks through a wrong result under the assumptionTRUE = 1and then notes the constant was changed; next toTRUE: Self = Self(-1)it now confuses more than it explains. It needs a rewrite stating the current contract.conditional_copy_bytestakestake_a: booland hand-builds its own mask instead of accepting aCondition, and the byte-helper naming (ct_eq_bytes) does not match the method vocabulary (is_equal). This is best revisited together with the existingCondition<u8>TODO, which would let the byte helpers consume proper masks.Most Condition<i64>items carry empty///doc comments, satisfyingforbid(missing_docs)in letter only. This PR filled inis_bit_setandis_negativewhere it added cross-references; the rest still need real text.Co-authored with Claude Code