Add framework for key exchange schemes and Diffie-Hellman #38374

vincentmacri · 2024-07-15T23:12:22Z

Motivated by (but does not yet close) #37305. Closes #11568.

This PR adds a basic framework to add public key exchange schemes to Sage, and includes an implementation of the Diffie-Hellman primarily as an example of this new framework.

Open to suggestions to improve the structure of the code. This code was based on the existing code for public-key encryption in Sage.

📝 Checklist

The title is concise and informative.
The description explains in detail what this PR is about.
I have linked a relevant issue or discussion.
I have created tests covering the changes.
I have updated the documentation and checked the documentation preview.

⌛ Dependencies

None

vincentmacri · 2024-07-15T23:13:39Z

@grhkm21 Thoughts on this?

vincentmacri · 2024-07-15T23:29:27Z

src/sage/crypto/key_exchange/pke.py

+
+class KeyExchangeScheme:
+
+    @experimental(37305)


This is marked as experimental because if this gets merged I'd like to add more cryptographic schemes to Sage in the near(ish) future as I have spare time. I don't want to be tied down yet to any particular way of structuring the code.

vincentmacri · 2024-07-15T23:29:50Z

src/sage/crypto/key_exchange/diffie_hellman.py

+
+class DiffieHellman(KeyExchangeScheme):
+
+    @experimental(37305)


See comment on KeyExchangeScheme on why this is marked experimental.

src/doc/en/reference/references/index.rst

github-actions · 2024-07-16T17:43:49Z

Documentation preview for this PR (built with commit fc1a4dc; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

JosePisco

Code looks good to me overall. Just being picky with style issues. We should check if docstring EXAMPLES and TESTS should be formatted and if so, how to do it properly.

Nice contribution, it's good to see old issues being solved ^^'

src/sage/crypto/key_exchange/diffie_hellman.py

src/sage/crypto/key_exchange/key_exchange.py

Co-authored-by: grnx <[email protected]>

grhkm21 · 2024-07-17T16:40:49Z

I can review later. I just got back from vacation.

…the key exchange scheme

vincentmacri · 2024-07-17T17:55:09Z

We should check if docstring EXAMPLES and TESTS should be formatted and if so, how to do it properly.

I can't find anything specific. Since the docstring EXAMPLES go into the documentation, I think it's most important that they're readable there.

I'm guessing you raised this because some of the examples are over 80 characters in the source code. I think most instances of this are because of the indenting before an example is already 12 characters. I don't think splitting from sage.crypto.key_exchange.diffie_hellman import DiffieHellman into two lines in the examples will help readability. I'm not sure there's much to be done for the tests where the return values are over 80 characters. I assume this is why the linter doesn't enforce the 80 character rule.

JosePisco

I can't find anything specific. Since the docstring EXAMPLES go into the documentation, I think it's most important that they're readable there.

I'm guessing you raised this because some of the examples are over 80 characters in the source code. I think most instances of this are because of the indenting before an example is already 12 characters. I don't think splitting from sage.crypto.key_exchange.diffie_hellman import DiffieHellman into two lines in the examples will help readability. I'm not sure there's much to be done for the tests where the return values are over 80 characters. I assume this is why the linter doesn't enforce the 80 character rule.

Yes this would make sense and formatting examples would probably make them less readable for the next person.

The rest of the code looks really neat 👍

vincentmacri · 2024-07-18T16:27:06Z

I can't find anything specific. Since the docstring EXAMPLES go into the documentation, I think it's most important that they're readable there.
I'm guessing you raised this because some of the examples are over 80 characters in the source code. I think most instances of this are because of the indenting before an example is already 12 characters. I don't think splitting from sage.crypto.key_exchange.diffie_hellman import DiffieHellman into two lines in the examples will help readability. I'm not sure there's much to be done for the tests where the return values are over 80 characters. I assume this is why the linter doesn't enforce the 80 character rule.

Yes this would make sense and formatting examples would probably make them less readable for the next person.

The rest of the code looks really neat 👍

Thanks for the review!

The last commit was just changing "public key exchange" to "key exchange" which I realized is the more standard terminology.

vincentmacri · 2024-07-29T16:07:33Z

@grhkm21 Sorry for the ping, but could I get a review on this?

grhkm21

comments. I have a few more comments (I think) but I have to test your code first, and my Sage is building now.

grhkm21 · 2024-07-29T22:40:10Z

src/sage/crypto/key_exchange/all.py

+from sage.misc.lazy_import import lazy_import
+
+lazy_import('sage.crypto.key_exchange.diffie_hellman', 'DiffieHellman')
+del lazy_import


Didn't think of this before, but seems like a good idea!

I just copied from the crypto/public_key folder for this.

I'm now thinking I may have done this wrong (and maybe public_key did too but I think that's out of scope for this PR, public_key needs some refactoring I think). I'm going to do some more testing of this.

