chore: x509 refactor, part 11: The Valley Of Horrors#2116
Open
istankovic wants to merge 59 commits intomainfrom
Open
chore: x509 refactor, part 11: The Valley Of Horrors#2116istankovic wants to merge 59 commits intomainfrom
istankovic wants to merge 59 commits intomainfrom
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
| } | ||
| } else { | ||
| return CredentialAuthenticationStatus::Unknown; | ||
| pub async fn validate_credential<'a>( |
Contributor
There was a problem hiding this comment.
I hope that later on in this PR you go through and add docs to all these public functions. The knowledge that passing a basic credential is a programmer error which will panic is critical, and exactly like what we'd want to see documented in inline docs and not just the commit message.
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
2 times, most recently
from
5264aa0 to
1f15539
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.
All certificates are considered local.
It is unused.
It is unused.
This is ugly, but actually fine because even though extract_identity calls IdentityStatus::from_cert, which makes use of the env to validate the certificate, here we're not actually inspecting the status field. Note that we actually do certificate validation just before the call to extract_identity. That validation (correctly) takes place in the freshly created inner environment, that contains just the trust anchors from the outer environment, plus all intermediates that we received from the ACME server with this leaf certificate.
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.
istankovic
force-pushed
the
ivan/x509-part-11
branch
from
1f15539 to
c671834
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.