Mark new symmetric output with a format marker and verify the key id during decryption - #40
Merged
Merged
Conversation
…during decryption Values written by `SymmetricKeyEncryption` now carry a marker between the key id and the encrypted part, `SymV1` from `encrypt()` and `SymAdV1` from `encryptWithAd()`, just like the two public-key classes have done since they were added. Feeding a value to the wrong class or the wrong method fails with an exception that says which class and method created the value and what to call instead. These messages are shared by all three classes, so they name the class together with the method: the value may come from another method of the very class the caller is using, and for that case the previous `decrypt it there` pointed nowhere. The key id and the marker go into what the decryption verifies, so changing either of them in a stored value makes decryption fail; so do rewriting a marked value into the older format, forging a marker onto an old value, and swapping the two markers. The "key id is not protected against tampering" caveat now applies only to values in the older format and to `AnonymousPublicKeyEncryption`, where a sealed value has no place to verify anything extra. Values in the older format without the marker, written by previous versions, keep decrypting. `needsReEncrypt()` returns true for them, so the usual re-encryption sweep migrates them to the marked format as a side effect; values created by `encryptWithAd()` have to be re-encrypted with the row's additional data, which the README now spells out. This is the breaking part: after the upgrade, `needsReEncrypt()` reports all previously written data, and versions without marker support cannot read the marked values, so deployments that share data have to be upgraded together before writing anything new. Every class now reads both formats, so the internals lose the now-dead code. The parser and formatter that accepted only the old format are gone from the `KeyEnvelope` trait, their replacements take over the plain names, and the `StoredFormat` enum is gone too: it existed to tell two format-error messages apart, and the one message left names both accepted shapes. `checkFormatMarker()` now returns the validated marker and the decryption paths build the verified value from it instead of repeating the constant they expect: the same thing today, but only the returned marker stays correct when a method someday accepts more than one marker. And because a corrupted value that happens to split into four parts is likelier than a version skew, the unknown-marker message now asks whether the data is corrupted, or encrypted by a newer version. The stored ciphertext fixtures from the previous releases stay in the tests verbatim and now double as the old-format compatibility proof, next to new pinned fixtures for the marked format. One test also rebuilds the verified value exactly as the README describes and decrypts a stored fixture with Halite directly, so the recipe can never change unnoticed, and its wrong-marker attempt proves that changing just the marker makes the verification fail.
There was a problem hiding this comment.
Pull request overview
This PR updates SymmetricKeyEncryption’s stored ciphertext format to include an explicit format marker (SymV1 / SymAdV1) and binds the key id + marker (and optionally user additional data) into what Halite verifies during decryption, improving misuse detection and tamper resistance while preserving legacy (unmarked) decrypt support.
Changes:
- Introduces symmetric format markers and switches symmetric encryption to
encryptWithAD()with a JSON “bound data” value that includeskeyId,marker, and optionaladditionalData. - Updates shared parsing/formatting in
KeyEnvelopeto accept both legacy ($keyId$ciphertext) and marked ($keyId$marker$ciphertext) shapes, and removes the now-unneededStoredFormatenum. - Expands/updates tests and README to cover the new format, legacy compatibility, marker mismatch diagnostics, and a pinned “Halite-direct decryption recipe”.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/SymmetricKeyEncryptionTest.phpt | Adds fixtures and extensive coverage for marked vs legacy formats, mismatch errors, and bound-additional-data verification. |
| tests/AuthenticatedPublicKeyEncryptionTest.phpt | Updates expected mismatch/unknown-marker error messages and adds symmetric-marker mismatch coverage. |
| tests/AnonymousPublicKeyEncryptionTest.phpt | Updates expected mismatch/unknown-marker error messages and adds symmetric-marker mismatch coverage. |
| src/SymmetricKeyEncryption.php | Implements symmetric markers, binds key id + marker into verified data, and preserves legacy decrypt paths. |
| src/Format/StoredFormat.php | Removes the internal enum used to distinguish old format-error messaging. |
| src/Format/KeyEnvelope.php | Replaces plain-only parsing/formatting with dual-format parsing and a single marked formatter; returns validated markers. |
| src/Format/FormatMarker.php | Adds SymV1 and SymAdV1 marker constants. |
| src/Exceptions/UnknownFormatMarkerException.php | Improves the unknown-marker message to also suggest corruption as a cause. |
| src/Exceptions/InvalidCipherTextFormatException.php | Simplifies to one message that names both accepted shapes. |
| src/Exceptions/FormatMarkerMismatchException.php | Extends mismatch mapping to symmetric markers and standardizes messages to name the correct decrypt method fully. |
| src/AuthenticatedPublicKeyEncryption.php | Adapts to the unified parseKeyCipherText() / formatKeyCipherText() and uses the validated marker returned by checkFormatMarker(). |
| src/AnonymousPublicKeyEncryption.php | Adapts to the unified parseKeyCipherText() / formatKeyCipherText(). |
| README.md | Documents the new symmetric marker format, migration/upgrade implications, and the bound-additional-data recipe. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Values written by
SymmetricKeyEncryptionnow carry a marker between the key id and the encrypted part,SymV1fromencrypt()andSymAdV1fromencryptWithAd(), just like the two public-key classes have done since they were added. Feeding a value to the wrong class or the wrong method fails with an exception that says which class and method created the value and what to call instead. These messages are shared by all three classes, so they name the class together with the method: the value may come from another method of the very class the caller is using, and for that case the previousdecrypt it therepointed nowhere. The key id and the marker go into what the decryption verifies, so changing either of them in a stored value makes decryption fail; so do rewriting a marked value into the older format, forging a marker onto an old value, and swapping the two markers. The "key id is not protected against tampering" caveat now applies only to values in the older format and toAnonymousPublicKeyEncryption, where a sealed value has no place to verify anything extra.Values in the older format without the marker, written by previous versions, keep decrypting.
needsReEncrypt()returns true for them, so the usual re-encryption sweep migrates them to the marked format as a side effect; values created byencryptWithAd()have to be re-encrypted with the row's additional data, which the README now spells out. This is the breaking part: after the upgrade,needsReEncrypt()reports all previously written data, and versions without marker support cannot read the marked values, so deployments that share data have to be upgraded together before writing anything new.Every class now reads both formats, so the internals lose the now-dead code. The parser and formatter that accepted only the old format are gone from the
KeyEnvelopetrait, their replacements take over the plain names, and theStoredFormatenum is gone too: it existed to tell two format-error messages apart, and the one message left names both accepted shapes.checkFormatMarker()now returns the validated marker and the decryption paths build the verified value from it instead of repeating the constant they expect: the same thing today, but only the returned marker stays correct when a method someday accepts more than one marker. And because a corrupted value that happens to split into four parts is likelier than a version skew, the unknown-marker message now asks whether the data is corrupted, or encrypted by a newer version.The stored ciphertext fixtures from the previous releases stay in the tests verbatim and now double as the old-format compatibility proof, next to new pinned fixtures for the marked format. One test also rebuilds the verified value exactly as the README describes and decrypts a stored fixture with Halite directly, so the recipe can never change unnoticed, and its wrong-marker attempt proves that changing just the marker makes the verification fail.