RFC8768: Add in support for CoAP Option Hop Limit loop detection #476

mrdeep1 · 2020-03-23T07:35:11Z

All this is handled by libcoap with coap-client having the ability to define
an initial Hop Count value for testing. No other application support is
required.

Incoming requests are checked for the Hop Limit option - if found, the value
is decremented in the PDU before passing it up to the application. If it
decrements to 0, a 5.08 response is generated.

coap_add_option() checks if this is a Proxy option for a request. If so, and
Hop Limit option does not exist, then the Hop Limit option is inserted into
the PDU with the default value (16).

coap_send() checks if it is a 5.08 response and if so, prepends the diagnostic
data with the IP address of the transmitting interface. If the IP address
already exists, then the PDU is dropped as the return path for the 5.08 response
is looping. An alternative method for return path looping check is also
used by initially setting, then testing and then decrementing the Hop Limit
value as response packets pass back through the proxies. If a proxy does not
copy the Hop Limit options when returning the 5.08 response, then the IP address
duplication is used.

Makefile.am:

Hide the coap_insert_option(), coap_update_option and coap_pdu_check_resize()
function from applications.

README.md:

Add in RFC8768 is supported by libcoap.

examples/client.c:

Add in the -H hoplimit option.

include/coap2/pdu.h:

Add in the default Hop-Limit: option COAP_OPTION_HOP_LIMIT.
Re-organize the COAP_OPTION_ descriptions to include
the C, U, N, R, E, I and U flags.
Expose the internal coap_pdu_check_resize() function so src/net.c can use it.
Define the two new internal functions coap_insert_option() and
coap_update_option() and expose them so that the test suites can use them.

man/coap-client.txt.in:

Document the new -H hoplimit option.

man/coap.txt.in:

Include the new RFC8768.

man/coap_pdu_setup.txt.in:

Update documentation for the CoAP options.

src/coap_debug.c:

Add in support for printing out COAP_OPTION_HOP_LIMIT.

src/net.c:

Make sure that COAP_OPTION_HOP_LIMIT is not returned in an error response.
Decrement received Hop-limit in handle_request() and generate 5.08 response
if appropriate.
Update coap_send() to prepend IP in 5.08 response.
Support COAP_OPTION_HOP_LIMIT being used in the 5.08 response message.

src/pdu.c:

Report on CoAP options that are repeated, but not defined as repeatable.
Define new (5.08) Hop Limit Reached error response.

Add in new internal coap_insert_option() function. This now means that
callers of coap_add_option() no longer have to have the options in the
correct order.

Add in new internal coap_update_option() function that updates the value of
an option.

Make coap_pdu_check_resize() non-static.

tests/test_pdu.c:

Check that options are getting inserted in the correct order when using
coap_insert_option(), even though they are provided in a random order.

Test the coap_update_option() function.

obgm · 2020-03-23T08:39:26Z

include/coap2/uri.h

@@ -79,17 +82,36 @@ coap_uri_t *coap_clone_uri(const coap_uri_t *uri);
 * Parses a given string into URI components. The identified syntactic
 * components are stored in the result parameter @p uri. Optional URI
 * components that are not specified will be set to { 0, 0 }, except for the
- * port which is set to @c COAP_DEFAULT_PORT. This function returns @p 0 if
- * parsing succeeded, a value less than zero otherwise.
+ * port which is set to default port for the protocol. This function returns


"to default port for the protocol" → "to the default port for the protocol"

mrdeep1 · 2020-06-15T08:55:50Z

Pushed code with proxy specific code abstracted to #511

obgm

In the commit message, 2nd para: "Incoming requests are check" → "Incoming requests are checked".

I was wondering if the new function coap_insert_option() should at least be part of the internal API to enable unit tests against this function. I tested a little this way but would like to see more test coverage.

obgm · 2020-07-07T09:54:22Z

examples/client.c

-      default:
-      ;
+    default:
+      break;


Although this makes no difference in the generated code: Why would you have a break statement here?

I adjusted the indentation spacing and out of habit put in the break. Updated

obgm · 2020-07-07T10:47:19Z

examples/client.c

+  value = atoi(arg);
+
+  value = strtol(optarg, NULL, 10);


