Simple ciphers: stream + symmetric cipher unification off the release branch, plus SHA-512/t for any t

[dghgit]

Consolidates the stacked pair #113 (feature/stream-cipher → release/0.1.3alpha) and #115
(feature/symmetric-cipher → feature/stream-cipher) into a single branch off the release
branch, and adds the SHA-512/t work on top.

feature/simple-ciphers was built by branching from release/0.1.3alpha and merging
feature/stream-cipher then feature/symmetric-cipher; both merges fast-forwarded, so the
content below the two new commits is exactly what #113 and #115 already carried — this is a
re-packaging, not new review surface. #113 (+21,938) and #115 (+2,310) sum to roughly the
+23,499 here.

New in this PR, on top of those two

core: SecurityStrength::from_bits / from_bytes become const fn (3912434) — two
keywords, no behaviour change. Needed so an associated const can derive a strength from a const
generic; split out ahead of the feature per CLAUDE.md's scope-of-changes rule.

sha2: SHA512t<T> is usable for every t FIPS 180-4 s. 5.3.6 defines a hash for (785dbef).
Previously only T = 224 and T = 256 had parameter impls, so nothing else could be named.

• The t-value checks are back, and they are the spec's. b11f8f6 had narrowed sha512t_h0 to
  100 <= t < 512 and emitted a fixed three digits, since only 224 and 256 were reachable. With
  arbitrary t reachable that would produce the "0256" spelling s. 5.3.6 explicitly forbids —
  and hence the wrong IV — for every t below 100. The one- and two-digit branches are restored
  and check_t asserts the section's own rule: positive, < 512, not 384.
• One deviation, documented: t must be a multiple of 8, because Hash is byte-oriented and
  a 100-bit digest has no representation here. BC Java's SHA512tDigest imposes the identical
  restriction, so the two libraries accept the same set of truncations.
• Unapproved truncations are gated the way ElectronicCodeBook::ENCRYPTION_APPROVED gates
  two-key TDEA: SHA512tParams::<T>::FIPS_APPROVED is public and SHA512Internal::new asserts it
  in an inline const, so an unapproved t is a compile error at the call site and
  new_allow_unapproved_t() is the deliberate way in. That also blocks Default, which keeps an
  unapproved truncation out of generic code by accident.
• ALG_NAME, OUTPUT_LEN and MAX_SECURITY_STRENGTH are derived from t, with const
  assertions pinning them to the values 224 and 256 previously had by hand.

Verification

• cargo test --workspace: 943 passed, 0 failed (was 923). cargo fmt --check clean; no new
  clippy warnings. Both new commits build independently.
• Spec text read from a freshly downloaded FIPS 180-4, not from recall.
• Test vectors cross-checked against BC Java's SHA512tDigest, an independent implementation:
  eight truncations (8, 16, 24, 88, 96, 104, 264, 504) spanning all three decimal branches, over
  the FIPS 180-4 Appendix C messages plus the one-million-'a' case. The 224/256 rows in the same
  table match the NIST-published values.
• cargo mutants on the changed files: 259 mutants — 180 caught, 5 timeout-kills, 69 unviable,
  5 missed. All five missed are the pre-existing XOR/OR equivalences already annotated at their
  sites in ch, maj and do_final_internal; no new missed mutants.

Note for reviewers

Adding const to a public core function is a forward compatibility commitment, and
SHA512tParams is currently its only caller. The alternative was a private copy of the rounding
ladder inside sha2, free to drift from the real one — happy to switch if the API-surface cost is
the greater worry.

🤖 Generated with Claude Code

[ounsworth]

Maybe it's me, but I can't figure out what this sentence is trying to tell me.

Could you try wording it a different way?

[dghgit]

