Add test for ECP group metadata #6203
Conversation
wernerlewis
added
enhancement
component-crypto
wernerlewis
added
the
needs-backports
wernerlewis
force-pushed
the
ecp_group_test
branch
from
54730da
to
2bd3961
Compare
Test cases added to check curve parameters and information for each curve supported. Expected parameters are taken from references: SEC 2 for NIST, RFC 5639 for Brainpool, and RFC 7748 for curve25519/curve448. Signed-off-by: Werner Lewis <werner.lewis@arm.com>
wernerlewis
force-pushed
the
ecp_group_test
branch
from
2bd3961
to
e54046c
Compare
wernerlewis
added
needs-ci
Spec values are now always used for test data, and conversion to internal representation is done in the test function. Signed-off-by: Werner Lewis <werner.lewis@arm.com>
tests/suites/test_suite_ecp.data
Outdated
| mbedtls_ecp_group_metadata:MBEDTLS_ECP_DP_BP384R1:384:MBEDTLS_ECP_TYPE_SHORT_WEIERSTRASS:"8cb91e82a3386d280f5d6f7e50e641df152f7109ed5456b412b1da197fb71123acd3a729901d1a71874700133107ec53":"7bc382c63d8c150c3c72080ace05afa0c2bea28e4fb22787139165efba91f90f8aa5814a503ad4eb04a8c7dd22ce2826":"4a8c7dd22ce28268b39b55416f0447c2fb77de107dcd2a62e880ea53eeb62d57cb4390295dbc9943ab78696fa504c11":"1d1c64f068cf45ffa2a63a81b7c13f6b8847a3e77ef14fe3db7fcafe0cbd10e8e826e03436d646aaef87b2e247d4af1e":"8abe1d7520f9c2a45cb1eb8e95cfd55262b70b29feec5864e19c054ff99129280e4646217791811142820341263c5315":"8cb91e82a3386d280f5d6f7e50e641df152f7109ed5456b31f166e6cac0425a7cf3ab6af6b7fc3103b883202e9046565":27 | ||
|
|
||
| Check ECP group metadata #11 bp512r1 (RFC 5639) | ||
| depends_on:MBEDTLS_ECP_DP_BP521R1_ENABLED |
There was a problem hiding this comment.
This should be MBEDTLS_ECP_DP_BP512R1_ENABLED instead. Interesting that this wasn't picked up by scripts as a typo...
There was a problem hiding this comment.
Right, check_names.py doesn't check tests.
There was a problem hiding this comment.
We can find out about typos in test dependencies from the result analysis job on Jenkins: it reports test cases that are not executed in any configuration. It isn't a CI failure to have those because some are legitimate and we haven't gotten around to analyzing the logs.
There was a problem hiding this comment.
I verified that all of #5919 apart from one (minor) have been addressed: mbedtls_ecp_tls_read_group_id is not called explicitly, but is called via mbedtls_ecp_tls_read_group.
I also verified that all of the data from RFC's is copied properly, apart from one detail: leading zeroes are removed. The library should handle it fine and I would even consider that a part of being able to handle raw data from RFC's, so it would be great to fix that. Also it's easier to search for full test data in the .data file and not pay attention to zeros. Affected curves:
- secp224k1;
- secp521r1;
- brainpoolP384r1.
One major typo found, leading zeroes are a minor and just a reccomendation from me.
Signed-off-by: Werner Lewis <werner.lewis@arm.com>
Signed-off-by: Werner Lewis <werner.lewis@arm.com>
Signed-off-by: Werner Lewis <werner.lewis@arm.com>
daverodgman
removed
the
needs-reviewer
| // Read group from buffer and compare with expected ID | ||
| TEST_EQUAL( mbedtls_ecp_tls_read_group_id( &read_g_id, &vbuf, olen ), | ||
| 0 ); | ||
| TEST_EQUAL( read_g_id, id ); |
There was a problem hiding this comment.
mbedtls_ecp_tls_read_group_id will modify vbuf.
| TEST_EQUAL( read_g_id, id ); | |
| TEST_EQUAL( read_g_id, id ); | |
| vbuf = buf; |
Signed-off-by: Werner Lewis <werner.lewis@arm.com>
AndrzejKurek
added
approved
|
@daverodgman Why was this merged while it was labeled needs-backport? As a general rule we wait for backports to be ready before we merge the main PR, and it's OK to make exceptions but IMO we should briefly state the reason why before merging, at least to make it clear that it was a conscious decision. |
|
Note: I just merged the backport #6204 (this is how I noticed) and I don't think it's a problem that it won't be in the same release as the main PR, considering this is just a test enhancement. |
mpg
removed
the
needs-backports
Description
Test cases added to check curve parameters and information for each
curve supported. Expected parameters are taken from references: SEC 2
for NIST, RFC 5639 for Brainpool, and RFC 7748 for curve25519/curve448.
Each group is compared with known curve parameters, other metadata is
checked and validated against test data. This includes loading the group by
TLS ID, by name, and from the result of
mbedtls_ecp_tls_write_group.This has been tested with the bug described in #5810, which is caught by
these tests. Resolves #5919.
Status
READY
Requires Backporting
Yes 2.28 #6204