Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tomislav's comments #223

Closed
BasileiosKal opened this issue Oct 19, 2022 · 5 comments · Fixed by #274
Closed

Tomislav's comments #223

BasileiosKal opened this issue Oct 19, 2022 · 5 comments · Fixed by #274
Labels
core Associated to the core spec
Milestone

Comments

@BasileiosKal
Copy link
Contributor

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

@tmarkovski
Copy link
Member

tmarkovski commented Oct 19, 2022

Posting them here.

My thoughts and findings:

  • 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.
  • Tests without header and presentation header might be useful as well
  • 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.
  • 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?
  • 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.

@tplooker
Copy link
Member

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 tplooker added the core Associated to the core spec label Nov 21, 2022
@tplooker
Copy link
Member

tplooker commented Dec 5, 2022

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

@BasileiosKal
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@tplooker tplooker added this to the draft-03 milestone May 8, 2023
This was referenced Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Associated to the core spec
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants