-
Notifications
You must be signed in to change notification settings - Fork 42
proof blank node and subject blank node collide #158
Comments
I've seen similar issues before.... I happen to think this is a better normalization approach: |
me too... but since we have urdna to work with for now what's happening here? this comment concerns me
proof statements are always first? |
doesn't look like that's the case here |
https://github.com/decentralized-identity/waci-presentation-exchange/issues/115 If test vectors can't be generated for the example here, we may need to consider not using the suite. |
For LD integrity proofs, the And is described in the spec here: https://w3c-ccg.github.io/ld-proofs/#create-verify-hash-algorithm This is done to allow multiple proofs to be independently created and attached to a document without coordination amongst the various signers / proof creators (the proofs altogether forming a "proof set"). It looks like the BBS+ suite implementation that is referenced here combines all of the statements (from the proof and the document) together: jsonld-signatures-bbs/src/BbsBlsSignatureProof2020.ts Lines 244 to 265 in a127175
Which seems, to me, to be a mistake that will produce conflicts (likely the root cause of the above). I would think that keeping these two sections separate and using two different index transformation maps would resolve the issue. cc: @tplooker, @kdenhartog |
Simply pasting a signed document into the JSON-LD playground's canonize tool will canonize the entire document vs. separating the proof value out and canonizing it separately to allow for "proof sets" (as described in my comment above). So what's missing here is that it's important to separate out the document's statements from the proof's statements. Both sets of statements are indeed signed, but they need to be canonized separately to allow for multiple signers to sign the document independently, if desired. |
+1 @dlongley ... IMO it was a mistake to "seperate the proof" before normalization... the complexity of this that the container semantics for proof are brutal....
|
One more note:
You'd run into the same issue of needing to separate document statements from proof statements with any other canonize function (e.g., json pointer, base64url, etc.) if multiple independent signatures are to be supported, so the problem is really orthogonal. |
Here is the code we used for the merkle proof suite... https://github.com/transmute-industries/merkle-disclosure-proof-2021/blob/main/src/merkle/normalization/urdna2015.ts IMO, proof should be handled along with the document... not separately from it... only the attributes of the proof that come from the signature should be omitted by This allows for us to replace URNDA with JSON Pointer, or any other |
Those |
It's fine if you want to canonize everything together -- just know that the trade off is that you can't have multiple independent signers and / or proofs. |
@dlongley yes, I have been weighing the trade offs wrt that... IMO, this not the appropriate place to address multi-signature formats... that work should be handled in a work item targeted at extending JOSE such as ietf-wg-jose/json-web-proof#10 We're shooting ourselves in the foot by making suite implementers care about all these concerns at once... instead of layering standards approaches on top of each other. I believe there was originally language related to |
I have created a PR #159 demonstrating a failing test. I suspect what @dlongley pointed out here #158 (comment) is the root cause of the issue here |
Thanks @brianorwhatever apologies it has taken me a few days to get to this issue, I see the cause now will start working on a fix tonight. |
Any updates on this issue? It would also be good to know the scope. Can a credential schema or a presentation request be designed to avoid the issue? |
@swcurran decentralized-identity/bbs-signature#10 (comment)
yes, but such a credential cannot have any "blank nodes"... |
To update this issue, upon investigation of the root cause, it is not due to subject blank nodes colliding it is because in some JSON-LD framing operations (that feature more than 1 blank node) the order of the statements in the derived proof can be different to the order in which they were signed. The syntax for the deriveProof API accommodates this (via the revealedMessages array), however the expression that gets appended to the proof value (the revealed messages bit array) does not account for this re-ordering, hence the signature layer is returning false when validating a derived proof because they are failing the integrity check. As this issue captures when it comes to what we support at the cryptographic layer we have a choice to make. We are currently doing 2 however some are arguing to do 1, in either of these cases the LD-Proof suite would need at a minimum a mapping array to accomodate the potential for the statements to be re-ordered. In the interest of progressing this here is one potential proposal define a new term like
|
@tplooker FYI, I have extended this suite to add several features, including multi-byte array representation of revealed indicies, which solves this issue. It looks like corresponding to no.3 in your comment. In other words, The extended suite is here, which depends on extended bbs-signatures and extended bls12381-key-pair. I am afraid that there are very few documents about my extension at this moment... I will prepare more documents to explain it later, but at this moment you can check the brief presentation in IIW33 and try it out using playground. |
So I have been debugging why I can't get this document to derive a credential that verifies.
and the following frame
I have finally found what I think is the problem. When I step through the code at this line
jsonld-signatures-bbs/src/BbsBlsSignatureProof2020.ts
Line 192 in a127175
I notice that the concatenation of the proof statements and the document statements results in this list
I believe my issue is that the
_:c14n0
blank node identifier has a collision between the proof and the vaccine..Is there something I am missing?
The text was updated successfully, but these errors were encountered: