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

[DI] Move files from tests/scripts to mbedtls-framework #12

Closed
ronald-cron-arm opened this issue Apr 29, 2024 · 7 comments
Closed

[DI] Move files from tests/scripts to mbedtls-framework #12

ronald-cron-arm opened this issue Apr 29, 2024 · 7 comments
Assignees
Labels
enhancement New feature or request size-m Estimated task size: medium (~1w)

Comments

@ronald-cron-arm
Copy link
Contributor

ronald-cron-arm commented Apr 29, 2024

Definition of Done for the work on a test script that must be move to mbedtls-framework (see table below) as part of this issue:

  • the test script is moved to mbedtls-framework
  • mbedtls 3.6 and development branches are using the test script from the framework and their CI run successfully
  • if the test script is relevant to TF-PSA-Crypto (see table below) there is in development an all.sh component in tf-psa-crypto/tests/scripts using that script and the component is run as part of the mbedtls CI (see TF-PSA-Crypto first all.sh component #40 for more information about tf-psa-crypto/tests/scripts components).

When the PR Mbed-TLS/mbedtls#9445 is merged, the work on the generate_*.py files that in the table below have a green tick in their "F" column will be completed: files in mbedtls-framework, used to build and test mbedtls 3.6 and development, used to build and test the tf-psa-crypto code in the test_cmake_tf_psa_crypto_out_of_source() all.sh code (component not yet in the right place though but this will be addressed by #40).

Before to move a test script to mbedtls-framework, it has to be the same in 3.6 and development branches as otherwise it is
very likely to not work properly with one of these branches.

The following table lists the files in tests/scripts/ as of 898066b.
Besides the work on depends.py that will be covered by other issues, the duration for the estimated work (not fully estimated but what is not estimated does not seem to be necessary for TF-PSA-Crypto split) is:
Necessary for the split (CI): 4M + 3S + 16XS ~ 8M
Done: 7XS
Remaining: 4M + 3S + 9XS ~ 7M

Not necessary for the split: 2XS

Table columns legend:
CI: Directly or indirectly used by the CI
F: To be moved to mbedtls-framework (y/n)
C: Used in TF-PSA-Crypto
I: Currently identical between 3.6 and development? (y/n/36/40/-) (36/40 =
present only in that branch, - = no longer present, presumably moved already)
S: Work size for the move and potentially changes needed for TF-PSA-Crypto

CI F C I S Issue/PR Comment
run-metatests.sh y y y y S
test-ref-configs.pl y y y n S+S Mbed-TLS/mbedtls#9057, Mbed-TLS/mbedtls#9565
all.sh y y y n M Mbed-TLS/mbedtls#8226 see #40
analyze_outcomes.py y y y y M analyze_driver_vs_reference tasks to adapt
basic-build-test.sh y y ? n S #12 (comment)
check-doxy-blocks.pl y y y y S Mbed-TLS/TF-PSA-Crypto#49 +doxygen.sh
check_files.py y y y n XS #7
check-generated-files.sh y y y n XS adaptation to TF-PSA-Crypto done
check_names.py y y y n M M for TF-PSA-Crypto adaptation
check-python-files.sh y y y y XS
check_test_cases.py y y y n XS
depends.py y y y n >M
generate_bignum_tests.py y ✔️ y - XS
generate_config_tests.py y ✔️ y - -
generate_ecp_tests.py y ✔️ y - XS
generate_pkcs7_tests.py y ✔️ n - -
generate_psa_tests.py y ✔️ y - XS Called by abi_check.py
generate_psa_wrappers.py y ✔️ y - XS
generate_test_code.py y ✔️ y - XS
generate_test_cert_macros.py y ✔️ n - XS
generate_test_keys.py y ✔️ y - XS
generate_tls13_compat_tests.py y y n y XS
list_internal_identifiers.py y y y n XS + list-identifiers.sh CI scripts
psa_collect_statuses.py y y y y S
recursion.pl y y y y -
run_demos.py y y y y XS
run-test-suites.pl y n n n - make test
tcp_client.pl y y n y - called by ssl-opt.sh
test_psa_compliance.py y y y n - adaptation to TF-PSA-Crypto done
test_psa_constant_names.py y y y n XS
test_zeroize.gdb y y y y -
translate_ciphers.py y y n y - called by compat.sh
audit-validity-dates.py n y n y XS
generate-afl-tests.sh n y y y -
generate_server9_bad_saltlen.py n y n y -
gen_ctr_drbg.pl n c y XS
gen_gcm_decrypt.pl n c y y -
gen_gcm_encrypt.pl n c y y -
gen_pkcs1_v21_sign_verify.pl n c y y -
set_psa_test_dependencies.py n y n y -
test_config_script.py n y y y XS changes for crypto as part of work in crypto_config.py
test_generate_test_code.py n ✔️ n - -
all-in-docker.sh n n n n XS Obsolete, should be removed
basic-in-docker.sh n n n y - Obsolete, should be removed
docker_env.sh n n n y - Obsolete, should be removed
travis-log-failure.sh n n n y - Obsolete, should be removed
@ronald-cron-arm ronald-cron-arm self-assigned this May 2, 2024
@ronald-cron-arm ronald-cron-arm changed the title Investigate files from tests/scripts to move to mbedtls-framework Move files from tests/scripts to mbedtls-framework May 6, 2024
@ronald-cron-arm ronald-cron-arm added the enhancement New feature or request label May 7, 2024
@ronald-cron-arm ronald-cron-arm removed their assignment May 7, 2024
@minosgalanakis minosgalanakis moved this to Mbed TLS framework in Mbed TLS Epics Aug 2, 2024
@mpg
Copy link
Contributor

mpg commented Sep 24, 2024

basic-build-test.sh

That's used on the CI - not in the PR job, and not via all.sh, but it's invoked directly in the nightly and release jobs (that's what provides numbers for the testing part of the release report). So I'm moving it to "CI: yes".

Edit: I think it should be in the framework because the 3.6 and 4.0 versions should be pretty close. The tf-psa-crypto version may differ significantly, so perhaps it will be more efficient for tf-psa-crypto to have its own copy. TBD.

@mpg
Copy link
Contributor

mpg commented Sep 26, 2024

gen_gcm_decrypt.pl, gen_gcm_encrypt.pl, gen_pkcs1_v21_sign_verify.pl and gen_ctr_drbg.pl

I see no reason for moving those to the framework: those were used to generate .data file content for crypto test suites. As such, they belong in tf-psa-crypto only - they are not used and will never be used on the mbedtls side. Edit: wrong: #12 (comment)

(Note that all 4 of these scripts are for mostly for historical purposes, I don't really expect we'll have to run them again at any point. But if we feel the need to run them again at some point, the output will only be used in tf-psa-crypto.)

(The input files were downloaded from the NIST website. It's unfortunate that the scripts don't give more details, like a speficic URL, or actually we could have just committed the input files as well. I we want to find the input files again, I guess we should start by looking for CAVS (cryptographic algorithm validation suite).

So, I'm moving them to a new section "move to tf-psa-crypto". Edit: moving them back.

@mpg mpg changed the title Move files from tests/scripts to mbedtls-framework [DI] Move files from tests/scripts to mbedtls-framework Sep 27, 2024
@mpg mpg added the size-m Estimated task size: medium (~1w) label Sep 27, 2024
@mpg mpg self-assigned this Oct 2, 2024
@mpg
Copy link
Contributor

mpg commented Oct 2, 2024

I see no reason for moving those to the framework: those were used to generate .data file content for crypto test suites. As such, they belong in tf-psa-crypto only - they are not used and will never be used on the mbedtls side.

That's not correct, due to the existence of Mbed TLS 3.6, which also has crypto. (Also, there might be LTS branches in tf-psa-crypto in the future and then the files would be shared between multiple branches on the crypto side.)

So, moving those files back to their original classification.

@davidhorstmann-arm
Copy link
Contributor

That's not correct, due to the existence of Mbed TLS 3.6, which also has crypto. (Also, there might be LTS branches in tf-psa-crypto in the future and then the files would be shared between multiple branches on the crypto side.)

It's not really my place to interfere, but I'd be in favour of us avoiding moving things to the framework unless absolutely necessary, for the following reasons:

  1. Moving things to the framework will restrain what we can do with development since the same scripts will have to be compatible with the LTSes as well. If we want to change the way development works we will have to add complexity to the framework script to deal with the differences.
  2. Every PR that needs to touch the tests and the generation scripts will be turned into 2 PRs (plus the extra effort of making the script work in 3.6).
  3. We've historically been able to keep scripts in sync between development and LTS versions just by backporting changes anyway.

Is sharing between development and LTS branches a sufficient reason to put something in the framework?

@gilles-peskine-arm
Copy link
Contributor

Is sharing between development and LTS branches a sufficient reason to put something in the framework?

I think it is. Historically, there haven't been a lot of changes in test case generation scripts that couldn't be backported. There are many that we didn't backport, but we could have, we just didn't bother because there weren't needed.

As to those particular scripts though (gen_gcm_decrypt.pl, gen_gcm_encrypt.pl, gen_pkcs1_v21_sign_verify.pl and gen_ctr_drbg.pl) — they're only of historical interest, since we aren't running them actively, so I don't think moving them is important. (But I think moving them is trivial, since we aren't running them actively.)

@mpg
Copy link
Contributor

mpg commented Oct 2, 2024

Is sharing between development and LTS branches a sufficient reason to put something in the framework?

Currently I think that's the agreed plan, yes. The primary purpose of the framework is to allow sharing between mbedtls and tf-psa-crypto, but a secondary goal is to allow sharing between branches in the same (or a different) repo. (In the specific case above, which is honestly not that interesting as the files are mostly historical relics, so we're fine either way, they would be shared between tf-psa-crypto/development and mbedtls/mbedtls-3.6 after the split.)

I wrote the following in the EPIC description (internal ref: SECLIB-1357):

Applicable = (1) Used by both tf-psa-crypto and mbedtls, or by more than 1 branch + (2) enough shared between repo/branches that it's easier to maintain a single version in the framework than multiple versions (in each repo/branch).

I believe it reflects the current plan (though the wording of (2) is mine). However, I don't think you're the only one having doubts about the scope. We might want to have another team-wide discussion about that to make sure we're all on the same page and check if we're still good with the current plan. (And yes, I think it is your place, and any team member's place, to express doubts about current plans.)

Regardless, I intend to prioritize moving files that will be used by tf-psa-crypto. I'm in the process of re-organizing the table above and in #16 in order to dispatch between stages 2 and 3, and prioritize according to whether it's used by tf-psa-crypto within each stage.

@mpg
Copy link
Contributor

mpg commented Oct 3, 2024

Closing as superseded by #52 and #53 - initially same content, but grouped differently.

@mpg mpg closed this as completed Oct 3, 2024
@mpg mpg removed this from Mbed TLS Epics Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size-m Estimated task size: medium (~1w)
Projects
None yet
Development

No branches or pull requests

4 participants