Skip to content

gcoap: forward_proxy: forward truncated responses#18091

Closed
cgundogan wants to merge 1 commit intoRIOT-OS:masterfrom
cgundogan:pr/proxy-forward-truncated
Closed

gcoap: forward_proxy: forward truncated responses#18091
cgundogan wants to merge 1 commit intoRIOT-OS:masterfrom
cgundogan:pr/proxy-forward-truncated

Conversation

@cgundogan
Copy link
Member

@cgundogan cgundogan commented May 11, 2022

Contribution description

#16378 introduced truncated responses for cases where the gcoap message buffer is too small. This PR extends the proxy check to also forward these truncated responses.

Testing procedure

The same way as described in #17801 (with sufficiently large files to trigger block-wise transfer)

Issues/PRs references

@cgundogan cgundogan added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking labels May 11, 2022
@cgundogan cgundogan requested a review from miri64 May 11, 2022 16:42
@cgundogan cgundogan added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 11, 2022
@github-actions github-actions bot added Area: CoAP Area: Constrained Application Protocol implementations Area: sys Area: System labels May 11, 2022
@miri64
Copy link
Member

miri64 commented May 12, 2022

(with sufficiently large files to trigger block-wise transfer)

I don't think that has anything to do with block-wise transfer, just responses too large for the gcoap listen buffer ;-).

@miri64 miri64 requested a review from chrysn May 12, 2022 06:33
@miri64
Copy link
Member

miri64 commented May 12, 2022

Mh... thinking about it: Does it make sense to forward a truncated packet at the proxy without the client having any indication, that it is truncated?

On the other hand, this is exactly the same situation #16378 was trying to fix in the first place and @cgundogan now already ran into the situation that the message was truncated...

Very conflicted about this one...

@miri64
Copy link
Member

miri64 commented May 12, 2022

Maybe instead send a 5.00 or 5.05 to the client instead in case of RESP_TRUNC? Even though 5.05 seems a bit off, just from its description in RFC 7252 it may fit:

5.9.3.6. 5.05 Proxying Not Supported

The server is unable or unwilling to act as a forward-proxy for the
URI specified in the Proxy-Uri Option or using Proxy-Scheme (see
Section 5.10.2).

@chrysn
Copy link
Member

chrysn commented May 12, 2022

Returning truncated responses is almost certainly a mistake; this needs to become some proxy error. IMO the CoAP buffer size (as long as we need one) in a proxy should even default to a UDP MTU -- this is not a system that is so constrained that it can't even keep a full one.

(One might consider trimming it to a block size and adding an adequate block option, but I don't even want to start thinking about the list of preconditions to that).

@miri64
Copy link
Member

miri64 commented May 12, 2022

Returning truncated responses is almost certainly a mistake; this needs to become some proxy error. IMO the CoAP buffer size (as long as we need one) in a proxy should even default to a UDP MTU -- this is not a system that is so constrained that it can't even keep a full one.

At the moment the proxy is just a module. A default config for the buffer size could be provided, when the module is included, but it still should be configurable under the discretion of the user. So if the buffer is too small in that case, in either case I agree, an error should be sent.

@miri64
Copy link
Member

miri64 commented May 12, 2022

Thinking a bit ahead: I think, to lessen the impact on the network, the "faulty" proxy should still cache the truncated packet (with an annotation that it is truncated), so it does not continue to ask for a response that is too large for its buffer. I can introduce this however in #17888.

@cgundogan cgundogan removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 12, 2022
@miri64
Copy link
Member

miri64 commented May 12, 2022

In #17888, I now used a 5.00 to report that the response was truncated (edit: apparently linking a specific line in a diff does not work properly cross-PR: I was referring to righthand side ll 223-230)

@cgundogan
Copy link
Member Author

#17888 obsoletes this PR.

@cgundogan cgundogan closed this May 13, 2022
@cgundogan cgundogan deleted the pr/proxy-forward-truncated branch May 13, 2022 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants