Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check that all test cases are executed at least once #2691

Closed
gilles-peskine-arm opened this issue Jun 12, 2019 · 5 comments
Closed

Check that all test cases are executed at least once #2691

gilles-peskine-arm opened this issue Jun 12, 2019 · 5 comments
Assignees
Labels
component-platform Portability layer and build scripts enhancement size-s Estimated task size: small (~2d)

Comments

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Jun 12, 2019

We have test cases that are never executed on our CI because they depend on a particular configuration that we don't test. This is a gap in test coverage: if we wrote those tests, it's presumably because they're useful for something.

We should check that when we run all.sh, every test case in every .data file is executed at least once, and every test case in `ssl-opt.sh is executed at least once.

For example, I think (by visual inspection) that entropy_nv_seed in test_suite_entropy has not been executed on CI for a long time, since it depends on MBEDTLS_PLATFORM_NV_SEED_ALT but we never set that on CI until #2684 (it wasn't set by full). And it also depends on MBEDTLS_ENTROPY_SHA512_ACCUMULATOR so before #2684 you'd have to set MBEDTLS_PLATFORM_NV_SEED_ALT and not set MBEDTLS_ENTROPY_FORCE_SHA256.

Goal of this issue: if a CI job succeeds, it guarantees that all test cases have been executed, except for a curated whitelist of test cases that may be skipped.

Prerequisites:

Remaining tasks for this issue:

  • Where the analysis in Fully analyze test cases that are not executed #5390 has determined that the test case should be executed, either fix the problem or add the test case to the whitelist with a note that this is temporary and a link to the GitHub issue.
  • Switch to running analyze_outcomes.py in a mode where it reports errors if a test case is not executed.
@gilles-peskine-arm
Copy link
Contributor Author

gilles-peskine-arm commented Jun 26, 2020

#3458 adds a script that reports the test cases that are not executed at least once. They are (on development):

Warning: Test case not executed: ssl-opt;DTLS client reconnect from same port: reconnect, nbio, valgrind
Warning: Test case not executed: ssl-opt;DTLS fragmenting: 3d, openssl client, DTLS 1.0
Warning: Test case not executed: ssl-opt;DTLS fragmenting: 3d, openssl client, DTLS 1.2
Warning: Test case not executed: ssl-opt;DTLS fragmenting: 3d, openssl server, DTLS 1.0
Warning: Test case not executed: ssl-opt;DTLS fragmenting: 3d, openssl server, DTLS 1.2
Warning: Test case not executed: ssl-opt;DTLS fragmenting: proxy MTU: auto-reduction (with valgrind)
Warning: Test case not executed: ssl-opt;DTLS proxy: 3d (drop, delay, duplicate), \"short\" PSK handshake
Warning: Test case not executed: ssl-opt;DTLS proxy: 3d, \"short\" (no ticket, no cli_auth) FS handshake
Warning: Test case not executed: ssl-opt;DTLS proxy: 3d, \"short\" RSA handshake
Warning: Test case not executed: ssl-opt;DTLS proxy: 3d, openssl server
Warning: Test case not executed: ssl-opt;DTLS proxy: 3d, openssl server, fragmentation
Warning: Test case not executed: ssl-opt;DTLS proxy: 3d, openssl server, fragmentation, nbio
Warning: Test case not executed: ssl-opt;Handshake memory usage (MFL $1)
Warning: Test case not executed: ssl-opt;PSA - ECDH with $1
Warning: Test case not executed: ssl-opt;PSA-supported ciphersuite: $1
Warning: Test case not executed: ssl-opt;RC4: server disabled, client enabled
Warning: Test case not executed: ssl-opt;RC4: server half, client enabled
Warning: Test case not executed: test_suite_cipher.gcm;AES GCM Decrypt empty buffer
Warning: Test case not executed: test_suite_psa_crypto;PSA key policy algorithm2: CTR, CBC
Warning: Test case not executed: test_suite_psa_crypto_entropy;PSA validate entropy injection: bad, too big
Warning: Test case not executed: test_suite_psa_crypto_entropy;PSA validate entropy injection: bad, too small using MBEDTLS_ENTROPY_BLOCK_SIZE
Warning: Test case not executed: test_suite_psa_crypto_entropy;PSA validate entropy injection: bad, too small using MBEDTLS_ENTROPY_MIN_PLATFORM
Warning: Test case not executed: test_suite_psa_crypto_entropy;PSA validate entropy injection: before and after crypto_init
Warning: Test case not executed: test_suite_psa_crypto_entropy;PSA validate entropy injection: good, max size
Warning: Test case not executed: test_suite_psa_crypto_entropy;PSA validate entropy injection: good, minimum size
Warning: Test case not executed: test_suite_psa_crypto_metadata;Asymmetric signature: SHA-256 + deterministic DSA using SHA-256 [#1]
Warning: Test case not executed: test_suite_psa_crypto_metadata;Asymmetric signature: SHA-256 + randomized DSA SHA-256 using SHA-256
Warning: Test case not executed: test_suite_psa_crypto_metadata;Asymmetric signature: deterministic DSA with wildcard hash [#1]
Warning: Test case not executed: test_suite_psa_crypto_metadata;Asymmetric signature: randomized DSA with wildcard hash
Warning: Test case not executed: test_suite_psa_crypto_metadata;Cipher: ChaCha20
Warning: Test case not executed: test_suite_psa_crypto_metadata;Hash: SHA-2 SHA-512/224
Warning: Test case not executed: test_suite_psa_crypto_metadata;Hash: SHA-2 SHA-512/256
Warning: Test case not executed: test_suite_psa_crypto_metadata;Hash: SHA-3 SHA3-224
Warning: Test case not executed: test_suite_psa_crypto_metadata;Hash: SHA-3 SHA3-256
Warning: Test case not executed: test_suite_psa_crypto_metadata;Hash: SHA-3 SHA3-384
Warning: Test case not executed: test_suite_psa_crypto_metadata;Hash: SHA-3 SHA3-512
Warning: Test case not executed: test_suite_psa_crypto_metadata;Key type: DSA key pair
Warning: Test case not executed: test_suite_psa_crypto_metadata;Key type: DSA public key
Warning: Test case not executed: test_suite_psa_crypto_metadata;MAC: HMAC-SHA-512/224
Warning: Test case not executed: test_suite_psa_crypto_metadata;MAC: HMAC-SHA-512/256
Warning: Test case not executed: test_suite_psa_crypto_metadata;MAC: HMAC-SHA3-224
Warning: Test case not executed: test_suite_psa_crypto_metadata;MAC: HMAC-SHA3-256
Warning: Test case not executed: test_suite_psa_crypto_metadata;MAC: HMAC-SHA3-384
Warning: Test case not executed: test_suite_psa_crypto_metadata;MAC: HMAC-SHA3-512
Warning: Test case not executed: test_suite_ssl;SSL TLS_PRF MBEDTLS_SSL_TLS_PRF_SHA256 SHA-256 not enabled

See a subsequent comment for analysis.

The complete outcomes.csv from running all.sh on Linux as of #3458 (i.e. shortly before Mbed TLS 2.23):
outcomes.csv.gz

@gilles-peskine-arm
Copy link
Contributor Author

gilles-peskine-arm commented Jun 26, 2020

Analysis of the cases reported as not executed

Deliberately skipped SSL tests

Some test cases are prefixed by skip_next_test. They are skipped deliberately. They should not be considered to be available.

All of these test cases trigger a bug in OpenSSL that is fixed in 1.0.2q, 1.1.1a and 3.0.0 (upcoming). On our CI we currently only run ssl-opt.sh with 1.0.2g (and 1.0.1j for legacy), and we only have 1.1.1 (next) besides that.

ssl-opt;DTLS fragmenting: 3d, openssl client, DTLS 1.0
ssl-opt;DTLS fragmenting: 3d, openssl client, DTLS 1.2
ssl-opt;DTLS fragmenting: 3d, openssl server, DTLS 1.0
ssl-opt;DTLS fragmenting: 3d, openssl server, DTLS 1.2
ssl-opt;DTLS proxy: 3d, openssl server
ssl-opt;DTLS proxy: 3d, openssl server, fragmentation
ssl-opt;DTLS proxy: 3d, openssl server, fragmentation, nbio

SSL tests specifically for valgrind

We don't run ssl-opt.sh with valgrind. all.sh can do it but only with an additional option --memory.

ssl-opt;DTLS client reconnect from same port: reconnect, nbio, valgrind
ssl-opt;DTLS fragmenting: proxy MTU: auto-reduction (with valgrind)

SSL tests with a double quote in the description

In ssl-opt.sh, the test cases are in double-quoted string literals. Some test cases have a description that contains a double quote, which is written \". There is a defect in check_test_cases.py in that it treats \" as being part of the description. This didn't matter for description validity and unicity checking, but it matters in the new analysis where the string is compared with the description. A simple fix would be to strip the backslash in check_test_cases.py, but we might as well not bother with that since a fix that works for parametrized SSL tests would also fix this as a side effect.

ssl-opt;DTLS proxy: 3d (drop, delay, duplicate), \"short\" PSK handshake
ssl-opt;DTLS proxy: 3d, \"short\" (no ticket, no cli_auth) FS handshake
ssl-opt;DTLS proxy: 3d, \"short\" RSA handshake

Parametrized SSL tests

Some test cases in ssl-opt.sh are parametrized. The analysis script (via check_test_cases.py) treats the uninterpolated shell literal as the test case description, with $1 instead of the parameter value. This isn't good enough to know the test case descriptions at runtime.

ssl-opt;Handshake memory usage (MFL $1)
ssl-opt;PSA - ECDH with $1
ssl-opt;PSA-supported ciphersuite: $1

RC4

These test cases check that a ciphersuite is disabled. But there is a generic mechanism that skips tests that use a disabled ciphersuite (requires_ciphersuite_enabled), so these test cases never run. This is a bug in ssl-opt.sh. Fixed in #3463.

ssl-opt;RC4: server disabled, client enabled
ssl-opt;RC4: server half, client enabled

Mistakes in dependencies

  • MBEDTLS_CIPHER_AES_128_GCM instead of MBEDTLS_AES_C
  • MBEDTLS_CIPHER_MODE_CBC_NOPAD instead of MBEDTLS_CIPHER_MODE_CBC
  • MBEDTLS_CHACHA_C instead of MBEDTLS_CHACHA20_C

Fixed in #3463.

test_suite_cipher.gcm;AES GCM Decrypt empty buffer
test_suite_psa_crypto;PSA key policy algorithm2: CTR, CBC
test_suite_psa_crypto_metadata;Cipher: ChaCha20

PSA entropy injection

This feature was developed for the needs of Pelion. In production, it requires additional functions, and we don't have a mock of these functions for testing. The feature needs a redesign, but the functions aren't very difficult to mock so we should do this.

test_suite_psa_crypto_entropy;PSA validate entropy injection: bad, too big
test_suite_psa_crypto_entropy;PSA validate entropy injection: bad, too small using MBEDTLS_ENTROPY_BLOCK_SIZE
test_suite_psa_crypto_entropy;PSA validate entropy injection: bad, too small using MBEDTLS_ENTROPY_MIN_PLATFORM
test_suite_psa_crypto_entropy;PSA validate entropy injection: before and after crypto_init
test_suite_psa_crypto_entropy;PSA validate entropy injection: good, max size
test_suite_psa_crypto_entropy;PSA validate entropy injection: good, minimum size

PSA crypto metadata

The PSA crypto metadata test cases only run if the corresponding algorithm might be enabled. The reason is that the macros that calculate the metadata might not give correct results for algorithms that are enabled. There are test cases for algorithms that are not implemented. Arguably either these test cases should be removed (if the metadata isn't correct) or they should be enabled in all configurations (if the metadata is correct).

Fixed in #3463 by removing the test cases.

test_suite_psa_crypto_metadata;Asymmetric signature: SHA-256 + deterministic DSA using SHA-256 [#1]
test_suite_psa_crypto_metadata;Asymmetric signature: SHA-256 + randomized DSA SHA-256 using SHA-256
test_suite_psa_crypto_metadata;Asymmetric signature: deterministic DSA with wildcard hash [#1]
test_suite_psa_crypto_metadata;Asymmetric signature: randomized DSA with wildcard hash
test_suite_psa_crypto_metadata;Hash: SHA-2 SHA-512/224
test_suite_psa_crypto_metadata;Hash: SHA-2 SHA-512/256
test_suite_psa_crypto_metadata;Hash: SHA-3 SHA3-224
test_suite_psa_crypto_metadata;Hash: SHA-3 SHA3-256
test_suite_psa_crypto_metadata;Hash: SHA-3 SHA3-384
test_suite_psa_crypto_metadata;Hash: SHA-3 SHA3-512
test_suite_psa_crypto_metadata;Key type: DSA key pair
test_suite_psa_crypto_metadata;Key type: DSA public key
test_suite_psa_crypto_metadata;MAC: HMAC-SHA-512/224
test_suite_psa_crypto_metadata;MAC: HMAC-SHA-512/256
test_suite_psa_crypto_metadata;MAC: HMAC-SHA3-224
test_suite_psa_crypto_metadata;MAC: HMAC-SHA3-256
test_suite_psa_crypto_metadata;MAC: HMAC-SHA3-384
test_suite_psa_crypto_metadata;MAC: HMAC-SHA3-512

TLS without SHA-256

depends-hashes.pl excludes SSL. This is deliberate but perhaps not desirable. One test case ends up never running as a consequence.

test_suite_ssl;SSL TLS_PRF MBEDTLS_SSL_TLS_PRF_SHA256 SHA-256 not enabled

@gilles-peskine-arm
Copy link
Contributor Author

I haven't done any analysis for LTS branches (2.16, 2.7) because they don't have the outcome file machinery, so we don't have a way to collect the data. I think that's acceptable: if we backport any applicable fix from development that affects test coverage, LTS branches shouldn't lack coverage.

@gilles-peskine-arm
Copy link
Contributor Author

Note that in 3.0, many test cases in ssl-opt.sh are not executed because the corresponding feature has been removed, but the test cases are still present. This is expected and tracked in #4564.

@bensze01 bensze01 modified the milestone: Reduce tech debt (Q4+) Jul 28, 2021
@bensze01 bensze01 removed this from the Reduce tech debt (Q4+) milestone Aug 11, 2021
@gilles-peskine-arm gilles-peskine-arm added size-s Estimated task size: small (~2d) and removed size-m Estimated task size: medium (~1w) labels Jan 7, 2022
@yanesca
Copy link
Contributor

yanesca commented Nov 19, 2024

All sub-issues have been closed and the enforcement mode was switched on in #9593 .

@yanesca yanesca closed this as completed Nov 19, 2024
@github-project-automation github-project-automation bot moved this to Done in Risks Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-platform Portability layer and build scripts enhancement size-s Estimated task size: small (~2d)
Projects
Status: Test cases not executed
Status: Done
Development

No branches or pull requests

5 participants