I think this should have been a mix of these two statements:

value = strtol(arg, NULL, 10);

Correct - well spotted.

obgm · 2020-07-07T10:50:52Z

include/coap2/pdu.h

-#define COAP_OPTION_CONTENT_FORMAT 12 /* E, uint, 0-2 B, (none) */
+/*
+ * The C, U, and N flags indicate the properties
+ * Critical, UnSafe, and NoCacheKey, respectively.


"UnSafe" → "Unsafe"

include/coap2/pdu.h

obgm · 2020-07-07T10:57:44Z

include/coap2/pdu.h

+#define COAP_OPTION_LOCATION_QUERY 20 /* ___RE__, String,   0-255 B, (none) */
+#define COAP_OPTION_PROXY_URI      35 /* CU-___U, String, 1-1034 B, (none) */
+#define COAP_OPTION_PROXY_SCHEME   39 /* CU-___U, String, 1-255 B, (none) */
+#define COAP_OPTION_SIZE1          60 /* __N_E_U, uint, 0-4 B, (none) */

 /* option types from RFC 7641 */


Just thinking aloud: When doing the re-ordering anyway, do we want to move the option numbers from extensions to the previous "Core CoAP list" and just note the extension RFC in the comment of each option?

I had done this in man/coap_pdu_setup.txt.in. Moved the code from there into pdu.h

src/net.c

obgm · 2020-07-07T12:50:06Z

src/net.c

+      coap_log(LOG_WARNING, "cannot send response for transaction %u\n",
+               pdu->tid);
+  }
+  return;


This statement is superfluous.

return; deleted

obgm · 2020-07-07T18:33:45Z

tests/test_pdu.c

