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

Implement Point#mul #307

Merged
merged 6 commits into from
Jun 17, 2024
Merged

Implement Point#mul #307

merged 6 commits into from
Jun 17, 2024

Conversation

headius
Copy link
Member

@headius headius commented Jun 6, 2024

Work in progress. Fixes #306.

@headius headius added this to the 0.10.5 milestone Jun 6, 2024
}

@JRubyMethod(name = "mul")
public IRubyObject mul(final ThreadContext context, final IRubyObject[] args) {

Choose a reason for hiding this comment

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

I think there is at least one other math operation on ECPoint that this change now makes it easy to support: add

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I'll do add in another PR!

@headius headius force-pushed the point_mul branch 2 times, most recently from fb9b026 to 6345910 Compare June 11, 2024 15:17
@headius
Copy link
Member Author

headius commented Jun 11, 2024

Initial implementation is in place and appears to work for your simple example:

[] jruby-openssl $ jruby -Ilib blah.rb
"\x04|\xF2{\x18\x8D\x03O~\x8AR8\x03\x04\xB5\x1A\xC3\xC0\x89i\xE2w\xF2\e5\xA6\vH\xFCGf\x99x\awU\x10\xDB\x8E\xD0@)=\x9A\xC6\x9Ft0\xDB\xBA}\xAD\xE6<\xE9\x82)\x9E\x04\xB7\x9D\"xs\xD1"
[] jruby-openssl $ ruby33 blah.rb  
"\x04|\xF2{\x18\x8D\x03O~\x8AR8\x03\x04\xB5\x1A\xC3\xC0\x89i\xE2w\xF2\e5\xA6\vH\xFCGf\x99x\awU\x10\xDB\x8E\xD0@)=\x9A\xC6\x9Ft0\xDB\xBA}\xAD\xE6<\xE9\x82)\x9E\x04\xB7\x9D\"xs\xD1"

I'm comparing uncompressed octet string output from each here. Better confirmation would be to run the related CRuby openssl tests, but there are other issues preventing us from running test_ec_point_mul and test_ec_point_add (4-arg constructor for Group is not implemented). This is at least a good start!

@headius
Copy link
Member Author

headius commented Jun 11, 2024

@alextwoods This now has add. Would you be able to test this branch against your code, or at least a more complex example? It will take more work to fill out Group logic to support the test in CRuby.

@kares Review please!

@headius headius requested a review from kares June 11, 2024 17:59
@headius headius modified the milestones: 0.10.5, 0.15.0 Jun 11, 2024
@alextwoods
Copy link

This works great! The AWS SDK For Ruby has a fairly extensive test suite for a signing algorithm based off of ECC and relies on the mul method (sigv4a) - this change passes all of our tests :-)

@headius
Copy link
Member Author

headius commented Jun 11, 2024

this change passes all of our tests

Gotta admit I'm surprised! But I think we can merge this in if @kares is ok with it. Fair warning: it's probably not going to be live until 0.15.0 releases unless we backport to 0.14 and do another release there.

@headius headius marked this pull request as ready for review June 11, 2024 18:37
Copy link
Member

@kares kares left a comment

Choose a reason for hiding this comment

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

There seems to be a regression, otherwise LGTM

src/main/java/org/jruby/ext/openssl/PKeyEC.java Outdated Show resolved Hide resolved
@headius headius merged commit adca91b into jruby:master Jun 17, 2024
14 checks passed
@headius headius deleted the point_mul branch June 17, 2024 22:50
@headius
Copy link
Member Author

headius commented Jun 17, 2024

I would like to add Point#add as well but hoping to get the related tests running first. That depends on getting #309 working.

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.

OpenSSL::PKey::EC::Point missing mul method
3 participants