coap_resource_parse_observe returns 0 (register) instead of 1 (deregister) upon a deregister of an unknown observer

[SureshotM6]

Describe the bug

coap_resource_parse_observe should return "the observe option value in case of success or negative in case of error." This should be 0 for register and 1 for deregister. However, if a deregister request (1) is received and the observer is already deregistered (e.g. upon a retransmission of a deregister or a stale deregister from a previous session), then coap_resource_parse_observe improperly returns a value of 0 which makes it appear that the client requested a registration instead.

This can cause the response packet to improperly include an Observe option if the response from coap_resource_parse_observe is checked for a value of 0 (subscribe) and can cause caller observation databases to get out of sync. This also violates RFC7641 section 4.1: "The resulting response MUST NOT include an Observe Option."

To fix this, the return from coap_service_remove_observer inside of coap_resource_parse_observe should be checked for a value of 0 (observer didn't exist) and coap_resource_parse_observe should still return 1 ("the observe option value") in that case as documented.

Regression

• This is a regression.

Steps to reproduce

1. Run a coap server with a resource supporting observation that calls coap_resource_parse_observe such as the observer.c sample for coap.
2. Send a Observe deregistration command (1) from a client for a token that does not exist.
3. coap_resource_parse_observe returns 0 (register) instead of 1 (deregister)
4. The observer.c sample performs r == 0 ? resource->age : 0, so resource->age may be improperly sent if another observer is present for the same resource.

Relevant log output

Impact

Annoyance – Minor irritation; no significant impact on usability or functionality.

Environment

Bug is present in latest main (2752814)

Additional Context

No response

[pdgendt]

@SureshotM6 thanks for reporting this! Would it make sense to return an error code -ENOENT for coap_resource_parse_observe to indicate the difference?
Similar as coap_resource_remove_observer.

[SureshotM6]

Thanks for fixing this! Unfortunately I couldn't easily submit a PR currently.

Would it make sense to return an error code -ENOENT for coap_resource_parse_observe to indicate the difference? Similar as coap_resource_remove_observer.

That works just as long as the caller can differentiate an internal error vs a duplicate unsubscribe (they can with some additional code below).

Since CoAP is lossy, it is likely that duplicate observe subscribe and unsubscribe packets will be received. The subscribe path is already handled -- it still returns 0 if a duplicate subscribe is received. The unsubscribe portion now returns -ENOENT, so the caller just need to be sure to treat that as a normal unsubscribe request (since the first unsubscribe response was likely just lost) and return a normal COAP_RESPONSE_CODE_CONTENT as per the RFC rather than a COAP_RESPONSE_CODE_INTERNAL_ERROR for the -ENOENT case.

-ENOENT is also currently returned when the service can't be found (I think that's the only spot, but that also should really be impossible to hit). So it looks like the right thing for a caller to do with the new code would either be to re-check the observe option on error, which should work:

if (r == -ENOENT && coap_get_option_int(request, COAP_OPTION_OBSERVE) == 1) {
    // duplicate / restransmitted unsubscribe: send normal response anyway
    ...
}

The observer.c sample will already handle this "properly" as it completely ignores errors (not great, but it's just sample code).