Fix #86: bind the HRBC2 field AAD with the canonical ':' separator - #87
Merged
Conversation
The CLI built the field AAD as utf8(recordID + fieldName) while web, macOS/iOS, Android and Windows all use utf8(recordID + ":" + fieldName). The ciphertext matched but the GCM tag did not, so every note the CLI sealed failed authentication on every other client, and vice versa. Verified against the cross-client known-answer vector pinned in harbor-swift: Go reproduces it exactly once the separator is present. - crypto.go: add the separator, and document that the byte sequence is an interop contract rather than a local choice. - crypto_test.go: sealRef reproduced the same wrong AAD as the code under test, so the suite guarded the bug instead of catching it. Split out sealRefAAD, which takes the AAD explicitly, and pin the shared vector as a hardcoded literal. Adds a negative test so a lost separator fails loudly. - README.md: the documented rule said 'no separator' - corrected, with a pointer to the canonical web definition. Also updates the burndown skill: delegate to subagents to keep the parent context light, and append a TL;DR whenever the run pauses for input.
The package doc still described the pre-fix AAD as 'no separator' — the same class of wrong-documentation the issue names as a root cause, and the more authoritative of the two docs since it is what 'go doc' prints. - crypto.go: correct the package doc; point fieldAAD at the right pinned literal; add sealFieldWithNonce so tests can reproduce the shared vector byte for byte, matching how harbor-swift and harbor-windows do it. - crypto_test.go: TestSealField_CrossClientVector now asserts the sealed envelope EQUALS the literal rather than merely decrypting under the canonical AAD, which satisfies the acceptance criterion literally. The random-nonce path keeps its own test. Pin the legacy no-colon envelope as a second literal and use the vector's nonce, so the negative test's inequality guard can actually fire — it could not before. - README.md: name the tests that pin the vector correctly.
Closed
4 tasks
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.
Closes #86
The CLI built the HRBC2 field AAD as
utf8(recordID + fieldName). Web, macOS/iOS, Android and Windows all useutf8(recordID + ":" + fieldName). One character, and it meant no note sealed by the CLI could be opened by any other Harbor client, and no note sealed elsewhere could be opened by the CLI.The fix
crypto/crypto.go— add the separator, and say plainly in the doc comment that this byte sequence is a cross-client contract, not a local choice:crypto/README.mddocumented the wrong rule as if it were the contract ("id first, no separator"). Corrected, with a pointer to the canonical web definition.Why the test suite did not catch it
crypto/crypto_test.go'ssealRefhelper — described as an independent reference implementation "so the tests prove OpenField conforms to the external contract" — built its expected envelope with[]byte(id+field), the same mistake the production code made. It agreed with the bug, soTestOpenField_ReferenceVectorpassed while being wrong.So the test change is the substance of this PR, not an afterthought:
sealRefAADtakes the AAD explicitly instead of deriving it. It can build a deliberately wrong envelope, which is what the negative tests need, and it can no longer silently inherit a mistake from the code under test.TestFieldAAD_RejectsLegacySeparatorlessproves the old format fails both directions, so a "simplified"fieldAADbreaks the build instead of shipping.Verification
The new tests fail without the fix. Reintroducing the missing separator (verbatim
go test ./crypto/ -count=1):Against the vector shared with harbor-swift (key
00..1f, nonce00..0b, idnote-1, fieldcontent, plaintextSecret note body):Only the GCM tag differed, which is why the failure mode was a clean "could not decrypt" rather than corrupted output.
Full suite:
End-to-end against a real server, using a throwaway account on a disposable test environment. Created a note, encrypted it with the built binary, then fetched the raw stored record and decrypted it with a standalone Python script that imports none of this repo's code:
Independent decryption of those stored bytes:
Same result for the
titlefield. And the reverse direction — the CLI reading the canonical format — is covered by the twoCrossClienttests above.Notes
burndownskill (delegate to subagents to keep the parent context light; append a TL;DR whenever the run pauses). Unrelated to the fix, bundled at Spicer's request.Incidental issues found while testing
Not fixed here, and none affect this change — flagging so they can be filed if wanted:
harbor whoamiignoresHARBOR_TOKENand reports "not logged in" even when the token works for every other command.harbor crypto setupcannot run underHARBOR_TOKENalone — the synthesized credentials have an emptydevice_id, whichsync/pushrejects. Blocks headless encryption setup.