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

Add regenerate data files tests #7866

Open
wants to merge 20 commits into
base: development
Choose a base branch
from

Conversation

yuhaoth
Copy link
Contributor

@yuhaoth yuhaoth commented Jul 3, 2023

Description

This PR try to guarantee tests/data_files are generated and there is no missed or un-tracked files in final output.

At this moment, there are still some wrong commands or no generate commands available. Those files are list with uncategorized.lst. And this PR will ignore those files.

Some commands are fixed in this file due to it can not be covered by uncategorized.lst.

#7649 and #7662 fix some wrong commands.

find_missed.py is for finding missed commands
test_new_data_files.sh is for local test.

@yuhaoth yuhaoth linked an issue Jul 3, 2023 that may be closed by this pull request
@yuhaoth yuhaoth added the needs-preceding-pr Requires another PR to be merged first label Jul 3, 2023
@yuhaoth yuhaoth marked this pull request as draft July 3, 2023 09:45
@yuhaoth yuhaoth force-pushed the pr/add-regenerate-data-files-tests branch 5 times, most recently from d745ebe to b16e5ba Compare July 6, 2023 08:39
@gilles-peskine-arm
Copy link
Contributor

Context: the directory tests/data_files contains many keys, certificates and related files (CSR, CRL, …) that are used by tests. In principle, tests/data_files/Makefile contains instructions to re-create these files. We keep these files in the repository for several reasons:

  • Historically, Makefile didn't exist, we just had the files themselves.
  • Many files contain embedded randomness or depend on the generation time, so we don't get identical files if you re-generate them.
  • The generation depends on several tools and on specific versions. For example I'm not sure if re-generating all the files works with a single openssl version.

The goal of this pull request is to validate that the instructions in the Makefile are correct. At the moment, nothing in the CI uses the Makefile.

@yuhaoth yuhaoth added needs-work and removed needs-preceding-pr Requires another PR to be merged first labels Jul 6, 2023
@tgonzalezorlandoarm tgonzalezorlandoarm self-assigned this Jul 6, 2023
@tgonzalezorlandoarm tgonzalezorlandoarm removed their assignment Jul 6, 2023
tests/scripts/all.sh Outdated Show resolved Hide resolved
component_test_regenerate_data_files_default () {
helper_regenerate_data_files neat
scripts/config.py full
helper_datafile_run_tests "default configuration with refreshed data files"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is a good approach for checking that the new files are good. We're checking that the tests pass, but that doesn't mean, but that doesn't mean that the tests do the same thing.

I would prefer a more reliable approach: compare the new files with the old files, using a comparison method that ignores some parts of the files (content of keys, signatures and hashes), comparing only the ASN.1 structure, the metadata that isn't derived from random data (names, serial numbers, etc.), and whether signatures are correct. It will take us some time to do it, but I think the benefits are worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. I will remove them later.

And I'd like reserve helper_datafile_run_tests as local test utility. I will move it to another scripts.

For non-ASN.1 files and malformed files, how to compare them ? Do you have any idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

For files without a valid ASN.1 structure, I don't have a plan to compare. Hopefully we won't need to regenerate those often.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I post those tests in this PR and reduce the scope of this PR.

About the compare script, I'd like write it in next PR. I think the script should only compare modified files. I have add TODO tag in regenerate_data_files_tests.sh . Please check it.

I limit openssl version to 3.0.2. I think it can generate all files. And I will fix miss or wrong commands in #7888.

@yuhaoth yuhaoth force-pushed the pr/add-regenerate-data-files-tests branch 3 times, most recently from 7893044 to c613aa3 Compare July 12, 2023 03:21
@yuhaoth yuhaoth force-pushed the pr/add-regenerate-data-files-tests branch 4 times, most recently from f790c75 to c4d8823 Compare July 13, 2023 07:43
@yuhaoth yuhaoth marked this pull request as ready for review July 13, 2023 10:02
@yuhaoth yuhaoth added component-test Test framework and CI scripts needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Jul 13, 2023
@tgonzalezorlandoarm tgonzalezorlandoarm dismissed their stale review August 11, 2023 13:07

Still reviewing, accidentally pushed the button

@tgonzalezorlandoarm
Copy link
Contributor

I would need some more explanation:

If "The goal of this pull request is to validate that the instructions in the Makefile are correct", how are we doing this on this PR?

@yuhaoth
Copy link
Contributor Author

yuhaoth commented Aug 21, 2023

rebase to resolve conflicts

@yuhaoth yuhaoth force-pushed the pr/add-regenerate-data-files-tests branch from 2cc372f to ea0827a Compare August 22, 2023 02:11
The components will remove all files, regenerate
and check if there are files that is missed or not
committed.

In future, we should check modified files are expected.

Signed-off-by: Jerry Yu <[email protected]>
And set openssl to 3.0.2

Signed-off-by: Jerry Yu <[email protected]>
- change neat target also.

Signed-off-by: Jerry Yu <[email protected]>
This is for regenerate all parse input files

Signed-off-by: Jerry Yu <[email protected]>
Those files are not commited and list in `all_intermediate`
and remove invalid final file

Signed-off-by: Jerry Yu <[email protected]>
The file is used by x509_cert_ino and verified with expected output

Signed-off-by: Jerry Yu <[email protected]>
- reserved list includes files that used as command input
- uncategoried includes files that commands fail or not covered and should
  be resolved in future

Signed-off-by: Jerry Yu <[email protected]>
Signed-off-by: Jerry Yu <[email protected]>
Signed-off-by: Jerry Yu <[email protected]>
@yuhaoth yuhaoth force-pushed the pr/add-regenerate-data-files-tests branch from a8feae9 to 52ff195 Compare November 16, 2023 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-test Test framework and CI scripts needs-review Every commit must be reviewed by at least two team members, priority-medium Medium priority - this can be reviewed as time permits
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

Add faketime test script
3 participants