[FS-721] Forward All Default Proposals by mdimjasevic · Pull Request #2628 · wireapp/wire-server

mdimjasevic · 2022-08-18T08:27:07Z

The PR adapts the Wire server such that it simply forwards all MLS default proposal types, including those that it currently does not support.

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

Checklist

The PR Title explains the impact of the change.
The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
changelog.d contains the following bits of information (details):
- A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

- This was introduced accidentally

pcapriotti · 2022-08-24T07:02:23Z

libs/wire-api/src/Wire/API/MLS/Message.hs

  let sig = BA.convert $ sign priv pub (rmRaw tbs)
  pure (Message tbs (MessageExtraFields sig Nothing Nothing))
+
+mkAppAckProposalMessage ::


This isn't used, is it?

True. I removed it in 39d7c31.

pcapriotti · 2022-08-24T07:02:41Z

libs/wire-api/src/Wire/API/MLS/Message.hs

+mkAppAckProposalMessage priv pub gid epoch ref mrs =
+  maybeCryptoError $ mkAppAckProposalMessageFailable priv pub gid epoch ref mrs
+
+mkAppAckProposalMessageFailable ::


This is only used for testing, couldn't it just be moved to the tests?

Maybe this function and mkRemoveProposalMessage can be factored to use a common function that signs a MessageTBS to (Message 'MLSPlainText)?

Also I noted that both functions use maybeCryptoError together with a pure which is just CryptoPassed. I think because of that in both functions nothing is acutally checked. Can we remove this indirection?

Yes, only in testing. I simplified and moved it to the test code in 39d7c31.

pcapriotti · 2022-08-24T07:05:21Z

libs/wire-api/src/Wire/API/MLS/Serialisation.hs

+  [a] ->
+  Put
+serialiseMLSVector p as = do
+  put @w . fromIntegral . length $ as


This is not correct. Vectors begin with the whole size of their binary representation, not the number of items. See https://www.rfc-editor.org/rfc/rfc5246#section-4.3.

Thanks for spotting! Fixed in cc6548a.

pcapriotti · 2022-08-24T07:07:02Z

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

      )
      @?= [Ed25519]
+
+propUnsupported :: TestM ()


This is a somewhat unconventional test. Maybe add a couple of lines of comment explaining what it is we're doing here, like the fact that we are manually reading mls-test-cli's store and extracting the private key.

Added a description in 39d7c31.

pcapriotti

Looks good except for the vector serialisation. See comment below.

pcapriotti · 2022-08-24T14:14:43Z

libs/wire-api/src/Wire/API/MLS/Serialisation.hs

+  let elLen = maybe 0 (LBS.length . runPut . p) . listToMaybe $ as
+  put @w . fromIntegral $ elLen * fromIntegral (length as)
+  traverse_ p as


This doesn't look right to me. Why is every element the same length?

To be fair, it's not easy to do this efficiently. As a compromise, we can do the following:

serialise the whole vector ignoring the size

convert to lazy bytestring

put count

put bytestring

To get good performance we need imperative access to the buffer, which is probably possible, but requires the unsafe API. Maybe the above is good enough.

Good points! I obviously overlooked this.

I changed the implementation and round-trip tests for serialiseMLSVector pass. I am not too familiar with how efficient the conversion from a lazy bytestring to a strict bytestring is, but that's what I did.

Lazy bytestrings are essentially lists of strict bytestrings (representing their concatenation). Converting from lazy to strict amounts to actually performing the concatenation (i.e. basically allocating a big buffer, copying, and likely leaving the original bytestrings to the GC). So it's basically a bunch of extra copies and allocations, good to avoid if possible.

- This change makes the round-trip test for the serialiseMLSVector pass (via the ExtensionVector test type)

pcapriotti · 2022-08-25T09:18:30Z

libs/wire-api/src/Wire/API/MLS/Serialisation.hs

+  serialiseMLSBytes @w . LBS.toStrict . toLazyByteString
+    . execPut
+    . traverse_ p


This looks correct, but we can avoid the bytestring conversion if we create a version of serialiseMLSBytes that takes a lazy bytestring and uses putLazyByteString.

Thanks again! I've implemented that in 8ce0da0.

mdimjasevic temporarily deployed to cachix August 18, 2022 08:27 Inactive

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

pcapriotti force-pushed the pcapriotti/mls-serialisation branch 2 times, most recently from c7b25b7 to 3d3265e Compare August 22, 2022 06:48

Base automatically changed from pcapriotti/mls-serialisation to develop August 22, 2022 08:43

mdimjasevic temporarily deployed to cachix August 23, 2022 12:18 Inactive

Marko Dimjašević and others added 4 commits August 23, 2022 14:31

Do not throw for unsupported proposal types

d97e1b8

Application code for serialising an AppAck

29a0aed

Add a test for unsupported proposals

b285562

Add a changelog

734c569

mdimjasevic force-pushed the fs-721/fwd-all-default-proposals branch from aed3c11 to 734c569 Compare August 23, 2022 12:49

mdimjasevic temporarily deployed to cachix August 23, 2022 12:49 Inactive

Remove a compiler flag in tests

e5dafd7

- This was introduced accidentally

mdimjasevic temporarily deployed to cachix August 23, 2022 12:52 Inactive

pcapriotti reviewed Aug 24, 2022

View reviewed changes

smatting self-requested a review August 24, 2022 07:52

Marko Dimjašević added 2 commits August 24, 2022 15:58

Simplify and move around test code

39d7c31

Fix serialisation of MLS vectors

cc6548a

mdimjasevic temporarily deployed to cachix August 24, 2022 13:59 Inactive

mdimjasevic requested a review from pcapriotti August 24, 2022 14:03

pcapriotti requested changes Aug 24, 2022

View reviewed changes

Marko Dimjašević added 2 commits August 25, 2022 10:16

A failing roundtrip test for serialiseMLSVector

fbfaf72

Fix the implementation of serialiseMLSVector

5a12c6b

- This change makes the round-trip test for the serialiseMLSVector pass (via the ExtensionVector test type)

mdimjasevic temporarily deployed to cachix August 25, 2022 08:39 Inactive

Provide a SerialiseMLS instance for Word16

d338477

mdimjasevic temporarily deployed to cachix August 25, 2022 08:45 Inactive

pcapriotti approved these changes Aug 25, 2022

View reviewed changes

Use serialiseMLSBytesLazy in serialiseMLSVector

8ce0da0

mdimjasevic temporarily deployed to cachix August 25, 2022 09:28 Inactive

mdimjasevic merged commit 33d8889 into develop Aug 25, 2022

mdimjasevic deleted the fs-721/fwd-all-default-proposals branch August 25, 2022 11:23

zebot mentioned this pull request Sep 1, 2022

Release 2022-09-01 - (expected chart version 4.23.0) #2664

Merged

Conversation

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

Uh oh!

Checklist

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!

smatting Aug 24, 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!

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!

pcapriotti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdimjasevic Aug 25, 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!

pcapriotti Aug 25, 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mdimjasevic commented Aug 18, 2022 •

edited

Loading

smatting Aug 24, 2022 •

edited

Loading

mdimjasevic Aug 25, 2022 •

edited

Loading

pcapriotti Aug 25, 2022 •

edited

Loading