Skip to content

Part Nirvana: Grand unification of stream and block encryptors under SymmetricCipherEncryptor/Decryptor, AES clean up - #115

Open
dghgit wants to merge 14 commits into
feature/stream-cipherfrom
feature/symmetric-cipher
Open

Part Nirvana: Grand unification of stream and block encryptors under SymmetricCipherEncryptor/Decryptor, AES clean up#115
dghgit wants to merge 14 commits into
feature/stream-cipherfrom
feature/symmetric-cipher

Conversation

@dghgit

@dghgit dghgit commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Issue Link

No linked issue.

Summary

Brings every cipher in the workspace to one arbitrary-length API: the block modes reach
SymmetricCipherEncryptor / SymmetricCipherDecryptor through the padding adapters, the stream
modes implement them directly, and the old one-shot-only SymmetricCipher trait is deleted.

Description

Stacked on #113, so this PR is based on feature/stream-cipher and shows six commits:

Commit
7f465f4 aes-lowmemory: AES_CBC_* and AES_ECB_* take a padding scheme as well as a direction
4f01a90 core: stream ciphers also implement SymmetricCipherEncryptor / SymmetricCipherDecryptor
1f2a392 core: SymmetricCipher deleted, its one-shots moved onto AEADCipher
6b91279 aes: bouncycastle-aes-lowmemory renamed to bouncycastle-aes
6c47625 aes: Block, PaddedMode and the AesParams types dropped from the public API
521efdc aes: ElectronicCodeBook is the only public route to the permutation

The problem. Five modes had four different front doors. Cbc and Ecb were block-aligned and
in-place; Cfb, Cfb8 and Ctr were stream ciphers taking any length; the padding adapters
presented a third shape; and SymmetricCipher sat over the lot as a one-shot-only trait that
nothing implemented. Code that wanted to accept "a symmetric cipher" had no single trait to name.

The block modes name their padding. AES_CBC_128<Dir> becomes
AES_CBC_128<Dir, Pad>, and likewise for ECB, so a caller writes
AES_CBC_128<Encrypting, PKCS7> or AES_ECB_256<Decrypting, NoPadding>. CBC and ECB are defined
only on whole blocks (SP 800-38A Sec 5.2), so on real data they are always mode plus scheme, and
the scheme changes the ciphertext and must be agreed by both ends. Putting it in the type makes a
mismatched pair a compile error instead of a decryption that returns plausible rubbish; there is a
test that a PKCS#7 ciphertext read back as NoPadding "succeeds" with the wrong answer, which is
the failure this prevents.

PaddedEncryptor and PaddedDecryptor are distinct types, so a plain alias cannot select between
them on Dir. A trait, PaddedMode, is implemented for the two direction markers and
the aliases project through it. It is crate-internal: a caller writes AES_CBC_128<Encrypting, PKCS7> without ever naming it, and the projection resolves without the trait being reachable. One trait serves both modes: CBC passes INIT_DATA_LEN = BLOCK_LEN,
ECB passes 0, and a future block mode needs an alias rather than another trait.

The stream modes gain the same traits, by two blanket impls in core with FINAL_LEN = 0, written
over the in-place methods. An implementor still writes only do_encrypt / do_decrypt, and a future
OFB gets the wider API for free. For a stream cipher update_out_len and encrypt_out_len are the
identity, decrypt_out_max_len is exact rather than an upper bound, and do_final has nothing to
produce.

SymmetricCipher is deleted. Its four one-shots move to AEADCipher, which was its only
remaining user, so AEADCipher drops the supertrait and declares them itself against NONCE_LEN.
This closes the todo that sat above the trait asking for exactly this. Their documentation is
rewritten for the AEAD case: no additional authenticated data, and a ciphertext layout that is the
implementation's business, because the tag has to go somewhere.

Alternatives considered.

  • Adding padded aliases beside the raw ones rather than changing them, and renaming both for
    clarity. Folding the scheme into the existing name was chosen so the short name is the one that
    handles real data; the block-aligned API is still reachable as modes::Cbc / modes::Ecb.
  • Hand-written impls per stream mode instead of blanket impls. Blanket wins on not repeating the
    same six methods three times and on covering future modes.

Scope and Risk

