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

Support OAEP encryption schema #126

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

methane
Copy link

@methane methane commented Jan 21, 2019

No description provided.

@coveralls
Copy link

coveralls commented Jan 21, 2019

Coverage Status

Coverage decreased (-0.01%) to 91.676% when pulling fd273d0 on methane:rsa_oaep into 483700a on sybrenstuvel:main.

@methane methane mentioned this pull request Mar 14, 2019
4 tasks
@sybrenstuvel
Copy link
Owner

Please give some context for this PR. As it stands now, the commit just adds a few functions to a file, and nothing else. There doesn't seem to be any interface for people to use it, and no documentation either. Not sure what to make of this.

@methane
Copy link
Author

methane commented Aug 5, 2019

@sybrenstuvel What API do you prefer?

  • rsa.OAEP_encrypt, rsa.OAEP_decrypt?
  • rsa.encrypt_OAEP, rsa.decrypt_OAEP?
  • rsa.encrypt_pkcs1_v2, rsa.decrypt_pkcs1_v2?

@sybrenstuvel
Copy link
Owner

I don't know, because for that I would have to know more about the intended audience, intended use cases, etc. Should it even be different functions in the public API, or should the switch between PKCS versions / encryption standards (that's my interpretation right now, let me know if I'm wrong) be done using arguments to the already-existing functions?

@methane
Copy link
Author

methane commented Aug 6, 2019

I don't know, because for that I would have to know more about the intended audience, intended use cases, etc.

OAEP is stronger than PKCS1_v1.5. When encrypting the same plaintext twice, OAEP produces different encrypted text. OAEP is recommended for new protocols. https://tools.ietf.org/html/rfc8017#section-7 says:

   Two encryption schemes are specified in this document: RSAES-OAEP and
   RSAES-PKCS1-v1_5.  RSAES-OAEP is REQUIRED to be supported for new
   applications; RSAES-PKCS1-v1_5 is included only for compatibility
   with existing applications.

Should it even be different functions in the public API

I think so because they have different arguments. OAEP supports optional "label". While this PR uses os.random directly, random_source option may be added later if someone proposes it.

For example, Go has two APIs with different signature: EncryptOAEP, EncryptPKCS1v15.

My current idea is:

  • rsa.encrypt_PKCS1_v15 -- new name for PKCS1-v1_5 encryption.
  • rsa.encrypt -- Alias of rsa.encrypt_PKCS1_v15. This is kept for backward compatibility.
  • rsa.encrypt_OAEP -- new recommended encryption function.

Additionally, I want to rename pcks1_v2.py to oaep.py because PKCS1-v1_5 is defined in PKCS#1 v2.2 too. But this is just a personal preference.

Base automatically changed from master to main February 15, 2021 20:06
@sybrenstuvel
Copy link
Owner

I think it's a good idea to support OAEP, so thanks for the patch & the explanation. Also, sorry for the delayed answer!

As far as the patch itself, I think some things can be improved. For one, by now there is no more need to support Python versions older than 3.6, as these are no longer supported by Python Software Foundation themselves. I also feel that the code uses too many tiny-named variables (like em and a).

My current idea is:

* `rsa.encrypt_PKCS1_v15` -- new name for PKCS1-v1_5 encryption.
* `rsa.encrypt` -- Alias of `rsa.encrypt_PKCS1_v15`.  This is kept for backward compatibility.
* `rsa.encrypt_OAEP` -- new recommended encryption function.

That looks good to me, at least for now. I'll want to refactor the code a bit more after OAEP support is in, to see if I can abstract things a little bit more and reduce the amount of copied code. That's easier to do once this patch is in, though.

Additionally, I want to rename pcks1_v2.py to oaep.py because PKCS1-v1_5 is defined in PKCS#1 v2.2 too. But this is just a personal preference.

I agree. Also this is something for another commit, such that functional and non-functional changes are separated.

I guess this'll become version 5.0 of the library :)

@sybrenstuvel
Copy link
Owner

PS: If you could look at resolving the current conflicts & renaming some variables so that they're more descriptive (and maybe add some type annotations to functions), I'll accept the patch and put it in a separate branch. The rest of the work can be done later by either of us in that branch, and later be merged to main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants