s2s/keys: clarify minimum_valid_until_ts query

[famfo]

Pull Request Checklist

• Pull request includes a changelog file
• Pull request includes a sign off
• Pull request is classified as 'other changes'

Preview: https://pr2191--matrix-spec-previews.netlify.app

[uhoreg]

TBH, I personally find the existing wording easier to follow and the proposed wording sounds awkward to me. Aside from the redundancy regarding milliseconds, and calling it certificates instead of keys, I don't see any problem with the existing wording. But if you think it's unclear, maybe something like:

A millisecond POSIX timestamp. The returned keys must be valid at least until this time to be useful to the requesting server.

[famfo]

That proposal is also more understandable, it would be nice to actually use RFC2119 keywords after they have been introduced at the beginning of the spec ^^

Not sure if the "to be useful to the requesting server" is adding anything useful though, it is kinda unnecessary and already implied in my opinion.

A millisecond POSIX timestamp. The returned keys MUST at least be valid until this timestamp.

[uhoreg]

I believe the intention here is not to say that the returned key MUST be valid until this timestamp in the RFC2119 sense, but rather that the parameter expresses the minimum time that the requesting server wants the keys to be valid for.

That is, the intention here is to explain the meaning of the parameter, rather than to set a requirement for the response. The current wording doesn't set a requirement on the response, so it would be a spec change to make it into a requirement.

[famfo]

The interpretation of the existing parameter could go either way here, exactly because defined language is not used (which is unfortunately all over in this spec).

Not having a requirement on the response for query parameters is... definitely unexpected reading a definition for said query parameters. Something along the lines of "The returned keys MAY be valid until at least this timestamp." would be more in line with this interpretation.

In my opinion, having such a parameter is kind of pointless if it does not impose any requirements (which, I agree, removing it would be a spec change. I don't see how requiring the parameter to have an effect would require a spec change though).

[anoadragon453]

I think @famfo's updated wording is much clearer, but @uhoreg is right in that this technically is a change to the spec.

The interpretation of the existing parameter could go either way here, exactly because defined language is not used

This is the crux of the issue. The language of the spec could be interpreted as "The returned keys MAY be valid until at least this timestamp", and thus a "MUST" is a change.

To move forwards here, I suggest:

• This PR change the wording to be MAY instead of MUST, so that this becomes a clarification.
• A (very small) MSC be opened to update the wording to MUST.

...as I agree that I don't see a need for homeservers to ask the remote for keys that must be valid after a certain date, only for the remote to return keys that aren't.

[uhoreg]

Yes, the current wording is kind of strange. It implies that something is expected from the responding server, but doesn't actually spell it out as a requirement, and doesn't say what should happen if the responding server is unable to meet that requirement.

Looking at the synapse code for responding to this request, it looks like synapse is using this parameter as a hint for whether it can use a cached value, or whether it should re-fetch the keys: if the cached value is new enough to satisfy the minimum_valid_until_ts, then it will just send the cached value, no matter how old it is. Otherwise, it will try to re-fetch the keys from the remote server. But unless I'm missing something, I don't see any attempt to actually filter out values that are too old. If the only value that the server can return is too old, AFAICT, it will still return it.

[anoadragon453]

In that case, I suggest we clarify this as "The returned keys MAY be valid until at least this timestamp" and file a separate issue to discuss whether this should be changed to a MUST.

Today, someone implementing the spec may misinterpret the vague wording and expect the wrong thing from i.e. Synapse.

[uhoreg]

I'm not sure that "The returned keys MAY be valid until at least this timestamp" is helpful wording. If I'm reading the spec, I want to know, as a requester, what value I should put in that parameter and why; or as a responder, what I should be doing when I see a value in that parameter. The current wording tries to communicate the former (you stick the timestamp in there that you want the key to be valid until, because older keys aren't useful to you), but doesn't to a great job of the latter. "The returned keys MAY be valid until at least this timestamp" doesn't really communicate either thing.

So if we're taking synapse's implementation as an example what it's supposed to be doing, I might use something similar to the current wording, but add something along the lines of "The responding server can use this and a hint to refresh any keys that it has cached, if they are older than the requested timestamp."

[uhoreg]

FWIW, I don't think there's much value in making an MSC to change the spec so that it's a MUST. I agree that it's pointless for the responding server to send keys that are useless to the requesting server. But it's also easy enough to filter out the useless values on the requesting server. And given that the existing behaviour seems to be to include the useless keys, the requesting servers would have to handle those values anyways, or we'd need to stick a new version on the endpoint. Which all seems like a lot of work for not much value.

I could, on the other hand, see a case for making it a SHOULD, and I could also see that an argument could be made that making it a SHOULD wouldn't require an MSC. But the wording should still answer the questions of "why would the requester set the parameter to any given value", and "what should the responder do with the value" in some way or other.

[famfo]

I am not sure what the aim of the spec is at this point. Either an implementable and understandable spec (which would require some MSC to either remove this endpoint or change it to return up to date keys or an error if there are none in my opinion) or a spec with a bunch of loosely defined endpoints (which is unfortunately the status quo, especially with some of the older endpoints which haven't been touched and presumably implemented in a while).

In my opinion the spec should be clear and the two options are:

1. an MSC to change this endpoint to MUST return valid keys
2. an MSC to remove this endpoint because other servers can't rely on a notary server to return up to date information anyways

Fwiw, Synapse is also not the reference implementation anymore, it should follow the spec, not the other way around.

[anoadragon453]

Suggested change

	Clarify what the minimum_valid_until_ts field means when it is set in key queries.
	Clarify what the `minimum_valid_until_ts` field means when it is set in key queries.

[anoadragon453]

I think @famfo's updated wording is much clearer, but @uhoreg is right in that this technically is a change to the spec.

The interpretation of the existing parameter could go either way here, exactly because defined language is not used

This is the crux of the issue. The language of the spec could be interpreted as "The returned keys MAY be valid until at least this timestamp", and thus a "MUST" is a change.

To move forwards here, I suggest:

• This PR change the wording to be MAY instead of MUST, so that this becomes a clarification.
• A (very small) MSC be opened to update the wording to MUST.

...as I agree that I don't see a need for homeservers to ask the remote for keys that must be valid after a certain date, only for the remote to return keys that aren't.