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

Remove dependency on libolm #8901

Merged
merged 13 commits into from
Sep 23, 2024
Merged

Remove dependency on libolm #8901

merged 13 commits into from
Sep 23, 2024

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented Sep 11, 2024

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Remove the dependency on the deprecated libolm library

Motivation and context

Remove the dependency on the deprecated libolm library. See more details here:

Screenshots / GIFs

Tests

  • Check migration from a previous installation to a new one

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist


@OptIn(ExperimentalStdlibApi::class)
private fun String.toSha256(): String {
return sha256.digest(toByteArray()).toHexString()
Copy link
Member

@BillCarsonFr BillCarsonFr Sep 13, 2024

Choose a reason for hiding this comment

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

Found it surprising that we use toHexString here. The legacy sha was returning a base64 string no? https://gitlab.matrix.org/matrix-org/olm/-/blob/master/tests/test_olm_sha256.cpp#L16

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch thanks! f726d16

Copy link
Member

Choose a reason for hiding this comment

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

Thx, maybe just a nit, it could be named convertToSha256Base64Encoded

@bmarty
Copy link
Member Author

bmarty commented Sep 17, 2024

I have tested again the database migration and this is working fine:

  • Checkout the current develop
  • Deploy the app
  • Login to an account
  • Verify the session, observe that existing messages can be decrypted
  • Send some messages in e2e rooms
  • Checkout the last commit of the branch
  • Use locally build crypto SDK (release still need to be done)
  • Deploy the application to the same device
  • Check in the log that the migration has been done
  • Observe that the application is not crashing and is still working as expected

@bmarty bmarty added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Sep 18, 2024
Bumps [org.matrix.rustcomponents:crypto-android](https://github.com/matrix-org/matrix-rust-components-kotlin) from 0.4.1 to 0.4.3.
- [Release notes](https://github.com/matrix-org/matrix-rust-components-kotlin/releases)
- [Commits](matrix-org/matrix-rust-components-kotlin@crypto-v0.4.1...crypto-v0.4.3)

---
updated-dependencies:
- dependency-name: org.matrix.rustcomponents:crypto-android
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
@ElementBot
Copy link

ElementBot commented Sep 20, 2024

Warnings
⚠️

matrix-sdk-android/build.gradle#L164 - A newer version of net.java.dev.jna:jna than 5.13.0 is available: 5.14.0

⚠️

matrix-sdk-android/build.gradle#L164 - A newer version of net.java.dev.jna:jna than 5.13.0 is available: 5.14.0

Generated by 🚫 dangerJS against e89bec4

sha256Converter.convertToSha256(
str = threePid.value.lowercase(Locale.ROOT) + " " + threePid.toMedium() + " " + pepper
)
)
Copy link
Member Author

Choose a reason for hiding this comment

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

I confirm that this code works using vector.im identity server.

@bmarty
Copy link
Member Author

bmarty commented Sep 23, 2024

I have tested the communication to the content scanner running a local synapse and a local instance of https://github.com/element-hq/matrix-content-scanner-python and the content scanner is able to decrypt the attachment.

Patch to apply (for information / future me):

contentScannerTest.patch

Result testing data from a file sent to a room (eicar.com.bin):

image

Content scanner logs:

2024-09-23 16:43:45,834 - matrix_content_scanner.scanner.file_downloader - 323 - INFO - POST-9 - Sending GET request to http://localhost:8080/_matrix/media/v3/download/localhost/KFBpUrxgQyLQrGbxJBYEVPXe
2024-09-23 16:43:45,847 - matrix_content_scanner.scanner.file_downloader - 181 - INFO - POST-9 - Remote server responded with 200
2024-09-23 16:43:45,848 - matrix_content_scanner.scanner.scanner - 417 - INFO - POST-9 - Decrypting encrypted file
2024-09-23 16:43:45,905 - matrix_content_scanner.scanner.scanner - 514 - ERROR - POST-9 - MIME type for file is forbidden: text/plain
2024-09-23 16:43:45,906 - matrix_content_scanner.scanner.scanner - 260 - INFO - POST-9 - Caching scan failure
2024-09-23 16:43:45,907 - aiohttp.access - 211 - INFO - POST-9 - 127.0.0.1 [23/Sep/2024:15:43:45 +0100] "POST /_matrix/media_proxy/unstable/scan_encrypted HTTP/1.1" 200 369 "-" "Element - dbg/1.6.22-dev (Google sdk_gphone64_arm64; Android 14; UE1A.230829.036.A4; Flavour GooglePlay; MatrixAndroidSdk2 1.6.22)"

@bmarty bmarty marked this pull request as ready for review September 23, 2024 15:03
Copy link
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

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

Approved

@bmarty bmarty merged commit ac01523 into develop Sep 23, 2024
11 of 12 checks passed
@bmarty bmarty deleted the feature/bma/removeLibolm branch September 23, 2024 17:26
@bmarty
Copy link
Member Author

bmarty commented Sep 24, 2024

Note: checking application upgrade from 1.5.32 to 1.6.22 and we got a migration error then the application crashes. Here are the log:

io.realm.exceptions.RealmMigrationNeededException: Migration is required due to the following errors:
- Property 'CryptoRoomEntity.rotationPeriodMs' has been added.
- Property 'CryptoRoomEntity.rotationPeriodMsgs' has been added.

Upgrading from 1.6.0 (which is 1-year old+) to 1.6.22 is working well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-NextRelease For issues and PRs which should be included in the NextRelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants