Add RSA support on encodeString #2412

Inder00 · 2021-10-12T12:56:16Z

Fixes #2097
This pull request adds RSA Support.

generateKeyPair

string, string generateKeyPair( string algorithm, table options, [ function callback ])

Available algorithms: rsa
Return strings representing private and public key

RSA on encode/decodeString

string encodeString( "rsa", string, { key = publicKey })
string decodeString( "rsa", string, { key = privateKey})

Resource for testing: rsatest.zip
Thank you.

Shared/mods/deathmatch/logic/luadefs/CLuaCryptDefs.cpp

Inder00 · 2021-10-15T16:54:01Z

Changed enum name because of #2016 (comment)

Shared/mods/deathmatch/logic/luadefs/CLuaCryptDefs.cpp

Shared/sdk/SharedUtil.Crypto.h

Shared/mods/deathmatch/logic/luadefs/CLuaCryptDefs.cpp

Shared/sdk/SharedUtil.Tests.hpp

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

Pirulax · 2021-10-17T17:14:31Z

I think the returned key should be in hex for consistency with other stuff we already have (like md5) What do others think?

React with emojis.

AlexTMjugador · 2021-10-17T21:51:08Z

I think that representing keys with hexadecimal characters in this PR would go against the precedents set by the encodeString and teaEncode functions to treat encryption keys as binary strings. Therefore, in my opinion, treating keys as binary strings in this PR minimizes Lua API perplexity.

However, I agree that hash functions should continue to return hexadecimal strings for their results.

Pirulax

LGTM if the test resource passes.

Shared/mods/deathmatch/logic/luadefs/CLuaCryptDefs.cpp

sbx320

Some minor things.

Also we need some nicer way of handling async callbacks... too much duplicaiton here. I'll have a look for getting a generalized solution for it going. Doesn't matter for this PR though.

Shared/mods/deathmatch/logic/luadefs/CLuaCryptDefs.cpp

Shared/sdk/SharedUtil.Tests.hpp

Shared/mods/deathmatch/logic/luadefs/CLuaCryptDefs.cpp

Inder00 · 2021-10-21T15:47:43Z

@sbx320 review

Shared/mods/deathmatch/logic/luadefs/CLuaCryptDefs.cpp

Inder00 · 2021-10-22T13:01:47Z

To the moon 🚀

Shared/mods/deathmatch/logic/luadefs/CLuaCryptDefs.cpp

Inder00 · 2021-10-25T17:12:54Z

To the moon 🚀 (review)

AlexTMjugador · 2021-11-02T21:53:41Z

Shared/mods/deathmatch/logic/luadefs/CLuaCryptDefs.cpp

+                                catch (const CryptoPP::Exception& ex)
+                                {
+                                    return std::make_pair(SString(ex.GetWhat()), false);
+                                }


This is better exception handling than just ignoring it and returning false. However, I'm not sure if we want to expose scripts to the implementation-defined messages of CryptoPP. According to the docs, it should be feasible to get an error type and control what string is logged ourselves.

That was @Pirulax idea to expose exception cause

I agree with exposing the exception cause somehow. But I'm not sure if we want to potentially make backwards compatibility more complicated just because some script decided to rely on some precise warning text. This is a pretty minor and far-fetched concern, though, and I'd be happy with this merged as-is.

AlexTMjugador · 2021-11-02T22:01:01Z

About the removed tests, I think @sbx320 was not pointing that they were unnecessary and should be removed, but that they were not that useful either because they just tested the entire encryption-decryption cycle, and as such a trivial identity function implementation would also satisfy those tests.

I think that tests are useful for this PR and belong to it, as long as they test encryption and decryption separately: at least one test should test that encryption of a known plaintext with a known key-pair and cipher parameters yields the expected ciphertext, and at least other test should check that decryption of another ciphertext is able to yield back the plaintext.

Inder00 · 2021-11-07T03:55:59Z

About the removed tests, I think @sbx320 was not pointing that they were unnecessary and should be removed, but that they were not that useful either because they just tested the entire encryption-decryption cycle, and as such a trivial identity function implementation would also satisfy those tests.

I think that tests are useful for this PR and belong to it, as long as they test encryption and decryption separately: at least one test should test that encryption of a known plaintext with a known key-pair and cipher parameters yields the expected ciphertext, and at least other test should check that decryption of another ciphertext is able to yield back the plaintext.

Test has been applied:

Encryption: encrypts data using random keypair (encrypt, decrypt and then compare) - encrypt cannot be compared as static data because used RSA encryption scheme OAEP using SHA outputs different encrypted data each time - have to be decrypted to compare.
Decryption: decrypts encrypted data in string using private key stored in another string and compare decypted data with expected data.

AlexTMjugador

Design-wise this looks fine to me, but I have not tested it, and other people might have some stylistic feedback.

Inder00 · 2021-11-08T02:56:20Z

Design-wise this looks fine to me, but I have not tested it, and other people might have some stylistic feedback.

Thanks for review, a test resource is included in pr 🚀

Inder00 · 2021-12-21T12:29:48Z

Review someone 🔥

TheNormalnij · 2021-12-21T19:28:37Z

Code looks well now. I think it should be refactored into "strategy" pattern with next added algorithm.

Github bug? I don't see unresolved thread

sbx320 · 2021-12-22T02:56:29Z

@TheNormalnij not a bug.

Github keeps it as "Changes Requested" regardless of unresolved threads, as there may be changes requested in a top level comment.

