signatures: use OwnedKeyId instead of String for PublicKeySet index

[Kladki]

No description provided.

[zecakeh]

Thanks! I have mostly nitpicks.

[Kladki]

Seems like the error has changed

thread 'functions::tests::verify_event_with_single_key_with_unknown_algorithm_should_not_accept_event' panicked at crates/ruma-signatures/src/functions.rs:1163:9:
match assertion failed
 pattern: `Err(Error::Verification(VerificationError::UnknownPublicKeysForSignature))`
   value: `Err(Verification(Signature(signature::Error { source: Some(Verification equation was not satisfied) })))`

I am not sure why though, as even when setting the let ok ... else statements to panic on else, it still seems to get this new error instead of panicking there. 🤔

[zecakeh]

A double space.

Suggested change

	- Use `OwnedServerSigningKeyId` for the keys of `PublicKeySet` instead of `String`.
	- Use `OwnedServerSigningKeyId` for the keys of `PublicKeySet` instead of `String`.

[zecakeh]

So I believe that we should remove split_id completely and replace it with ServerSigningKeyId everywhere. This should happen in this PR, I'll make a separate PR to copy the validation from split_id to KeyName.

[Kladki]

So should I wait for your pull request to be merged, and then rebase on top of that?

[zecakeh]

It's probably better, if you don't want to have to edit your code again. But if you don't mind that, you can probably start to make changes.

[zecakeh]

Related to this change, is there any value for PublicKeyMap to use OwnedServerName for the keys? Or is it used in cases where the entity is not a server name?

[Kladki]

I think it could be good, would make working with this function much more clear, and also ensure that consumers of the function are passing in the correct type.

[Kladki]

Should I do that in this pull request, or a new one?

[zecakeh]

You can do that separately, it will make this PR a bit smaller.

[Kladki]

Would it be ok to also convert entity_id of sign_json and friends to &ServerName (currently &str)?

[zecakeh]

Looking more into it, both sign_json and verify_json could be used on the client side (to sign/verify devices or cross-signing keys signatures), so they shouldn't use server signatures specific types.

[Kladki]

Seems like this isn't as big of a deal as I thought for us anyways, and if there is a usecase outside of servers, then I think it would be best to close this then.

[zecakeh]

So I believe that we should remove split_id completely and replace it with ServerSigningKeyId everywhere. This should happen in this PR, I'll make a separate PR to copy the validation from split_id to KeyName.

It actually turns out that KeyName is validated indirectly, because the characters of the substring are validated when parsing KeyId. So there is no other PR needed for that, you can just get rid of split_id.

There is a small difference in validation in that compat-signatures-id on ruma-signatures only allows a few more characters for the version, while compat-key-id on ruma-common doesn't validate the version at all. But that will have no conflict with this so we can see it separately.

[Kladki]

As per #1808 (comment)