+static void
+t_encode_pdu15(void) {
+  coap_optlist_t *optlist = NULL;
+  int n;


If you make n of data type size_t you wont need casting to int all the time in the following.

Updated as suggested

obgm · 2020-07-07T18:37:15Z

tests/test_pdu.c

@@ -790,6 +789,40 @@ t_encode_pdu14(void) {
  coap_delete_optlist(optlist);
 }

+static void
+t_encode_pdu15(void) {
+  coap_optlist_t *optlist = NULL;


Since optlist is not used in this function it can be deleted here as can the call to coap_delete_optlist() at the end.

mrdeep1 · 2020-07-11T17:39:20Z

In the commit message, 2nd para: "Incoming requests are check" → "Incoming requests are checked".

Corrected

I was wondering if the new function coap_insert_option() should at least be part of the internal API to enable unit tests against this function. I tested a little this way but would like to see more test coverage.

It now is - as well as a new internal helper function coap_update_option(). Test have been added for this.

Code changes pushed.

mrdeep1 · 2020-07-11T20:03:23Z

Builds failed if tests/testdriver was not built against the static libraries. Now fixed by allowing coap_insert_option() and coap_update_option() to be exposed in the .so file.

Perhaps in the future the test suite is built against the library files that expose all the internal functions so that the internal API functions can be properly tested. The external API can then be a suitably restricted subset of functions that the applications should be using.

mrdeep1 · 2020-07-12T18:08:11Z

By making the following change (.la -> .a), the tests can make use of options that are not exposed to general applications (it requires that --enable-static is set)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index d47be10..04fb6a7 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -26,7 +26,7 @@ testdriver_SOURCES = \
  test_wellknown.c \
  test_tls.c

-testdriver_LDADD = $(CUNIT_LIBS) $(DTLS_LIBS) $(top_builddir)/.libs/libcoap-$(LIBCOAP_NAME_SUFFIX).la
+testdriver_LDADD = $(CUNIT_LIBS) $(DTLS_LIBS) $(top_builddir)/.libs/libcoap-$(LIBCOAP_NAME_SUFFIX).a

 # If there is a API change to something $(LIBCOAP_API_VERSION) > 1 there is
 # nothing to adopt here. No needed to implement something here because the test

Is it worth doing this as a separate option?

obgm · 2020-07-16T08:09:31Z

Regarding static builds for unit tests: I think it is reasonable to enforce --enable-static with --enable-tests automatically (possibly with a comment in the help output or a diagnostic message). This way, we can avoid another dependency branch in the options set. I presume that many builds that allow dynamic libraries do not really care whether or not static libraries are created as well.

mrdeep1 · 2020-07-16T08:48:21Z

#525 was created to handle this and currently just requires --enable-static to be set (which it is by default) if --enable-tests is set. I will update #525 to force set --enable-static if --enable-tests is set with a warning output and update the help output.

obgm · 2020-07-17T12:48:27Z

examples/client.c

+  value = strtol(arg, NULL, 10);
+  if (value < 1 || value > 255) {
+    fprintf(stderr, "Hop Limit has to be > 0 and < 256\n");
+    exit(-1);


I advise against functions that have a hard program termination as side-effect. If you really need the program to terminate (instead of, say, give a warning that the default value is used due to a wrong parameter), I suggest to indicate via return value that the function arguments were not processed. And then, in the main switch statement, exit may be called.

Code updated (and pushed) along the suggested lines

All this is handled by libcoap with coap-client having the ability to define an initial Hop Count value for testing. No other application support is required. Incoming requests are checked for the Hop Limit option - if found, the value is decremented in the PDU before passing it up to the application. If it decrements to 0, a 5.08 response is generated. coap_add_option() checks if this is a Proxy option for a request. If so, and Hop Limit option does not exist, then the Hop Limit option is inserted into the PDU with the default value (16). coap_send() checks if it is a 5.08 response and if so, prepends the diagnostic data with the IP address of the transmitting interface. If the IP address already exists, then the PDU is dropped as the return path for the 5.08 response is looping. An alternative method for return path looping check is also used by initially setting, then testing and then decrementing the Hop Limit value as response packets pass back through the proxies. If a proxy does not copy the Hop Limit options when returning the 5.08 response, then the IP address duplication is used. Makefile.am: Hide the coap_insert_option(), coap_update_option and coap_pdu_check_resize() function from applications. README.md: Add in RFC8768 is supported by libcoap. examples/client.c: Add in the -H hoplimit option. include/coap2/pdu.h: Add in the default Hop-Limit: option COAP_OPTION_HOP_LIMIT. Re-organize the COAP_OPTION_ descriptions to include the C, U, N, R, E, I and U flags. Expose the internal coap_pdu_check_resize() function so src/net.c can use it. Define the two new internal functions coap_insert_option() and coap_update_option() and expose them so that the test suites can use them. man/coap-client.txt.in: Document the new -H hoplimit option. man/coap.txt.in: Include the new RFC8768. man/coap_pdu_setup.txt.in: Update documentation for the CoAP options. src/coap_debug.c: Add in support for printing out COAP_OPTION_HOP_LIMIT. src/net.c: Make sure that COAP_OPTION_HOP_LIMIT is not returned in an error response. Decrement received Hop-limit in handle_request() and generate 5.08 response if appropriate. Update coap_send() to prepend IP in 5.08 response. Support COAP_OPTION_HOP_LIMIT being used in the 5.08 response message. src/pdu.c: Report on CoAP options that are repeated, but not defined as repeatable. Define new (5.08) Hop Limit Reached error response. Add in new internal coap_insert_option() function. This now means that callers of coap_add_option() no longer have to have the options in the correct order. Add in new internal coap_update_option() function that updates the value of an option. Make coap_pdu_check_resize() non-static. tests/test_pdu.c: Check that options are getting inserted in the correct order when using coap_insert_option(), even though they are provided in a random order. Test the coap_update_option() function.

include/coap2/pdu.h

src/net.c

PR obgm#476 added in coap_insert_option() which could be called by coap_add_option() with with a coap_log() message set to LOG_DEBUG. When PR obgm#539 was merged in, this upped the logging level to LOG_WARNING which now causes test/testdriver to report unecessary messages. Logging level reverted to LOG_DEBUG.

obgm reviewed Mar 23, 2020

View reviewed changes

mrdeep1 force-pushed the rfc_8768 branch from b0a344c to 24c45f6 Compare March 23, 2020 13:10

mrdeep1 force-pushed the rfc_8768 branch from 24c45f6 to 0cd07e8 Compare April 2, 2020 08:49

mrdeep1 mentioned this pull request Apr 7, 2020

Single context for both a server and client with multiple sessions #481

Closed

mrdeep1 force-pushed the rfc_8768 branch 2 times, most recently from f9507bb to d90b9ae Compare May 6, 2020 11:37

mrdeep1 force-pushed the rfc_8768 branch from d90b9ae to 65fe4cf Compare May 12, 2020 14:56

mrdeep1 force-pushed the rfc_8768 branch from 65fe4cf to 451ea79 Compare June 15, 2020 08:44

mrdeep1 changed the title ~~RFC8768: Add in support and update Proxy-Uri Support~~ RFC8768: Add in support for CoAP Option Hop Limit loop detection Jun 15, 2020

mrdeep1 mentioned this pull request Jun 15, 2020

Proxy: Update Proxy support for Proxy-Uri: CoAP option #511

Merged

mrdeep1 force-pushed the rfc_8768 branch from 451ea79 to a0aed65 Compare July 3, 2020 16:07

obgm reviewed Jul 10, 2020

View reviewed changes

mrdeep1 force-pushed the rfc_8768 branch from a0aed65 to 34496eb Compare July 11, 2020 17:39

mrdeep1 force-pushed the rfc_8768 branch from 34496eb to 00c2732 Compare July 11, 2020 20:00

mrdeep1 force-pushed the rfc_8768 branch from 00c2732 to cfc29b0 Compare July 13, 2020 08:42

mrdeep1 force-pushed the rfc_8768 branch from cfc29b0 to 96c161d Compare July 26, 2020 19:42

mrdeep1 force-pushed the rfc_8768 branch 2 times, most recently from 99c3b0a to 6fd45b2 Compare August 15, 2020 11:02

obgm reviewed Sep 7, 2020

View reviewed changes

mrdeep1 force-pushed the rfc_8768 branch from 6fd45b2 to 7033555 Compare September 7, 2020 11:34

obgm approved these changes Sep 8, 2020

View reviewed changes

include/coap2/pdu.h Show resolved Hide resolved

src/net.c Show resolved Hide resolved

obgm merged commit ffc1979 into obgm:develop Sep 8, 2020

mrdeep1 deleted the rfc_8768 branch September 8, 2020 16:17

mrdeep1 mentioned this pull request Oct 16, 2020

coap_add_option: Fix code rebase logging level issue #555

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC8768: Add in support for CoAP Option Hop Limit loop detection #476

RFC8768: Add in support for CoAP Option Hop Limit loop detection #476

mrdeep1 commented Mar 23, 2020 •

edited

obgm Mar 23, 2020

mrdeep1 Mar 23, 2020

mrdeep1 commented Jun 15, 2020

obgm left a comment

obgm Jul 7, 2020

mrdeep1 Jul 10, 2020

obgm Jul 7, 2020

mrdeep1 Jul 10, 2020

obgm Jul 7, 2020

mrdeep1 Jul 10, 2020

obgm Jul 7, 2020

mrdeep1 Jul 10, 2020

obgm Jul 7, 2020

mrdeep1 Jul 10, 2020

obgm Jul 7, 2020

mrdeep1 Jul 10, 2020

obgm Jul 7, 2020

mrdeep1 Jul 10, 2020

mrdeep1 commented Jul 11, 2020

mrdeep1 commented Jul 11, 2020 •

edited

mrdeep1 commented Jul 12, 2020

obgm commented Jul 16, 2020

mrdeep1 commented Jul 16, 2020

obgm Jul 17, 2020

mrdeep1 Sep 7, 2020

RFC8768: Add in support for CoAP Option Hop Limit loop detection #476

RFC8768: Add in support for CoAP Option Hop Limit loop detection #476

Conversation

mrdeep1 commented Mar 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrdeep1 commented Jun 15, 2020

obgm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrdeep1 commented Jul 11, 2020

mrdeep1 commented Jul 11, 2020 • edited

mrdeep1 commented Jul 12, 2020

obgm commented Jul 16, 2020

mrdeep1 commented Jul 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrdeep1 commented Mar 23, 2020 •

edited

mrdeep1 commented Jul 11, 2020 •

edited