[rest] add entity tags and If-Match to the dataset endpoints - #3553
[rest] add entity tags and If-Match to the dataset endpoints#3553LorbusChris wants to merge 1 commit into
Conversation
set_pending_dataset_tlvs() can create (the If-None-Match guard) or replace deliberately (allow_replace=True), but a caller that read the in-flight dataset and stamped a successor above it still cannot say "replace exactly the one I read": another controller can slip a new pending dataset in between the read and the write. Add get_pending_dataset_tlvs_with_etag(), returning the dataset together with the entity tag a border router hands out for it, and an if_match keyword on set_pending_dataset_tlvs() that passes the tag back: the router then only replaces the dataset the tag stands for, checked atomically with the write (openthread/ot-br-posix#3553). An older border router hands out no tag and would ignore the header, so the path degrades to the allow_replace behaviour. Both refusals -- a pending dataset already in place on a guarded write, and the read one having changed on a conditional replace -- now raise PendingDatasetConflictError, an OTBRError subclass, so callers can tell them apart from other errors without matching messages. Assisted-By: Claude Fable 5
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for HTTP conditional headers (If-Match, If-None-Match, and ETag) to the active and pending dataset REST endpoints, enabling compare-and-swap operations. The OpenAPI specification, REST web server implementation, and integration tests have been updated accordingly. Review feedback identifies an issue in ParseIfMatch where syntactically invalid headers containing only empty list elements or whitespace are incorrectly accepted as valid instead of being rejected with a 400 Bad Request error.
e87c59e to
50d6678
Compare
50d6678 to
ed63268
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3553 +/- ##
==========================================
- Coverage 55.77% 48.34% -7.43%
==========================================
Files 87 143 +56
Lines 6890 17608 +10718
Branches 0 1439 +1439
==========================================
+ Hits 3843 8513 +4670
- Misses 3047 8474 +5427
- Partials 0 621 +621 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
b80c43a to
a314aa8
Compare
|
@LorbusChris , can you help rebase? |
a314aa8 to
69cd231
Compare
69cd231 to
734c22e
Compare
| description: |- | ||
| The entity tag of the stored dataset, for use in `If-Match`. Derived | ||
| from the dataset TLVs, so it changes exactly when the stored dataset | ||
| does and is shared by the JSON and TLV representations. |
There was a problem hiding this comment.
This is not 100% honest: The delay timer TLV is left out, and I think it's worth pointing this out here too.
Especially since this actually violates RFC 9110 §8.8.1.
A "strong validator" is representation metadata that changes value whenever a change occurs to the representation data that would be observable in the content of a 200 (OK) response to GET.
So this actually makes a case emitting a weak tag. However, this then doesn't allow to use the tag in the If-Match. Unless we break RFC 9110 §13.1.1
An origin server MUST use the strong comparison function when comparing entity tags for If-Match (Section 8.8.3.2), since the client intends this precondition to prevent the method from being applied if there have been any changes to the representation data.
We are a bit between a rock and a hard place here: But I think I'd go with weak E-Tag by default, and accept weak comparison in the If-Match. This violates the spec too, but it feels more honest to me, since we don't need to lie on the read side and it is the write side where we consider the delay timer not relevant for dataset comparison.
In a way, this is what we are aiming for:
This weakness might be due to limitations in how the value is calculated (e.g., clock resolution), an inability to ensure uniqueness for all possible representations of the resource, or a desire of the resource owner to group representations by some self-determined set of equivalency rather than unique sequences of data.
Besides, Claude also claims that reverse proxies (which we also considered adding to the HA OTBR app) potentially convert E-Tag to weak ones when using gzip. I haven't verified that, but using weak E-Tags from the origin neatly sides steps that too.
We also share the tag between JSON and plain text/TLV representation, which technically is not spec compliant:
A strong validator is unique across all versions of all representations associated with a particular resource over time.
| description: Successfully updated the pending operational dataset. | ||
| headers: | ||
| ETag: | ||
| $ref: "#/components/headers/DatasetEtag" |
There was a problem hiding this comment.
Can we also add Cache-Control: no-store to the GET method to avoid caching? Especially with ETags present, this potentially return outdated delay timers if the response gets cached along the way. Also we include keys which we don't want to be cached 😅 . The latter is technically pre-existing, but since we add ETags which are also cache relevant, it still fits with this PR IMHO.
| { | ||
| break; | ||
| } | ||
| if (tlv[0] != OT_MESHCOP_TLV_DELAYTIMER) |
There was a problem hiding this comment.
We should include openthread/dataset.h probably. It appears to compile, so I guess it is included through other headers, but this is potentially fragile.
| } | ||
| sha256.Finish(hash); | ||
|
|
||
| return '"' + Utils::Bytes2Hex(hash.GetBytes(), 8) + '"'; |
There was a problem hiding this comment.
Same here, we should include utils/hex.hpp for this.
Maybe we should also ask ourselfs if this is really a use case we want to/need to handle 🤔 |
734c22e to
876939c
Compare
|
Moved to draft while I give this some more thought. |
set_pending_dataset_tlvs() can create (the If-None-Match guard) or replace deliberately (allow_replace=True), but a caller that read the in-flight dataset and stamped a successor above it still cannot say "replace exactly the one I read": another controller can slip a new pending dataset in between the read and the write. Add get_pending_dataset_tlvs_with_etag(), returning the dataset together with the entity tag a border router hands out for it, and an if_match keyword on set_pending_dataset_tlvs() that passes the tag back: the router then only replaces the dataset the tag stands for, checked atomically with the write (openthread/ot-br-posix#3553). An older border router hands out no tag and would ignore the header, so the path degrades to the allow_replace behaviour. A conditional replace that the router refuses raises the same PendingDatasetConflictError the guarded write already raises, with a message naming the other reason: the dataset changed since it was read. Assisted-By: Claude Fable 5
set_pending_dataset_tlvs() can create (the If-None-Match guard) or replace deliberately (allow_replace=True), but a caller that read the in-flight dataset and stamped a successor above it still cannot say "replace exactly the one I read": another controller can slip a new pending dataset in between the read and the write. Add get_pending_dataset_tlvs_with_etag(), returning the dataset together with the entity tag a border router hands out for it, and an if_match keyword on set_pending_dataset_tlvs() that passes the tag back: the router then only replaces the dataset the tag stands for, checked atomically with the write (openthread/ot-br-posix#3553). An older border router hands out no tag and would ignore the header, so the path degrades to the allow_replace behaviour. A conditional replace that the router refuses raises the same PendingDatasetConflictError the guarded write already raises, with a message naming the other reason: the dataset changed since it was read. Assisted-By: Claude Fable 5
If-None-Match gives a client create-only semantics; what it cannot express is "replace, but only the dataset I read". A controller that reads a dataset, stamps a successor and writes it back has no way to detect another writer slipping in between the read and the write. Give the dataset resources entity tags: GET answers with an ETag derived from the stored TLVs (hashed, so the tag does not echo the credentials it stands for), and a successful PUT reports the tag of what it stored. The JSON representation is now rendered from the same stored TLV bytes the tag covers, rather than the struct getters. The tag is weak (RFC 9110 section 8.8.1). A pending dataset's delay timer counts down, so a tag covering it would change every second and could never serve as a precondition; leaving it out is what a client doing a read-stamp-write actually needs, but the countdown is visible in the body a GET returns, and a strong validator has to change whenever anything in that body does. The same tag also stands for both the JSON and the TLV representation, which the specification calls weak on its own. Weak is what it offers for this case: grouping representations "by some self-determined set of equivalency rather than unique sequences of data". PUT honors If-Match per RFC 9110 section 13.1.1: the write only happens when one of the given entity tags matches the stored dataset, or, with `*`, when any dataset is stored. The precondition is evaluated before If-None-Match (section 13.2.2), atomically with the write in the main-loop task. A failed precondition answers 412, a syntactically invalid header value 400, and the role check on the active dataset still answers 409 first (section 13.2.1). This turns a read-stamp-write sequence into a compare-and-swap. Tags are compared with the weak function (section 8.8.3.2) rather than the strong one section 13.1.1 asks for. The two requirements cannot both be met here: strong comparison never matches a weak tag, and this resource cannot honestly offer a strong one. Comparing weakly keeps the deviation on the write side, where this server decides what counts as the same dataset, instead of putting it on the read side, where a strong-looking tag would mislead caches as well as clients. GET answers no-store. A dataset carries the network key and the PSKc, and a pending one carries a delay timer that is stale as soon as it is stored, so nothing on the way should keep a copy -- the more so now that an entity tag invites revalidation. Neither the header nor the entity tag is CORS-safelisted, so add If-Match to the allowed request headers and expose ETag to cross-origin scripts. Bump the REST API version to 0.6.0, covering the conditional-write capability this completes: openthread#3552 added If-None-Match, this adds entity tags and If-Match, and they ship as one version. Assisted-By: Claude Opus 5 Signed-off-by: Christian Glombek <c.glombek@cosa.systems>
876939c to
cac9482
Compare
set_pending_dataset_tlvs() refuses whenever a pending dataset is in place, and deliberately offers no blanket override: superseding races the delay timer on every device holding the old dataset and silently undoes whatever that dataset was doing. What a caller may reasonably want is narrower: "replace exactly the dataset I read", as an explicit, informed action. Add get_pending_dataset_tlvs_with_etag(), returning the dataset together with the entity tag a border router hands out for it, and an if_match keyword on set_pending_dataset_tlvs() that passes the tag back: the router then only replaces the dataset the tag stands for, checked atomically with the write (openthread/ot-br-posix#3553). An older border router hands out no tag and would ignore the header, making the replace there unconditional, which is why callers must only expose this as an explicit choice, never as a default. A conditional replace that the router refuses raises the same PendingDatasetConflictError the guarded write already raises, with a message naming the other reason: the dataset changed since it was read. Assisted-By: Claude Fable 5
set_pending_dataset_tlvs() refuses whenever a pending dataset is in place, and deliberately offers no blanket override: superseding races the delay timer on every device holding the old dataset and silently undoes whatever that dataset was doing. What a caller may reasonably want is narrower: "replace exactly the dataset I read", as an explicit, informed action. Add get_pending_dataset_tlvs_with_etag(), returning the dataset together with the entity tag a border router hands out for it, and an if_match keyword on set_pending_dataset_tlvs() that passes the tag back: the router then only replaces the dataset the tag stands for, checked atomically with the write (openthread/ot-br-posix#3553). An older border router hands out no tag and would ignore the header, making the replace there unconditional, which is why callers must only expose this as an explicit choice, never as a default. A conditional replace that the router refuses raises the same PendingDatasetConflictError the guarded write already raises, with a message naming the other reason: the dataset changed since it was read. Assisted-By: Claude Fable 5
3 participants
Follow-up to #3552, which has merged; rebased onto main, so this is now a single commit.
If-None-Match gives a client create-only semantics; what it cannot
express is "replace, but only the dataset I read". A controller that
reads a dataset, stamps a successor and writes it back has no way to
detect another writer slipping in between the read and the write.
Give the dataset resources entity tags: GET answers with an ETag
derived from the stored TLVs (hashed, so the tag does not echo the
credentials it stands for; the pending dataset's delay timer is left
out of the hash since the running countdown does not make it a
different dataset), and a successful PUT reports the tag of what it
stored. The JSON representation is now rendered from the same stored
TLV bytes the tag covers, rather than the struct getters.
PUT honors If-Match per RFC 9110 section 13.1.1: the write only
happens when one of the given entity tags matches the stored dataset,
or, with
*, when any dataset is stored. Tags are compared with thestrong comparison function (section 8.8.3.2), and the precondition is
evaluated before If-None-Match (section 13.2.2), atomically with the
write in the main-loop task. A failed precondition answers 412, a
syntactically invalid header value 400, and the role check on the
active dataset still answers 409 first (section 13.2.1). This turns a
read-stamp-write sequence into a compare-and-swap.
cc @agners