s2s/keys: clarify minimum_valid_until_ts query #2191
Conversation
| A millisecond POSIX timestamp in milliseconds indicating when the returned | ||
| certificates will need to be valid until to be useful to the requesting server. | ||
| A millisecond POSIX timestamp indicating until when the returned | ||
| keys MUST at least be valid. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
famfo
force-pushed
the
keys-query-valid
branch
from
43ff9d0 to
70f6749
Compare
| @@ -0,0 +1 @@ | |||
| Clarify what the minimum_valid_until_ts field means when it is set in key queries. | |||
There was a problem hiding this comment.
| 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. |
| A millisecond POSIX timestamp in milliseconds indicating when the returned | ||
| certificates will need to be valid until to be useful to the requesting server. | ||
| A millisecond POSIX timestamp indicating until when the returned | ||
| keys MUST at least be valid. |
There was a problem hiding this comment.
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.
3 participants
Pull Request Checklist
Preview: https://pr2191--matrix-spec-previews.netlify.app