-
Notifications
You must be signed in to change notification settings - Fork 70
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
Interfacing with Rust #263
Conversation
4bc152d
to
d413c64
Compare
35f8df9
to
82399fa
Compare
5041663
to
dac7767
Compare
564963f
to
f349de5
Compare
Will help CI for the time being
umbral/curve_scalar.py
Outdated
|
||
# TODO: in most cases, we want this number to be secret. | ||
# OpenSSL 1.1.1 has `BN_priv_rand_range()`, but it is not | ||
# currently exported by `cryptography`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the past, we've opened PRs to cryptography.io to expose such endpoints
""" | ||
Derives a symmetric encryption key from a pair of password and salt. | ||
It uses Scrypt by default. | ||
Umbral secret (private) key. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually the term "private" key implicitly signals an asymmetric cryptography scheme (hinting at the private vs public dichotomy), while "secret" key is usually associated to symmetric cryptography.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was following the name convention from rust-umbral
, which, in turn, follows the terms used by the RustCrypto stack. Is using "secret" instead of "private" here very confusing, or just somewhat unusual?
As an extension of uniformity with RustCrypto, I was thinking of just re-exporting the RustCrypto key objects themselves instead of wrapping them in our own types. This way the keys can be used with other libraries based on RustCrypto, making it unnecessary to have conversion methods like to_cryptography_privkey()
we have in PyUmbral.
Also being able to abbreviate to "sk" and "pk" is quite useful.
raise ValueError(f"The integer encoded with given bytes ({repr(bytes_seq)}) " | ||
"is not under the provided modulus.") | ||
|
||
if apply_modulus: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an awesome idea 🤯
rust-umbral#43 is closed
set_consttime_flag was always True anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the walkthrough yesterday! The PR looks great and to me it's a relief seeing this simplification in so many aspects of Umbral.
I think I gave you my main comments during the walkthrough and I see you are working on that in part 2. I still think the use of "SecretKey" is not the best choice taking into consideration the uses in the industry and academia for public-key cryptosystems. As a random example, look at the recent draft for Hybrid Public Key Encryption from the IETF (https://www.ietf.org/archive/id/draft-irtf-cfrg-hpke-08.html#name-notation); they use the term "private key" even though they use the sk
abbreviation.
But that's something that's not critical at all in the scope of this PR.
pysha3 = "*" | ||
# NuCypher | ||
bytestring-splitter = "*" | ||
constant-sorrow = ">=0.1.0a7" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay!
b"to sum up: the symmetric ciphertext is now called the 'Chimney'." | ||
b"the chimney of the capsule, of course" | ||
b"tux [9:32 AM]" | ||
b"wat") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good thing we're keeping this fixture
Perhaps. Let us put a pin in that and revisit it after existing PRs — changing it in 4 PRs now will lead to horrible rebases. I agree with following the existing conventions, but I wonder why RustCrypto chose to call it SecretKey - the author is usually very diligent in implementing crypto standards. |
Refactor to achieve binary compatibility (and API uniformness) with
rust-umbral
(see the corresponding PR nucypher/rust-umbral#41):unsafe_hash_to_point()
in the same way asrust-umbral
hash_to_curvebn()
in the same way RustCrypto does itrust-umbral
rust-umbral
rust-umbral
For reviewers:
The size may seem daunting, but a lot of it is due to vector changes. Perhaps it will be better to review commit-by-commit, or just file by file. The size of both tests and the library itself shrunk by 30-40%, and 25% of the library is just OpenSSL wrapper functions.
I'm planning to make a test PR to
nucypher
to see if this new version has enough functionality for it. There may be slight changes in the API because of that, but the bulk of the changes is already made.In particular,
BytestringSplitter
may have to be brought back. I removed it temporarily because I couldn't figure out splitter composition, and didn't want that to block me.Performance:
There are two differences between this version and the main branch currently.
generate_kfrags
is ~15% slower. This is caused by one secret key -> public key computation, which is cached in the main branch. Fixed, but in Python version only - just to avoid explaining why there's a performance regression. It starts getting less and less important the higherm
andn
you take.reencrypt
is ~2x faster. The main branch verifies the capsule twice (4 curve point * scalar multiplications), and kfrag once (1 curve point * scalar multiplication). This effectively doubles the number of multiplications, which are the main bottleneck in this operation. In this PR, capsules are verified on deserialization only, andreencrypt()
does not attempt to verify kfrags (the caller must call verification explicitly).