Packages impacted: bouncycastle-core (blanket impls, SymmetricCipher deleted, AEADCipher
gains four methods), bouncycastle-core-test-framework (a suite moves, guards added),
bouncycastle-aes (alias shapes, new bouncycastle-padding dependency).

Breaking changes, all compile-time:

  • AES_CBC_* and AES_ECB_* take two parameters, and are the arbitrary-length API rather than the
    block one. Existing uses do not compile. modes::Cbc and modes::Ecb are unchanged for callers
    who want block-aligned, in-place, compile-time-checked lengths.
  • SymmetricCipher no longer exists. Nothing in the workspace implemented it.
  • A mode now offers do_encrypt_init through two traits with identical signatures, so code with
    both in scope must qualify the call. Nothing in the workspace hit this; the new test file is
    written that way on purpose to show it is workable.

Likelihood of regression: low. No cipher's ciphertext changes: the modes themselves are
untouched, and the padding layer and the blanket impls are wiring over existing, tested code. The
1853 ACVP CTR, 2138 CFB128, 2138 CFB8 and 2150 CBC vectors all still pass unchanged.

Worst case: the blanket impls sit on the path of every stream-cipher call made through the wider
trait, so a mistake there would be a wrong-ciphertext bug rather than a compile error. That is why
the new tests check the separate-output API against the in-place one byte for byte, and why all
three stream modes now run the same conformance suite the padded adapters run.

Validation

cargo test --workspace          # 889 tests
cargo fmt --all --check
RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace

Seventeen new tests: ten on the aliases, seven on the stream modes through the wider traits.

Mutation testing needs --test-workspace for anything in core, because core has no implementors
of its own traits and a plain -p bouncycastle-core run reports long-standing, well-tested methods
as missed. Scoped to the blanket impls and run that way:

cargo mutants -p bouncycastle-core --test-workspace true --in-diff <the diff>
# 45 mutants: 22 caught, 19 unviable, 4 missed

The four missed are one equivalent mutant repeated, [] against [0; 0] and [1; 0] for a
zero-length array, which are the same value; both sites carry a comment saying so. The run also
found a genuinely uncovered mutant, the decryptor's output-buffer length comparison, which now has a
test. The SymmetricCipher removal has no executable code to mutate.

Also fixed, because this PR moved the code: two security-strength loops in the test framework
unwrapped set_security_strength at all five strengths, which a key shorter than 32 bytes cannot
carry, so they would have panicked for the first AEADCipher implementor — ASCON-128 and
AES-128-GCM among them. This was recorded as outstanding in core-test-framework/summary.md.
Relocating one of them into a method the AEAD suite calls would have made it worse, so both now
carry the same key-length guard the block and stream suites already had. Every strength loop in the
file is guarded, and the summary is updated.

The AES crate: renamed, and its public API narrowed

Three follow-up commits, added after review of the crate's surface.

Renamed bouncycastle-aes-lowmemory to bouncycastle-aes: directory, package, umbrella
re-export (bouncycastle::aes) and criterion group prefix. There is one AES in the workspace, so
the qualifier described a property rather than distinguishing the crate from anything, and the
low-memory design it referred to is unchanged. Pure substitution, 108 lines each way. The crate has
not been published under the old name, so the release notes simply introduce it under the new one.

Six items left the public APIBlock, PaddedMode, AesParams, Aes128Params,
Aes192Params, Aes256Params — after checking every import of the crate across the workspace.
Nothing outside crypto/aes/src/ names any of them. They stay pub inside private modules rather
than becoming pub(crate): the latter makes the private_interfaces lint fire on
pub type Aes128 = Aes<Aes128Params> and on all fifteen mode aliases, and that lint is worth
keeping. Unreachable-pub removes them from rustdoc with a clean build.

ElectronicCodeBook is now the only public route to the permutation. Aes carried
encrypt_block, decrypt_block, encrypt_blocks2, decrypt_blocks2 and three new methods as
inherent pub fns that the trait impls duplicated exactly — the impls were one-line delegations, so
there were two names for every operation and the inherent one shadowed the trait at the call site.
All seven are now pub(crate). Because the names match, method resolution falls through to the
trait: the ~40 call sites in the ACVP, FIPS 197 and SP 800-38A suites, the benches and the
stack-memory harness are unchanged, and five files gained a use line. With nothing public left on
it, Aes itself is no longer exported; type.Aes128.html carries the full trait documentation, so
nothing became callable-but-undiscoverable.

