OSCORE: Added user-defined callback for finding oscore context #1331

mopsiok · 2024-02-21T11:30:10Z

Added functionality to restore OSCORE context from external storage (e.g. FLASH), when no context was found by oscore_find_context.

mrdeep1 · 2024-02-21T12:26:52Z

Thanks for raising this PR.

I would like to see coap_register_find_context_handler() replaced by coap_register_oscore_context_handler() so that it is obviously a part of OSCORE and not some other generic context function.

I would also like to see man/coap_oscore.txt.in updated to include this new function.

include/coap3/coap_net_internal.h

src/coap_net.c

mrdeep1 · 2024-02-21T13:01:58Z

If you can also change the commit title to

OSCORE: Added user-defined callback for finding oscore context

that would be helpful.

mopsiok · 2024-02-22T15:05:53Z

Hi,
thank you for your comments! I will get back to this PR in the following week. Currently our team is implementing OSCORE context storage using non-volatile memory. We think our insights could be useful to more people, but we need more time to prepare them for open source consumption.

mrdeep1 · 2024-02-22T17:51:39Z

If you are keeping all the oscore_ctx_t outside of libcoap, then I suspect you will need an 'add to external' function as well as the find_context_handler.

A suggested place to do this would be in oscore_enter_context() where it (optionally) gets stored externally and the original one freed off. This function would then need to return the updated (RAM) value for subsequent use.

Then coap_register_oscore_context_handler() can be updated to include the add and find functions.

Edit: A 'remove from external' will be needed as well.

mopsiok · 2024-03-06T10:16:20Z

Hello again!
I resolved the points you mentioned earlier regarding naming convention and the documentation. The team and I needed to provide some additional changes, which made the NVM context restoring possible. Please take a look when you got some time.

Just to clarify, we didn't want to keep all oscore_ctx_t outside of libcoap - I think it is much better to use existing internal logic to take care of most of the OSCORE work. What we wanted to do, is simply reusing the oscore contexts between device reboots. To achieve it, we decided to use the already available coap_oscore_save_seq_num_t callback, which periodically updates contexts stored in NVM with their respective SSN values. Then, after reboot, we can:

(on the client side) manually create proper OSCORE session based on NVM data - completely on the application layer
(on the server side) automatically restore the OSCORE session if no matching context is found inside library's RAM, using the callback provided in this PR

This way, the library still works as designed, yet providing additional functionalities in case they are needed. What do you think about such approach?

BTW. I still need to take care of the failing tests, but first, please take a look to make sure you're ok with the general idea.

mrdeep1

In general looking good.

mrdeep1 · 2024-03-06T10:13:45Z

include/coap3/coap_oscore.h

-typedef int (*coap_oscore_save_seq_num_t)(uint64_t sender_seq_num, void *param);
+typedef int (*coap_oscore_save_seq_num_t)(uint64_t sender_seq_num, 
+                                          const coap_bin_const_t *recipient_id, 
+                                          const coap_bin_const_t *id_context,
+                                          void *param);


This is a change to the Public API which we do not want to have to do unless it is absolutely critical.

My suggestion would be to add in save (and delete) function parameters to coap_register_oscore_context_handler() and call those functions in the appropriate points in the code. Then from a user perspective, everything is in one place for doing the definitions.

Update coap_context-t to include something similar to external_oscore_update_context_handler and external_oscore_delete_context_handler.

mrdeep1 · 2024-03-06T10:29:00Z

include/coap3/coap_oscore.h

+/**
+ * Opaque pointer for internal oscore_ctx_t type.
+ */
+typedef struct oscore_ctx_t * oscore_ctx_handle_t;
+
+/**
+ * Opaque pointer for internal oscore_recipient_ctx_t type.
+ * 
+ */
+typedef struct oscore_recipient_ctx_t * oscore_recipient_ctx_handle_t;


In general for libcoap, all the other typedef struct X X; statements do not have the * (pointer) included. We need to be consistent here.

In general, any forward declarations should be included in the appropriate place in include/coap3/coap_forward_decls.h.

As we are trying to keep the public API to have everything prefixed with coap_ to prevent any name clashes, I think perhaps the 2 forward references (in include/coap3/coap_forward_decls.h) should be

typedef struct oscore_ctx_t coap_oscore_handle_t typedef struct oscore_recipient_ctx_t coap_oscore_recipient_ctx_handle_t

and then referenced in your changes with the new names.

mrdeep1 · 2024-03-06T10:31:11Z

include/coap3/coap_oscore.h

+ * @param context   The current coap context to use.
+ * @param handler   User callback.
+ */
+void coap_register_oscore_context_handler(coap_context_t *context, external_oscore_find_context_handler_t handler);


As mentioned previously, this should include an add/update handler as well as possibly a delete handler. Even if delete is not currently used, it is a placeholder that will not break the API with that as a change.

mrdeep1 · 2024-03-06T10:32:23Z

include/coap3/coap_session.h

@@ -24,6 +24,8 @@
 * @{
 */

+#include <stdbool.h>


For historical reasons that I cant recall, it was decided that there should not be the use of boolean in the libcoap code, so this should not be used.

mrdeep1 · 2024-03-06T10:33:10Z

include/coap3/coap_session.h

+ *
+ * @return True if session is encrypted, false if session is not encrypted.
+ */
+bool coap_session_is_encrypted(const coap_session_t *session);


Use int instead of bool, and document return value accordingly.

mrdeep1 · 2024-03-06T10:41:17Z

man/coap_session.txt.in

@@ -26,6 +26,7 @@ coap_session_get_proto,
 coap_session_get_state,
 coap_session_get_tls,
 coap_session_get_type,
+coap_session_is_encrypted,


As you may have discovered, you will need to add in an entry line into man/Makefile.am

@echo ".so man3/coap_session.3" > coap_session_get_type.3 + @echo ".so man3/coap_session.3" > coap_session_is_encrypted.3 @echo ".so man3/coap_string.3" > coap_delete_bin_const.3

I would also prefer to see this entry at the end of the list, after the set/get functions.

mrdeep1 · 2024-03-06T10:41:59Z

man/coap_session.txt.in

@@ -68,6 +69,8 @@ coap_tls_library_t *tls_lib);*

 *coap_session_type_t coap_session_get_type(const coap_session_t *_session_);*