I was doing it wrong. After the commit I just pushed, DiffieHellman can now be accessed without the user needing to explicitly import it.

This does raise the question of how we want the schemes to be accessed by the user. Most people probably won't use this feature (although I guess that applies to most things in Sage) so maybe it makes sense to access it as something like crypto.DiffieHellman, to avoid polluting the auto-suggest feature in some interfaces, but several of the existing symmetric ciphers are just accessed directly so I've done the same for consistency.

Happy to change it to something like crypto.DiffieHellman or key_exchange.DiffieHellman if that's preferred though.

Right now I have it so DiffieHellman is accessible without import, but KeyExchangeScheme requires an import. I don't think an abstract base class needs to be exposed to users without them explicitly importing it.

For my PR of a similar nature, I created a catalog file, see here. (Suggested by Travis, another Sage contributor)

I think having key_exchange in global scope (I think?) is a bit questionable. Do you think it's a good idea?

I think it's less questionable than SubstitutionCryptosystem being global scope (which it currently is).

I don't think it's too different than how distributions is global scope in the PR you linked. The alternative (which maybe is better) would be to access the key exchange schemes under crypto.key_exchange, so you'd do crypto.key_exchange.DiffieHellman. We could then in the future move the other cryptography code to crypto.[something] as well and take things like SubstitutionCryptosystem out of global scope.

After writing that I've convinced myself that it should be changed to crypto.key_exchange.

And the reason I didn't consider putting it under crypto directly (so crypto.DiffieHellman) is some schemes (like RSA) have multiple uses. RSA can be used for public key encryption, key exchange, and cryptographic signing. I think having something like crypto.pke.RSA, crypto.key_exchange.RSA, and crypto.signing.RSA would be better than crypto.RSAEncryption, crypto.RSAKeyExchange, and crypto.RSASign. Especially because for discoverability, a user is more likely to want to know "what key exchange schemes are in Sage" than "what RSA variants are in Sage". Thinking about the future and how many cryptography schemes (public key encryption, key exchange, signing, symmetric ciphers, etc.) could be implemented in Sage, putting it all under crypto would be too much.

I think crypto.key_exchange.DiffieHellman looks good (definitely not crypto.DiffieHellman), but again I don't have a strong opinion.

After writing that I've convinced myself that it should be changed to crypto.key_exchange.

Lol, it happens :)

Okay, I realized that making it accessible under crypto.key_exchange would involve some major restructuring to the crypto module and I think that would be out of scope for this PR. Unless I'm forgetting some syntax, I don't think Python import statements let you do from a.b.c import x as y.z. Basically above I was advocating to change from sage.crypto.all import * to import sage.crypto.all as crypto in the all.py file in src/sage which currently only stats does (but stats still imports everything to global scope anyway).

For now I think it's fine to leave it as key_exchange in global scope. It's at least better than what we have for other stuff in the crypto module. But if you have other suggestions I'm open.

Okay if it's too much irrelevant stuff for this PR then it's okay to leave it (and optionally make a new issue that hopefully someone gets to.)

src/sage/crypto/key_exchange/diffie_hellman.py

Co-authored-by: grhkm21 <[email protected]>

grhkm21 · 2024-07-30T22:00:55Z

I'll look through it again once but I think it's okay. I can approve when you reply to the {crypto.}key_exchange.DiffieHellman scope thing

grhkm21

LGTM. Looking forward to implementing some crypto protocols myself too! ~~Get people away from Magma~~ x)

grhkm21 · 2024-07-31T00:07:14Z

conda 3.11 CI is failing but clearly the errors are irrelevant.

vincentmacri · 2024-07-31T17:24:26Z

Thanks for the review! By the way, do you know what needs to happen for the PR status to change from "needs review" to "positive review"?

Looking forward to implementing some crypto protocols myself too!

To avoid duplicated work, I'll say that I'll probably be doing ECDH next, then CSIDH.

grhkm21 · 2024-07-31T19:01:15Z

@vincentmacri As long as there is positive review (and not disputed e.g. one positive one negative) then you or the reviewers can change it to positive review. I forgot to, thanks for the reminder.

Regarding the crypto, I have some CSIDH code lying around too if you want them.

vincentmacri · 2024-07-31T19:08:05Z

@vincentmacri As long as there is positive review (and not disputed e.g. one positive one negative) then you or the reviewers can change it to positive review. I forgot to, thanks for the reminder.

