Add test for PK parsing of keys using compressed points #6937

valeriosetti · 2023-01-16T15:31:02Z

Description

Following merging of PR #6282, PK parse module should now be able to parse EC compressed points. This PR adds some test for that purpose.
Resolves #6886

Gatekeeper checklist

changelog not required - tests only
backport not required - tests for a feature that's only in development
tests provided - the point of this PR

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

tests/data_files/Makefile

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

mpg

What's there looks good to me, but I have a question about what isn't there: why no ec_224_pub.comp.pem (and associated Parse Public EC Key #3a test case)?

valeriosetti · 2023-01-25T09:22:40Z

What's there looks good to me, but I have a question about what isn't there: why no ec_224_pub.comp.pem (and associated Parse Public EC Key #3a test case)?

Because I'm not sure those curves are supported:
https://github.com/Mbed-TLS/mbedtls/blob/development/ChangeLog.d/mbedtls_ecp_point_read_binary-compressed-fmt.txt

Indeed if I try to add it then mbedtls_ecp_sw_derive_y return at line 1280

mpg · 2023-01-25T11:26:42Z

Ah good point :) Still, it might be better to be explicit about it and add a negative test case with a comment then. Otherwise it's hard to know if the missing case is intentional or an oversight when looking at it in the future.

valeriosetti · 2023-01-25T12:48:15Z

Ah good point :) Still, it might be better to be explicit about it and add a negative test case with a comment then. Otherwise it's hard to know if the missing case is intentional or an oversight when looking at it in the future.

I thought about adding a comment, but you're right: a negative test is probably better here in order to enforce/verify the check

The test is expected to fail, so we verify that this is really not suppported Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

mpg

Looks good except one point where I think a cleaner solution is available.

tests/suites/test_suite_pkparse.function

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

mpg

Just one bit of documentation to improve.

include/mbedtls/ecp.h

gilles-peskine-arm · 2023-02-07T09:20:26Z

CI is still going, but already unhappy

mpg

LGTM

mpg · 2023-02-08T10:28:09Z

I created #7060 to track the parts that we decided to consider out of scope here. @gilles-peskine-arm perhaps that should be added to some tech debt list?

Also, CI is now green so this is ready for re-review.

mpg · 2023-02-08T12:34:17Z

Note to self & other gatekeepers: I suspect this might conflict (as in, not seen by git but only by the CI) with #6970 which was just merged, so we want to wait for the nightly re-run of pr-merge before merging this. Temporarily labeling "do not merge" "needs-ci" as a reminder.

mpg · 2023-02-10T09:51:21Z

No conflict with #6970. Now that I see the results, I should have known: 6970 is about building without ECDSA, and we'll run into problem with compressed points only when building without ECP. Anyway, better safe than sorry.

gilles-peskine-arm

Just one problem in the documentation.

gilles-peskine-arm · 2023-02-10T14:02:50Z

include/mbedtls/ecp.h

