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

[WIP] Rsa keygen #731

Closed
wants to merge 2 commits into from
Closed

[WIP] Rsa keygen #731

wants to merge 2 commits into from

Conversation

est31
Copy link

@est31 est31 commented Dec 5, 2018

Opened the PR so that I can discuss with @briansmith about the code. Nothing works yet. DON'T MERGE!

List of tasks:

  • Implement primality test (Rust port of C's BN_primality_test function)
  • Implement FIPS 186-4 section B 3.3 algorithm to obtain (p,q) prime pair
  • Use (p,q) to create a ring::signature::RSAKeyPair instance
  • PKCS8 formatting of the created key

Fixes #219

@est31
Copy link
Author

est31 commented Dec 5, 2018

@briansmith I've implemented some of the first steps of the primality testing algorithm. How does it look so far?

As for the further steps, I don't know how to do the subtraction in Rust. There is subtraction support in the bignum module but it implements arithmetic modulo some number. This code however doesn't do that, it just uses vanilla bignums. There are functions limb_sub, etc in crypto/limbs/limbs.inl. Can I expose them to Rust? How should I do that?

@@ -59,7 +57,64 @@ static uint16_t shift_and_add_mod_u16(uint16_t r, uint32_t a, uint16_t d,
return t;
}

uint16_t bn_mod_u16_consttime(const BIGNUM *bn, uint16_t d) {

// Function taken from boringssl's bn.c file.
Copy link
Owner

Choose a reason for hiding this comment

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

Any code that comes from a file X in BoringSSL must be in a file named X in ring. Also, the license must be the same. The best way to add functions from bn.c would be to add one commit that is the latest unmodified version from BoringSSL, and then another commit that deletes everything except the license and the code you need.

Note that the licenses are different. That is actually why div.c and div_extra.c are separate files; the _extra files are ISC-licensed, the other files are "OpenSSL licensed," i.e. the bad license everybody is trying to get away from.

@@ -150,3 +150,6 @@ mod private {
mod tests {
bssl_test!(test_constant_time, bssl_constant_time_test_main);
}

// Only here for development
pub use rsa::key_generation::probable_primality_test;
Copy link
Owner

Choose a reason for hiding this comment

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

It seems like you maybe forgot to git add this?

@briansmith
Copy link
Owner

  • Implement primality test (Rust port of C's BN_primality_test function)

We cannot "port" code from BoringSSL unless it is ISC-licensed like div-extra. In particular, BN_primality_test has the wrong license and we can't use it even as a model. Instead, look at the algorithm from Wikipedia or a textbook.

Unfortunately, because of the licensing issues, I need to close this PR. Please sort out the license-related stuff (email me if it is confusing: brian@briansmith.org) and submit a new PR. I appreciate your effort here but I need to make sure the licenses are respected and that the ring Rust code is under the ISC license.

@briansmith briansmith closed this Dec 5, 2018
@est31
Copy link
Author

est31 commented Dec 5, 2018

Hmmm will make a new attempt tomorrow then

@est31
Copy link
Author

est31 commented Dec 6, 2018

@briansmith okay I've removed the parts that I've taken from the C function BN_primality_test and re-did them myself. I've opened a new PR.

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.

2 participants