fix(identity): derive the enrollment ID of a boolpolicy identity from its members - #2146
fix(identity): derive the enrollment ID of a boolpolicy identity from its members#2146EvanYan1024 wants to merge 10 commits into
Conversation
c5d62dd to
a52cc93
Compare
… 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>
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, |
… 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>
|
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
| } | ||
| } | ||
|
|
||
| return eids[0], nil |
There was a problem hiding this comment.
I think this fixes less of the auditor balance corruption than the PR description claims, and I'd like a second opinion on my reading.
As far as I can tell the output side still expands a policy owner into one row per component, each at the full quantity: token/request.go:1435 calls inputsAndOutputs(ctx, true, false, false), so noOutputForRecipient is false and the output loop iterates recipients, which for a policy owner comes from Deobfuscate → TypedIdentityDeserializer.Recipients → the component list.
So for a $0 AND $1 policy whose two members share enrollment wallet-42 receiving 100, I believe OutputStream.ByEnrollmentID("wallet-42").Sum() is 200. Spending it back produces a single Input row (both drivers set Senders to a 1-element slice — fabtoken/v1/transfer.go:97, zkatdlog/nogh/v1/transfer.go:202) contributing 100, so ttxdb.Movements records +100 where the true net is 0.
If that's right, the drift goes from +2q to +q per round trip rather than to 0, and either the output-side expansion needs deduplicating by EID too, or the PR body should be softened. An integration assertion on HoldingsFilter.ByEnrollmentId for a multi-member same-enrollment policy would settle it either way — I haven't run one, so I could be wrong about the output loop.
There was a problem hiding this comment.
You're right, and thanks for working through the arithmetic — I traced both sides and your reading of the output loop holds.
One detail worth adding, because it explains why the drift is +2q rather than +q before this PR: the two sides resolve enrollment IDs from different identities. Output rows are per component, so each already gets a real EID from GetEIDAndRH(recipient, ...). The input row carries the composite identity itself (sender.Identity is the token's owner, and both drivers set Senders to a single element), so its EID comes from the policy identity — which is precisely what returned "" before this change. For a two-member same-enrollment policy round-tripping q, where the correct movement is 0:
- before this PR: received 2q, sent 0 (the input is booked under ""), movement +2q
- with this PR: received 2q, sent q, movement +q
- with fix(token): count composite-owner outputs once per enrollment ID in amount sums #2148 as well: received q, sent q, movement 0
So this PR fixes the attribution half, and the amount half is #2148, which is already open: ttxdb/store.go applies UniquePerOutput() to the received sums and UniquePerInput() to the sent sum, collapsing member rows only where amounts aggregate while leaving the identity and revocation consumers seeing every row. Since HoldingsFilter sums MovementRecords, the ByEnrollmentId assertion you suggest is settled by the pair rather than by either PR alone.
That cross-reference is missing from the PR body here, which is what makes the claim read as broader than it is — I'll scope the body to the attribution gap and point at #2148 for the amount correction. Happy to add the integration assertion on HoldingsFilter.ByEnrollmentId for a multi-member same-enrollment policy in #2148 if you'd like it landed with that half.
| if err := pi.Deserialize(id); err != nil { | ||
| return "", errors.Wrapf(err, "failed to deserialize policy identity") | ||
| } | ||
| if len(pi.Identities) != len(ei.IdentityAuditInfos) { |
There was a problem hiding this comment.
I'm wondering whether this should call validateComponentIdentities(pi.Identities) here. It's a new deserialization boundary over wire-supplied pi.Identities, and both siblings in this file do validate — GetAuditInfoMatcher (line 99) and DeserializeVerifier (line 124). The doc comment on the function in identity.go:170-178 also says validation "must also happen at the deserialization boundary to actually close the gap."
Concretely: an AND policy with the same identity in both component slots is rejected by DeserializeVerifier but I think accepted here, yielding a derived enrollment ID. That seems to matter most on the auditor path, since the auditor never runs the verifier as far as I can see (integration/token/fungible/views/auditor.go goes Audit → business checks → NewAuditApproveView → auditdb.Append, no IsValid), so Movements/TransactionRecords rows would get attributed for an identity that only gets rejected later at commit.
Low severity, but it looks like a one-liner.
There was a problem hiding this comment.
Agreed, added in f0ac58c.
On reachability: Audit reaches AuditRecord via inputsAndOutputs(ctx, true, false, false), so verifyActions is false there. The Validate path doesn't close the gap either — integration/token/fungible/views/auditor.go:42 does call auditor.Validate(tx) before Audit, which reaches Request.IsValid, but IsValid only verifies actions structurally: VerifyTransfer checks amounts, types and action shape and never deserializes an owner verifier. Owner verifiers are deserialized solely by the validator, at token/core/fabtoken/v1/validator/validator_transfer.go:48, which runs at commit time. So validateComponentIdentities is genuinely unreachable on the auditor path, as you say.
Worth flagging one behavioural consequence: such an identity now fails at audit time rather than being attributed and rejected later at commit. That's the opposite direction from the empty-component change earlier in this PR, but the two cases differ — an empty component audit info is legal, whereas a duplicate or empty component identity is already rejected by WrapPolicyIdentity and DeserializeVerifier, so no legitimate identity is affected. Covered by TestPolicyEnrollmentIDInvalidComponentIdentities.
| if err != nil { | ||
| return "", errors.Wrapf(err, "failed to deserialize audit info of component [%d]", k) | ||
| } | ||
| eids[k] = memberAuditInfo.EnrollmentID() |
There was a problem hiding this comment.
memberAuditInfo is dereferenced without a nil check. a.inner is the interface injected by the exported constructor, and the counterfeiter fake for driver2.AuditInfoDeserializer returns (nil, nil) when unstubbed, so I think NewAuditInfoDeserializer(defaultMock).DeserializeAuditInfo(...) panics rather than returning an error. The analogous recursive code in htlc/deserializer.go:177 sidesteps it by returning ai without dereferencing.
Probably not reachable through the real multiplexer wiring, so this is mostly about not handing callers a panic — if memberAuditInfo == nil { continue } above this line would do it.
There was a problem hiding this comment.
Confirmed and fixed in f0ac58c. I reproduced it with a fake returning (nil, nil): SIGSEGV at deserializer.go:216, so it is a panic rather than an error as you expected. Guarded with continue rather than an error, which keeps it consistent with the missing-audit-info case just above — the component simply contributes no enrollment ID. TestPolicyEnrollmentIDNilMemberAuditInfo covers it.
You're right that it isn't reachable through the real multiplexer: EIDRHDeserializer.DeserializeAuditInfo either errors or returns the leaf result, and none of the x509, idemix, multisig or boolpolicy leaves return (nil, nil). The guard is there because NewAuditInfoDeserializer takes an exported interface.
…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>
2 participants
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.