gcoap_forward_proxy: copy Max-Age from forwarded Valid if it exists

[miri64]

Contribution description

At the moment, the Max-Age option is not set in a forwarded Valid message by a forward proxy. This is a bug, as the downstream clients then assume that the Max-Age is the default 60s, possibly breaking Cache validation, if Max-Age actually was smaller. This PR fixes that.

During testing this fix, I found another bug in the client-side cache validation: when the ETag is shorter than 8 bytes (e.g. with the fileserver), it is shortened after the fact. For that the wrong bitmask was used to protect the Option Delta.

Testing procedure

I applied the following patch to add a Max-Age option to gcoap_fileserver

Details

diff --git a/sys/net/application_layer/gcoap/fileserver.c b/sys/net/application_layer/gcoap/fileserver.c
index eaf14a252b..45bf652507 100644
--- a/sys/net/application_layer/gcoap/fileserver.c
+++ b/sys/net/application_layer/gcoap/fileserver.c
@@ -172,2 +172,3 @@ static ssize_t _get_file(coap_pkt_t *pdu, uint8_t *buf, size_t len,
         coap_opt_add_opaque(pdu, COAP_OPT_ETAG, &etag, sizeof(etag));
+        coap_opt_add_uint(pdu, COAP_OPT_MAX_AGE, 5U);
         return coap_opt_finish(pdu, COAP_OPT_FINISH_NONE);
@@ -182,2 +183,3 @@ static ssize_t _get_file(coap_pkt_t *pdu, uint8_t *buf, size_t len,
     coap_opt_add_opaque(pdu, COAP_OPT_ETAG, &etag, sizeof(etag));
+    coap_opt_add_uint(pdu, COAP_OPT_MAX_AGE, 5U);
     coap_block_slicer_t slicer;

The following script should succeed:

#! /usr/bin/env python3
import sys
import time

from scapy.all import load_contrib, AsyncSniffer
from scapy.contrib.coap import CoAP

from riotctrl.ctrl import RIOTCtrl

sys.path.append("dist/pythonlibs")
from riotctrl_shell.netif import IfconfigListParser, Ifconfig


def set_proxy(client, proxy):
    netifs = IfconfigListParser().parse(proxy.ifconfig_list())
    proxy_addr = list(netifs.values())[0]["ipv6_addrs"][0]["addr"]
    res = client.cmd(f"coap proxy set {proxy_addr} 5683")
    assert f"coap proxy set {proxy_addr} 5683" in res


def get_test_txt(client, server_addr):
    ret = client.cmd(f"coap get -c {server_addr} 5683 /vfs/test.txt")
    if "00000000  48  65  6C  6C  6F  20  57  6F  72  6C  64  21" not in ret:
        # may come in asynchronously, (synchronously when cached) so we need to expect
        # instead of checking result
        client.riotctrl.term.expect_exact(
            "00000000  48  65  6C  6C  6F  20  57  6F  72  6C  64  21"
        )


def test_proxy(modules=["gcoap_forward_proxy", "nanocoap_cache"]):
    RIOTCtrl.MAKE_ARGS = ("-j", "--no-print-directory")
    apps = ["examples/gcoap_fileserver"] + (2 * ["examples/gcoap"])
    envs = [
        {"PORT": "tap0", "QUIETER": "1", "USEMODULE": "vfs_auto_format"},
        {
            "USEMODULE": " ".join(modules),
            "PORT": "tap1",
            "BINDIRBASE": "proxy-bin",
            "QUIETER": "1",
        },
        {"USEMODULE": "nanocoap_cache", "PORT": "tap2", "QUIETER": "1", "TERMLOG":
            "test.log"},
    ]
    ctrls = [
        RIOTCtrl(application_directory=app, env=env)
        for app, env in zip(apps, envs)
    ]
    for ctrl in ctrls:
        ctrl.flash(stdout=None, stderr=None)
    with (
        ctrls[0].run_term(reset=False),
        ctrls[1].run_term(reset=False),
        ctrls[2].run_term(reset=False),
    ):
        shells = []
        for ctrl in ctrls:
            ctrl.term.logfile = sys.stdout
            shells.append(Ifconfig(ctrl))
        netifs = IfconfigListParser().parse(shells[0].ifconfig_list())
        server_addr = list(netifs.values())[0]["ipv6_addrs"][0]["addr"]
        set_proxy(shells[2], shells[1])
        sniffer = AsyncSniffer(iface="tapbr0")
        sniffer.start()
        get_test_txt(shells[2], server_addr)
        get_test_txt(shells[2], server_addr)   # should come from cache
        time.sleep(6)
        get_test_txt(shells[2], server_addr)   # should be validated
        time.sleep(6)
        get_test_txt(shells[2], server_addr)   # should be validated
        pkts = sniffer.stop()
        coap_pkts = [pkt for pkt in pkts if CoAP in pkt]
        # 1 GET request was fulfilled, 1 cached, 2 validated
        # => 3 GET requests, 1 Content, 2 Valid, multiplied by two due to the proxy
        get_pkts = [pkt for pkt in coap_pkts if pkt[CoAP].code == 1]
        assert len(get_pkts) == 6, f"{len(get_pkts)} != 6"
        content_pkts = [pkt for pkt in coap_pkts if pkt[CoAP].code == ((2 << 5) | 5)]
        assert len(content_pkts) == 2, f"{len(content_pkts)} != 2"
        valid_pkts = [pkt for pkt in coap_pkts if pkt[CoAP].code == ((2 << 5) | 3)]
        assert len(valid_pkts) == 4, f"{len(valid_pkts)} != 4"
        assert len(coap_pkts) == (6 + 2 + 4), f"{len(coap_pkts)} != (6 + 2 + 4)"
        for shell in reversed(shells):
            shell.cmd("help")


if __name__ == "__main__":
    load_contrib("coap")
    test_proxy()
    print("")

On master (with 1135cc0 cherry-picked, otherwise the proxy will mangle the ETag), this does not succeed, as for the fourth get_test_txt() the client does not validate, but gets it from a wrongly configured cache.

Traceback (most recent call last):
  File "/home/mlenders/Repositories/RIOT-OS/RIOT/./test_upstream_proxy.py", line 88, in <module>
    test_proxy()
  File "/home/mlenders/Repositories/RIOT-OS/RIOT/./test_upstream_proxy.py", line 76, in test_proxy
    assert len(get_pkts) == 6, f"{len(get_pkts)} != 6"
AssertionError: 4 != 6

Issues/PRs references

[chrysn]

Changes look good, and I convinced myself that indeed COAP_ETAG_LENGTH_MAX is a CoAP limit and thus we don't expect that someone would change that.

As I'm having trouble following the riotctrl test, I was trying to do some testing on my own ... is there really neither a test nor an example of the gcoap_forward_proxy module around?

[miri64]

As I'm having trouble following the riotctrl test, I was trying to do some testing on my own ... is there really neither a test nor an example of the gcoap_forward_proxy module around?

No, but that's probably something we should change long-term...

[miri64]

Changes look good, and I convinced myself that indeed COAP_ETAG_LENGTH_MAX is a CoAP limit and thus we don't expect that someone would change that.

If that's something up for discussion, it probably should have already been discussed in #17888... Maybe the bug that is fixed here would have been spotted earlier then too 😅

[miri64]

As I'm having trouble following the riotctrl test, […]

What trouble are you facing?

[miri64]

Ping?

[chrysn]

I was trying to see what is all being started where in riotctrl -- but for reviewing this it's actually not critical: The changes look good, the tests look plausible and I trust you've run them.

[chrysn]

Looks good, sorry for the delay in reviewing.

[miri64]

Let's give Murdock a final spin on this... the last build was from mid-August.

[maribu]

Let's get the jammy fixes in first. Looks like CI runs will fail anyway until those are sorted out.