+*bool coap_session_is_encrypted(const coap_session_t *session);*


Use int instead of bool.

mrdeep1 · 2024-03-06T10:42:57Z

man/coap_session.txt.in

+*coap_session_is_encrypted*() returns true if the session is using OSCORE encryption, or false if plain CoAP packets are used.
+


Correct return types not to be boolean. Also, move this to after the get functions.

mrdeep1 · 2024-03-06T10:44:12Z

man/coap_session.txt.in

+The *coap_session_is_encrypted*() function is used to check if incoming _session_ is encrypted.
+


This should be moved to after the get functions.

mrdeep1 · 2024-03-06T10:45:03Z

src/coap_oscore.c

+                                   rcp_ctx->recipient_id,
+                                   osc_ctx->id_context,


Unfortunately this breaks the public API - See previous comments.

mrdeep1 · 2024-03-06T10:52:09Z

we decided to use the already available coap_oscore_save_seq_num_t callback

Unfortunately as mentioned above, this breaks any backwards public API use. See alternative suggestions in my comments. Wherever coap_oscore_save_seq_num_t is called, just call your newly defined add/update handler.

Thanks for doing all this work.

mrdeep1 · 2024-04-26T13:38:17Z

@mopsiok Are you able to progress with this PR?

mrdeep1 · 2024-05-21T10:04:18Z

@mopsiok Are you able to progress with this PR?

mopsiok · 2024-05-21T11:45:33Z

Hi,
sorry for the delay. I will need to consult it within my team. I see some of your points, some I would like to discuss first (mainly about the consequences on the application layer which result from current solution) - but I don't have any spare time to continue the topic right now.

For now, feel free to close this PR if you like, and I will contact you when I'm allowed to move this task forward.

Thanks for your time and effort!
Best regards,
Patryk

mopsiok force-pushed the feature/add_context_search_callback branch from 1ac0938 to 12629c4 Compare February 21, 2024 12:00

mrdeep1 reviewed Feb 21, 2024

View reviewed changes

include/coap3/coap_net_internal.h Outdated Show resolved Hide resolved

include/coap3/coap_net_internal.h Outdated Show resolved Hide resolved

src/coap_net.c Outdated Show resolved Hide resolved

mopsiok force-pushed the feature/add_context_search_callback branch from 12629c4 to 9f2f4ec Compare March 6, 2024 09:32

OSCORE: Added user-defined callback for finding oscore context

79426d6

mopsiok force-pushed the feature/add_context_search_callback branch from 9f2f4ec to 79426d6 Compare March 6, 2024 09:49

mopsiok changed the title ~~Added user-defined callback for finding oscore context~~ OSCORE: Added user-defined callback for finding oscore context Mar 6, 2024

mrdeep1 reviewed Mar 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OSCORE: Added user-defined callback for finding oscore context #1331

OSCORE: Added user-defined callback for finding oscore context #1331

mopsiok commented Feb 21, 2024

mrdeep1 commented Feb 21, 2024

mrdeep1 commented Feb 21, 2024

mopsiok commented Feb 22, 2024

mrdeep1 commented Feb 22, 2024 •

edited

Loading

mopsiok commented Mar 6, 2024

mrdeep1 left a comment

mrdeep1 Mar 6, 2024

mrdeep1 Mar 6, 2024

mrdeep1 Mar 6, 2024

mrdeep1 Mar 6, 2024

mrdeep1 Mar 6, 2024

mrdeep1 Mar 6, 2024

mrdeep1 Mar 6, 2024

mrdeep1 Mar 6, 2024

mrdeep1 Mar 6, 2024

mrdeep1 Mar 6, 2024

mrdeep1 commented Mar 6, 2024

mrdeep1 commented Apr 26, 2024

mrdeep1 commented May 21, 2024

mopsiok commented May 21, 2024

		@@ -68,6 +69,8 @@ coap_tls_library_t tls_lib);

		coap_session_type_t coap_session_get_type(const coap_session_t _session_);*

		bool coap_session_is_encrypted(const coap_session_t session);*

		coap_session_is_encrypted() returns true if the session is using OSCORE encryption, or false if plain CoAP packets are used.

		The coap_session_is_encrypted() function is used to check if incoming _session_ is encrypted.

OSCORE: Added user-defined callback for finding oscore context #1331

Are you sure you want to change the base?

OSCORE: Added user-defined callback for finding oscore context #1331

Conversation

mopsiok commented Feb 21, 2024

mrdeep1 commented Feb 21, 2024

mrdeep1 commented Feb 21, 2024

mopsiok commented Feb 22, 2024

mrdeep1 commented Feb 22, 2024 • edited Loading

mopsiok commented Mar 6, 2024

mrdeep1 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

mrdeep1 commented Mar 6, 2024

mrdeep1 commented Apr 26, 2024

mrdeep1 commented May 21, 2024

mopsiok commented May 21, 2024

mrdeep1 commented Feb 22, 2024 •

edited

Loading