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.

Sign up for GitHub

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

Jump to bottom

Framework: Split check_test_cases.py and outcome_analysis.py #55

Merged
merged 742 commits into from
Oct 10, 2024
Merged

Framework: Split check_test_cases.py and outcome_analysis.py #55

merged 742 commits into from
Oct 10, 2024

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Oct 3, 2024 •
edited
Loading

  • Move collect_test_cases.py (split from check_test_cases.py), check_test_cases.py and outcome_analysis.py (split from analyze_outcomes.py) to the framework repository. Prior to the move, all files were made identical in 3.6 and development.

  • By default, it is now an error if a test case is not executed, but the calling script can downgrade it back to a warning.

  • Quality of life improvement for when outcome analysis fails: the log of outcome analysis will be the outcome_analysis-analyze_outcomes.log.xz artifact on the CI. That way you won't have to dig into the classic pipeline view to see the failures.

  • Quality of life improvement for when outcome analysis fails: the existing artifact outcomes.csv.xz can now be used locally without decompressing it. (This requires a mypy upgrade, done in each consuming branch.)

  • development: Split check_test_cases.py and outcome_analysis.py mbedtls#9668

  • 3.6: Backport 3.6: Split check_test_cases.py and outcome_analysis.py mbedtls#9669

mpg and others added 30 commits September 2, 2024 12:46
@mpg
Allow no authentication of the server in 1.3
6901504 
See notes about optional two commits ago for why we're doing this.

Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@ronald-cron-arm @mpg
ssl-opt.sh: Fix test case titles
7a442c9 
Signed-off-by: Ronald Cron <[email protected]>
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@ronald-cron-arm @mpg
tls13: Add support for trusted certificate callback
bfbecf8 
Signed-off-by: Ronald Cron <[email protected]>
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@ronald-cron-arm @mpg
ssl-opt.sh: Test trusted certificate callback in TLS 1.3
95dd6f5 
Signed-off-by: Ronald Cron <[email protected]>
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Restrict the scope of a few variables
5bdadbb 
In particular, make sure pointer variables are initialized right after
being declared.

Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Improve a variable's name
fd800c2 
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Add support for context f_vrfy callback in 1.3
843a00d 
This was only supported in 1.2 for no good reason.

Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Minor refactoring of generic SSL certificate verif
908f57d 
Rename as there was a name collision with a static function in another
file: ssl_parse_certificate_verify in ssl_tls12_server.c is the function
that parses the CertificateVerify message, which seems appropriate. Here
it meant "the 'verify' step after parsing the Certificate message".
Use a name that focuses on what it does: verify, not parse.

Also, take ciphersuite_info as an argument: when TLS 1.3 calls this
function, it can pass NULL as the ciphersuite has no influence there.

Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Merge 1.2 and 1.3 certificate verification
19dd9f5 
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Improve some comments
a040548 
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Fix two dependency declarations in ssl-opt
67072bf 
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg @davidhorstmann-arm
Fix typos in comments
9e3e991 
Co-authored-by: David Horstmann <[email protected]>
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Fix guards around function now used by 1.3 as well
5398e58 
Actually moved the function rather than trying to edit guards around it,
because the relevant guards are not nearby, the function was part of
larger blocks, so it seemed risky.

Also, that seems logically correct: the function is no longer part of
the "TLS 1.2 handshake functions common to server and client" section,
it's part of the "helper functions common to 1.2 and 1.3 server and
client" block. Ideally in the future perhaps the file structure should
reflect that (`ssl_generic.c` vs `ssl_tls12_generic.c`?) but that's out
of scope here.

Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Add a ChangeLog entry
b721ccc 
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Merge pull request #9508 from eleuzi01/docs-fix
c645f11 
Fix typo in psa-transition.md
@eleuzi01
Make error line consistent with the header
c21675e 
Signed-off-by: Elena Uziunaite <[email protected]>
@mpg
Merge pull request #9515 from mpg/windows-makefile-fixes
c37877f 
Misc small makefile fixes
@mpg
Merge pull request #9327 from eleuzi01/issue-9319
f3cd97d 
Remove hacks about asm vs constant-flow testing
@davidhorstmann-arm
Merge pull request #9252 from gabor-mezei-arm/9114_replace_MBEDTLS_MD…
36fe918 
…_CAN_SHA512_with_PSA_WANT

