Require using Schnorr proofs from signature proof for predicates #42
Conversation
Signed-off-by: Michael Lodder <mike@litprotocol.com>
|
Thanks, Mike. @mark-moir — love to get your review of this. |
There was a problem hiding this comment.
Thanks @mikelodder7, nice work! I have made a few comments and suggestions, but overall I think the approach is effective to ensure that predicate proofs are tied to signed attributes.
I have not reviewed the changes to BBS, which reportedly are (mostly?) not related to the issue being addressed in this PR (as signaled in both PR description and commit comment). IMO, those should be in a different PR explicitly for that purpose. I'd be happy to re-review this PR if those changes are removed from this one (I'd prefer that this one be completed first and then the BBS changes unrelated to this issue be done in a subsequent PR, but will do my best either way).
It seems I do not have sufficient privileges to "Request Changes", so my request is in this comment 😄.
| @@ -3,6 +3,7 @@ use credx::claim::{ClaimType, HashedClaim, NumberClaim, RevocationClaim}; | |||
| use credx::credential::{ClaimSchema, CredentialSchema}; | |||
| use credx::issuer::Issuer; | |||
| use credx::knox::bbs::BbsScheme; | |||
| use credx::knox::ps::PsScheme; | |||
There was a problem hiding this comment.
This change appears to be unnecessary. However, it makes me wonder if there is a plan for running all tests against both BBS and PS. As it stands, some tests are run with PsScheme and others with BbsScheme.
| @@ -38,6 +39,11 @@ range_test_with!(in_range_max_implicit, isize::MAX, Some(0), None, false); | |||
| range_test_with!(out_of_range_below, 0, Some(1), Some(isize::MAX), true); | |||
| range_test_with!(out_of_range_above, 1001, Some(0), Some(1000), true); | |||
|
|
|||
| #[test] | |||
There was a problem hiding this comment.
This test is expressed inconsistently with the others, and also has a name that does not reflect the test accurately. How about instead putting:
range_test_with!(in_range_max_right_explicit, 1000, Some(0), Some(1000), false);
right after
range_test_with!(in_range_max_implicit, isize::MAX, Some(0), None, false);
?
(Maybe the names could be improved: right is intended to convey right hand limit of range.)
| pub(crate) s_rho: Scalar, | ||
| pub(crate) s_delta_sigma: Scalar, | ||
| pub(crate) s_delta_rho: Scalar, | ||
| pub(crate) s_y: Scalar, |
There was a problem hiding this comment.
Only s_y needs to be pub(crate), does it make sense to restrict this change to that field?
| @@ -115,4 +143,42 @@ impl<S: ShortGroupSignatureScheme> Presentation<S> { | |||
|
|
|||
| Ok(()) | |||
| } | |||
|
|
|||
| fn get_sig_hidden_messages( | |||
There was a problem hiding this comment.
I suggest renaming this to get_sig_hidden_messages_proofs. Otherwise it suggests that the Presentation can reveal the hidden values.
| @@ -32,12 +34,20 @@ impl<S: ShortGroupSignatureScheme> Presentation<S> { | |||
| for (id, pred_statement) in &predicate_statements { | |||
| match (pred_statement, self.proofs.get(*id)) { | |||
| (Statements::Revocation(aa), Some(PresentationProofs::Revocation(proof))) => { | |||
| let verifier = RevocationVerifier::new(aa, proof, nonce); | |||
| let hidden_messages = self.get_sig_hidden_messages(schema, &aa.reference_id)?; | |||
There was a problem hiding this comment.
These four lines are repeated (modulo different Statement types) several times below and could be repeated again if other ZKP features were added in future. I suggest refactoring to avoid this.
| b: Scalar, | ||
| r: Scalar, | ||
| } | ||
|
|
||
| impl<S: ShortGroupSignatureScheme> PresentationBuilder<S> for VerifiableEncryptionBuilder<'_> { | ||
| fn gen_proof(self, challenge: Scalar) -> PresentationProofs<S> { | ||
| let message_proof = self.b + challenge * self.message; | ||
| // Message proof will be passed from signature proof |
There was a problem hiding this comment.
I think this comment could be removed. It may do more harm than good, as it is explaining why something isn't how it used to be 😄.
|
The changes to BBS were required to also fix this. I’ll not be breaking those apart |
|
The eprint uses a signature blinding in the proof making predicate proof linking impossible. Thus the revert to IETF. |
I see, thanks @mikelodder7. So I will change my request 😄: Please:
I have not reviewed the BBS signature changes in detail (not sure if I'll have time, but in any case I'd like to know what document to reference and the status of the implementation w.r.t. that document). However, I have looked a bit at the changes now w.r.t. the stated purpose of this PR and am happy with them at that level. I also have a couple more small comments, to follow. |
| ) -> CredxResult<()> { | ||
| if (self.a_bar.is_identity() | self.b_bar.is_identity()).into() { | ||
| if (self.a_bar.is_identity() | self.b_bar.is_identity() | self.t.is_identity()).into() { | ||
| return Err(Error::General("Invalid proof - identity")); | ||
| } | ||
| if public_key.is_invalid().into() { | ||
| return Err(Error::General("Invalid public key")); | ||
| } | ||
| let mut points = Vec::with_capacity(public_key.y.len() + 2); |
There was a problem hiding this comment.
I think this should be + 3 now, due to the change at line 75 below.
|
|
||
| let mut proof = ProofCommittedBuilder::new(G1Projective::sum_of_products); | ||
| let mut points = Vec::with_capacity(msgs.len()); | ||
| let mut hidden_messages = Vec::with_capacity(msgs.len()); |
There was a problem hiding this comment.
I think this should now be +2 due to the changes at lines 60 and 62 below.
Before, each predicate proof kept a copy of the hidden message proof. However, this wasn't checked against the signature proof. This PR removes the copy and just uses the signature proof hidden message proofs directly which guarantees to check if hidden messages were manipulated.