feat: add keyed hash providers for message authentication - #19
Merged
Conversation
…on [patch] Records three things the 2026-08-19 spec leaves open: that issue #5 now ships without the AEAD half, how the three HMAC providers share one linked core rather than duplicating against each other, and why that matters now that main fails its duplication gate at 6.8%. The interface design of record is unchanged. Adds a status note to the earlier spec so its superseded Delivery section does not read as current.
Seven tasks, each ending in an independently testable deliverable: the fixed-time comparison helper, the interface and its buffering default, the shared HMAC core with HMAC-SHA-256, the two remaining algorithms, DI wiring, the encryption documentation, and the README and CLAUDE.md updates. Known-answer vectors come from RFC 4231 cases 1, 2, and 6. Case 6 uses a 131-byte key, which forces the hash-the-key-first path that round-trip tests cannot reach.
… suppression [patch]
Document IKeyedHashProvider and FixedTimeComparison in the README and CLAUDE.md, covering the three HMAC-SHA256/384/512 packages, the confidentiality-only nature of IEncryptionProvider, and a usage example pairing encryption with authentication. Also note in IIncrementalHash's remarks that IKeyedHashProvider.CreateIncremental returns one too.
Six corrections the review loop surfaced, recorded so the plan matches what shipped rather than what was originally written: - default interface members are not callable through a concrete-typed local, which invalidated 25 test sites across tasks 2 to 4 - CA2007 requires ConfigureAwait(false) on every test await, 4 sites - line endings are LF by .gitattributes, not CRLF as the global CLAUDE.md says - bytesWritten must be reset on the digest-length mismatch exit, 2 sites - the AddKeyedHashProviders XML summary must say 'bundled' like its 11 siblings - the authentication advice must cover the IV and not only the ciphertext, since IEncryptionProvider takes the IV as a separate parameter and CBC recovers the first plaintext block as IV XOR D(C0) Also corrects an unfollowable instruction to edit project names inside a .csproj that contains none.
…ashing example [patch] The Keyed Hashing README example authenticated the ciphertext alone, contradicting IEncryptionProvider's own remarks that an unauthenticated IV lets an attacker rewrite it and corrupt the first decrypted block undetected. Compute and verify the tag over the IV and ciphertext together instead, and reword the IEncryptionProvider cross-reference note to match.
Renames FixedTimeComparison.Equals to FixedTimeEquals, because a static class inherits object.Equals(object, object) and the old name silently accepted Memory<byte>/ReadOnlyMemory<byte> arguments that never reached the fixed-time path, returning false for byte-identical tags. Adds RFC 4231 coverage for the HmacSha384 and HmacSha512 stream and CreateIncremental paths, and extends the pooled-buffer scrubbing tests to the two keyed hashing call sites the branch introduced. Refreshes DESCRIPTION.md and TAGS.md with keyed hashing, HMAC, and message authentication so the new packages are discoverable on NuGet. Also documents FixedTimeComparison in the README API reference and its incremental-authentication escape hatch, extends the async-path inventories in README.md and CLAUDE.md to mention keyed hash stream hashing, documents key material expectations on IKeyedHashProvider, and drops a qualifier on AesEncryptionProvider that contradicted IEncryptionProvider's unconditional authentication requirement.
|
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.
Closes #5.
A caller can now compute and verify a message authentication code without leaving Essentials. Before this, every hash provider was unkeyed, so authenticating ciphertext meant dropping to the base class library, which is what GitLfsCache had to do.
What ships
IKeyedHashProviderin the interfaces package, mirroringIHashProvidermember for member with the key prepended. Three abstract members, eight defaults, so an implementer writes two methods:Three packages, one per algorithm, matching the convention the 15 unkeyed hash providers already follow:
ktsu.Essentials.KeyedHashProviders.HmacSha256ktsu.Essentials.KeyedHashProviders.HmacSha384ktsu.Essentials.KeyedHashProviders.HmacSha512Verify(key, data, expectedTag)computes and compares in one step, so the caller never writes a tag comparison.FixedTimeComparison.FixedTimeEqualsis public for callers holding a tag obtained elsewhere.HMAC-MD5 and HMAC-SHA1 are not shipped. They are not broken as MACs, but putting them in a new security-facing category invites misuse, and a consumer who needs one implements the two primitives.
The other half of the issue
Issue #5 is as much about an expectation gap as a missing algorithm:
IEncryptionProvidergives confidentiality but not integrity, and nothing in its surface said so. It now does, and so doesAesEncryptionProvider, which names CBC with PKCS7, ciphertext malleability, and the padding oracle a decrypt-then-parse caller becomes.That documentation says to authenticate the initialization vector and the ciphertext together, not the ciphertext alone. The interface takes the IV as a separate parameter, and CBC recovers the first plaintext block as
IV XOR D(C0), so a tag over the ciphertext alone leaves that block rewritable. Review caught this twice, once in the XML docs and once in the README example.Sharing, not duplicating
Shared/HmacKeyedHashCore.csisinternaland linked into all three provider projects, followingShared/NonCryptoIncrementalHash.cs. It has to beinternal: each package compiles its own copy, so a public type would collide for anyone referencing two of them.This was deliberate.
maincurrently fails its SonarCloud duplication gate at 6.8%, and the largest contributor is the FNV cluster, four 168-line files differing in two lines of logic. Three HMAC providers written independently would have produced the same shape. Each one is instead about 25 lines of distinct content.Testing
636 tests passing, 37 new. Correctness is pinned by RFC 4231 known-answer vectors (cases 1, 2, and 6) for all three algorithms rather than round trips, so interoperability is proven and not merely self-consistency. Case 6 uses a 131-byte key, which forces the hash-the-key-first path that round-tripping cannot reach. There is also an independent cross-check against the BCL's own
HMACSHA256.Beyond that: agreement across all four paths (one-shot, stream, incremental, async), tamper and wrong-key rejection, the buffer-length contract, pooled-buffer scrubbing at both new sites, and DI resolution.
Key material is copied and zeroed in a
finallyon every path including exceptional ones, and pooled read buffers are returned withclearArray: true.Risk
Purely additive, so
[minor]. New types, new packages, one new registration call inAddEssentials(). No existing signature changes and no behavior changes to existing providers. Builds clean across all six target frameworks with no conditional compilation, since every API used exists on netstandard2.1.Follow-ups deliberately not in this PR
IAuthenticatedEncryptionProviderand AES-GCM. Part 2 of the 2026-08-19 spec designs them, and they need a tag in the surface, which is a separate conversation from keyed hashing.ProviderContractTests, so it gets the same uniform contract coverage as the 15 unkeyed providers.