Replace MBEDTLS_MD_CAN_SHA512 with its PSA_WANT counterpart
@davidhorstmann-arm
Merge pull request #9526 from mpg/refactor-tls123-verif-dev
1d98d9d 
Refactor tls123 verif dev
@eleuzi01
Replace MBEDTLS_MD_CAN_SHA1 with PSA_WANT_ALG_SHA_1
9fc5be0 
Signed-off-by: Elena Uziunaite <[email protected]>
@mpg
Add optionally unsafe variant of exp_mod for perf
7305002 
Attempt to partially solve the performance regression in 3.6.0 without
adding too much code size.

Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@yanesca @mpg
Improve documentation of MBEDTLS_MPI_IS_PUBLIC
91537eb 
Signed-off-by: Janos Follath <[email protected]>
@yanesca @mpg
Make _optionally_safe functions internal
9a8b1f4 
The complexity of having functions whose security properties depend on a
runtime argument can be dangerous. Limit misuse by making any such
functions local.

Signed-off-by: Janos Follath <[email protected]>
@yanesca @mpg
Move mixed security code to small local functions
4726cb8 
The complexity of having functions whose security properties depend on a
runtime argument can be dangerous. Limit risk by isolating such code in
small functions with limited scope.

Signed-off-by: Janos Follath <[email protected]>
@yanesca @mpg
Move MBEDTLS_MPI_IS_* macros to bignum_core.h
5b69fad 
These macros are not part of any public or internal API, ideally they
would be defined in the source files. The reason to put them in
bignum_core.h to avoid duplication as macros for this purpose are
needed in both bignum.c and bignum_core.c.

Signed-off-by: Janos Follath <[email protected]>
@yanesca @mpg
Make MBEDTLS_MPI_IS_PUBLIC thumb friendly
24fb8c9 
In Thumb instructions, constant can be:

- any constant that can be produced by shifting an 8-bit value left by any
  number of bits within a 32-bit word
- any constant of the form 0x00XY00XY
- any constant of the form 0xXY00XY00
- any constant of the form 0xXYXYXYXY.

Signed-off-by: Janos Follath <[email protected]>
@yanesca @mpg
Move _public parameters next to their target
8fc736d 
It is easier to read if the parameter controlling constant timeness with
respect to a parameter is next to that parameter.

Signed-off-by: Janos Follath <[email protected]>
@yanesca @mpg
Use actual exponent size for window calculation
a099ac9 
The allocated size can be significantly larger than the actual size. In
the unsafe case we can use the actual size and gain some performance.

Signed-off-by: Janos Follath <[email protected]>
@yanesca @mpg
Add tests for optionally safe codepaths
df5e55b 
The new test hooks allow to check whether there was an unsafe call of an
optionally safe function in the codepath. For the sake of simplicity the
MBEDTLS_MPI_IS_* macros are reused for signalling safe/unsafe codepaths
here too.

Signed-off-by: Janos Follath <[email protected]>
@mpg
Copy link
Contributor

mpg commented Oct 8, 2024

Having moved files to the framework myself, I now realize that this move does not preserve file history.

For collect_test_cases.py and outcome_analysis.py, since they were created just before being moved, I'm not sure how useful it is to preserve their history. But for check_test_cases.py indeed that could be a nice thing.

@eleuzi01
Copy link
Contributor

eleuzi01 commented Oct 8, 2024

Having moved files to the framework myself, I now realize that this move does not preserve file history.

For collect_test_cases.py and outcome_analysis.py, since they were created just before being moved, I'm not sure how useful it is to preserve their history. But for check_test_cases.py indeed that could be a nice thing.

Yes, I meant check_test_cases.py , doesn't seem that there is much history to be preserved though but maybe it's something to consider.

@gilles-peskine-arm
Copy link
Contributor Author

I agree that we should preserve the history. Even with the newly created files, we should keep the history around since their early history has content moved from other files. I'll try out David's script.

@gilles-peskine-arm
Adjust paths for impending moves to the framework
3146772 
Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm
Upgrade mypy to the last version supporting Python 3.6
199c919 
Upgrade mypy to 0.971, which is the last version that supports Python 3.6
(the oldest Python version that we currently run on the CI).