It say's it throws an error if it's asked to pad (so it's used with the arbitrary length construct to ensure the data is block aligned). Would you tell me what the point of confusion is?

[ounsworth]

The point of confusion is that I read the sentence 5 times and had no idea what it was trying to tell me. lol.

The sentence is:

NoPadding never writes a byte: asking it to is the error that tells the caller their data was not block-aligned, and a "padded" block is all data.

The implementation of NoPadding always returns an error. Calling it is an error. Period. That is its purpose. So what does "tells the caller their data was not block-aligned" mean? What does "and a "padded" block is all data" mean? I literally don't understand what that's trying to tell me.

Wouldn't it be clearer to replace this sentence with:

NoPadding is a padding mode that always fails, it exists to ...

but I'm struggling to understand the point of a function that always fails, so I don't even know how to finish the sentence.

[ounsworth]

I'm always a little bit leery of syntactic sugar when I'm not 100% sure that it does.
In this case, I'm leery about this creating a deep-copy under the hood (which would be bad on a GB-sized data.

This one seems ok, but just for the notes: the docs for as_chunks_mut weren't clear about whether this can ever result in a copy, so I had Fable weigh in and check the assembly. This seems ok, so I think that going forward, as_chunks is safe to use.

[ounsworth]

There's an asymmetry between how this constructor for the Encryptor works, and the anagous constructor of the Decryptor.

I think I prefer the pattern used in the Decryptor, so I'm going to make this match.

[ounsworth]

The fact that you have to do a "turbofish" here -- ie the <>:: pattern, is a bit of a code smell. It means that the PKCS7 struct is impl'ing more than one trait that defines a pad() function. I think we should avoid having multiple traits define functions with the same name, both because it's hard to read, and because if we do this in the public API, then we're begging for users to grab for the wrong one and end up confused.

Perhaps this should go into QUALITY_AND_STYLE.md ?

[ounsworth]

Oh, I see, the turbofish here is actually serving a different purpose: it's to bind K. But it turns out that the compiler can deduce that from context, so this line can be simplified to:

PKCS7::pad(&mut block, data_len).unwrap();

I think the point about potentially amending QUALITY_AND_STYLE.md to prefer not overloading functions might still be a good idea.

[ounsworth]

Note-to-self: remove the comments.

[ounsworth]

Struct names like Ctr are idiomatic rust, but in keeping with the library's QUALITY_AND_STYLE.md, these should be capitalized to match SP 800-38A, which would be ECB, CBC, CFB, CFB8, and CTR.

That just a refactor of the struct names, which my IDE can handle easily, but will affect dependent PRs.

[dghgit]

Sure.

[ounsworth]

Clever!

[ounsworth]

Aes128CcmShortTag seems like an odd example to put here since it doesn't exist in the AES crate, and I don't think it really adds to this tutorial about how to use the CCM<> struct to create type aliases.

Question: if Aes128CcmShortTag is a real thing that's used in the wild, then should it be defined in the AES crate?

[ounsworth]

Is this a good idea?
If ECB is really meant as an internal building block, and maybe a toy for students / researchers, but otherwise hazmat, then maybe we shouldn't expose it through the BlockCipherEncryptor trait so that people don't get the wrong idea?

I think I'm suggesting just deleting these impl's.

[dghgit]

It's a knotty one... FIPS still test for it, so I'd leave it in. It's not that the mode is inherently insecure (after all you could argue that every other block cipher mode is built on it!) it's more that you need to know what you're doing if you use it. It's probably also better to leave it in so there's a way of doing it - the alternative is we'll find people using ElectronicCodeBook directly (as they say, life will always find a way...), which would definitely suck!

[ounsworth]

But, like, it's not even a good fit for the BlockCipher API because that API forces it to return an IV, which for ECB will always be a zero-length array.

let (bytes_written, no_iv): (usize, [u8; 0]) = Aes128Ecb::<Encrypting>::encrypt(&key, &mut data).expect("encryption");
assert_eq!(no_iv.len(), 0, "EBC mode returns the IV as an empty array");
assert_eq!(data[..16], data[16..], "equal plaintext blocks give equal ciphertext blocks");

and on the decrypt side, you have an init() that has to be handed something that the compiler knows is the empty array.

    fn do_decrypt_init(
        key: &KeyMaterial<KEY_LEN>,
        _init_data: &[u8; 0],
    ) 

which you have to call like this:

Aes128Ecb::<Decrypting>::decrypt(&key, &[], &mut data)

That just feels clunky.

---

To me, we have two reasons not to impl BlockCipher on ECB:

1. It's misleading; we'll end up having to include AES_ECB, Aria_ECB, etc in the BlockCipherFactory, which is just wrong cause we really don't want people finding them and using them by accident.
2. ECB is a bad fit for the BlockCipher API anyway.

If someone really wants to use AES_ECB and knows what they're doing, they can use it through the ElectronicCodeBook trait, which is pub.

What's the advantage of also exposing an API via BlockCipher?
Is there some structural thing that needs ECB to be exposed as a clunky BlockCipher? Does anything break if we just delete it?

[ounsworth]

This impl block seems like it's basically duplicating all the functions of the SymmetricCipherEncryptor / Decryptor traits, but as inherents. I suggest that this should instead actually impl SymmetricCipher.