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.

Sign up for GitHub

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

Jump to bottom

Remove libcrypto (use tutasdk RSA impl for iOS/Android) #7344

Merged
merged 4 commits into from
Aug 16, 2024
Merged

Remove libcrypto (use tutasdk RSA impl for iOS/Android) #7344

merged 4 commits into from
Aug 16, 2024

Conversation

paw-hub
Copy link
Contributor

@paw-hub paw-hub commented Aug 6, 2024

Closes #6603

@paw-hub paw-hub changed the title Kick libcrypto Kick libcrypto (use tutasdk RSA impl for iOS) Aug 6, 2024
@paw-hub paw-hub force-pushed the kick-libcrypto branch 4 times, most recently from 7898bf1 to a95edf8 Compare August 6, 2024 13:57
@paw-hub paw-hub force-pushed the kick-libcrypto branch 7 times, most recently from c3a0d6a to 746ccf4 Compare August 6, 2024 16:00
@paw-hub paw-hub marked this pull request as ready for review August 6, 2024 16:01
@paw-hub paw-hub force-pushed the kick-libcrypto branch 2 times, most recently from 027ee99 to c8c3b27 Compare August 6, 2024 16:30
@paw-hub
Copy link
Contributor Author

paw-hub commented Aug 6, 2024 •
edited
Loading

A couple comments after quickly looking over all of this:

  • Should we be passing key components by value? We don't do this for Kyber, although with Kyber, we use the encoded form. Changing the code to pass the encoded form for RSA would probably be pain. As it is now is probably fine.
  • SimpleRSAError could probably be RSAError. I made it SimpleRSAError to differentiate from KyberError since that is pub and I thought it was used external to simple_crypto, but it's actually not. edit: this is now fixed

@paw-hub paw-hub changed the title Kick libcrypto (use tutasdk RSA impl for iOS) Kick libcrypto (use tutasdk RSA impl for iOS/Android) Aug 6, 2024
@paw-hub paw-hub mentioned this pull request Aug 7, 2024
Closed
@paw-hub paw-hub linked an issue Aug 7, 2024 that may be closed by this pull request
Use tutasdk RSA impl for iOS and Android #6603
Closed
7 tasks
@paw-hub paw-hub force-pushed the kick-libcrypto branch 3 times, most recently from f7a35fb to 3fb9b6b Compare August 7, 2024 10:12
charlag
charlag reviewed Aug 8, 2024
View reviewed changes
tuta-sdk/rust/sdk/src/simple_crypto.rs
let prime_p = Zeroizing::new(prime_p);
let prime_q = Zeroizing::new(prime_q);

let modulus = Zeroizing::new(BASE64_STANDARD.decode(modulus).map_err(|_| RSAError::InvalidKey { reason: "modulus is not valid base64".to_owned() })?);
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like all the base64 should be done outside but also this is how RSA keys always are so maybe it's fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It probably shouldn't be in base64 to begin with. I'm fairly certain we don't store it as base64, and it is just an intermediate format on the TypeScript side.

We could move the base64 decoding to the TypeScript side, but since this method will anyway be invoked from TypeScript, it doesn't really change anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

I assumed we store them like this but turns out it's really us??

tutanota/packages/tutanota-crypto/lib/encryption/Rsa.ts

Line 341 in 016286b

function _arrayToPrivateKey(privateKey: BigInteger[]): RsaPrivateKey {

whyyy

we should get rid of it at some point

tuta-sdk/rust/sdk/src/simple_crypto.rs Show resolved Hide resolved
charlag
charlag reviewed Aug 8, 2024
View reviewed changes
app-ios/TutanotaSharedFramework/Crypto/AES.swift Outdated Show resolved Hide resolved
tuta-sdk/rust/sdk/src/crypto/rsa.rs Outdated Show resolved Hide resolved
@paw-hub paw-hub mentioned this pull request Aug 8, 2024
Closed
@tutao-mac tutao-mac force-pushed the kick-libcrypto branch 2 times, most recently from 87aff17 to 1570c6a Compare August 9, 2024 08:29
@paw-hub paw-hub force-pushed the kick-libcrypto branch 4 times, most recently from fbc3b43 to 9801633 Compare August 15, 2024 12:13
@paw-hub paw-hub changed the base branch from dev-mail to master August 15, 2024 12:14
@paw-hub paw-hub changed the base branch from master to dev-mail August 15, 2024 12:14
@paw-hub
[ios] Remove libcrypto; use tutasdk RSA impl
61d1243 
Our usage of libcrypto (OpenSSL) had a lot of issues. For example, it
was compiled a long time ago and the only Macs it ran on were x86 Macs.
As such, we were unable to run tests on newer ARM-based Macs.

We had a recompiled version made a long time ago that worked on new Mac,
but it was postponed, and now that it is months out of date and uses the
old build system (xcodeproj instead of xcodegen), it is way less work to
just switch to using the SDK. We do this with Kyber anyway...

Closes #6603
@paw-hub
[ios] Use macOS 14+ARM in workflow; bump xcodeVersion
4676b05 
We no longer use libcrypto in builds, so we are not beholden to an x86
dependency for simulator.

Bumping xcode version also fixes PinsStorage issue from earlier.
@paw-hub
[android] Use tutasdk RSA impl in Android
e2c9eee 
This brings it in line with the earlier iOS change. It also means we can
test encryption results, since seeds are accepted.
@paw-hub paw-hub merged commit e2c9eee into dev-mail Aug 16, 2024
4 checks passed
@paw-hub paw-hub deleted the kick-libcrypto branch August 16, 2024 09:00
@charlag charlag added this to the 240.240816.0 + milestone Aug 26, 2024
@charlag charlag added the state:done meets our definition of done label Sep 3, 2024
@charlag charlag changed the title Kick libcrypto (use tutasdk RSA impl for iOS/Android) Remove libcrypto (use tutasdk RSA impl for iOS/Android) Sep 3, 2024
@charlag charlag added android issues that concern the android client but not all clients ios issues that concern the ios client but not all clients labels Sep 3, 2024
@charlag charlag removed this from the 244.240903.1 milestone Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charlag charlag charlag left review comments

Assignees
No one assigned
Labels
android issues that concern the android client but not all clients ios issues that concern the ios client but not all clients state:done meets our definition of done
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use tutasdk RSA impl for iOS and Android
2 participants
@paw-hub @charlag