Fix failure detection and failures in basic-build-test.sh with SSL3 in full #3180

gilles-peskine-arm · 2020-04-09T17:54:10Z

Fix basic-build-test.sh exits successfully even if some tests fail #3178: make basic-build-test.sh exit with a failure status if some test cases fail
Minor log readability improvements
Fix SSLv3 failing in basic-build-tests.sh on development #3179: bring back SSL3 in the full config

Needs partial backports. This PR is release-critical, but the backports aren't (we can check the logs manually to verify that there are no failures).

Alternative that doesn't change the full config: #3181

Signed-off-by: Gilles Peskine <[email protected]>

We used to enable MBEDTLS_ENTROPY_FORCE_SHA256 in the full config, but this is no longer the case. So the seedfile now needs to be 64 bytes, nor 32 bytes. Signed-off-by: Gilles Peskine <[email protected]>

Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm · 2020-04-09T17:55:58Z

CI run of basic-build-test.sh: https://jenkins-internal.mbed.com/job/mbedtls-release-new/717/ → PASS

This reverts commit 2b9ebce. config.py full includes everything that can be tested together, even deprecated algorithms and protocols. Signed-off-by: Gilles Peskine <[email protected]>

This reverts commit 03035eb. Since SSL3 is back in the full config, test it again. Signed-off-by: Gilles Peskine <[email protected]>

The full config includes everything that can be tested together, even deprecated features (except deprecated features that lack proper testing or require additional build dependencies). Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm · 2020-04-09T18:57:33Z

New CI run of basic-build-test.sh: https://jenkins-internal.mbed.com/job/mbedtls-release-new/718/ → PASS (with no failed tests)

Run with only the changes in basic-build-test.sh (should fail due to the test case failures, but still emit a report at the end): https://jenkins-internal.mbed.com/job/mbedtls-release-new/720/ → FAIL as expected

Signed-off-by: Gilles Peskine <[email protected]>

ronald-cron-arm

It looks good to me. I've run successfully basic-build-test.sh locally and seen the effect of some of the changes. I put comments about a possible typo and two commit messages that could maybe be improved.

ronald-cron-arm · 2020-04-10T07:59:28Z

tests/scripts/all.sh

@@ -898,9 +898,12 @@ component_test_full_cmake_clang () {
 }


The commit message is confusing to me: turning off deprecated features in something that builds deprecated stuff?

Sorry, which commit?

The commit message of this commit "Turn off deprecated features in component_build_deprecated."

Ah, I guess it's the name of the component that's confusing. What it actually does is to check that if you build the library without deprecated features, and you arrange disable deprecated features (in either of two ways: compiler warning + -Werror, or compiling them out altogether), the build still works. I guess it should be called component_build_without_deprecated.

Yes I agree that component_build_without_deprecated would be a better name in that case. Probably not for this PR to change that name though but if you include some of your above explanation in the commit message it would be all clear.

tests/scripts/basic-build-test.sh

mpg

So, as commented out of github, for now I really prefer #3181 (ie the other PR, not this one). In the longer term, I don't have any objections to switching to the approach in this PR, but I think it needs more comments in some places.

mpg · 2020-04-10T08:54:45Z

scripts/config.py

