Clean up & improve PK write test functions #7449

valeriosetti · 2023-04-17T15:37:30Z

Reduce memory footprint for pkwrite's tests.

Resolves #7446

Gatekeeper checklist

changelog not required - test only
backport Backport: Clean up & improve PK write test functions #7456
tests provided

Signed-off-by: Valerio Setti <[email protected]>

mpg

Going in the right direction, still found a few issues, mostly minor. Apparently the CI has complaints too, which I didn't check.

tests/suites/test_suite_pkwrite.function

tests/suites/test_suite_pkwrite.data

Signed-off-by: Valerio Setti <[email protected]>

tests/data_files/Makefile

tests/suites/test_suite_pkwrite.function

Signed-off-by: Valerio Setti <[email protected]>

mpg

Thanks for addressing my feedback! LGTM!

mpg · 2023-04-18T11:02:52Z

Actually, I think we want to backport this to 2.28 - we're trying to keep testing aligned when possible, in order to facilitate backports for future bug fixes, which generally come with their regression test.

AndrzejKurek

Looks good, although I think a bit about the testing times:
We're testing the same things over again in DER and PEM... How about testing with DER only if there's no MBEDTLS_PEM_PARSE_C and MBEDTLS_PEM_WRITE_C?

AndrzejKurek · 2023-04-20T12:30:37Z

tests/data_files/Makefile

@@ -999,6 +999,57 @@ ec_bp512_pub.comp.pem: ec_bp512_pub.pem
 	$(OPENSSL) ec -pubin -in $< -out $@ -conv_form compressed
 all_final += ec_bp512_pub.comp.pem

+################################################################
+#### Convert PEM keys in DER format


Suggested change

#### Convert PEM keys in DER format

#### Convert PEM keys to DER format

gilles-peskine-arm · 2023-04-20T16:32:44Z

How about testing with DER only if there's no MBEDTLS_PEM_PARSE_C and MBEDTLS_PEM_WRITE_C?

Let's not. There could be some mix-up in the parsing code that breaks DER parsing when PEM is enabled. The cost is negligible.

mpg · 2023-04-21T07:27:12Z

Tests are failing on Windows. Interestingly enough, what's failing is the PEM tests, which were already present, not the DER tests which have just been added. I didn't investigate, but this smells CR+LF vs LF issues - we changed the way files are read I think, so this probably had an impact somehow?

gilles-peskine-arm · 2023-04-21T21:13:29Z

tests/suites/test_suite_pkwrite.function


-    TEST_ASSERT(ilen == pem_len);
-    TEST_ASSERT(memcmp((char *) buf, (char *) check_buf, ilen) == 0);
+    ASSERT_COMPARE(start_buf, buf_len, check_buf, check_buf_len);


As Manuel suspects, this fails with PEM files on Windows because of its handling of text files. mbedtls_pk_write_key_pem writes to the buffer, without going through the I/O layer, so newlines in start_buf are a \n character. mbedtls_pk_load_file loads files in binary mode (it has no choice since it must support DER), so newlines in check_buf are \n on non-Windows but \r\n on Windows (because Git treats PEM files as text, so it converts \n to \r\n upon checkout).

This wasn't a problem with the old code because it called fopen in text mode to read check_buf, so the I/O layer converted \r\n in the file to \n in memory. But of course that would have corrupted DER files.

I guess the solution is to remove CR characters from check_buf if the format is PEM. Or keep calling fopen directly, but choose text vs binary mode depending on whether the data is DER or PEM.

Thanks a lot for your help! It really saved me a lot of time since I was not aware of this difference and I didn't have a Windows machine at hand.

We got hit by this on another PR not so long ago. While I understand why git it doing it, I still find it always surprising that what we check out (on Windows) is not byte-for-byte identical to what we commited (on Linux), at least with the default settings. Thanks Gilles for the reminder!

Signed-off-by: Valerio Setti <[email protected]>

mpg

LGTM except for one truncated comment and one choice of libc function.

tests/suites/test_suite_pkwrite.function

Signed-off-by: Valerio Setti <[email protected]>

mpg

LGTM

test: memory footprint optimization for pkwrite tests

8959095

Signed-off-by: Valerio Setti <[email protected]>

valeriosetti requested a review from mpg April 17, 2023 15:37

valeriosetti self-assigned this Apr 17, 2023

valeriosetti added 2 commits April 17, 2023 18:43

test: add support for DER format in pkwrite tests

c60bc5e

Signed-off-by: Valerio Setti <[email protected]>

test: add DER file format for pkwrite tests

28567ab

Signed-off-by: Valerio Setti <[email protected]>

mpg requested changes Apr 18, 2023

View reviewed changes

valeriosetti added 4 commits April 18, 2023 10:42

test: use proper macros for checks

3401b30

Signed-off-by: Valerio Setti <[email protected]>

test: add Makefile target for the generated DER files

