Skip to content

Conversation

@gilles-peskine-arm
Copy link
Collaborator

@gilles-peskine-arm gilles-peskine-arm commented Sep 10, 2019

Documentation only. Explore potential strategies for invasive testing. Formulate some rules and guideline. Succinctly describe some solutions.

This is a proposal submitted for discussion.

I deliberately included solutions with “TODO” in them for things that we should do, but aren't doing yet.

I've changed headers to define macros mentioned explicitly in the strategy document so that one can start implementing parts of the strategy based on this PR. But implementing the proposed strategy is out of scope of this PR.

@gilles-peskine-arm gilles-peskine-arm added enhancement New feature or request needs: review The pull request is ready for review. This generally means that it has no known issues. labels Sep 10, 2019

Solution: TODO.

See the [secure element driver interface test strategy](driver-interface-test-strategy.html) for more information.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: currently in #246. Rendered HTML.

@mpg
Copy link
Contributor

mpg commented Sep 11, 2019

I read the document and it looks very good.

I'd just like to check if I understood something right on a concrete example. Sometimes we have internal functions that we'd like to unit-test because that's the easiest and most efficient way. For example, mbedtls_ssl_dtls_replay_update() and mbedtls_ssl_dlts_replay_check(). Currently they're unconditonaly non-static (visible by the linker) and declared in ssl_internal.h, which is included from the file defining the test functions, but is not supposed to be included by applications.

That's mostly satisfying except that making the functions static would allow more compiler optimizations and possibly reduce the code size. This is a serious concern, as in another instance in the baremetal branch we observed significant reductions in code size (several hundred bytes) just by making a bunch of functions static (see https://github.com/ARMmbed/mbedtls-restricted/pull/652 for details).

IIUC, the proposed rules would encourage us to:

  • in non- MBEDTLS_TEST_HOOKS builds: make the functions static (allowing the compiler to optimize as fully as it can), but then skip the associated unit tests in the test suite.
  • in MBEDTLS_TEST_HOOKS-enabled builds: make the functions non-static (potentially increasing the code size) and build and run the associated unit tests as we do now.

Is that correct?

@gilles-peskine-arm
Copy link
Collaborator Author

@mpg Thanks for the feedback. This is a use case that I hadn't thought of. I agree with your solution. I'll add a paragraph about this to the document.

If a function isn't part of the public API, I encourage declaring it in a header in library, not in a public header. We do that for PSA (see library/psa_*.h); most of these are exposed for cross-module references, but psa_crypto_invasive.h is intended for test only and would be wrapped in a MBEDTLS_TEST_HOOKS guard with the new rules.

@mpg
Copy link
Contributor

mpg commented Sep 12, 2019

If a function isn't part of the public API, I encourage declaring it in a header in library, not in a public header.

I think that's good practice, and I wish it was implemented in Mbed TLS as well.

Copy link
Contributor

@dgreen-arm dgreen-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few typos, but otherwise looks good.

* Portability: tests should work on every platform. Skipping tests on certain platforms may hide errors that are only apparent on such platforms.
* Maintenability: tests should only enforce the documented behavior of the product, to avoid extra work when the product's internal or implementation-specific behavior changes. We should also not give the impression that whatever the tests check is guaranteed behavior of the product which cannot change in future versions.

Where those goals conflit, we should at least mitigate the goals that cannot be fulfilled, and document the architectural choices and their rationale.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: conflit -> conflict

* Correctness: we want to test the actual product, not a modified version, since conclusions drawn from a test of a modified product may not apply to the real product.
* Effacement: the product should not include features that are solely present for test purposes, since these increase the attack surface and the code size.
* Portability: tests should work on every platform. Skipping tests on certain platforms may hide errors that are only apparent on such platforms.
* Maintenability: tests should only enforce the documented behavior of the product, to avoid extra work when the product's internal or implementation-specific behavior changes. We should also not give the impression that whatever the tests check is guaranteed behavior of the product which cannot change in future versions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: Maintenability -> Maintainability

Also appears in a few other places.

Evaluate possible approaches for invasive testing.

State some rules.
Clarify that using a header in library/ rather than include/ for
internal functions is a rule, not just a possibility.

As suggested by Manuel, state a rule for functions that need to be
static for best optimization but that we want to unit-test.
When this option is enabled, the product includes additional
interfaces that enable additional tests. This option should not be
enabled in production, but is included in the "full" build to enable
the extra tests.
Define MBEDTLS_STATIC_TESTABLE as described in
mbed-crypto-invasive-testing.md. Since this is for internal library
use only, define it in a header in library/. Since there is no
suitable header, create one.
@gilles-peskine-arm
Copy link
Collaborator Author

I rebased on top of development to resolve the conflict in docs/architecture/Makefile and also fixed the typos that Darryl pointed out. No semantic change.

@gilles-peskine-arm
Copy link
Collaborator Author

Moved to Mbed-TLS/mbedtls#3121

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request needs: review The pull request is ready for review. This generally means that it has no known issues.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants