refactor: externalize Arcs and make CoreCrypto: !Clone [WPB-25220]#2107
Open
coriolinus wants to merge 67 commits into
Open
refactor: externalize Arcs and make CoreCrypto: !Clone [WPB-25220]#2107coriolinus wants to merge 67 commits into
Arcs and make CoreCrypto: !Clone [WPB-25220]#2107coriolinus wants to merge 67 commits into
Conversation
coriolinus
force-pushed
the
prgn/refactor/25220-cc-not-clone
branch
from
0ebe48f to
889c6ad
Compare
We're going to eventually remove the inner PKI environment and this is the first step towards that. The goal is to have any code external to the e2e-identity crate be sure that if it has a reference to the outer PKI environment, it can verify X509 credentials. In particular, the crypto crate knows whether a PKI environment has been configured by user code. This also allows us to simplify e2e-identity internally because if we have a reference to the outer PKI environment, we know we also have the inner one and can proceed without any checks.
We're not going to implement openmls auth service trait, leaving that to the crypto crate. This crate should merely provide a way to validate X509 credentials, which is now done via PkiEnvironment::validate_credential. Calling that function with a basic credential is a programmer error and will panic. Ideally, we would have a way to ensure correct credential type at compile time, but this is good enough for now.
We are going to be computing time of interest (TOI) as needed, when we do certificate validation. External users will not be able to set arbitrary TOI. This will also allows us to drop the `toi` field from the (inner) PkiEnvironment struct.
And use it in places where TOI is needed.
TOI is computed internally during validation.
This is only temporary for testing in the crypto crate. It is going to go away eventually.
Also use the outer PKI environment wherever possible. We now know if we have a reference to the outer PKI env, there is also the inner PKI env. This simplifies a number of places.
This just forwards to the inner PKI environment. The idea is that we move all users of the inner env to the outer env so we can properly hide the inner env, and eventually remove it.
They don't make sense anymore as most x509 things are moving to the e2e-identity crate.
This functionality is now in the e2e-identity crate.
This is not necessary since clients can just check where the PKI env is set.
Nothing there is used anymore.
Amazingly, this is unused.
It is now unused.
We have
pki_environment: Arc<RwLock<Option<Arc<PkiEnvironment>>>>
^ ^ ^ ^
| | | |
| | | `----- because of FFI and PkiEnvironment: !Clone; the same
| | | PKI env instance must be referenced by foreign language
| | | objects and Rust
| | |
| | `----------- because clients can set the PKI env to None
| |
| `------------------ because of FFI: we have to use interior mutability
| since uniffi does not support &mut self references
|
`----------------------- because we need CoreCrypto: Clone
(instances are shared in tests)
This is purely for testing.
These tests previously worked with the inner PKI env directly, but we've moved everything to the outer PKI env so we also need to adjust the tests.
Trying to create a new db transaction may result in a deadlock if there is already one in progress.
…vironment It is unused.
This is only temporary, until we get x509 tests fully working.
This is only temporary, until we get x509 tests fully working.
…sync This is really not ideal. It's caused by IdentityStatus::from_cert being async, which in turn is caused by cert validation on PKI env being async (has to be because the inner PKI env is behind an async mutex). So, sadly, we cannot avoid it without refactoring the whole User/Device identity status stuff, which we should do eventually, also for other reasons. But not today.
We already have one at the top of the file.
istankovic
force-pushed
the
ivan/x509-part-11
branch
from
c671834 to
ed52adb
Compare
We use the "immediate" API because if there's already a transaction in progress, we want to get an error, instead of risking a deadlock.
istankovic
force-pushed
the
ivan/x509-part-11
branch
from
ed52adb to
01391f3
Compare
We don't actually need or want this, essentially ever. So let's get rid of it. What we really want is a single Arc which captures the whole thing.
We want an `Arc<CoreCrypto>` essentially every time instead of cloning; this implements that.
We can use much simpler types than we had been, and that makes life better.
Given that we simplified the return values, we now have to adapt the call sites.
…onService` These smart pointers can all be externalized to simplify the types and reduce the number of pointless duplication of ref-counters.
…nticationService`
coriolinus
force-pushed
the
prgn/refactor/25220-cc-not-clone
branch
from
6041ab6 to
5ae1939
Compare
istankovic
force-pushed
the
ivan/x509-part-11
branch
2 times, most recently
from
3bdfe0d to
eba0fdd
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's new in this PR
Note: we'd do something similar for
Database, externalizing its internalArcs, but that ends up with a problem. For context, eachCoreCryptoinstance + several other places need to store anArc<Database>, not just aDatabase, because we have to copy the database around into a bunch of places. Unfortunately, the ProteusPreKeyStoretrait takes mutable references, which would mean that all those places need to store some kind ofArc<RwLock<Database>>, and at that point the cost-benefit analysis kind of stops being worth it.PR Submission Checklist for internal contributors
SQPIT-764feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.