chore: x509 refactor, part 11: The Valley Of Horrors#2116
Open
istankovic wants to merge 56 commits intomainfrom
Open
chore: x509 refactor, part 11: The Valley Of Horrors#2116istankovic wants to merge 56 commits intomainfrom
istankovic wants to merge 56 commits intomainfrom
Conversation
istankovic
changed the title
istankovic
force-pushed
the
ivan/x509-part-11
branch
from
bfb1476 to
333f83f
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 doesn't really make sense and it calls mls_init twice which probably is not a good idea.
We don't have federation/cross-signing tests in crypto anymore.
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.
istankovic
force-pushed
the
ivan/x509-part-11
branch
from
333f83f to
6906abf
Compare
We have
pki_environment: Arc<RwLock<Option<Arc<PkiEnvironment>>>>
^ ^ ^ ^
| | | |
| | | `----- because of FFI: 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.
istankovic
force-pushed
the
ivan/x509-part-11
branch
from
6906abf to
cbfbde1
Compare
istankovic
force-pushed
the
ivan/x509-part-11
branch
from
cbfbde1 to
c4ee6b1
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
1 participant
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.