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

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

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Apr 9, 2020

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 does change the full config: #3180

Backports: #3375 (2.16), #3376 (2.7).

@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, 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 gilles-peskine-arm force-pushed the basic-build-test-status-with-deprecated branch from 49162f3 to 1de46d8 Compare April 10, 2020 07:35
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

I'm generally very happy with this PR, except for a possible discrepancy between commit message and code, that I'd like clarified before I can approve.

} | tee tests/cov-$TEST_OUTPUT

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

Choose a reason for hiding this comment

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

The commit message says "exit immediately" but I'm not seeing what part of the code actually does that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. I amended the commit.

The seedfile needs to have the size of the entropy accumulator, which
is 64 bytes (512 bits) since the entropy accumulator uses SHA-512 and
the seed size needs to be the same as the hash output (or larger).

We used to enable MBEDTLS_ENTROPY_FORCE_SHA256 in the full config, so
the entropy accumulator was 256 bits (32 bytes), and therefore a
32-byte seedfile worked. But we no longer turn on this option in the
full config, so the 32-byte seedfile no longer works.

Signed-off-by: Gilles Peskine <[email protected]>
The "full" configuration excludes some deprecated or experimental
features. Enable the ones that have tests, don't have extra
requirements and don't turn off some other feature.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-arm force-pushed the basic-build-test-status-with-deprecated branch from 1de46d8 to c877c24 Compare April 10, 2020 09:34
@gilles-peskine-arm gilles-peskine-arm requested a review from mpg April 10, 2020 09:35
@gilles-peskine-arm
Copy link
Contributor Author

gilles-peskine-arm commented Apr 10, 2020

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

Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Thanks for adding the missing exit and fixing the typo. Looks good to me now. (Let's hope the CI agrees!)

Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

LGTM as well, thanks for the improvements.

@gilles-peskine-arm gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests labels Apr 10, 2020
@gilles-peskine-arm
Copy link
Contributor Author

The PR CI has passed and basic-build-test.sh has passed. Backports can wait until after the upcoming release, since LTS branches are not affected meaningfully by the issues that this PR fixes. Merging.

@gilles-peskine-arm gilles-peskine-arm merged commit e62bdef into Mbed-TLS:development Apr 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-platform Portability layer and build scripts needs-backports Backports are missing or are pending review and approval.
Projects
None yet
3 participants