Skip to content

Conversation

@daxpedda
Copy link
Contributor

This could maybe alternatively be fixed "upstream" in EncodedPoint::len()?
Fixes #442.

@codecov-commenter
Copy link

codecov-commenter commented Sep 30, 2021

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.

Project coverage is 64.51%. Comparing base (2d91c16) to head (0ba5440).
Report is 640 commits behind head on master.

Files with missing lines Patch % Lines
p256/src/arithmetic/projective.rs 71.42% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #443      +/-   ##
==========================================
+ Coverage   64.45%   64.51%   +0.05%     
==========================================
  Files          28       28              
  Lines        3584     3590       +6     
==========================================
+ Hits         2310     2316       +6     
  Misses       1274     1274              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tarcieri
Copy link
Member

This is an unfortunate mismatch between the GroupEncoding API and SEC1 encoding.

This could maybe alternatively be fixed "upstream" in EncodedPoint::len()?

EncodedPoint::len is doing the right thing: the SEC1 encoding for the identity point is a 1-byte 0 tag.

Technically this is an invalid SEC1 encoding, but there's nothing else that can be done with a fixed-sized GroupEncoding::Repr, and it's certainly preferable to panicking.

@daxpedda
Copy link
Contributor Author

Technically this is an invalid SEC1 encoding, but there's nothing else that can be done with a fixed-sized GroupEncoding::Repr, and it's certainly preferable to panicking.

We could change the interface to return an Option?

@tarcieri
Copy link
Member

GroupEncoding is defined in the group crate, so the change would have to be made there.

Really it's an eccentricity of the SEC1 encoding, and I doubt they would want to make the API fallible just to support that.

@daxpedda
Copy link
Contributor Author

GroupEncoding is defined in the group crate, so the change would have to be made there.

Ah, I wasn't aware this isn't owned by RustCrypto.

@tarcieri
Copy link
Member

It'd also be good to make sure that GroupEncoding::from_bytes can decode this encoding, and also that the problem is also solved for the GroupEncoding impl on AffinePoint.

@tarcieri
Copy link
Member

This is fine as a start. I'll take care of the other comments I raised.

@tarcieri tarcieri merged commit 9a93c97 into RustCrypto:master Sep 30, 2021
tarcieri added a commit that referenced this pull request Oct 2, 2021
`GroupEncoding` uses a fixed-width `Repr` whereas SEC1 is variable-width
with respect to compressed points versus the identity.

This change allows 33-bytes of zeroes to be used as the identity.
Technically that's not a valid SEC1 encoding: the SEC1 encoding for the
identity is 1-byte: 0x00. However, this is the best we can do with a
fixed-width encoding.

See also: #443 and #444
tarcieri added a commit that referenced this pull request Oct 2, 2021
`GroupEncoding` uses a fixed-width `Repr` whereas SEC1 is variable-width
with respect to compressed points versus the identity.

This change allows 33-bytes of zeroes to be used as the identity.
Technically that's not a valid SEC1 encoding: the SEC1 encoding for the
identity is 1-byte: 0x00. However, this is the best we can do with a
fixed-width encoding.

See also: #443 and #444
@tarcieri tarcieri mentioned this pull request Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

<ProjectivePoint as GroupEncoding>::to_bytes() can panic

3 participants