@@ -166,6 +166,7 @@ def include_in_full(name):
    if name in [
            'MBEDTLS_CTR_DRBG_USE_128_BIT_KEY',
            'MBEDTLS_DEPRECATED_REMOVED',
+            'MBEDTLS_DEPRECATED_WARNING',


I think something similar to the commit message should be added as a comment above this list, to avoid any ambiguity in the future.

We have a backlog of writing comments to explain this list. A long time ago I wrote some for config.pl but that never got merged.

Well I think it adjusting the definition of full is the best time to finally add those comments. Otherwise this PR will only fix one half of the problem (the definition was suboptimal) but not the other half (different team members at different times had different enough conceptions of what "full" meant that we broke things), which is IMO at least as important.

mpg · 2020-04-10T09:08:18Z

tests/scripts/all.sh

@@ -898,9 +898,12 @@ component_test_full_cmake_clang () {
 }

 component_build_deprecated () {
-    msg "build: make, full config + DEPRECATED_WARNING, gcc -O" # ~ 30s
+    msg "build: make, full config - deprecated + DEPRECATED_WARNING, gcc -O" # ~ 30s


Partially pre-existing: I don't understand the purpose of this component. Probably deserves a comment in the code.

In the meantime I saw your reply to Ronald. I think something similar should be added as comments in the code.

The comments could also perhaps mention one point that wasn't immediately clear to me: we need to disable deprecated config.h options when building with DEPRECATED_WARNINGS because they use #warning which isn't affected by -Wno-deprecated (unlike deprecated functions which use the deprecated attribute).

AFAIR that code has existed since before the componentization of all.sh. The pure-full jobs might not have!

gilles-peskine-arm · 2020-04-10T09:27:03Z

tests/scripts/basic-build-test.sh

+} | tee tests/cov-$TEST_OUTPUT
+
+if [ "$(tail -n1 tests/cov-$TEST_OUTPUT)" != "SUCCESS" ]; then
+    echo >&2 "Fatal: 'make lcov' failed"


As @mpg noted in the alternative PR there's an exit missing here.

gilles-peskine-arm · 2020-04-10T13:08:21Z

Closed in favor of #3180. Some of the changes to config.py and all.sh may be made in a separate PR after further discussion.

gilles-peskine-arm added 6 commits April 9, 2020 18:28

Exit with a failure status if some tests failed

6d6ee98

Signed-off-by: Gilles Peskine <[email protected]>

Note that we keep going even if some tests fail

8ee5060

Signed-off-by: Gilles Peskine <[email protected]>

If 'make lcov' failed, exit immediately

5dbb35f

Signed-off-by: Gilles Peskine <[email protected]>

Create a large enough seedfile

8ee093a

We used to enable MBEDTLS_ENTROPY_FORCE_SHA256 in the full config, but this is no longer the case. So the seedfile now needs to be 64 bytes, nor 32 bytes. Signed-off-by: Gilles Peskine <[email protected]>

Add a few echo statements to make the logs easier to follow

5dc5887

Signed-off-by: Gilles Peskine <[email protected]>

MBEDTLS_MEMORY_BACKTRACE is no longer included in the full config

117dd51

Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm added mbed TLS team needs-backports Backports are missing or are pending review and approval. component-platform Portability layer and build scripts needs-ci Needs to pass CI tests labels Apr 9, 2020

gilles-peskine-arm added 3 commits April 9, 2020 20:52

Revert "Remove deprecated modules from config.py full"

dcf1901

This reverts commit 2b9ebce. config.py full includes everything that can be tested together, even deprecated algorithms and protocols. Signed-off-by: Gilles Peskine <[email protected]>

Revert "Stop testing ssl3 when it isn't enabled"

3bd10bb

This reverts commit 03035eb. Since SSL3 is back in the full config, test it again. Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm force-pushed the basic-build-test-status branch from d5f8b5c to aab670d Compare April 9, 2020 18:52

gilles-peskine-arm requested review from mpg and ronald-cron-arm April 9, 2020 18:56

gilles-peskine-arm added the needs-review Every commit must be reviewed by at least two team members, label Apr 9, 2020

Turn off deprecated features in component_build_deprecated

201b194

Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm changed the title ~~Fix failure detection and failures in basic-build-test.sh~~ Fix failure detection and failures in basic-build-test.sh with SSLv3 in full Apr 9, 2020

gilles-peskine-arm changed the title ~~Fix failure detection and failures in basic-build-test.sh with SSLv3 in full~~ Fix failure detection and failures in basic-build-test.sh with SSL3 in full Apr 9, 2020

gilles-peskine-arm mentioned this pull request Apr 9, 2020

Fix failure detection and failures in basic-build-test.sh with SSL3 in basic-build-test #3181

Merged

ronald-cron-arm approved these changes Apr 10, 2020

View reviewed changes

mpg reviewed Apr 10, 2020

View reviewed changes

gilles-peskine-arm commented Apr 10, 2020

View reviewed changes

gilles-peskine-arm closed this Apr 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix failure detection and failures in basic-build-test.sh with SSL3 in full #3180

Fix failure detection and failures in basic-build-test.sh with SSL3 in full #3180

gilles-peskine-arm commented Apr 9, 2020 •

edited

Loading

gilles-peskine-arm commented Apr 9, 2020 •

edited

Loading

gilles-peskine-arm commented Apr 9, 2020 •

edited

Loading

ronald-cron-arm left a comment

ronald-cron-arm Apr 10, 2020

gilles-peskine-arm Apr 10, 2020

ronald-cron-arm Apr 10, 2020

gilles-peskine-arm Apr 10, 2020

ronald-cron-arm Apr 10, 2020

mpg left a comment •

edited

Loading

mpg Apr 10, 2020

gilles-peskine-arm Apr 10, 2020

mpg Apr 10, 2020

mpg Apr 10, 2020

mpg Apr 10, 2020

gilles-peskine-arm Apr 10, 2020

gilles-peskine-arm Apr 10, 2020

gilles-peskine-arm commented Apr 10, 2020

Fix failure detection and failures in basic-build-test.sh with SSL3 in full #3180

Fix failure detection and failures in basic-build-test.sh with SSL3 in full #3180

Conversation

gilles-peskine-arm commented Apr 9, 2020 • edited Loading

gilles-peskine-arm commented Apr 9, 2020 • edited Loading

gilles-peskine-arm commented Apr 9, 2020 • edited Loading

ronald-cron-arm 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

mpg left a comment • edited Loading

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

gilles-peskine-arm commented Apr 10, 2020

gilles-peskine-arm commented Apr 9, 2020 •

edited

Loading

gilles-peskine-arm commented Apr 9, 2020 •

edited

Loading

gilles-peskine-arm commented Apr 9, 2020 •

edited

Loading

mpg left a comment •

edited

Loading