Skip to content

p256: implement AffinePoint::identity() and ::is_identity()#167

Merged
tarcieri merged 1 commit into
masterfrom
p256/affine-identity
Sep 5, 2020
Merged

p256: implement AffinePoint::identity() and ::is_identity()#167
tarcieri merged 1 commit into
masterfrom
p256/affine-identity

Conversation

@tarcieri

@tarcieri tarcieri commented Sep 5, 2020

Copy link
Copy Markdown
Member

This is a corresponding change ala #165, but for the p256 crate.

@tarcieri tarcieri requested review from nickray and str4d September 5, 2020 00:51
@tarcieri tarcieri force-pushed the p256/affine-identity branch from 836c2c8 to 7155812 Compare September 5, 2020 00:52
@codecov-commenter

codecov-commenter commented Sep 5, 2020

Copy link
Copy Markdown

Codecov Report

Merging #167 into master will increase coverage by 0.35%.
The diff coverage is 91.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #167      +/-   ##
==========================================
+ Coverage   60.35%   60.71%   +0.35%     
==========================================
  Files          25       25              
  Lines        3572     3597      +25     
==========================================
+ Hits         2156     2184      +28     
+ Misses       1416     1413       -3     
Impacted Files Coverage Δ
p256/src/arithmetic/affine.rs 90.58% <86.95%> (+4.02%) ⬆️
p256/src/arithmetic/projective.rs 84.57% <93.75%> (+0.84%) ⬆️
k256/src/arithmetic/affine.rs 87.95% <100.00%> (+0.29%) ⬆️
k256/src/arithmetic/projective.rs 85.58% <100.00%> (+0.06%) ⬆️
p256/src/ecdsa.rs 90.47% <100.00%> (+2.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2377afa...e56ab3b. Read the comment docs.

This is a corresponding change ala #165, but for the `p256` crate.
@tarcieri tarcieri force-pushed the p256/affine-identity branch from 7155812 to e56ab3b Compare September 5, 2020 01:00
@tarcieri tarcieri merged commit 2c2491c into master Sep 5, 2020
@tarcieri tarcieri deleted the p256/affine-identity branch September 5, 2020 01:08
@nickray

nickray commented Sep 5, 2020

Copy link
Copy Markdown
Member

No strong opinions, but from a mathematical point of view, an "affine point" is one in the affine plane (i.e., not the point at infinity). So then what one might want is a kind of CtEnum consisting of either an affine point (the (x,y) coordinates, that must lie on the curve) or the infinite point.

@tarcieri

tarcieri commented Sep 5, 2020

Copy link
Copy Markdown
Member Author

@nickray I agree, but unfortunately there is not such a thing as a CtEnum AFAIK (at least in Rust)

@nickray

nickray commented Sep 5, 2020

Copy link
Copy Markdown
Member

Right, I made that up (as I was thinking of just an enum at first, and then saw your careful use of constant-time operations). You could get by with a CtOption<AffinePoint> (where AffinePoint is the coordinate pair) if you want something your ProjectivePoint maps to.

@tarcieri

tarcieri commented Sep 5, 2020

Copy link
Copy Markdown
Member Author

That’s how things previously worked prior to this PR.

The goal of this PR though is to support an infallible bidirectional conversion between AffinePoint and ProjectivePoint in service of supporting the group::Curve trait (#164).

It also makes AffinePoint much more subtle::CtOption-friendly in general, since several methods on CtOption are bounded on a Default impl (which AffinePoint::identity() makes possible)

@nickray

nickray commented Sep 5, 2020

Copy link
Copy Markdown
Member

I'm just saying this AffinePoint is a misnomer (and there is no such bijection); but it's fine -- one just has to mentally substitute either AffinePointOrInfinity or ProjectivePointWithNormalizedCoordinates or some such thing wherever it says AffinePoint :)

@tarcieri

tarcieri commented Sep 5, 2020

Copy link
Copy Markdown
Member Author

Yeah. I tried introducing a NonZeroScalar type to avoid it (RustCrypto/traits#241), which as it were ended up being helpful for other reasons (#144).

I think unless you're dealing explicitly with scalars which by definition must be non-zero (e.g. using NonZeroScalar in the ECDSA case mentioned above) it's still really nice to avoid having a fallible conversion, and especially in the case of writing constant-time code this particular conversion has been quite annoying, especially because without an AffinePoint::default impl it isn't possible to use the Option-like combinators on CtOption.

@tarcieri tarcieri mentioned this pull request Sep 18, 2020
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.

3 participants