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

Add EC_POINT_add support #261

Merged
merged 1 commit into from
Jul 4, 2019
Merged

Add EC_POINT_add support #261

merged 1 commit into from
Jul 4, 2019

Conversation

jdhollis
Copy link
Contributor

@jdhollis jdhollis commented Jul 3, 2019

Hi, all.

I added support for EC_POINT_add.

I'll be the first to admit that I'm still learning my way around elliptic curve cryptography in general and OpenSSL in particular. This is also the first time I've ventured down to C with Ruby.

All of that's to say, I welcome your feedback.

@ioquatix
Copy link
Member

ioquatix commented Jul 3, 2019

This looks pretty reasonable, but I'm no SSL expert, do you mind explaining what it does?

@ioquatix ioquatix self-assigned this Jul 3, 2019
@ioquatix ioquatix requested a review from rhenium July 3, 2019 22:58
@jdhollis
Copy link
Contributor Author

jdhollis commented Jul 3, 2019

I'm not sure how well I can explain the math behind it (still getting my head around it), but as far as a practical application, I'm working with a client to build a Ruby implementation of their zero-knowledge authentication protocol.

We need to be able to add EC points together as part of the proof verification process.

@ioquatix
Copy link
Member

ioquatix commented Jul 3, 2019

Okay, do you think it makes sense to add a few more details to the documentation about what it's used for? Even if it's just a one sentence summary of what you said.

GetECPoint(result, point_result);

if (EC_POINT_add(group, point_result, point_self, point_other, ossl_bn_ctx) != 1) {
ossl_raise(eEC_POINT, "EC_POINT_add");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this actually mean?

Copy link
Contributor Author

@jdhollis jdhollis Jul 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave this some thought earlier today, and I’m not sure more documentation makes sense here.

In ECC, point multiplication acts as a one-way hash, and while it probably isn’t implemented this way under the hood for performance reasons, point addition is a more primitive operation that forms the basis for point multiplication.

Most people won’t typically need this operation unless they’re implementing their own protocol (at which point, they’ll know what it is and what to do with it).

As you can see from the Wikipedia overview, the math is somewhat involved (though this post on Hacker Noon may be easier to follow).

I’m not sure a fuller description of ECC makes sense in context of the API docs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough - I was just specifically wondering how this could fail, and under what situations the exception would be raised.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. So digging through the OpenSSL source code, it looks like the top level checks involve whether a point is compatible with a particular curve. But given how points are created in Ruby, this would trigger an exception elsewhere. I suspect there are also other low-level checks further in depending on the group type.

I had assumed from looking at the other code in the openssl gem that ossl_raise would just do the right thing in any case.

@ioquatix ioquatix merged commit 3b2126c into ruby:master Jul 4, 2019
@jdhollis jdhollis deleted the add-EC_POINT_add-support branch July 4, 2019 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants