decentralized-identity / bbs-signature

The BBS Signature Scheme
https://identity.foundation/bbs-signature/draft-irtf-cfrg-bbs-signatures.html
Apache License 2.0
79 stars 26 forks source link

Tomislav's comments #223

Closed BasileiosKal closed 1 year ago

BasileiosKal commented 2 years ago

This is a tracking issue to address comments made by @tmarkovski around test vectors and the spec in general!

tmarkovski commented 2 years ago

Posting them here.

My thoughts and findings:

tplooker commented 1 year ago

Most negative tests are hard to validate for the reason they specify. Once my positive tests worked, I only assumed the negative tests are working as well, since they were all failing before. The algorithm doesn't provide ways to test exactly where the failure happened.

Agreed, I think even if an implementation does not verify the specific reason for a failure when testing with the negative test vectors, they still provide some value. The "reason" for failure is really only there for informational purposes, so we should clarify that.

Tests without header and presentation header might be useful as well

+1

I stumbled on the use of hex and treating messages as encoded scalars, but this has already been addressed I think. Might be useful to add variable length message inputs.

Yes I believe this has been addressed

Why does the Sign algorithm accept SK and PK as input, but doesn't validate if the PK is correct. SkToPk is fairly inexpensive operation, it seems that only SK is needed. Is there a security concern similar to what affected ECDSA?

Correct it is because of performance reasons, not sure which security concern you are referring to with ECDSA, do you mean EdDSA. We have investigated that even without confirming the PK's relationship to the SK in sign there is no security concern, the signature would just fail to validate. Implementations are free to do this validation and we could add in an implementation consideration to that effect?

Should PK parameters be passed as points to Verify, ProofGen, ProofVerify, instead as octet string? This will simplify the validation and be inline with how we pass the messages as scalars. Same question for SK.

I understand this perspective, however I think its good to be consistent, both proofs and signatures are passed in as their encoded form instead of as internal structures and I view PK the same.

tplooker commented 1 year ago

Related to #7, we may want to merge with that so we dont have a duplicate.

BasileiosKal commented 1 year ago

Discussed in the WG call on the 6th of February. Will re-visit after no-header fixtures are added and #239 is merged. Consensus is to check with the CFRG for reasons not to add negative test vectors, and if that is not the case add a limited set of negative cases capturing weird cases.

BasileiosKal commented 1 year ago

Discussed on WG call on 20th of Mar. No action to take rn. PK encoding related to #246. Will revisit after IETF for v03.