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

[HOLD] Upgrade mbedtls to 3.5.2 #1802

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

Conversation

johnmlee101
Copy link
Contributor

@johnmlee101 johnmlee101 commented Jul 9, 2024

Turns out 3.6.0 doesn't work with TLS v1.3

Was trying to figure this out for awhile and got errors like Mbed-TLS/mbedtls#9223

and internal errors, but once I downgraded to 3.5.2 it worked instantly.

The rest is optimizations and improvements to move to 3.0+ but we'll still need to upgrade at some point to a higher version once they fix the issues.

@johnmlee101 johnmlee101 self-assigned this Jul 9, 2024
@johnmlee101 johnmlee101 changed the title [HOLD] Upgrade mbedtls to 3.6.0 [HOLD] Upgrade mbedtls to 3.5.2 Jul 9, 2024
@johnmlee101 johnmlee101 requested review from tylerkaraszewski and a team July 9, 2024 21:12
@melvin-bot melvin-bot bot requested review from Julesssss and removed request for a team July 9, 2024 21:13
Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

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

I'm not following all of these changes, are the private SSL changes necessary for the newer version of mbedtls?

@johnmlee101
Copy link
Contributor Author

Will need to make sure Auth and the other plugins work with this as well, which is why its on hold before we merge

@johnmlee101
Copy link
Contributor Author

are the private SSL changes necessary for the newer version of mbedtls?

Yeah they explicitly make sure its accessed internally or externally with private_ prefixes now

@johnmlee101
Copy link
Contributor Author

@johnmlee101
Copy link
Contributor Author

Okay Fuzzybot, ExpensifyTableManager, ExpensifyBackupManager all compile and run with the updated version, so we'll want to merge this first, merge auth, then deploy asap so that auth using the bedrock prod branch doesn't break things

@tylerkaraszewski
Copy link
Contributor

Approved this, but letting you figure out the required deploy dance.

@johnmlee101
Copy link
Contributor Author

Oh do you mind approving https://github.com/Expensify/Auth/pull/11565 as well then?

@tylerkaraszewski
Copy link
Contributor

Done.

@Julesssss
Copy link
Contributor

Is this still held @johnmlee101?

@johnmlee101
Copy link
Contributor Author

yes, since this will require a specific deployment order to prevent auth from crashing in tests

@johnmlee101
Copy link
Contributor Author

I'm going to be OOO, and I don't think this is too time-sensitive, so I might resume this once I'm back, and since its also Friday I don't want to force a deploy today.

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

Successfully merging this pull request may close these issues.

None yet

3 participants