Skip to content

AEAD architecture: FAuthObj + multi-call GCM (split from PR #90 concern A) - #5

Open
omonien wants to merge 2 commits into
developmentfrom
package/aead-architecture
Open

AEAD architecture: FAuthObj + multi-call GCM (split from PR #90 concern A)#5
omonien wants to merge 2 commits into
developmentfrom
package/aead-architecture

Conversation

@omonien

@omonien omonien commented Jul 25, 2026

Copy link
Copy Markdown
Owner

AEAD architecture (PR MHumm#90 Concern A only)

Package A from the cleanup roadmap: architecture for authenticated modes, without ChaCha/Poly1305.

Donor: PR #90. GCM multi-call correctness matches PR MHumm#99 semantics (absorbed).

What changed

Area Change
Mode object Single FAuthObj: TAuthenticatedCipherModesBase instead of dual FGCM/FCCM
Public API IDECAuthenticatedCipher unchanged
Protected API EncodeGCM/DecodeGCM/EncodeCCM/DecodeCCM kept as wrappers
GCM Multi-call: GHASH partial + CTR keystream remainder + finalize in Done
Lifecycle Init → Encode/Decode* → Done → read tag; Done idempotent; post-Done Encode/Decode raise
CCM Still one-shot; ExpectedTag checked in TDECCipherModes.Done
InitMode Always free previous auth object (leak fix)

Not in this PR

Behaviour note

Call Done before reading CalculatedAuthenticationResult for multi-call GCM.

Tests (Delphi 13 Win32 Console DUnit)

  • GCM 19/19, CCM 16/16, non-AEAD modes green
  • Residual reds: pre-existing Keccak only

Design notes: Docs/plans/2026-07-25-aead-architecture.md

This branch lives on the fork so it can be opened against upstream MHumm/development when ready.


Note

High Risk
Changes core GCM/AEAD lifecycle (tag only valid after Done) and streaming crypto state; callers that read the tag or stream chunks without Done may break despite preserved public API names.

Overview
This PR refactors authenticated cipher modes around a single FAuthObj (TAuthenticatedCipherModesBase) instead of separate FGCM / FCCM, while keeping IDECAuthenticatedCipher and the protected EncodeGCM/DecodeGCM/EncodeCCM/DecodeCCM entry points as dispatch wrappers.

GCM no longer computes the tag inside each Encode/Decode. It now maintains streaming GHASH state (partial blocks, ciphertext length) and CTR keystream remainder across calls, materializes CalculatedAuthenticationTag in Done, blocks further Encode/Decode after finalize, and rejects late AAD or tag bit lengths above 128. TDECCipherModes.Done calls FAuthObj.Done before the expected-tag check; InitMode always frees the previous auth object on mode change.

The AEAD base gains a virtual Done, SetDataToAuthenticate, and documented multi-call lifecycle. Tests add multi-chunk GCM vectors, Done lifecycle cases, and fix the large 256-bit-key GCM vector tag in test data.

Reviewed by Cursor Bugbot for commit 713a102. Bugbot is set up for automated code reviews on this repo. Configure here.

@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 594243cd-c773-4360-a700-b8e4e7a37232

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch package/aead-architecture

Comment @coderabbitai help to get the list of available commands.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 47c7463. Configure here.

Comment thread Source/DECCipherModesGCM.pas
@augmentcode

augmentcode Bot commented Jul 25, 2026

Copy link
Copy Markdown
🤖 Augment PR Summary

Summary: Refactors AEAD handling to use a single authenticated-mode object and adds correct multi-call (streaming) GCM finalization semantics.

Changes:

  • Introduced a common AEAD base contract with a virtual Done finalization step (DECAuthenticatedCipherModesBase.pas)
  • Reworked TDECCipherModes to manage one FAuthObj instead of separate GCM/CCM instances, including leak-safe mode re-initialization
  • Implemented streaming GCM state (running GHASH, partial blocks, CTR keystream remainder) and tag materialization in TGCM.Done
  • Made post-Done GCM Encode/Decode calls raise until re-Init
  • Kept protected Encode/DecodeGCM/Encode/DecodeCCM entry points as wrappers to preserve API surface
  • Updated/expanded DUnit tests to cover multi-chunk GCM, Done idempotency, and post-Done rejection
  • Corrected a large GCM CAVS response tag vector

Technical Notes: Callers must call Done before reading CalculatedAuthenticationResult when using multi-call GCM streams; CCM remains effectively one-shot with tag computed during Encode/Decode.

🤖 Was this summary useful? React with 👍 or 👎

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 2 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread Source/DECCipherModesGCM.pas Outdated
Comment thread Source/DECCipherModes.pas Outdated
…m#99)

Extract Concern A from the ChaCha PR MHumm#90 donor without ChaCha/Poly1305.

- Polymorphic FAuthObj replaces dual FGCM/FCCM fields; public IDECAuthenticatedCipher unchanged
- Keep EncodeGCM/DecodeGCM/EncodeCCM/DecodeCCM as protected wrappers (no rename break)
- GCM multi-call: absorb PR MHumm#99 streaming (GHASH partial + CTR keystream remainder + Done/FFinalized)
- Reject PR MHumm#90 fIsLastBlock CTR model (incorrect for unaligned multi-chunk)
- Base Done virtual (no-op for CCM one-shot); TDECCipherModes.Done finalizes then verifies ExpectedTag
- InitMode always FreeAndNil(FAuthObj) before recreate (leak fix)
- Multi-chunk and Done-lifecycle unit tests; corrected large GCM rsp tag

See Docs/plans/2026-07-25-aead-architecture.md.
@omonien
omonien force-pushed the package/aead-architecture branch from 47c7463 to 58b54d1 Compare July 27, 2026 09:17
Reject DataToAuthenticate after GHASH has absorbed AAD or after Done; cap
AuthenticationTagBitLength at 128 bits with a defensive tag copy; keep
EncodeCCM/DecodeCCM as independent protected entry points that call FAuthObj
directly so EncodeGCM overrides no longer affect CCM.
@omonien

omonien commented Jul 27, 2026

Copy link
Copy Markdown
Owner Author

Addressed 2026-07-27 (fork cleanup pass)

Linear stack restored (A → B → C) and Cursor Bugbot / Augment findings handled:

Area Fix
GCM AAD after Encode/Done rejected (sGCMAADLocked)
GCM tag length > 128 EDECAuthLengthException + defensive copy
EncodeCCM / DecodeCCM independent FAuthObj dispatch (no longer forwards through EncodeGCM)
Poly1305 + block ciphers stream-only (BlockSize < 16)
ChaCha Poly init FBufferSize restored to Context.BufferSize
Win32 AVX XMM save/restore on X86ASM
AES-256 AES-NI schedule no +240 encode key; decode at (FRounds+1)*Blocks

Tests (D13 Win32 Console): GCM 21/21, ChaCha20Poly1305 12/12, AESNI 7/7. Pre-existing Keccak reds only.

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