[FS-919] Support Basic Processing of External Commits #2765

mdimjasevic · 2022-10-13T11:13:40Z

The PR introduces basic support for external commits in MLS. Note that this does not resubmit external pending proposals submitted by the backend; this is to be done in https://wearezeta.atlassian.net/browse/FS-920.

Tracked by https://wearezeta.atlassian.net/browse/FS-919.

Checklist

Add a new entry in an appropriate subdirectory of changelog.d
Read and follow the PR guidelines

- Extract the key package from the update path

libs/wire-api/src/Wire/API/Error/Galley.hs

elland · 2022-10-26T09:25:17Z

services/galley/src/Galley/API/MLS/Message.hs

-              Just cli -> pure $ updateKeyPackageMapping lconv qusr cli (Just senderRef) updatedRef
-              Nothing -> pure $ pure ()
-          (_, Nothing) -> pure $ pure () -- ignore commits without update path
+              Just cli -> pure (updateKeyPackageMapping lconv qusr cli (Just senderRef) updatedRef, action)


Would moving pure outside of the case match work?

I don't think it would. At first it looked weird to me as well. All the branches of the bigger pattern-match have to have the same type. As far as my understanding goes, this block is written this way so it can be executed later, but then I suppose the same could have been achieved if at the top we had a let binding instead of monadic binding and using one level of monads less.

I'm fine either way, more of a nitpick :D

libs/wire-api/src/Wire/API/Error/Galley.hs

pcapriotti · 2022-10-26T09:27:38Z

services/galley/src/Galley/API/MLS/Message.hs

+    paRemove :: ClientMap,
+    -- The backend does not process external init proposals, but still it needs
+    -- to know if a commit has one when processing external commits
+    paExternalInit :: Maybe ()


This Maybe () is pretty weird. Can we use Any instead?

Sure, done in commit 20162df. But aren't these two types isomorphic as well, i.e., don't the same arguments you had yesterday in the call apply to Any as well?

What arguments? My only argument here is that Maybe () is a circuitous way to express a boolean, while Any is more direct. Using Bool there is also fine. It's a minor thing, anyway.

Sure, I see your point.

pcapriotti · 2022-10-27T11:31:24Z

services/galley/src/Galley/API/MLS/Message.hs

+                    throwS @'MLSRemovalUserMismatch
+                  pure (Just r)
+
+            updateKeyPackageMapping lconv qusr (ciClient cid) remRef newRef


Shouldn't you validate the key package first? See for example applyProposal.

Your observation has seemed valid and with that in mind, I looked at other cases when a key package is updated or mapped and then some of them seemed equally wrong because they also just call updateKeyPackageMapping. For example, in the case of an existing member updating their key package via the update path we blindly accept the new key package without checking if it is for the same protocol, same cipher suite, etc., but this seems wrong, doesn't it?

I realized updateKeyPackageMapping does validation (the ciphersuite, identity matching, signature scheme, etc.) if the old key package is non-existent (i.e., we pass in Nothing to updateKeyPackageMapping), by calling an internal endpoint in Brig, for which there's a handler upsertKeyPackage.

So before making a call to updateKeyPackageMapping in the case of a NewMemberSender, I just added a call to addKeyPackageRef, which calls upsertKeyPackage in Brig and validates it before inserting into the database.

Does that seem OK?

As long as validation is happening.

services/galley/test/integration/API/MLS/Util.hs

* Check external commit criteria - Extract the key package from the update path - Validate key package before replacing the old one * Fix the serialiseMLS instance for `Sender 'MLSPlainText` * Update the mls-test-cli reference * Integration tests for external commits

mdimjasevic temporarily deployed to cachix October 13, 2022 11:13 Inactive

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Oct 13, 2022

mdimjasevic force-pushed the fs-919/external-commits branch from 2db5319 to 70a607f Compare October 13, 2022 11:15

mdimjasevic temporarily deployed to cachix October 13, 2022 11:16 Inactive

mdimjasevic temporarily deployed to cachix October 14, 2022 13:16 Inactive

mdimjasevic force-pushed the fs-919/external-commits branch from d0f0b69 to 9a1c4c4 Compare October 14, 2022 15:00

mdimjasevic temporarily deployed to cachix October 14, 2022 15:00 Inactive

mdimjasevic temporarily deployed to cachix October 19, 2022 12:02 Inactive

mdimjasevic temporarily deployed to cachix October 20, 2022 09:02 Inactive

mdimjasevic temporarily deployed to cachix October 20, 2022 15:00 Inactive

mdimjasevic temporarily deployed to cachix October 24, 2022 15:01 Inactive

mdimjasevic temporarily deployed to cachix October 25, 2022 06:01 Inactive

