User profiles phase 1 #410

kkohbrok · 2025-04-10T14:28:51Z

This PR introduces user profiles that can be encrypted and uploaded to the AS, where other users can download it when seeing the user in a group. Keys are distributed via the group state on the DS.

Copilot

Copilot reviewed 55 out of 64 changed files in this pull request and generated no comments.

Files not reviewed (9)

backend/.sqlx/query-28b419939a08f57c86836f0fc1f8a9735debc1f68d7e46004a03328c103bc3a6.json: Language not supported
backend/.sqlx/query-75767ac0c4fd0d80b05374023441177850ad84c37d647ae99f623f6f0438ebf6.json: Language not supported
backend/.sqlx/query-973891122d0736c69c439a140d5cab6a2e59e00f0e52d14a214f5c48b8cbc0d8.json: Language not supported
backend/.sqlx/query-a040c4f8d827be5ef6aa82143e9e110ae041e31f34b04ffa24a71a4254bda015.json: Language not supported
backend/.sqlx/query-fb077ef59b81b3659e0bc280679606b0b6d620387abd433d96c7acec525f8bd6.json: Language not supported
backend/migrations/20240927070412_create_initial_as_tables.sql: Language not supported
backend/migrations/20240927094408_create_initial_qs_tables.sql: Language not supported
coreclient/.sqlx/query-0c3b957fd18fd409d563d75db91f4ef129aef5222a028487218c9dcc08e146b2.json: Language not supported
coreclient/.sqlx/query-2c5823929ace6b56806ca3e023bee1e32cf9dff0f1009f3b923a90bd402f32e4.json: Language not supported

Comments suppressed due to low confidence (2)

backend/src/ds/group_operation.rs:367

[nitpick] Consider renaming the tuple type 'AddedUserInfo' to more explicitly reflect that it now includes an encrypted user profile key (e.g., 'AddedUserInfoWithProfile') to improve clarity for future maintainers.

type AddedUserInfo = (KeyPackage, EncryptedIdentityLinkKey, EncryptedUserProfileKey);

backend/src/ds/group_operation.rs:419

[nitpick] Consider adding an inline comment explaining the association between the new encrypted user profile keys and the existing identity link keys to aid clarity in this multi-zip chain.

.zip(aad_payload.new_encrypted_user_profile_keys.clone())

…to konrad/as_user_profiles

boxdot

Nice. I added minor comments and a small comment about storage to initiate a discussion.

boxdot · 2025-04-14T08:03:43Z

coreclient/migrations/20250115104336_init.sql

    friendship_token BLOB NOT NULL,
    key_package_ear_key BLOB NOT NULL,
    connection_key BLOB NOT NULL,
+    user_profile_key BLOB NOT NULL,


This change is not needed since contacts is recreated in the next migration.

boxdot · 2025-04-14T08:14:56Z

coreclient/src/clients/user_profile.rs