8b7d432

Signed-off-by: Valerio Setti <[email protected]>

test: specify input file type through enum

c9cb532

Signed-off-by: Valerio Setti <[email protected]>

test: fix dependencies in DER and PEM tests

15cac17

Signed-off-by: Valerio Setti <[email protected]>

mpg reviewed Apr 18, 2023

View reviewed changes

tests/data_files/Makefile Outdated Show resolved Hide resolved

tests/suites/test_suite_pkwrite.function Outdated Show resolved Hide resolved

valeriosetti added 2 commits April 18, 2023 12:53

test: fix extension in DER test files

232a006

Signed-off-by: Valerio Setti <[email protected]>

test: properly check written PEM buffer len

2280895

Signed-off-by: Valerio Setti <[email protected]>

mpg previously approved these changes Apr 18, 2023

View reviewed changes

mpg added the needs-backports Backports are missing or are pending review and approval. label Apr 18, 2023

valeriosetti mentioned this pull request Apr 18, 2023

Backport: Clean up & improve PK write test functions #7456

Merged

3 tasks

valeriosetti removed the needs-ci Needs to pass CI tests label Apr 19, 2023

AndrzejKurek self-requested a review April 20, 2023 11:49

AndrzejKurek previously approved these changes Apr 20, 2023

View reviewed changes

AndrzejKurek removed the needs-backports Backports are missing or are pending review and approval. label Apr 20, 2023

gilles-peskine-arm added needs-work needs-ci Needs to pass CI tests and removed approved Design and code approved - may be waiting for CI or backports labels Apr 21, 2023

gilles-peskine-arm reviewed Apr 21, 2023

View reviewed changes

valeriosetti dismissed stale reviews from AndrzejKurek and mpg via a12801c April 24, 2023 06:52

fix new line difference in Windows

7bacaf8

Signed-off-by: Valerio Setti <[email protected]>

valeriosetti force-pushed the issue7446 branch from a12801c to 7bacaf8 Compare April 24, 2023 06:53

valeriosetti requested review from mpg, AndrzejKurek and gilles-peskine-arm April 24, 2023 07:30

mpg requested changes Apr 24, 2023

View reviewed changes

tests/suites/test_suite_pkwrite.function Outdated Show resolved Hide resolved

tests/suites/test_suite_pkwrite.function Outdated Show resolved Hide resolved

valeriosetti added 2 commits April 24, 2023 10:24

fix typos

547b3a4

Signed-off-by: Valerio Setti <[email protected]>

test_suite_pkwrite: replace memcpy with memmove

bf974b9

Signed-off-by: Valerio Setti <[email protected]>

AndrzejKurek approved these changes Apr 24, 2023

View reviewed changes

valeriosetti requested a review from mpg April 24, 2023 08:41

mpg approved these changes Apr 24, 2023

View reviewed changes

mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-work labels Apr 24, 2023

This was referenced Apr 24, 2023

Fixing USE_PSA_INIT/DONE in SSL/X509/PK test suites #7461

Merged

Fix test gap in PK write: private (opaque) -> public #7480

Closed

mpg merged commit 0281d76 into Mbed-TLS:development Apr 24, 2023

valeriosetti mentioned this pull request Apr 24, 2023

Read and write RFC8410 keys #6838

Merged

3 tasks

valeriosetti deleted the issue7446 branch December 6, 2024 06:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up & improve PK write test functions #7449

Clean up & improve PK write test functions #7449

valeriosetti commented Apr 17, 2023 •

edited

Loading

mpg left a comment

mpg left a comment

mpg commented Apr 18, 2023

AndrzejKurek left a comment

AndrzejKurek Apr 20, 2023

gilles-peskine-arm commented Apr 20, 2023

mpg commented Apr 21, 2023

gilles-peskine-arm Apr 21, 2023

valeriosetti Apr 24, 2023

mpg Apr 24, 2023

mpg left a comment •

edited

Loading

mpg left a comment

	#### Convert PEM keys in DER format
	#### Convert PEM keys to DER format

Clean up & improve PK write test functions #7449

Clean up & improve PK write test functions #7449

Conversation

valeriosetti commented Apr 17, 2023 • edited Loading

Gatekeeper checklist

mpg left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mpg commented Apr 18, 2023

AndrzejKurek left a comment

Choose a reason for hiding this comment

AndrzejKurek Apr 20, 2023

Choose a reason for hiding this comment

gilles-peskine-arm commented Apr 20, 2023

mpg commented Apr 21, 2023

gilles-peskine-arm Apr 21, 2023

Choose a reason for hiding this comment

valeriosetti Apr 24, 2023

Choose a reason for hiding this comment

mpg Apr 24, 2023

Choose a reason for hiding this comment

mpg left a comment • edited Loading

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

valeriosetti commented Apr 17, 2023 •

edited

Loading

mpg left a comment •

edited

Loading