Verify only the required signatures on `verify_event` #394

gnieto · 2021-01-17T14:31:44Z

The spec says that the required signatures for a signed event is the
signature of sender's server (unless is a third party invite) and the
event_id server (in v1 and v2 room versions).

This changes the previous behaviour, which tried to verify the
signatures for all the servers in the PublicKeyMap, instead of
checking only the required signatures.

Relevant spec section:
https://matrix.org/docs/spec/server_server/r0.1.4#validating-hashes-and-signatures-on-received-events

The spec says that the required signatures for a signed event is the signature of sender's server (unless is a third party invite) and the `event_id` server (in v1 and v2 room versions). This changes the previous behaviour, which tried to verify the signatures for all the servers in the `PublicKeyMap`, instead of checking only the required signatures. Relevant spec section: https://matrix.org/docs/spec/server_server/r0.1.4#validating-hashes-and-signatures-on-received-events

ruma-signatures/src/functions.rs

gnieto · 2021-01-17T14:59:32Z

ruma-signatures/src/functions.rs

-/// assert!(verify_event(&public_key_map, &object, &RoomVersionId::Version6).is_ok());
+/// let verification_result = verify_event(&public_key_map, &object, &RoomVersionId::Version6);
+/// assert!(verification_result.is_ok());
+/// assert!(matches!(verification_result.unwrap(), Verified::All));
 /// ```
 pub fn verify_event(
    public_key_map: &PublicKeyMap,


This PublicKeyMap forces to load all the known server keys before calling to this function. I was thinking about adding a trait to be able to provide multiple implementations and, for example, be able to provide an implementation which loads the keys on demand.

What do you think about it?

Seems like the kind of situation where you would use a Stream, but I'm not sure. Whatever you think works best.

Well, as it is right now, PublicKeyMap should contain all the public keys for all known servers. What I was thinking on was to have an alternative implementation which lazy loads the public keys on demand (Stream does not solve this problem, as far as I know). If I add the trait, I would do it on another PR.

Thanks anyway for the suggestion!

Why does Stream not solve this? A stream is basically just an asynchronous (which is just a guess that it'd be useful for homeservers) iterator, and iterators only do work when iterated. When you stop iteration (or polling in the case of a stream) early, any logic that's part of the iterator / stream itself is not invoked again to produce the remaining elements. Of course it somewhat depends on how the stream is constructed whether this decreases work.

I think that I probably missed in giving enough context. What the verify_event needs is random access to one or two keys on the PublicKeyMap ( for example, it will need to look up the server keys for matrix.org if it needs to authorize an event sent from a user on matrix.org). The function does not need to iterate over all the known server keys.

The Stream approach could work if the signing keys were selected before hand (for example, select matrix.org and anotherserver.com) and then use this stream as an input for this function. The unique problem I see with this approach is that it could potentially be misused by, for example, give an empty Stream and do not verify any signature.

Makes a lot of sense! So your idea was to provide a generic argument that the function can then ask for the keys to a certain homeserver through a trait? Sounds like a good idea!

I wonder whether it actually needs a new trait. Could it just be a FnMut(&ServerName) -> PublicKeySet / FnMut(&str) -> PublicKeySet or sth. along those lines?

Yes, that could work as well! I thought on adding a trait just for the semantics.

I do not know if it makes any difference on compilation times 🤔

Not sure how helpful this is but here is the code for dealing with this so far in conduit.

jplatte

Thanks for the PR, especially for including a changelog entry, documentation improvements and tests!

I'm not super keen on owning / maintaining ruma-signatures so haven't tried to understand the main logic change. If you want that double-checked you will have to find sbd. else to review it.

Feel free to merge this PR yourself whenever you think it's ready.

ruma-signatures/src/functions.rs

gnieto · 2021-01-17T15:51:15Z

ruma-signatures/src/functions.rs

-/// assert!(verify_event(&public_key_map, &object, &RoomVersionId::Version6).is_ok());
+/// let verification_result = verify_event(&public_key_map, &object, &RoomVersionId::Version6);
+/// assert!(verification_result.is_ok());
+/// assert!(matches!(verification_result.unwrap(), Verified::All));
 /// ```
 pub fn verify_event(
    public_key_map: &PublicKeyMap,


Well, as it is right now, PublicKeyMap should contain all the public keys for all known servers. What I was thinking on was to have an alternative implementation which lazy loads the public keys on demand (Stream does not solve this problem, as far as I know). If I add the trait, I would do it on another PR.

Thanks anyway for the suggestion!

ruma-signatures/src/functions.rs

gnieto · 2021-01-17T16:05:03Z

I'm not super keen on owning / maintaining ruma-signatures so haven't tried to understand the main logic change. If you want that double-checked you will have to find sbd. else to review it.

@DevinR528 I see that the latest changes on ruma-signatures are yours, maybe you want to review it?

Feel free to merge this PR yourself whenever you think it's ready.

@jplatte I'm not authorized to do that 😅

jplatte · 2021-01-17T16:19:27Z

Feel free to merge this PR yourself whenever you think it's ready.

@jplatte I'm not authorized to do that sweat_smile

I've made a few adjustments, are you still getting an authorization error?

gnieto · 2021-01-17T16:21:04Z

It seems that I see the merge button now. Thanks!

DevinR528 · 2021-01-18T01:32:56Z

Everything looks good to me too! I don't see any reason why this wouldn't work the same as before but more streamlined. If you want to make double sure I can test it out in conduit tomorrow but I think it should be fine.

gnieto · 2021-01-18T20:22:41Z

I don't see any reason why this wouldn't work the same as before but more streamlined

There's a subtle but important change compared to the previous version: Before it was checking that all keys provided on public_key_map and fail in case of any missing keys or any invalid signature.

The new code only requires to check the signature for sender's homeserver. For example, imagine an event like:

{
  "sender": "@user:server",
  ...
  "signatures": {
      "another_server": {
          "ed25519:1": "valid signature",
      }
  }
}

This event would be accepted by the previous version (signature for another_server is correct), but the new version will fail: Signature from sender's home server is missing or invalid.

@DevinR528 If you don't have any new concern after this clarification, I will go ahead and merge this PR. Thanks for reviewing it, anyway!

DevinR528 · 2021-01-18T21:23:18Z

Yeah, go for it! So this means that I shouldn't be using the origin field from https://github.com/ruma/ruma/blob/main/ruma-federation-api/src/transactions/send_transaction_message/v1.rs#L28 and instead get it from the sender of that particular PDU?

gnieto · 2021-01-18T22:20:01Z

So this means that I shouldn't be using the origin field from https://github.com/ruma/ruma/blob/main/ruma-federation-api/src/transactions/send_transaction_message/v1.rs#L28 and instead get it from the sender of that particular PDU?

I don't think I know the answer to your question, but: I guess that in almost all cases origin and sender's home server should be the same. I do not know how third part invites works, but the section I linked on the PRs description talks about some case in which sender's server could be different from origin.

Maybe this is one of those cases in which origin is redundant (see https://github.com/matrix-org/matrix-doc/issues/1664)

gnieto commented Jan 17, 2021

View reviewed changes

ruma-signatures/src/functions.rs Show resolved Hide resolved

gnieto commented Jan 17, 2021

View reviewed changes

jplatte approved these changes Jan 17, 2021

View reviewed changes

ruma-signatures/src/functions.rs Outdated Show resolved Hide resolved

ruma-signatures/src/functions.rs Show resolved Hide resolved

ruma-signatures/src/functions.rs Outdated Show resolved Hide resolved

gnieto added 2 commits January 17, 2021 16:47

fixup! Verify only the required signatures on verify_event

d900270

fixup! Verify only the required signatures on verify_event

2b15b02

gnieto commented Jan 17, 2021

View reviewed changes

gnieto merged commit 0635b40 into ruma:main Jan 18, 2021

gnieto deleted the task/check-signatures branch January 18, 2021 22:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify only the required signatures on `verify_event` #394

Verify only the required signatures on `verify_event` #394

gnieto commented Jan 17, 2021

gnieto Jan 17, 2021

jplatte Jan 17, 2021

gnieto Jan 17, 2021

jplatte Jan 17, 2021 •

edited

Loading

gnieto Jan 17, 2021

jplatte Jan 17, 2021

jplatte Jan 17, 2021

gnieto Jan 17, 2021

DevinR528 Jan 18, 2021

jplatte left a comment •

edited

Loading

gnieto Jan 17, 2021

gnieto commented Jan 17, 2021

jplatte commented Jan 17, 2021

gnieto commented Jan 17, 2021

DevinR528 commented Jan 18, 2021

gnieto commented Jan 18, 2021

DevinR528 commented Jan 18, 2021

gnieto commented Jan 18, 2021

Verify only the required signatures on verify_event #394

Verify only the required signatures on verify_event #394

Conversation

gnieto commented Jan 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jplatte Jan 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jplatte left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnieto commented Jan 17, 2021

jplatte commented Jan 17, 2021

gnieto commented Jan 17, 2021

DevinR528 commented Jan 18, 2021

gnieto commented Jan 18, 2021

DevinR528 commented Jan 18, 2021

gnieto commented Jan 18, 2021

Verify only the required signatures on `verify_event` #394

Verify only the required signatures on `verify_event` #394

jplatte Jan 17, 2021 •

edited

Loading

jplatte left a comment •

edited

Loading