feat: enable oscore credentials lookup and storage

[BitFis]

Work related to #1892

Enabling:

• ensure oscore ctx is cleaned up
• allow storage and provide oscore credentials via find function
• write example to load oscore credentials from file system, using -O
• add API functions to override find and store echo and PIV / window values for storage

OPEN:

• separate echo handler into a read and write handler
• remove old structure, only expand via callback and config defintions

[mrdeep1]

It is worth installing and running pre-commit to tidy up the source formatting.

[mrdeep1]

I suggest that you go through all the failed tasks and fix the associated issue before doing the next push. There is some overlap, but a lot of discrete issues that need to get fixed.

[BitFis]

I suggest that you go through all the failed tasks and fix the associated issue before doing the next push. There is some overlap, but a lot of discrete issues that need to get fixed.

Apologies for the spam, I am used to that all runs are canceled automatically if a new push has triggered it.

[BitFis]

@mrdeep1 please have another look. I will finalize it next week, but please check if this follows the appropriate format.
I followed up now the coap_oscore*_conf_t format. Which means, the find function returns the config object, which then internally is converted to the oscore ctx. This removes the need to provide access to the underlying oscore ctx, enabling for internal optimisations. But to enable that echo challange and PIV + window work as expected, I had to add some additional handlers. Also I added a lifetime tracker recipiant->ref to ensure that the oscore temporary credentials can be safely cleaned up, it seems something like it is required, since the recipient currently could be removed and may lead to unexpected behavior if still referenced.

For now maybe focus on the API expansions in coap_oscore and the general flow. I will cleanup some internals next week, so it should then be ready for final scrutiny should this approach be fine.

[mrdeep1]

With this input, I guess best to build on 1952 by expanding the coap_oscore_rcp_conf_t type with SSN...

Agreed. One of the reasons for doing #1952 as coap_oscore_rcp_conf_t is expanded for group support.

Additionally I propose to drop all current config setter functions in this PR,....

