-
Notifications
You must be signed in to change notification settings - Fork 11
feat: add Pki Environment API #1752
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
base: main
Are you sure you want to change the base?
Conversation
e9881d2 to
0e441d8
Compare
0e441d8 to
a10d72d
Compare
|
Except for vergen making problems with building, this is now only missing the swift test and support for the pki environment constructor in swift. For that we want to add a db.getLocation() function to the public api, then remove the swift database wrapper. |
a1eae2c to
1e65cdf
Compare
coriolinus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of comments, but most of them are nits. Great work!
I do think we should discuss whether the database instances can actually be independent or not. If they can't--as I now kind of think--then we might not need to keep track of a separate PKI DB after all. But if they can, then we might need more work in that case also:
- a
migrate_pki_data_to_new_databaseto be called once ever to move existing data from the old DB to the new - separate migrations for the new DB
crypto/src/e2e_identity/pki_env.rs
Outdated
| /// Only used to authenticate with the user's identity provider | ||
| async fn authenticate(&self, idp: String, key_auth: String, acme_aud: String) -> OAuthResponse; | ||
|
|
||
| /// Only used for DPoP challenge | ||
| async fn fetch_backend_access_token(&self, dpop: String) -> String; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you expand on the doc-comments for these methods a little? Could be I just don't have the right mental context right now, but if I were an implementer looking at this documentation I'd have a hard time figuring out what I was supposed to make these methods do.
| "Getting PKI environment from transaction context", | ||
| ))?; | ||
|
|
||
| let database = pki_environment.database(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that even though we've documented that the PKI environment database is theoretically independent of the CC database, it's not, because OpenMLS is still doing PKI things with the CC database. Which means that if we start doing updates to some independent PKI database while OpenMLS is still looking at the CC databse for PKI stuff, everything seems likely to explode.
The simplest resolution to all this is to just remove the documentation that the PKI environment can be an independent database.
crypto-ffi/bindings/jvm/src/test/kotlin/com/wire/crypto/E2EITest.kt
Outdated
Show resolved
Hide resolved
3427fc5 to
7eaa100
Compare
1c5757e to
fc0e6fc
Compare
|
Commit 6557504 says: Why do we need to know if the PKI env is set during MLS session init? |
OpenMls requires us to have a |
When initializing the mls session we need to know if a pki environment is set. If set we need to share the PkiEnvironmentProvider with the session during mlsInit. Therefore, the transaction context needs to know the pki environment.
If a mls session is initalized after a PKI environment was set then we need to get the PkiEnvironmentProvider's reference into the MlsCryptoProvider. If the PKi environment was not set before we will take a default and update it whenever the PKI is set through CC.
Interactions with the pki happen in a transaction context. We need these getters and the update function to alter the pki environment from the transaction context.
This getter allows us to get the pki environment from the inner transaction context assuming it was set before.
Idb doesn't close the database on drop. Since the pki database can be different from the cc database we close it explicitly on cc close. In case it is the same database this call is idempotent.
We still can't fix these without further refactoring. See WPB-22861
This allows us to translate the callback trait between ffi and core similar to mls transport.
e7a7a3b to
337e33c
Compare
If it was possible to include them, we'd like to, but the generated typescript module doesn't export them.
337e33c to
3db87e7
Compare
What's new in this PR
This PR adds a new PkiEnvironment to the API and decouples some pki related things from the mls session.
PR Submission Checklist for internal contributors
SQPIT-764feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.