- * Point formats, from RFC 4492's enum ECPointFormat
+/**
+ * The uncompressed point format for Short Weierstrass curves
+ * (MBEDTLS_ECP_DP_SECP* and MBEDTLS_ECP_DP_BP*). //no-check-names


This includes “//no-check-names” in the documentation. That's not good.

Our normal convention is to write XXX or xxx for a wildcard, not *. And check_names.py has an exception for names containing these.

Suggested change

* (MBEDTLS_ECP_DP_SECP* and MBEDTLS_ECP_DP_BP*). //no-check-names

* (MBEDTLS_ECP_DP_SECP_XXX and MBEDTLS_ECP_DP_BP_XXX).

and likewise below.

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

gilles-peskine-arm

LGTM

gilles-peskine-arm · 2023-02-14T18:53:34Z

include/mbedtls/ecp.h

+ * \warning     While this format is supported for all concerned curves for
+ *              writing, when it comes to parsing, it is not supported for all
+ *              curves. Specifically, parsing compressed points on
+ *              MBEDTLS_ECP_DP_SECP224R1 and MBEDTLS_ECP_DP_SECP224K1 is not


Non-blocker:

Suggested change

* MBEDTLS_ECP_DP_SECP224R1 and MBEDTLS_ECP_DP_SECP224K1 is not

* #MBEDTLS_ECP_DP_SECP224R1 and #MBEDTLS_ECP_DP_SECP224K1 is not

(Doxygen automatically makes links for foo(), but not for a macro name.)

test: check pkparse with compressed ec

c60611b

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti requested a review from mpg January 16, 2023 15:31

valeriosetti added enhancement needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) labels Jan 16, 2023

test: extend makefile to generate keys with compressed points

0c96016

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti self-assigned this Jan 16, 2023

gstrauss reviewed Jan 16, 2023

View reviewed changes

tests/data_files/Makefile Outdated Show resolved Hide resolved

test: data: fix makefile error

ff15953

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti removed the needs-ci Needs to pass CI tests label Jan 17, 2023

valeriosetti added 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 and removed needs-reviewer This PR needs someone to pick it up for review labels Jan 18, 2023

mpg requested changes Jan 24, 2023

View reviewed changes

test: add failing check for secp224r1 with compressed format

de7bb5b

The test is expected to fail, so we verify that this is really not suppported Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti requested review from mpg and gstrauss January 26, 2023 09:05

valeriosetti force-pushed the issue6886 branch from 4bbad5e to 247d123 Compare January 26, 2023 09:07

mpg requested changes Jan 26, 2023

View reviewed changes

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

test: fix complementary domain testing for !MBEDTLS_ECP_C

fa49a8e

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti force-pushed the issue6886 branch from 247d123 to fa49a8e Compare January 26, 2023 16:17

mpg requested changes Feb 7, 2023

View reviewed changes

include/mbedtls/ecp.h Outdated Show resolved Hide resolved

valeriosetti force-pushed the issue6886 branch from c4a7f05 to ad0adf6 Compare February 7, 2023 10:11

mpg previously approved these changes Feb 7, 2023

View reviewed changes

mpg mentioned this pull request Feb 8, 2023

Improve testing of ECC keys parsing #7060

Open

mpg added DO-NOT-MERGE needs-ci Needs to pass CI tests needs-review Every commit must be reviewed by at least two team members, and removed DO-NOT-MERGE needs-ci Needs to pass CI tests labels Feb 8, 2023

gilles-peskine-arm requested changes Feb 10, 2023

View reviewed changes

ecp: add documentation for compressed points limitations

78f79d3

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>

valeriosetti dismissed mpg’s stale review via 78f79d3 February 10, 2023 15:33

valeriosetti force-pushed the issue6886 branch from ad0adf6 to 78f79d3 Compare February 10, 2023 15:33

mpg approved these changes Feb 13, 2023

View reviewed changes

mpg requested a review from gilles-peskine-arm February 13, 2023 08:22

gilles-peskine-arm approved these changes Feb 14, 2023

View reviewed changes

gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Feb 14, 2023

gilles-peskine-arm merged commit c5e2a4f into Mbed-TLS:development Feb 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test for PK parsing of keys using compressed points #6937

Add test for PK parsing of keys using compressed points #6937

valeriosetti commented Jan 16, 2023 •

edited by mpg

Loading

mpg left a comment

valeriosetti commented Jan 25, 2023

mpg commented Jan 25, 2023

valeriosetti commented Jan 25, 2023

mpg left a comment

mpg left a comment

gilles-peskine-arm commented Feb 7, 2023

mpg left a comment

mpg commented Feb 8, 2023 •

edited

Loading

mpg commented Feb 8, 2023 •

edited

Loading

mpg commented Feb 10, 2023

gilles-peskine-arm left a comment

gilles-peskine-arm Feb 10, 2023

gilles-peskine-arm left a comment

gilles-peskine-arm Feb 14, 2023

	* (MBEDTLS_ECP_DP_SECP* and MBEDTLS_ECP_DP_BP*). //no-check-names
	* (MBEDTLS_ECP_DP_SECP_XXX and MBEDTLS_ECP_DP_BP_XXX).

	* MBEDTLS_ECP_DP_SECP224R1 and MBEDTLS_ECP_DP_SECP224K1 is not
	* #MBEDTLS_ECP_DP_SECP224R1 and #MBEDTLS_ECP_DP_SECP224K1 is not

Add test for PK parsing of keys using compressed points #6937

Add test for PK parsing of keys using compressed points #6937

Conversation

valeriosetti commented Jan 16, 2023 • edited by mpg Loading

Description

Gatekeeper checklist

mpg left a comment

Choose a reason for hiding this comment

valeriosetti commented Jan 25, 2023

mpg commented Jan 25, 2023

valeriosetti commented Jan 25, 2023

mpg left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Feb 7, 2023

mpg left a comment

Choose a reason for hiding this comment

mpg commented Feb 8, 2023 • edited Loading

mpg commented Feb 8, 2023 • edited Loading

mpg commented Feb 10, 2023

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Feb 10, 2023

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Feb 14, 2023

Choose a reason for hiding this comment

valeriosetti commented Jan 16, 2023 •

edited by mpg

Loading

mpg commented Feb 8, 2023 •

edited

Loading

mpg commented Feb 8, 2023 •

edited

Loading