This fixes the error
```
framework/scripts/mbedtls_framework/outcome_analysis.py:119: error: Incompatible return value type (got "IO[Any]", expected "TextIO")
framework/scripts/mbedtls_framework/outcome_analysis.py:121: error: Incompatible return value type (got "IO[Any]", expected "TextIO")
```
As far as I can tell the fix is python/mypy#9275
which was released in mypy 0.940.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm
Downgrade mypy to a version that works with our code base
fecb9b9 
mypy >=0.960 rejects macro_collector.py.
#50

We currently need mypy >=0.940, <0.960. Pick 0.942, which works, and is the
system version on Ubuntu 22.04.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm
Default to allowing partial test coverage
2c17ec7 
Currently, many test cases are not executed. A follow-up pull request will
take care of that. In the meantime, continue allowing partial test coverage.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm
Copy link
Contributor Author

I've addressed Manuel's comments. I intend to push a revised history that includes the history of the moved files and ends with identical content once I get David's script working.

mpg
mpg reviewed Oct 9, 2024
View reviewed changes
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.

Content LGTM at 53b6a46 - waiting for the version with identical content and improved history before I fully approve.

@gilles-peskine-arm
Move some files to framework repository
ce00cfe 
Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm
Merge branch 'tmp-branch-move-files-to-framework' into move-files-int…
4103f92 
…o-framework
@gilles-peskine-arm
Adjust import path after script moves
48c005f 
Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm
Also write to a log file by default
67e415f 
Write the log of outcome analysis to a log file by default. This is a cheap
way of getting the outcome analysis log in an easy-to-access form on the CI:
with our current CI scripts, they are now automatically available as an
artifact called `analyze_outcomes.log`.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm
Support compressed outcome files transparently
b111d9f 
Transparently read outcome files compressed with xz (which we currently use
on the CI) or with gzip.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm
Default to requiring full test coverage
9c9d9fa 
Command-line options allow choosing whether a never-executed test results in
a warning or an error. Also, a consuming script can make the default
permissive by setting FULL_COVERAGE_BY_DEFAULT to False.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm
collect_test_cases.py is now a separate module
d63709d 
Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm
Make open calls more uniform
6759e80 
Text mode ('t') is the default for the open builtin, but not for gzip.open
and its imitators. Always specify it explicitly to avoid making maintiners
wonder if there's an intended difference in behavior.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-arm force-pushed the dev/gilles-peskine-arm/analyze_outcome-split-framework branch from 53b6a46 to 6759e80 Compare October 9, 2024 12:13
@gilles-peskine-arm
Copy link
Contributor Author

I have rebased my changes on top of the result of David's script. The final content is identical, but moved files now have their history from before the repository move.

@gilles-peskine-arm gilles-peskine-arm added priority-high High priority - will be reviewed soon and removed needs-reviewer This PR needs someone to pick it up for review labels Oct 9, 2024
eleuzi01
eleuzi01 approved these changes Oct 9, 2024
View reviewed changes
Copy link
Contributor

@eleuzi01 eleuzi01 left a comment

Choose a reason for hiding this comment

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

LGTM

mpg
mpg approved these changes Oct 10, 2024
View reviewed changes
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.

LGTM, thanks!

@mpg mpg 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, labels Oct 10, 2024
@mpg
Copy link
Contributor

mpg commented Oct 10, 2024 •
edited
Loading

I'm happy with the content, but the history seems to include some commits that are not relevant.

I mean, looking at the first three they only touch tests/ssl-opt.sh and library/ssl_xxx.c so how are they relevant here? Edit: I misunderstood, see Mbed-TLS/mbedtls-docs#145 (comment)

Still approving as I don't think the extra commits are causing much harm, and I don't want this PR to get blocked while we sort out the script.

@mpg mpg mentioned this pull request Oct 10, 2024
Open
@gilles-peskine-arm gilles-peskine-arm merged commit 1de0641 into main Oct 10, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eleuzi01 eleuzi01 eleuzi01 approved these changes

@mpg mpg mpg approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
Roadmap Board for Mbed TLS
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.