The crate's public API is now twenty items: three engine aliases, fifteen mode aliases, and
BLOCK_LEN / CTR_NONCE_LEN.

Verified at each commit: 889 tests pass, the workspace builds with --all-targets and no warnings,
cargo fmt --all --check is clean, rustdoc is clean under -D warnings, and aes128-ecb through
the CLI matches OpenSSL byte for byte. unwrap and Err() counts are unchanged.

AI Usage Statement

Did you use AI in creating this pull request:

  • No
  • Yes, indirectly - no submitted code was generated by AI (e.g., answering questions, performing a review, suggestions, etc.)
  • Yes, trivial code changes were generated by AI (e.g., autocompletion of a single line, reformatting, or spell-checking)
  • Yes, non-trivial code changes were generated by AI

If submitted code changes were generated by AI, fill in the following declaration:
Assisted-by: Claude Code:claude-fable-5-1
Assisted-by: Claude Code:claude-opus-5

@dghgit dghgit changed the title Part Nirvana: Grand unification of stream and block encryptors under SymmetricCipherEncryptor/Decryptor Part Nirvana: Grand unification of stream and block encryptors under SymmetricCipherEncryptor/Decryptor, AES clean up Sep 7, 2026
@hubot
hubot force-pushed the feature/stream-cipher branch from 93ee992 to 0404ab9 Compare September 8, 2026 03:56
@dghgit
dghgit force-pushed the feature/symmetric-cipher branch from 521efdc to ca66411 Compare September 8, 2026 03:59
@dghgit
dghgit force-pushed the feature/stream-cipher branch from 0404ab9 to 4adbebb Compare September 9, 2026 01:25
…eme as well as a direction, via a PaddedMode projection, so the two block modes name their padding in the type
…ricCipherDecryptor with FINAL_LEN = 0, by blanket impls over the in-place methods, so any of the five modes can be held through one trait
…to AEADCipher, its only remaining user; the framework suite follows, and both AEAD security-strength loops gain the key-length guard the other suites already had
@dghgit
dghgit force-pushed the feature/symmetric-cipher branch from ca66411 to 8f932ca Compare September 9, 2026 02:19
…128 / AES_192 / AES_256, AESParams and Rcon, and the sealing supertrait becomes AESParamsInternalTrait after the mlkem pattern (from Mike Ounsworth's 736b0ac review of #105); QUALITY_AND_STYLE.md records the spec-capitalisation exception to the clippy naming rules
…_2blocks (were *_blocks2), the reading Mike Ounsworth gave them in 736b0ac; modes, aes, the framework suite, benches and notes follow
… a debug self-check that sub_word's eight broadcast planes agree, in-place benches with decrypt paths for every key length and key-expansion throughput, summary.md removed, and acvp_tests.rs becomes bc-test-data.rs; the rest of Mike Ounsworth's 736b0ac review that still applied
…decrypt_8blocks (were *_blocks8), so they read like the pair methods; modes, the framework suite, benches and notes follow
…pt_4blocks (was eight): AES fills a pair and the u16/u32-plane engines fill four, so eight was two passes for every engine and left a four-lane engine half-empty on a 4-to-7-block tail; modes chunk fours, then pairs, then singles, the framework suite and the rotated-four toy pin the four path, benches and notes follow
…ipherEncryptor / SimpleCipherDecryptor; the framework suite becomes TestFrameworkSimpleCipher and the modes API test file is renamed to match; aes, padding, modes and the notes follow
@dghgit

dghgit commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Merge issue with of AES "Commit 736b0ac 2026-09-06 Mike Ounsworth MikeO adjustments to aes-lowmemory while
reviewing #105"

The doc trimming across lib.rs, aes.rs, bitslice.rs, schedule.rs and the memory bench. It removes the spec-correspondence rationale that CLAUDE.md asks for." Checkout feature/symmetric-cipher, do the edit with claude and get it to update CLAUDE.md appropriately, I can then apply it across tdes, sm4, aria, and camellia as well.

Suggest looking in lib.rs/aes.rs and identifying what shouldn't be there and just telling claude to remove it. Get it update CLAUDE.md, check one the other files, tell claude to clean it up, assuming all goes well it will have removed the excess documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant