chore: x509 refactor, part 11: The Valley Of Horrors#2116
Merged
Conversation
istankovic
changed the title
istankovic
force-pushed
the
ivan/x509-part-11
branch
4 times, most recently
from
cbfbde1 to
c4ee6b1
Compare
coriolinus
reviewed
May 8, 2026
istankovic
force-pushed
the
ivan/x509-part-11
branch
from
fa09a67 to
ef18ac2
Compare
coriolinus
reviewed
May 8, 2026
Contributor
coriolinus
left a comment
coriolinus
left a comment
There was a problem hiding this comment.
- commit 2797333 has a message which appears entirely unrelated to its contents
Contributor
|
And as mentioned in chat, it's not obvious why 4d60315 is necessary. If we can remove that, then we can remove several downstream changes. |
Member
Author
I admit it's not quite obvious, but if you look at the types being imported, you will see that the |
istankovic
force-pushed
the
ivan/x509-part-11
branch
3 times, most recently
from
1f15539 to
c671834
Compare
coriolinus
reviewed
May 11, 2026
3 tasks
istankovic
force-pushed
the
ivan/x509-part-11
branch
5 times, most recently
from
3bdfe0d to
eba0fdd
Compare
coriolinus
requested changes
May 13, 2026
Contributor
coriolinus
left a comment
coriolinus
left a comment
There was a problem hiding this comment.
I really want to get this PR into place, but the trust anchor stuff is underbaked. Could you just split the last commits into the start of part 12 so I can approve this?
coriolinus
approved these changes
May 13, 2026
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 finally removes the last remaining user of `mls_pki_env_provider`.
It is no longer used.
It is not needed anymore because `add_trust_anchor` does everything already.
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.
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.
It is now unused. We allow multiple trust anchors, which one can get via get_trust_anchors.
istankovic
force-pushed
the
ivan/x509-part-11
branch
from
6cdd314 to
911d137
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
2 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.
This comprises several changes:
AuthenticationServicethat is just a wrapper around a PKI env; the purposeof this type is linking the MLS parts of the
cryptocrate and PKI env/e2e-identityPkiEnvironmentProvidercryptocrate are disabled because they are broken with this PR. The idea is to fix them as part of a later PR.