Page MenuHomePhabricator

[Keyserver] Validate incoming tunnelbroker messages
ClosedPublic

Authored by kamil on Aug 25 2023, 12:55 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 6:12 AM
Unknown Object (File)
Sun, Dec 15, 6:21 AM
Unknown Object (File)
Sun, Dec 15, 6:21 AM
Unknown Object (File)
Sun, Dec 15, 6:21 AM
Unknown Object (File)
Sun, Dec 15, 6:21 AM
Unknown Object (File)
Sun, Dec 15, 6:21 AM
Unknown Object (File)
Sun, Dec 15, 6:19 AM
Unknown Object (File)
Sat, Dec 14, 6:14 AM
Subscribers

Details

Summary

When receiving messages from tunnelbroker, we should be able to
verify the specific message and validate the content of the message.

https://linear.app/comm/issue/ENG-3707

Depends on D8752

Test Plan
  • Start keyserver, identity service, tunnelbroker

Alter the services/commtest/tests/identity_keyserver_tests.rs to deplete
one time keys of the keyserver. Verify that it can pull more one time keys

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Aug 25 2023, 1:10 PM
Harbormaster failed remote builds in B22097: Diff 30331!
keyserver/src/socket/tunnelbroker.js
33–35

being able to do something like this would be more elegant once we have more messages, but not sure if flow+JS supports it

keyserver/src/socket/tunnelbroker.js
33–35

if I remember correctly I received comments in the past that we should try to avoid switch/case in JS and stick to if/else and early exit, but I don't recall the reason behind this 😅

46

This line is problematic for me because we are not sure yet, that type of message is TBMessage, it wasn't validated yet.

I think first we need to call a validator for the entire TBMessage (see my comment in tunnelbroker-messages.js), and then call handleTBMessage (it expects TBMessage so it should be already validated).

Then in handleTBMessage you can run some action based only on type field of message, without additional validation,

lib/types/tunnelbroker-messages.js
42

maybe refreshKeysValidatorTBMessage? the point is to try to avoid starting const name with a capital letter to avoid confusion.

51

When you have (not yet, but in the future it will be) a union of types you can do the same with validators.

something like:

export const messageFromTBValidator: TUnion<TBMessage> =
  t.union([
    TBRefreshKeysValidator,
  ]);

and then only once call messageFromTBValidator.is(message).

marcin requested changes to this revision.Aug 28 2023, 8:36 AM

I agree with @kamil. We should:

  1. Introduce one validator for all TBMessages. We can introduce a union of validators or single validator that accepts union in type field.
  2. The use one generic validator in handleTBMessageEvent so that handleTBMessage can be sure that its argument is of TBMessage type and doesn't have to perform additional validation.
This revision now requires changes to proceed.Aug 28 2023, 8:36 AM

@kamil, would you mind commandeering this revision?

kamil edited reviewers, added: jon; removed: kamil.

Tested with real keyserver:

  • send the correct message and check if it passes validation
  • send a malformed message and check if an error is thrown
keyserver/src/socket/tunnelbroker.js
33 ↗(On Diff #30875)

we should call this on union of validators to reflect the flow type, but we can not create tcomb's union with only one element: tcomb: expected an array of at least 2 types

lib/types/tunnelbroker-messages.js
49 ↗(On Diff #30875)
lib/types/tunnelbroker-messages.js
40 ↗(On Diff #30875)

Would it be valuable to have a generic type like: TBRequest which would contain type field being an union of fixed strings and deviceId field? Correct me if I am wrong but I think we discussed such possibility and agreed it would be most beneficial for good typing there.

This revision is now accepted and ready to land.Sep 12 2023, 2:17 AM
lib/types/tunnelbroker-messages.js
40 ↗(On Diff #30875)

Current types are based on client <-> keyserver types. Not sure how the types you suggested should work