Skip to content

Commit

Permalink
Documentation update to CONTRIBUTING.md
Browse files Browse the repository at this point in the history
* Guide to features and #defines

* test clarifications and wording improvements

---------

Co-authored-by: Laurence Lundblade <lgl@securitytheory.com>
  • Loading branch information
laurencelundblade and Laurence Lundblade authored Jun 11, 2023
1 parent 7d1b668 commit 2c57235
Showing 1 changed file with 101 additions and 6 deletions.
107 changes: 101 additions & 6 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,33 @@ will be merged.

### Basic tests

Running the standard make will produce `t_cose_test` for a particular
crypto library. It runs a thorough set of tests.
Make will produce the executable t_cose_test. Running it will perform
all the regression tests. When new features are added regression tests
for them must be added here.

However, it only runs with one crypto library, one compiler and one
set of #defines at a time. See tdv/b.sh for that.

### CI tests

GitHub CI is used to build and test against several crypto libraries.
GitHub CI is used to provide the general benefit of CI. It gives test
fanout for the standard major crypto libraries and versions, plus
build environments. It does NOT give full fanout for #define
configurations or for other special configurations. See tdv/b.sh for
that.

The time GitHub CI takes to run must be kept to a few minutes so as to
not cause excess delay in the commit/merge cycle and disrupt the
human workflow. This is the main reason testing done by it is
limited. The full fanout from tdv/b.sh takes tens of minutes so
it can't be run during CI.

### tdv/b.sh tests

In the "tdv" repository there are some makes files and build scripts
that do some more thorough testing. They are too large and take too
long to run to make them part of normal CI. Here's a few things they
do:
run the full configuration fan out testing. They are too large and
take too long to run to make them part of normal CI. Here's a few
things they do:

* Build with a fairly maximal set of compiler warning flags set (e.g.,
-Wall)
Expand Down Expand Up @@ -83,3 +97,84 @@ guidelines](https://git.trustedfirmware.org/TF-M/trusted-firmware-m.git/tree/doc
resulting in code with mixed styles. For better or worse, an Arm-style
version of UsefulBuf is created and used and so there is a duplicate
of UsefulBuf. The two are identical. They just have different names.


## Features and conditionality

The goal is for t_cose's use of #defines to be much simpler than most other libraries.

A couple of principles govern feature variation and conditional compiliation:

* A #define is NOT required to enable features. All features are "on" by
default.

* The internal function link dependency of t_cose is designed to work
well with the compiler -dead_strip option so linked object code is
minimized.

* Attempts to call t_cose APIs or use t_cose features which are not
supported by the crypto library often result in an error code being
returned and sometimes in a link error.

* The primary use of #defines is to disable features to reduce object
code size for use cases that need small code size.

### Users of t_cose

Most users of t_cose do not need to be concerned with #defines as they
are not needed to enable features in the library. All the features are
already enabled.

Users may find that sometimes they get error codes indicating a
feature isn't supported. This will usually be because the underlying
crypto library they are linking against doesn't support the feature,
not because the feature needs to be enabled in t_cose.

Users may also find that sometimes they get a link error reporting an
undefined symbol. This will again usually be because the underlying
crypto library being linked against doesn't support the feature.

For example, t_cose works with several versions of MbedTLS older
versions of which don't support AES key wrap. You can use t_cose with
these older versions just fine as long as you don't call any t_cose
APIs or use any t_cose features that need key wrap. If you do, you'll
probably get a T_COSE_ERR_UNSUPPORTED_XX error.

If your use case needs small object code, then it may be time to make
use of T_COSE_DISABLE_XXXX #defines and recompile the t_cose library.
But it also might be that the minimized link dependency and
-dead_strip does everything you need too.


### Contributing to t_cose

If you are adding a feature to t_cose don't assume that a #define will
be needed and try hard to avoid one.

Try to implement with minimal symbol/link dependency. One way to do
this is to create a new signer or recipient object just for the
feature. This might involve a new function in the crypto adaptor layer
for a new feature from the crypto library put to use. The new function
is only linked when the new feature is used.

The crypto layer has a facility for listing and discovering crypto
algorithms that are supported by a particular crypto library. This
facility is mostly used to know what to test, but not exclusively.

If a feature can work in multiple modes or have multiple behaviors,
don't control that with a #define. Instead control that with an option
flag or with some API methods. That makes install and configuration of
the library simpler. It also works much better for a shared library
because different users of it can use different modes.

Then, in the end, the only reason to use a #ifdef should be to reduce
object code if there's no other way.

When an #fdef has to be used, it should be in the least intrusive way
and it shouldn't make the code hard to read. Maybe even restructure
the code a little so the #ifdef is cleaner.





0 comments on commit 2c57235

Please sign in to comment.