Framework: Split check_test_cases.py and outcome_analysis.py #55
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
See notes about optional two commits ago for why we're doing this. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Signed-off-by: Ronald Cron <ronald.cron@arm.com> Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Signed-off-by: Ronald Cron <ronald.cron@arm.com> Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Signed-off-by: Ronald Cron <ronald.cron@arm.com> Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
In particular, make sure pointer variables are initialized right after being declared. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
This was only supported in 1.2 for no good reason. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
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 <manuel.pegourie-gonnard@arm.com>
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Co-authored-by: David Horstmann <david.horstmann@arm.com> Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
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 <manuel.pegourie-gonnard@arm.com>
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Fix typo in psa-transition.md
Signed-off-by: Elena Uziunaite <elena.uziunaite@arm.com>
Misc small makefile fixes
Remove hacks about asm vs constant-flow testing
…_CAN_SHA512_with_PSA_WANT Replace MBEDTLS_MD_CAN_SHA512 with its PSA_WANT counterpart
Refactor tls123 verif dev
Signed-off-by: Elena Uziunaite <elena.uziunaite@arm.com>
Attempt to partially solve the performance regression in 3.6.0 without adding too much code size. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Signed-off-by: Janos Follath <janos.follath@arm.com>
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 <janos.follath@arm.com>
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 <janos.follath@arm.com>
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 <janos.follath@arm.com>
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 <janos.follath@arm.com>
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 <janos.follath@arm.com>
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 <janos.follath@arm.com>
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 <janos.follath@arm.com>
For |
Yes, I meant |
|
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. |
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
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 <Gilles.Peskine@arm.com>
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 <Gilles.Peskine@arm.com>
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 <Gilles.Peskine@arm.com>
|
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. |
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
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 <Gilles.Peskine@arm.com>
Transparently read outcome files compressed with xz (which we currently use on the CI) or with gzip. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
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 <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
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 <Gilles.Peskine@arm.com>
gilles-peskine-arm
force-pushed
the
dev/gilles-peskine-arm/analyze_outcome-split-framework
branch
from
53b6a46
to
6759e80
Compare
|
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
added
priority-high
mpg
added
approved
|
I'm happy with the content, but the history seems to include some commits that are not relevant.
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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Move
collect_test_cases.py(split fromcheck_test_cases.py),check_test_cases.pyandoutcome_analysis.py(split fromanalyze_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.xzartifact 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.xzcan 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