+
+impl CoreUser {
+    pub async fn update_user_profile(&self, user_profile: &UserProfile) -> anyhow::Result<()> {
+        let mut notifier = self.store_notifier();


notifier.notify() has to be called at some point, otherwise the notification is dropped. Here, it seems to make sense to call it after phase 1.

Good point, thanks!

boxdot · 2025-04-14T08:21:45Z

backend/src/ds/group_state/mod.rs

            group_info,
            ratchet_tree,
            encrypted_identity_link_keys,
+            encrypted_user_profile_keys,


NB: Looks like I overlooked versioning of responses of DS

I created an issue: #415.

boxdot · 2025-04-14T08:23:13Z

backend/src/ds/group_state/mod.rs

    pub(super) client_queue_config: QsReference,
    pub(super) activity_time: TimeStamp,
    pub(super) activity_epoch: GroupEpoch,
+    pub(super) encrypted_user_profile_key: EncryptedUserProfileKey,


This will break all groups deserialization on the server, right? Asking just to make sure that we are aware of it.

Yes, that's true.

boxdot · 2025-04-14T08:51:06Z

coreclient/src/key_stores/indexed_keys.rs

+    pub(crate) fn decrypt(
+        wrapper_key: &IdentityLinkWrapperKey,
+        encrypted_key: &EncryptedUserProfileKey,
+        user_name: QualifiedUserName,


Technically, there is no need to own this value, because key derivation only needs access by ref to it.

I attached a small exercise in lifetime below. Feel free to apply it or not.

diff --git a/coreclient/src/clients/create_user.rs b/coreclient/src/clients/create_user.rs index 0073f1f..7438fa0 100644 --- a/coreclient/src/clients/create_user.rs +++ b/coreclient/src/clients/create_user.rs @@ -288,7 +288,7 @@ impl PostRegistrationInitState { qs_client_id_encryption_key: qs_encryption_key, }; - let user_profile_key = UserProfileKey::random(user_name.clone())?; + let user_profile_key = UserProfileKey::random(user_name)?; user_profile_key .store_own(pool.acquire().await?.as_mut()) .await?; diff --git a/coreclient/src/clients/process/process_qs.rs b/coreclient/src/clients/process/process_qs.rs index 9e33953..b990326 100644 --- a/coreclient/src/clients/process/process_qs.rs +++ b/coreclient/src/clients/process/process_qs.rs @@ -315,7 +315,7 @@ impl CoreUser { let user_profile_key = UserProfileKey::from_base_secret( friendship_package.user_profile_base_secret.clone(), - user_name, + &user_name, )?; // UnconfirmedConnection Phase 2: Fetch the user profile. diff --git a/coreclient/src/contacts/mod.rs b/coreclient/src/contacts/mod.rs index da89424..683b99e 100644 --- a/coreclient/src/contacts/mod.rs +++ b/coreclient/src/contacts/mod.rs @@ -58,7 +58,7 @@ impl Contact { ) -> Result<(Self, UserProfileKey), LibraryError> { let user_profile_key = UserProfileKey::from_base_secret( friendship_package.user_profile_base_secret, - client_id.user_name().clone(), + client_id.user_name(), )?; let contact = Self { user_name: client_id.user_name().clone(), diff --git a/coreclient/src/groups/mod.rs b/coreclient/src/groups/mod.rs index 4557adc..4bba393 100644 --- a/coreclient/src/groups/mod.rs +++ b/coreclient/src/groups/mod.rs @@ -412,7 +412,7 @@ impl Group { UserProfileKey::decrypt( welcome_attribution_info.identity_link_wrapper_key(), &eupk, - ci.user_name().clone(), + ci.user_name(), ) .map(|user_profile_key| ProfileInfo { user_profile_key, @@ -530,11 +530,12 @@ impl Group { .map(|client_auth_info| client_auth_info.client_credential().identity()), ) .map(|(eupk, ci)| { - UserProfileKey::decrypt(&identity_link_wrapper_key, &eupk, ci.user_name().clone()) - .map(|user_profile_key| ProfileInfo { + UserProfileKey::decrypt(&identity_link_wrapper_key, &eupk, ci.user_name()).map( + |user_profile_key| ProfileInfo { user_profile_key, member_id: ci.clone(), - }) + }, + ) }) .collect::<Result<Vec<_>, _>>()?; diff --git a/coreclient/src/key_stores/indexed_keys.rs b/coreclient/src/key_stores/indexed_keys.rs index a5fba24..50c417a 100644 --- a/coreclient/src/key_stores/indexed_keys.rs +++ b/coreclient/src/key_stores/indexed_keys.rs @@ -27,7 +27,7 @@ use tracing::error; // The `LABEL` constant is used to identify the key type in the database. pub(crate) trait KeyType { - type DerivationContext: tls_codec::Serialize + Clone; + type DerivationContext<'a>: tls_codec::Serialize + Clone; const LABEL: &'static str; } @@ -169,16 +169,20 @@ impl<KT, ST, const LENGTH: usize> From<Secret<LENGTH>> for TypedSecret<KT, ST, L impl<KT, ST, const LENGTH: usize> TypedSecret<KT, ST, LENGTH> {} #[derive(TlsSerialize, TlsSize)] -struct DerivationContext<KT: KeyType> { - context: KT::DerivationContext, +struct DerivationContext<'a, KT: KeyType> { + context: KT::DerivationContext<'a>, key_type_instance: KeyTypeInstance<KT>, } -impl<KT: KeyType> KdfDerivable<BaseSecret<KT>, DerivationContext<KT>, AEAD_KEY_SIZE> for Key<KT> { +impl<KT: KeyType> KdfDerivable<BaseSecret<KT>, DerivationContext<'_, KT>, AEAD_KEY_SIZE> + for Key<KT> +{ const LABEL: &'static str = "key"; } -impl<KT: KeyType> KdfDerivable<BaseSecret<KT>, DerivationContext<KT>, AEAD_KEY_SIZE> for Index<KT> { +impl<KT: KeyType> KdfDerivable<BaseSecret<KT>, DerivationContext<'_, KT>, AEAD_KEY_SIZE> + for Index<KT> +{ const LABEL: &'static str = "index"; } @@ -194,7 +198,7 @@ pub(crate) struct IndexedAeadKey<KT> { impl<KT: KeyType> IndexedAeadKey<KT> { pub(crate) fn from_base_secret( base_secret: BaseSecret<KT>, - context: KT::DerivationContext, + context: KT::DerivationContext<'_>, ) -> Result<Self, LibraryError> { let derive_context = DerivationContext { context, @@ -234,7 +238,7 @@ impl<KT> EarKey for IndexedAeadKey<KT> {} pub(crate) struct UserProfileKeyType; impl KeyType for UserProfileKeyType { - type DerivationContext = QualifiedUserName; + type DerivationContext<'a> = &'a QualifiedUserName; const LABEL: &'static str = "user_profile_key"; } @@ -246,7 +250,7 @@ pub(crate) type UserProfileBaseSecret = BaseSecret<UserProfileKeyType>; pub(crate) type UserProfileKey = IndexedAeadKey<UserProfileKeyType>; impl UserProfileKey { - pub(crate) fn random(user_name: QualifiedUserName) -> Result<Self, RandomnessError> { + pub(crate) fn random(user_name: &QualifiedUserName) -> Result<Self, RandomnessError> { let base_secret = BaseSecret::random()?; Self::from_base_secret(base_secret, user_name).map_err(|e| { error!(error = %e, "Key derivation error"); @@ -264,7 +268,7 @@ impl UserProfileKey { pub(crate) fn decrypt( wrapper_key: &IdentityLinkWrapperKey, encrypted_key: &EncryptedUserProfileKey, - user_name: QualifiedUserName, + user_name: &QualifiedUserName, ) -> Result<Self, DecryptionError> { let base_secret = BaseSecret::decrypt(wrapper_key, encrypted_key)?; Self::from_base_secret(base_secret, user_name).map_err(|e| {

I spent about half an hour trying to do that, but got tangled up in lifetime bound issues. You approach works, thanks!

boxdot · 2025-04-14T08:55:45Z

types/src/credentials/pseudonymous_credentials.rs

    ) -> Result<(PseudonymousCredentialPlaintext, IdentityLinkKey), IdentityLinkVerificationError>
    {
-        let identity_link_key = IdentityLinkKey::derive(connection_key, self.tbs.clone())?;
+        let identity_link_key = IdentityLinkKey::derive(connection_key, &self.tbs)?;


Nice! (here and in other places)

boxdot · 2025-04-14T09:13:45Z

backend/migrations/20240927070412_create_initial_as_tables.sql

    voprf_server BYTEA NOT NULL
 );

+CREATE TYPE aead_ciphertext AS (


NB: This change will require wiping the server db.

Good point. I'll try to remember that when merging.

boxdot · 2025-04-14T09:18:55Z

backend/src/auth_service/client_api/user_profile.rs

+    ) -> Result<GetUserProfileResponse, GetUserProfileError> {
+        let GetUserProfileParams { client_id } = params;
+
+        let user_record = UserRecord::load(&self.db_pool, client_id.user_name())


Just a note: We could also store the encrypted user profile in S3-like storage. However, the question under which key. This could be the hashed client_id, but then only a single profile per user can be stored. Otherwise, it should be possible to use the encrypted profile key for that. Maybe we could discuss it as a possible future change.

I think the plan is to compromise here and store larger things like the profile picture on S3 and then store on the AS DB only a struct containing things like the S3 URL, key, nonce, etc. We can do that when we also do attachments/assets.

boxdot · 2025-04-14T09:20:10Z

backend/src/auth_service/user_record.rs

+        ) -> Result<(), StorageError> {
+            let password_file = BlobEncoded(&self.password_file);
+            query!(
+                "UPDATE as_user_records SET password_file = $1, encrypted_user_profile = $2 WHERE user_name = $3",


Line too long.

boxdot · 2025-04-14T09:25:11Z

backend/src/ds/group_operation.rs

    let added_users = staged_commit
        .add_proposals()
        .map(|ap| ap.add_proposal().key_package().clone())
        .zip(aad_payload.new_encrypted_identity_link_keys.clone())


It is possible to take aad_payload by value to avoid cloning both vectors.

raphaelrobert

This looks good at a conceptual level.

kkohbrok added 9 commits April 7, 2025 09:23

snapshot

e9d05c2

snapshot

40b98be

snapshot

34fa611

user profile loading

547c75a

snapshot

73dffa1

Merge branch 'main' into konrad/as_user_profiles

55456da

storable user profile key

4fe602e

snapshot

ec43357

snapshot

1a23074

kkohbrok changed the title ~~Konrad/as user profiles~~ User profiles Apr 10, 2025

kkohbrok added 4 commits April 11, 2025 10:16

working tests

b9b3801

add user name in derivation context

48efedc

formating and reuse header

cbcc665

clippy

46f16c2

kkohbrok mentioned this pull request Apr 11, 2025

Introduce AS-based user profiles #413

Closed

5 tasks

Merge branch 'main' into konrad/as_user_profiles

a6816c8

kkohbrok requested a review from Copilot April 11, 2025 10:00

kkohbrok marked this pull request as ready for review April 11, 2025 10:00

kkohbrok changed the title ~~User profiles~~ User profiles phase 1 Apr 11, 2025

Copilot AI reviewed Apr 11, 2025

View reviewed changes

kkohbrok added 2 commits April 11, 2025 12:02

nits

cffc3b0

Merge branch 'konrad/as_user_profiles' of github.com:phnx-im/infra in…

5cd6a5a

…to konrad/as_user_profiles

boxdot approved these changes Apr 14, 2025

View reviewed changes

raphaelrobert approved these changes Apr 14, 2025

View reviewed changes

kkohbrok added 6 commits April 14, 2025 14:50

use username reference

15b535a

remove unnecessary migration change

20534ac

call notifier.notify properly

b7426eb

fix long queries and whitespaces

1280795

fix more queries

4f1f867

avoid cloning vectors

fdc1d4e

kkohbrok enabled auto-merge (squash) April 14, 2025 15:01

kkohbrok merged commit 77e7a5a into main Apr 14, 2025
15 of 16 checks passed

kkohbrok deleted the konrad/as_user_profiles branch April 14, 2025 15:14

User profiles phase 1 #410

User profiles phase 1 #410

Uh oh!

Conversation

kkohbrok commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

boxdot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raphaelrobert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kkohbrok commented Apr 10, 2025 •

edited

Loading