If you wrap any callback invocations with coap_lock_callback(() or coap_lock_callback_ret(), then it is safe for the callback to invoke a libcoap function that does a call_lock_lock(). This should remove a lot of threading concerns.

With that, the question stays, how to effectively provide echo values and window state.

I think I would persevere with 2 at this point.

[BitFis]

@mrdeep1 update accordingly, dropped the API extensions besides register handlers.
However I moved loading the echo, window and last sequence counter via config in.
Felt simpler then adding multiple functions. Please check if the current approach is sufficient.

[mrdeep1]

Thanks for doing this work.

I'm currently running regression tests to see if any issues raised.

[magdalenaszumny]

Looks great! I have a question regarding the order of execution of finding OSC ctx using external callback and libcoap's RAM. Why this order? For me it seems that we should first search libcoap's RAM and then use external callback if no OSC ctx was found. If you imagine that you have external storage (like FLASH memory) thta you are using to persist OSC ctx through devices reboots, then it would be better to reach to RAM first and if OSC ctx is not available in RAM then reach to FLASH memory. For embedded devices reading FLASH would be suboptimal.

[BitFis]

@magdalenaszumny thanks for the feedback. If you consider the API extension to only focus on storage in flash, then I agree. But the main feature of the integration is to allow to expand or even potentially replace the libcoap internal storage. With this inital goal, it seemed more appropriate to call the lookup function first and keeping the control by the application. If we call the lookup function first, it allows the integrator of the function to control the behavior, if we call the internal lookup first, the control moves back to libcoap.

However, the PR has been through a few iterations. By example, reusing the libcoap provided external coap_oscore_conf_t type and requiring the textual format (for now). So how it is currently, its seems follow more of a fallback structure. But the inital idea was to prioritize external storage as the "source of truth.". I am open to change if the current order seems to not provide a clear benefit, given the full context. @mrdeep1 feel free to add your PoV here aswell.

[magdalenaszumny]

@magdalenaszumny thanks for the feedback. If you consider the API extension to only focus on storage in flash, then I agree. But the main feature of the integration is to allow to expand or even potentially replace the libcoap internal storage. With this inital goal, it seemed more appropriate to call the lookup function first and keeping the control by the application. If we call the lookup function first, it allows the integrator of the function to control the behavior, if we call the internal lookup first, the control moves back to libcoap.

However, the PR has been through a few iterations. By example, reusing the libcoap provided external coap_oscore_conf_t type and requiring the textual format (for now). So how it is currently, its seems follow more of a fallback structure. But the inital idea was to prioritize external storage as the "source of truth.". I am open to change if the current order seems to not provide a clear benefit, given the full context. @mrdeep1 feel free to add your PoV here aswell.

Thank you for explanation!

I think that our use cases are similar but different. In the future we would like to contribute to libcoap possibility to restore OSCORE context from the flash (or some other place). I think the flow would be very similar, but it would have to be done only if OSCORE context is not found in libcoap and then it would serve as fallback. I wonder if it would be possible to have one solution for both problems.

[mrdeep1]

I think that our use cases are similar but different.

Is it possible for the application layer to decide whether to get the appropriate information out of RAM / Flash?

[magdalenaszumny]

I think that our use cases are similar but different.

Is it possible for the application layer to decide whether to get the appropriate information out of RAM / Flash?

I think it should be possible, but probably it would require to first search through libcoap's RAM to check if there is a context. If context is not found in libcoap's RAM then we would search in FLASH.

[mrdeep1]

I have started on testing using the new functionality - apologies for taking so long to get around to it.

[mrdeep1]

I'm in the process of providing a reasonable number of changes to get this working properly and will generate a diff file against this PR.

• ID Context may not be defined
• Memory leak of oscore context that is created every time the oscore context needs to get looked up
• Other -fsanitize issues fixed.
• Documentation updated.

are some of the areas updated.

As there is a large overhead of creating the oscore context every packet, I have moved the find_cb to after checking if the oscore context already exists.

[mrdeep1]

Apologies for taking so long.

The example coap-server's coap_oscore_ctx_find() function is normally only called once per different incoming session doing OSCORE. The resultant recipient context is associated with the session which is used for all the subsequent lookups. However, if the incoming session is supporting multiple OSCORE sessions (e.g. a downstream proxy is multiplexing multiple OSCORE clients) then coap_oscore_ctx_find() will get called multiple times as the recipient context needs to be updated.

This should satisfy the FLASH requirements.

patch1.txt contains the suggested changes to 98b7a84 - there are a fair number of them.

• Coding style changes to be consistent.
• Documentation updated.
• Renamed file name to _<kid_context>.txt as most likely to have kid and kid_context is optional.
• coap_oscore_ctx_find() no longer adds in the kid / kid_context / last_seq / sliding_window / echo values, but uses 2 new set functions instead to the recipient configuration.
• Replaced variable recipient with rcp_ctx or recipient_ctx to give a better context of what sort of recipient is being updated.
• Moved recipient_ctx ref count updates to where they were being attached to, or removed from some structure to better handle configuration errors.
• Changed integer64 to unsigned64 configuration parameter to better reflect what it actually does.
• onfigure last_seq, sliding_window or echo_value need to be defined within a complex_recipient,config'' wrapper.

I'm not sure why echo_value needs to be tracked and re-used after a server startup, but have left the code in.

With the 2 new set functions, I am not sure that we need to be able configure last_seq, sliding_window or echo_value in a configuration file.

[BitFis]

I will look into the patch tomorrow and cherry-pick it, while provide some background information and feature preferences, which are important to me. Wont get to it today

[BitFis]

I rebased and changes attached as fixup. Nice approach with reusing the existing session recipient_id. Seems there are some issues, I will look into it tomorrow, maybe I didnt apply the patch correctly? Will check then again.

About storing the echo. I was a bit conservative, was not sure if it is enought just using the session. With the added change, we might get away of not needing to store the echo challenge in the external storage. Setting the echo challange was always related to, ensuring that the echo information is kept. Since, as you identified, for each request the security credentials are retrieved from the external storage.

Lastly, seems we only check the rcpkey_id. We dont need to check the ctxkey_id since we loaded it earlier? Aka. if it changed in the request?

[mrdeep1]

To keep the scan-build happy, the following needs to be applied

diff --git a/src/oscore/oscore_context.c b/src/oscore/oscore_context.c
index 5b03e59d..e5f17ed7 100644
--- a/src/oscore/oscore_context.c
+++ b/src/oscore/oscore_context.c
@@ -368,8 +368,9 @@ oscore_find_context(coap_session_t *session,
       }
       tmp_ctx->recipient_chain = *recipient_ctx;
       coap_oscore_session_set_recipient_ctx(session, *recipient_ctx);
-      /* Remove the tmp_ctx reference */
-      (*recipient_ctx)->ref--;
+      /* Remove the just added reference */
+      if (*recipient_ctx)
+        (*recipient_ctx)->ref--;
       return tmp_ctx;
     }
   }

Otherwise, you have all my changes integrated.

Lastly, seems we only check the rcpkey_id. We dont need to check the ctxkey_id since we
loaded it earlier? Aka. if it changed in the request?

Good question. I did not check out Appendix B.2 usage. It is where ID Context is actually used and changed that may need to be checked.

[BitFis]

Generally it seems Appendix 2.B will be depricated in the future with https://datatracker.ietf.org/doc/draft-ietf-core-oscore-key-update/. I am not at all familiar with KUDOS. It seems however, it also uses the randomized context id.
However, truthfully, I would not mind moving on with this merge request and resolve it to at a later stage. Generally not sure how well the handlers will work in combination with the Appendix B.2. . I focused on Appendix B.1 mainly.
I am open to adjust it to have the check, just to be sure, but we can also leave it as is. I leave this up to you. Whenever you are fine, I would rebase the commits to one with or without the additional check.

[mrdeep1]

This seems to be working for OSCORE B.2.
patch3.txt Edit: Documentation updated v patch2.

At some point the commits need to be squashed into a single commit.

The storage of the echo_value does need to be re-evaluated as the echo value is normally randomly generated for the phase of the OSCORE setup.