fix(identity): derive the enrollment ID of a boolpolicy identity from its members - #2146
Conversation
a52cc93 to
5db7643
Compare
|
Thanks a lot for finding and solving this issues. Great work. Thanks a million. Policy identities (boolpolicy) always returned an empty enrollment ID, so policy-owned inputs and outputs lost their ownership attribution and eid-based accounting either dropped or misbooked those amounts. The fix resolves each component's audit info recursively through the multiplexer and returns the enrollment ID that all components share. If a component has none, or they disagree, the result is "". If a component can't be resolved or its audit info is malformed, it's an error. My concern is the missing case, not the malformed one. An empty component audit info is legal today: WrapAuditInfo accepts empty entries, and SignatureService.GetAuditInfo returns nil for an identity that isn't registered locally. With this change that empty entry fails resolution, and token/request.go turns the failure into a rejected audit record — so a transaction that used to audit fine with eid="" now fails. @adecaro Thanks a lot, |
|
Thanks Akram, this is a valid compatibility concern. You are right that the current implementation conflates two different cases: a malformed outer audit-info with the wrong number of component entries, and a correctly shaped policy audit-info containing an empty component entry. The latter is legal in the existing flow: GetAuditInfo may return nil for an identity that is not registered locally, and WrapAuditInfo preserves that empty entry. Before this PR, such a policy resolved to an empty enrollment ID without failing the audit path. I will preserve that behavior by treating an empty component audit-info as “no enrollment ID available”, while continuing to resolve all remaining non-empty components so that a later malformed entry is not masked. Count mismatches and non-empty but unresolvable audit infos will remain errors. I’ll add regression tests for the empty-entry case and for an empty entry followed by malformed data. |
There was a problem hiding this comment.
Thanks for the fixes that you added the empty-component case reads correctly now, and the three new regression tests cover it.
I left three comments while reading the rest. The first is the only one I'd like to discuss with you to see if we need to resolve it before merging; the other two are minor.
Regards,
Akram
… its members A policy identity's AuditInfo always reported an empty enrollment ID, so policy-owned inputs and outputs reach the auditor with no enrollment attribution and downstream eid-keyed accounting misbooks them. Resolve each component's audit info through the parent multiplex deserializer (same recursive pattern as htlc.NewAuditDeserializer) and report the enrollment ID shared by all components. A component with no enrollment ID of its own (e.g. a nested composite spanning enrollments) or disagreeing components yield the legacy empty value. Malformed audit info -- an unresolvable component or a component count mismatch -- is an error rather than a silent empty value, and every component is resolved before the result is declared so later corruption is never masked. Add a fuzz target for the new recursive deserialization path and wire it into the nightly fuzz workflow. Signed-off-by: Evan <evanyan@sign.global>
… EID An empty component audit info is legal: WrapAuditInfo accepts empty entries and audit info lookup returns nil for identities not registered locally. Resolve such components to "no enrollment ID" instead of failing, so transactions that audited fine with an empty enrollment ID keep doing so. Errors remain for malformed data: a non-empty component that cannot be resolved or a component count mismatch, and a missing component does not mask corruption in a later one. Signed-off-by: Evan <evanyan@sign.global>
…info commonEnrollmentID accepted wire-supplied component identities without the validation both siblings in the file apply, so a policy carrying a duplicate or empty component still yielded a derived enrollment ID. The audit path never runs the verifier that would reject it: Audit reaches AuditRecord with verifyActions false, and auditor.Service.Validate, the only entry point that calls IsValid, has no caller. Such an identity is therefore attributed in movements and transaction records and only rejected later at commit. Also skip a component whose inner deserializer reports neither audit info nor error instead of dereferencing it. That is unreachable through the real multiplexer, but NewAuditInfoDeserializer takes an exported interface, so an implementation returning (nil, nil) turns a caller-visible error into a panic. Signed-off-by: Evan <evanyan@sign.global>
…ype resolution The boolpolicy enrollment-ID note said "an unresolvable component" is an error without saying which rule wins when a component is both of an unknown identity type and carries no audit info. The exemption is applied before the member's type is resolved, so that case yields an empty enrollment ID. Say so, and pin the contract with a test alongside the existing unknown-type-with-audit-info error case. Signed-off-by: Evan <evanyan@sign.global>
4988ad0 to
77c3dba
Compare
|
Thanks a lot for the great work you did on this PR. I really appreciate it! Best reagards, SummaryThis PR fixes an issue where boolean-policy identities were missing their enrollment ID during auditing, which could lead to incorrect audit records. It derives the EID from the policy's components and adds several hardening changes, tests, and fuzz coverage. Problem
FixDerive the EID from the policy's components: deserialize each component's audit info, resolve its enrollment ID, and return it if all components agree on the same EID. Otherwise, fall back to The fix also includes some additional hardening:
Out of scopeThe output-side double-counting issue in amount sums is deliberately out of scope and is tracked separately in issue #2148. |
Fixes #2145
What
Derive the enrollment ID of a boolpolicy identity from its member identities, mirroring the recursive
htlc.NewAuditDeserializerpattern:boolpolicy.NewAuditInfoDeserializernow takes the parent multiplex deserializer and resolves each component's audit info through it; the policy identity reports the enrollment ID shared by all components. The fabtoken and zkatdlog drivers pass the multiplexer at registration.GetAuditInforeturns nil for an identity that is not registered locally andWrapAuditInfopreserves the empty entry, so such a component contributes no enrollment ID rather than failing the audit path. This exemption is applied before subtype resolution, so a component of an unknown identity type carrying no audit info also yields no enrollment ID rather than an error.GetAuditInfoMatcherandDeserializeVerifieralready do. Nothing else on the audit path reaches that validation:Request.IsValidonly checks action structure and metadata consistency, and owner verifiers are deserialized solely by the driver validator at commit.Why
AuditInfo.EnrollmentID()returned""for every policy identity, and the two sides of a transaction are affected differently.The input side carries the composite identity itself — both drivers put a single sender in
TransferInputMetadata— so a policy-owned input reached the auditor with no enrollment attribution at all, andcompleteInputsWithEmptyEIDthen reassigned it to whichever enrollment ID the output stream reports first. That is the gap this PR closes.The output side is separate: output rows are expanded per component and already resolve each member's own enrollment ID, so they are attributed correctly but counted once per member. That duplicate amount accounting is fixed by #2148. Only the two together bring a policy wallet's audited movements back to the true net; see #2145 for a worked example.
In our test environments the auditor's audit balances for policy wallets were corrupted by this pair of defects.
Testing
FuzzDeserializeAuditInfoNoPanicfuzzes the recursive deserialization path (seeds: valid single/two-member policies in both identity encodings, empty, truncated, empty audit-info blob, count mismatch, unknown member type, 5-deep nesting; inputs capped at 64 KiB like the neighbouring identity targets), wired into.github/workflows/nightly-fuzz.yml; 20s local run clean at ~50k execs/sec.