Thanks! I don't think I have the permission to set labels so I can't do it myself.

Regarding the crypto, I have some CSIDH code lying around too if you want them.

Sure!

sagemathgh-38374: Add framework for key exchange schemes and Diffie-Hellman Motivated by (but does not yet close) sagemath#37305. Closes sagemath#11568. This PR adds a basic framework to add public key exchange schemes to Sage, and includes an implementation of the Diffie-Hellman primarily as an example of this new framework. Open to suggestions to improve the structure of the code. This code was based on the existing code for public-key encryption in Sage. ### 📝 Checklist  - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies None URL: sagemath#38374 Reported by: Vincent Macri Reviewer(s): grhkm21, grnx, Vincent Macri

Initial work on public key exchange in Sage

62787b2

vincentmacri changed the title ~~First work to add public key exchange schemes~~ Add framework for public key exchange schemes and Diffie-Hellman Jul 15, 2024

vincentmacri added 2 commits July 15, 2024 17:16

Fix formatting error in references

0b2d8ab

Style and doc date

2655712

vincentmacri commented Jul 15, 2024

View reviewed changes

src/doc/en/reference/references/index.rst Outdated Show resolved Hide resolved

Fix annotations and add missing check

73bffc6

vincentmacri added 7 commits July 16, 2024 14:18

References

c183db2

Missing tests

cc513e7

Formatting

5ee6d0a

Cleanup and tests

4cb006f

Rename file

16d3d98

LaTeX doctest

f0a8ea0

Style fix

bacebe9

vincentmacri marked this pull request as ready for review July 16, 2024 21:29

github-actions bot added s: needs review v: moderate labels Jul 16, 2024

Fix doc formatting

b12addd

vincentmacri force-pushed the pke branch from b349a09 to b12addd Compare July 16, 2024 22:24

JosePisco suggested changes Jul 17, 2024

View reviewed changes

Apply suggestions from code review

6f7b5e2

Co-authored-by: grnx <[email protected]>

vincentmacri added 4 commits July 17, 2024 10:47

Formatting for when referring to the DiffieHellman class rather than …

f7b0744

…the key exchange scheme

Add missing test

6b86174

Formatting and type annotation fixes

a97866a

String formatting

babeb3c

JosePisco approved these changes Jul 18, 2024

View reviewed changes

Use more standard wording

6298553

vincentmacri changed the title ~~Add framework for public key exchange schemes and Diffie-Hellman~~ Add framework for key exchange schemes and Diffie-Hellman Jul 18, 2024

vincentmacri added 3 commits July 18, 2024 10:58

Clearer wording

7801f9d

Merge branch 'develop' into pke

7790037

Merge branch 'develop' into pke

f5491a2

grhkm21 suggested changes Jul 29, 2024

View reviewed changes

vincentmacri and others added 7 commits July 30, 2024 11:21

Apply suggestions from code review

f74d913

Co-authored-by: grhkm21 <[email protected]>

Add reference to RFC

474e09d

Refactoring/cleanup

441fdfc

Import tweaks

d58532b

Fix tests

cdb0962

Doc formatting and date update

e11ad4e

Formatting consistency

5726da8

vincentmacri requested a review from grhkm21 July 30, 2024 20:16

Use catalog file

fc1a4dc

grhkm21 approved these changes Jul 31, 2024

View reviewed changes

grhkm21 added s: positive review and removed s: needs review labels Jul 31, 2024

vbraun merged commit 316f656 into sagemath:develop Aug 3, 2024

github-actions bot removed the s: positive review label Aug 3, 2024

vincentmacri deleted the pke branch August 6, 2024 21:47

Uh oh!

Add framework for key exchange schemes and Diffie-Hellman #38374

Add framework for key exchange schemes and Diffie-Hellman #38374

Uh oh!

Conversation

vincentmacri commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Checklist

⌛ Dependencies

Uh oh!

vincentmacri commented Jul 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JosePisco left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

grhkm21 commented Jul 17, 2024

Uh oh!

vincentmacri commented Jul 17, 2024

Uh oh!

JosePisco left a comment

Choose a reason for hiding this comment

Uh oh!

vincentmacri commented Jul 18, 2024

Uh oh!

vincentmacri commented Jul 29, 2024

Uh oh!

grhkm21 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincentmacri Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

grhkm21 commented Jul 30, 2024

Uh oh!

grhkm21 left a comment

Choose a reason for hiding this comment

Uh oh!

vincentmacri commented Jul 15, 2024 •

edited

Loading

github-actions bot commented Jul 16, 2024 •

edited

Loading

JosePisco left a comment •

edited

Loading

vincentmacri Jul 30, 2024 •

edited

Loading