search

Fraunhofer-AISEC / uoscore-uedhoc

OSCORE and EDHOC libraries for or constrained (and non-constrained) devices. See https://arxiv.org/pdf/2103.13832.pdf for more background.
Other
16 stars 11 forks source link

Simplifying repository structure for sources and includes #8

Closed stoprocent closed 2 years ago

stoprocent commented 2 years ago

Hi @StefanHri

As discussed before I think it would be nice to reorganize a little bit how the repository is structured. Today I understand it's split into modules in a pattern, source: module_name/src, headers: modules_name/inc and now since change to CDDL we have also module_name/cbor with a mix of scripts, cddl files, headers, and sources. Also the main header for a module I located directly in module_name folder. This is causing the multiple include paths and complexity in #includes around the code.

As an example, we end up having something like this in oscore2coap.c

#include "oscore.h"
#include "../../common/inc/byte_array.h"
#include "../../common/inc/oscore_edhoc_error.h"
#include "../../common/inc/memcpy_s.h"
#include "../inc/aad.h"
#include "../inc/coap.h"
#include "../inc/nonce.h"
#include "../inc/option.h"
#include "../inc/oscore_cose.h"
#include "../../common/inc/print_util.h"
#include "../inc/security_context.h"

My proposal would be to make it reorganize repository to a pattern like this:

Headers:

REPO_ROOT/inc/oscore
REPO_ROOT/inc/edhoc
REPO_ROOT/inc/common
REPO_ROOT/inc/cbor

Sources:

REPO_ROOT/src/oscore
REPO_ROOT/src/edhoc
REPO_ROOT/src/common
REPO_ROOT/src/cbor

CCDL Files

REPO_ROOT/models

CCDL Generation Script

REPO_ROOT/scripts

Making these changes will allow us to simplify header search paths and cleanup the code so mentioned above header part could look like this:

#include "oscore/oscore.h"

#include "common/byte_array.h"
#include "common/oscore_edhoc_error.h"
#include "common/memcpy_s.h"
#include "common/print_util.h"

#include "oscore/aad.h"
#include "oscore/coap.h"
#include "oscore/nonce.h"
#include "oscore/option.h"
#include "oscore/oscore_cose.h"
#include "oscore/security_context.h"

I will also ask others at the team to have a look and comment.

rlubos commented 2 years ago

As for the headers cleanup, I agree that the proposed inclusion scheme looks way better. I'm just a bit concerned about mixing public and private headers in a single directory, it might not be straightforward at the first look what APIs are intended to use in the applications and which not. Perhaps keeping private headers in a separate directory would be a good idea, we'd just need to add both to the include path for the library build (the application could stick to the public headers only, effectively preventing it from using private headers).

stoprocent commented 2 years ago

There is no public and private headers right now so not sure if this is a valid point tho.

rlubos commented 2 years ago

What about https://github.com/Fraunhofer-AISEC/uoscore-uedhoc/blob/main/modules/edhoc/edhoc.h and https://github.com/Fraunhofer-AISEC/uoscore-uedhoc/blob/main/modules/oscore/oscore.h then? Aren't those meant to be public APIs?

stoprocent commented 2 years ago

No, I dont think so. If they would be public and everything else would be private than why oscore.h contains:

#include "inc/byte_array.h"
#include "inc/error.h"
#include "inc/print_util.h"
#include "inc/security_context.h"
#include "inc/supported_algorithm.h"

this will mean that byte_array.h and others from this file are either public or ... public header oscore.h includes private headers :) I think module or main include is oscore.h and edhoc.h but there is no point in defining private and public headers in such a library.

StefanHri commented 2 years ago

yes edhoc.h, edhoc_internal.h and oscore.h Are meant to contain the public API. Everything else should not be used by the user.

stoprocent commented 2 years ago

@StefanHri please see my comment above.

StefanHri commented 2 years ago

OK I will clean up the all API in way that they contain only standard types. Then we can remove all these includes and have public headers. Is this OK?

stoprocent commented 2 years ago

This is an open-source library which means that anyone can include whatever they want. There is no point in introducing a more complex folder structure.

There are 2 modules: oscore - by including oscore.h edhoc - by including edhoc.h

I seriously cannot recollect any library like this where I would see 

#include `private/header.h`
StefanHri commented 2 years ago

This is an open-source library which means that anyone can include whatever they want. There is no point in introducing a more complex folder structure.

There are 2 modules: oscore - by including oscore.h edhoc - by including edhoc.h

I seriously cannot recollect any library like this where I would see

#include `private/header.h`

I don't understand? Will be ok to put the oscore.h and edhoc.h in the root of the project directory and remove all includes that they contain?

mopsiok commented 2 years ago

As I understand this: since there is no distinction between private and public headers in c, the user can include anything he wants. So the cost of sacrificing overall clarity of code and folder structure may be too high for a feature that doesn't really exclude anything from the access of the user.

Proposed folder structure looks transparent and could be more beneficial than trying to hide some stuff which is accessible anyway.

rlubos commented 2 years ago

At least we all agree that some cleanup is needed.

this will mean that byte_array.h and others from this file are either public or ... public header oscore.h includes private headers :)

If structures/utility functions defined in byte_array.h are intended to be used by the application, then yes, the header should be public. Indeed, the "core" API headers oscore.h/edhoc.h do include a subset of other headers which should be public (or the header should be refactored to avoid inclusion), but that doesn't mean everything needs to be accessible from the application.

I seriously cannot recollect any library like this where I would see

include private/header.h

I have not suggested such a solution at any point, so I really don't get the argument. My point was not to mix public/private headers, from the library source files POV it wouldn't even be noticed as it all can be resolved at the include path level. But the application doesn't get access to the private part, and can easily determine what APIs should be used by the app just by looking at the include directory.

A common approach is to have a root-level include directory with public APIs and keep private headers together with the sources. As a contrary - neither of the projects I know puts all headers together, but you'll probably come up with some example.

AleksanderDemianowski commented 2 years ago

I agree that would be good to have public API in a separate directory, not together with private API and code. A separate directory just for pirave headers sounds good, I prefere it even more then mix it together with source code files.

StefanHri commented 2 years ago

Check the zephyr_contrib branch. Is this what you were imagining?

In the next days, I plan to refactor the oscore.h and edhoc.h so that they do not contain any types defined in other headers.

AleksanderDemianowski commented 2 years ago

@StefanHri Looks very good! One note. What about moving the 3 headers of public API into inc directory? For me it would be more intuitive when we will have all headers in inc, and API exposed directly in this folder.

StefanHri commented 2 years ago

resolved