This reverts commit e7e3ba5.

Inder00 added 2 commits October 12, 2021 14:21

RSA support

1ecf95f

RSA Support (Tests)

e2da886

StrixG added the enhancement New feature or request label Oct 12, 2021

CrosRoad95 reviewed Oct 12, 2021

View reviewed changes

Shared/mods/deathmatch/logic/luadefs/CLuaCryptDefs.cpp Outdated Show resolved Hide resolved

Inder00 added 3 commits October 12, 2021 18:21

Removed unnecessary rsa key size check

25d777a

Fix typo

289c429

Change enum StringEncryptFunction to StringEncodeFunction

f024d09

Pirulax requested changes Oct 17, 2021

View reviewed changes

Pirulax code review suggestions

034fdd7

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

Code review fixes

16b8aef

Inder00 requested a review from Pirulax October 17, 2021 22:17

Pirulax approved these changes Oct 18, 2021

View reviewed changes

Shared/mods/deathmatch/logic/luadefs/CLuaCryptDefs.cpp Outdated Show resolved Hide resolved

Replace lua expressions with lua namespace

3a0d7b4

sbx320 previously requested changes Oct 18, 2021

View reviewed changes

Shared/mods/deathmatch/logic/luadefs/CLuaCryptDefs.cpp Outdated Show resolved Hide resolved

Shared/mods/deathmatch/logic/luadefs/CLuaCryptDefs.cpp Outdated Show resolved Hide resolved

Shared/sdk/SharedUtil.Tests.hpp Outdated Show resolved Hide resolved

Inder00 added 3 commits October 18, 2021 19:47

Remove unnecessary RSA test

72cf3de

Remove annoying keysize warning

f3a9bd0

Change keysize limitations

27adca9

Inder00 requested a review from sbx320 October 18, 2021 18:31

Expose exception cause to debugscript warning

1102a9c

Inder00 requested a review from Pirulax October 20, 2021 20:46

Pirulax reviewed Oct 21, 2021

View reviewed changes

Shared/mods/deathmatch/logic/luadefs/CLuaCryptDefs.cpp Show resolved Hide resolved

Inverted result logic inside async block

4d80765

Refactor generateRsaKeyPair function

01b622d

tederis reviewed Oct 22, 2021

View reviewed changes

Shared/mods/deathmatch/logic/luadefs/CLuaCryptDefs.cpp Outdated Show resolved Hide resolved

Inder00 added 2 commits October 22, 2021 13:13

Adds missing const modifer inside async block

af86fc0

Refactor generateRsaKeyPair to generateKeyPair for future compatible

dfd83b7

Inder00 requested a review from Pirulax October 22, 2021 12:59

tederis requested changes Oct 22, 2021

View reviewed changes

Shared/mods/deathmatch/logic/luadefs/CLuaCryptDefs.cpp Outdated Show resolved Hide resolved

Shared/mods/deathmatch/logic/luadefs/CLuaCryptDefs.cpp Outdated Show resolved Hide resolved

Shared/mods/deathmatch/logic/luadefs/CLuaCryptDefs.cpp Outdated Show resolved Hide resolved

Fix missing references

352260c

Pirulax reviewed Oct 22, 2021

View reviewed changes

Shared/mods/deathmatch/logic/luadefs/CLuaCryptDefs.cpp Outdated Show resolved Hide resolved

Inder00 added 2 commits October 22, 2021 17:33

Remove key size and expose exception cause

f62c895

Fix sync key pair generation

277baba

AlexTMjugador reviewed Nov 2, 2021

View reviewed changes

RSA Debug Tests

c652a22

Inder00 force-pushed the feature/cryptopp_rsa branch from fdfd13c to c652a22 Compare November 7, 2021 04:11

AlexTMjugador approved these changes Nov 7, 2021

View reviewed changes

TheNormalnij merged commit e7e3ba5 into multitheftauto:master Dec 21, 2021

TheNormalnij added a commit to TheNormalnij/mtasa-blue that referenced this pull request Dec 22, 2021

Revert "Add RSA support on encodeString (multitheftauto#2412)"

d989476

This reverts commit e7e3ba5.

patrikjuvonen added this to the Next Release (1.6.0) milestone Aug 24, 2022

Uh oh!

Add RSA support on encodeString #2412

Add RSA support on encodeString #2412

Uh oh!

Conversation

Inder00 commented Oct 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

generateKeyPair

RSA on encode/decodeString

Uh oh!

Uh oh!

Inder00 commented Oct 15, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Pirulax commented Oct 17, 2021

Uh oh!

AlexTMjugador commented Oct 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pirulax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbx320 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!

Inder00 commented Oct 21, 2021

Uh oh!

Uh oh!

Inder00 commented Oct 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Inder00 commented Oct 25, 2021

Uh oh!

AlexTMjugador Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

Inder00 Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

AlexTMjugador Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

AlexTMjugador commented Nov 2, 2021

Uh oh!

Inder00 commented Nov 7, 2021

Uh oh!

AlexTMjugador left a comment

Choose a reason for hiding this comment

Uh oh!

Inder00 commented Nov 8, 2021

Uh oh!

Inder00 commented Dec 21, 2021

Uh oh!

TheNormalnij commented Dec 21, 2021

Uh oh!

sbx320 commented Dec 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Inder00 commented Oct 12, 2021 •

edited

Loading

AlexTMjugador commented Oct 17, 2021 •

edited

Loading

sbx320 left a comment •

edited

Loading