mdimjasevic force-pushed the fs-919/external-commits branch from a3ff2ce to bc0b9af Compare October 25, 2022 07:37

mdimjasevic temporarily deployed to cachix October 25, 2022 07:37 Inactive

mdimjasevic force-pushed the fs-919/external-commits branch from bc0b9af to 937f13a Compare October 25, 2022 08:11

mdimjasevic temporarily deployed to cachix October 25, 2022 08:11 Inactive

mdimjasevic force-pushed the fs-919/external-commits branch from 937f13a to 62e5bdf Compare October 25, 2022 08:15

mdimjasevic temporarily deployed to cachix October 25, 2022 08:15 Inactive

mdimjasevic force-pushed the fs-919/external-commits branch from 62e5bdf to 61b5bab Compare October 25, 2022 14:47

mdimjasevic temporarily deployed to cachix October 25, 2022 14:47 Inactive

mdimjasevic force-pushed the fs-919/external-commits branch from 61b5bab to e6beea8 Compare October 25, 2022 14:59

mdimjasevic temporarily deployed to cachix October 25, 2022 14:59 Inactive

mdimjasevic added 3 commits October 26, 2022 09:15

Check external commit criteria

8116842

- Extract the key package from the update path

Fix the serialiseMLS instance for Sender 'MLSPlainText

3cece55

Update the mls-test-cli reference

a7b32f6

mdimjasevic force-pushed the fs-919/external-commits branch from e6beea8 to 38ae3ef Compare October 26, 2022 07:15

mdimjasevic temporarily deployed to cachix October 26, 2022 07:15 Inactive

mdimjasevic added 2 commits October 26, 2022 11:10

Integration tests for external commits

873b0ee

Add a changelog

c69966d

mdimjasevic force-pushed the fs-919/external-commits branch from 38ae3ef to c69966d Compare October 26, 2022 09:13

mdimjasevic temporarily deployed to cachix October 26, 2022 09:13 Inactive

mdimjasevic marked this pull request as ready for review October 26, 2022 09:16

elland reviewed Oct 26, 2022

View reviewed changes

pcapriotti reviewed Oct 27, 2022

View reviewed changes

services/galley/test/integration/API/MLS/Util.hs Outdated Show resolved Hide resolved

Use MLSProtocolError instead of specific errors

0224a42

mdimjasevic temporarily deployed to cachix October 28, 2022 08:03 Inactive

Replace Maybe () with Any

20162df

mdimjasevic temporarily deployed to cachix October 28, 2022 08:12 Inactive

Remove debugging code

9e10875

mdimjasevic temporarily deployed to cachix October 28, 2022 08:15 Inactive

Apply formatting

3450145

mdimjasevic temporarily deployed to cachix October 28, 2022 08:19 Inactive

mdimjasevic added 2 commits October 28, 2022 11:47

fixup! Use MLSProtocolError instead of specific errors

be9381c

Validate key package before replacing the old one

650aa88

mdimjasevic temporarily deployed to cachix October 28, 2022 11:26 Inactive

pcapriotti approved these changes Oct 28, 2022

View reviewed changes

mdimjasevic merged commit 6ea9e19 into develop Oct 31, 2022

mdimjasevic deleted the fs-919/external-commits branch October 31, 2022 05:20

zebot mentioned this pull request Nov 2, 2022

Release 2022-11-02 - (expected chart version 4.26.0) #2814

Closed

zebot mentioned this pull request Nov 3, 2022

Release 2022-11-03 - (expected chart version 4.26.0) #2821

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[FS-919] Support Basic Processing of External Commits #2765

[FS-919] Support Basic Processing of External Commits #2765

Uh oh!

mdimjasevic commented Oct 13, 2022 •

edited

Loading

Uh oh!

Uh oh!

elland Oct 26, 2022

Uh oh!

mdimjasevic Oct 26, 2022

Uh oh!

elland Oct 27, 2022

Uh oh!

Uh oh!

pcapriotti Oct 26, 2022

Uh oh!

mdimjasevic Oct 28, 2022

Uh oh!

pcapriotti Oct 28, 2022

Uh oh!

mdimjasevic Oct 31, 2022

Uh oh!

pcapriotti Oct 27, 2022

Uh oh!

mdimjasevic Oct 28, 2022 •

edited

Loading

Uh oh!

pcapriotti Oct 28, 2022

Uh oh!

Uh oh!

Uh oh!

[FS-919] Support Basic Processing of External Commits #2765

[FS-919] Support Basic Processing of External Commits #2765

Uh oh!

Conversation

mdimjasevic commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

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!

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!

mdimjasevic Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mdimjasevic commented Oct 13, 2022 •

edited

Loading

mdimjasevic Oct 28, 2022 •

edited

Loading