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

Allow auto-generation of library/certs.c from tests/data_files #2596

Closed

Conversation

hanno-becker
Copy link

@hanno-becker hanno-becker commented Apr 24, 2019

Summary: This PR annotates the hardcoded test certificates and keys in library/certs.c with a formal comment syntax describing their type and origin in tests/data_files, and adds a script scripts/generate_certs.sh which parses these comments and updates the hardcoded data. This aims to simplify the synchronization of tests/data_files with library/certs.c as well as the addition of new test data. This was suggested by @AndrzejKurek in #2260.

This is based on #2260.

@hanno-becker hanno-becker added enhancement mbed TLS team needs-review Every commit must be reviewed by at least two team members, component-tls needs-preceding-pr Requires another PR to be merged first labels Apr 24, 2019
@hanno-becker
Copy link
Author

@mpg @gilles-peskine-arm What's your view on this PR? As it stands, every modification of CRT or key files is prone to introduce a mismatch between library/certs.c and tests/data_files, and this PR is a way to detect this, and to automatically keep both in sync. I'm aware that the script I've written isn't optimal, and I'm sure @gilles-peskine-arm would be able to come up with something much better, but considering that we're short on time and debating our engagement even in much more important problems such as #3564, I wonder if it's worth taking this PR as-is and leaving improvements of the script for another time. WDYT?

Hanno Becker added 2 commits August 17, 2020 10:23
This commit adds the script scripts/generate_certs.sh which
parses library/certs.c and inserts certificate and key files
for any block of the form

   /* BEGIN FILE [string|binary] [variable|macro] NAME FILE */
   ...
   /* END FILE */

Here, the first argument string / binary indicates whether the
file should be inserted as a string or as a binary array. The
second argument indicates whether the resulting object should
be registered as a C variable or a macro.

This script allows to easily update certs.c in case any of the
test certificates from tests/data_files change, or new test
certificates / keys need to be added.
Re-generate library/certs.c from the data files in tests/data_files
using scripts/generate_certs.sh added in the last commit.

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

mpg commented Aug 17, 2020

@hanno-arm I'd be inclined to go with this script for starters. We can always rewrite in Python or otherwise later, but IMO this script is an improvement over the existing situation.

@hanno-becker
Copy link
Author

@mpg Ok, I'll clean up and rebase this PR, then 👍

Hanno Becker added 2 commits August 17, 2020 10:43
A previous modification had erroneously overwritten the generation
of `server1.crt.der` as the DER-form of `server1.crt` by some CRT
generation command, and re-introduced the previous name `server1.der`
for `server1.crt.der`.

Signed-off-by: Hanno Becker <[email protected]>
Signed-off-by: Hanno Becker <[email protected]>
@hanno-becker hanno-becker force-pushed the autogenerated_test_certs branch from e345c8e to 955d7ff Compare August 17, 2020 09:56
@hanno-becker hanno-becker changed the title Draft: Allow auto-generation of library/certs.c from tests/data_files Allow auto-generation of library/certs.c from tests/data_files Aug 17, 2020
@hanno-becker hanno-becker self-assigned this Aug 17, 2020
@hanno-becker hanno-becker added needs-ci Needs to pass CI tests and removed needs-preceding-pr Requires another PR to be merged first labels Aug 17, 2020
@gilles-peskine-arm
Copy link
Contributor

I like the general approach to generate certs.c automatically instead of manually updating it.

Please add the script to check-generated-files.sh.

I'd prefer for the new script to be in Python. More team members know Python than sh and the language has fewer gotchas. I'm not sure if xxd is even present in our CI environment. Python code would be more readable than sh. The sh script has many points of failure that would go undetected even with the addition of set -e, whereas python code can be relied on to exit with an uncaught exception if something unexpected happens.

@gilles-peskine-arm
Copy link
Contributor

FYI this script doesn't work on 2.7 because it doesn't have “BEGIN FILE”…“END FILE” markers. That's not a big deal, just something extra to do in the backports. More concerning, running this script on mbedtls-2.7 has no effect if I use bash, but with dash, it mangles the file: \r\n gets turned into CRLF. I haven't investigated why, probably a non-portable use of echo. Yet another argument to write this in python instead of sh.

@ronald-cron-arm
Copy link
Contributor

I also think it would be much better for the script to be written in Python. @hanno-arm do you think you would have time, would fancy rewriting the script in Python?

@daverodgman daverodgman added the needs-reviewer This PR needs someone to pick it up for review label Oct 5, 2020
@gilles-peskine-arm gilles-peskine-arm added needs-work and removed needs-ci Needs to pass CI tests needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Oct 6, 2020
@tom-cosgrove-arm tom-cosgrove-arm added historical-reviewed Reviewed & agreed to keep legacy PR/issue and removed historical-reviewed Reviewed & agreed to keep legacy PR/issue labels Jul 29, 2022
@tom-cosgrove-arm
Copy link
Contributor

Closing this in favour of #5986, which does the same thing in Python

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants