Split numeric string conversions out of the OID module #9413

sezrab · 2024-07-19T14:22:52Z

The OID module is in the crypto library because a small part of it is used by crypto, but most of it is only useful for X.509, and we have had complaints about code size (ARMmbed/mbed-crypto#134). The goal of this task is to to move the binary-string conversions to the X.509 module. We should do that before the files move to separate repositories.

Move the function declarations to x509.h.

Move mbedtls_oid_get_numeric_string to x509.c and its unit tests to test_suite_x509parse.*.

Movembedtls_oid_from_numeric_string to x509_create.c and its unit tests to test_suite_x509write.*.

(In other words, move these two functions together with the X.509 code that is their sole user.)

PR checklist

changelog provided
development PR provided
framework PR not required
3.6 PR not required: This change is API breaking and is designed for release 4.0.
2.28 PR not required
tests not required because they already exist

ChangeLog.d/split-numeric-string-conversions-out-of-the-oid-module.txt

tf-psa-crypto/drivers/builtin/include/mbedtls/oid.h

This commit moves the function declarations for mbedtls_oid_get_numeric_string and mbedtls_oid_from_numeric_string from oid.h to x509.h. Signed-off-by: Sam Berry <sam.berry@arm.com>

This commit moves the mbedtls_oid_get_numeric_string function definition from oid.c to x509.c. Signed-off-by: Sam Berry <sam.berry@arm.com>

This commit moves all related mbedtls_oid_get_numeric_string unit tests from test_suite_oid to test_suite_x509parse. Signed-off-by: Sam Berry <sam.berry@arm.com>

This commit moves the mbedtls_oid_from_numeric_string function definition from oid.c to x509_create.c Signed-off-by: Sam Berry <sam.berry@arm.com>

This commit moves all related mbedtls_oid_from_numeric_string unit tests from test_suite_oid to test_suite_x509write. Signed-off-by: Sam Berry <sam.berry@arm.com>

This commit moves static functions that are necessary for mbedtls_oid_get_numeric_string and mbedtls_oid_from_numeric_string from oid.c to x509.c Signed-off-by: Sam Berry <sam.berry@arm.com>

Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>

davidhorstmann-arm

One test case dependency typo, LGTM otherwise

tests/suites/test_suite_x509parse.function

davidhorstmann-arm

LGTM, thanks!

No backports required as this is an API-breaking refactor that we are doing for 4.0. Please add words to that effect after the backport checkboxes in the PR description and tick them.

Co-authored-by: David Horstmann <david.horstmann@arm.com> Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>

eleuzi01 · 2024-09-23T13:32:23Z

library/x509.c

@@ -805,6 +805,75 @@ static char nibble_to_hex_digit(int i)
    return (i < 10) ? (i + '0') : (i - 10 + 'A');
 }

+/* Return the x.y.z.... style numeric string for the given OID */


Shouldn't this also be guarded by #if defined(MBEDTLS_OID_C)?

I am not too sure, since issue 9379 mentions that this function is explicity used by mbedtls_x509_dn_gets and I believe the only guard should be MBEDTLS_X509_USE_C.

I am unsure however, so is anyone able to clear this up further?

The functions are moving from the OID module to an X.509 module. So none of the code added by this PR should be guarded by MBEDTLS_OID_C. Instead it should be guarded by whatever MBEDTLS_X509_XXX options are needed at the point where the X.509 code uses them.

eleuzi01 · 2024-09-24T11:10:27Z

library/x509_create.c

@@ -278,6 +278,186 @@ static int parse_attribute_value_hex_der_encoded(const char *s,
    return MBEDTLS_ERR_X509_INVALID_NAME;
 }

+#if defined(MBEDTLS_OID_C)


Suggested change

#if defined(MBEDTLS_OID_C)

Removed, thanks

This commit removes the MBEDTLS_OID_C guard from the static functions in the library/x509_create.c as this function is no longer included in the oid.c file. Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>

davidhorstmann-arm

LGTM, thanks!

Moved functions are guarded by MBEDTLS_X509_USE_C and MBEDTLS_X509_CREATE_C since these guard x509.c and x509_create.c respectively.

eleuzi01

LGTM too, thanks!

sezrab requested a review from gilles-peskine-arm July 19, 2024 14:23

sezrab added the needs-ci Needs to pass CI tests label Jul 19, 2024

sezrab removed the request for review from gilles-peskine-arm July 19, 2024 14:24

sezrab added needs-work component-x509 component-crypto Crypto primitives and low-level interfaces size-xs Estimated task size: extra small (a few hours at most) labels Jul 19, 2024

tom-cosgrove-arm changed the title ~~Split numeric string conversions out of the OID module~~ Split numeric string conversions out of the OID module Jul 23, 2024

sezrab force-pushed the split_numeric_string_conversions_oid-development branch 3 times, most recently from 63871e7 to 858ac4e Compare August 14, 2024 13:36

Harry-Ramsey force-pushed the split_numeric_string_conversions_oid-development branch 4 times, most recently from 11928d6 to 26ac2e7 Compare September 13, 2024 14:16

Harry-Ramsey self-assigned this Sep 13, 2024

Harry-Ramsey force-pushed the split_numeric_string_conversions_oid-development branch from 8878168 to ac6e0bf Compare September 13, 2024 16:49

davidhorstmann-arm self-requested a review September 13, 2024 17:17

davidhorstmann-arm reviewed Sep 13, 2024

View reviewed changes

ChangeLog.d/split-numeric-string-conversions-out-of-the-oid-module.txt Show resolved Hide resolved

Harry-Ramsey 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-work labels Sep 16, 2024

davidhorstmann-arm reviewed Sep 18, 2024

View reviewed changes

tf-psa-crypto/drivers/builtin/include/mbedtls/oid.h Outdated Show resolved Hide resolved

sezrab and others added 7 commits September 18, 2024 21:23

Move the function declarations to x509.h

f8db5b6

This commit moves the function declarations for mbedtls_oid_get_numeric_string and mbedtls_oid_from_numeric_string from oid.h to x509.h. Signed-off-by: Sam Berry <sam.berry@arm.com>

Move function mbedtls_oid_get_numeric_string to x509.c

4f76194

This commit moves the mbedtls_oid_get_numeric_string function definition from oid.c to x509.c. Signed-off-by: Sam Berry <sam.berry@arm.com>

Move mbedtls_oid_get_numeric_string unit tests to test_suite_x509parse

4aee6a2

This commit moves all related mbedtls_oid_get_numeric_string unit tests from test_suite_oid to test_suite_x509parse. Signed-off-by: Sam Berry <sam.berry@arm.com>

Move mbedtls_oid_from_numeric_string to x509_create.c

c71abc3

This commit moves the mbedtls_oid_from_numeric_string function definition from oid.c to x509_create.c Signed-off-by: Sam Berry <sam.berry@arm.com>

Move mbedtls_oid_from_numeric_string unit tests to test_suite_x509write

2bb3f4d

This commit moves all related mbedtls_oid_from_numeric_string unit tests from test_suite_oid to test_suite_x509write. Signed-off-by: Sam Berry <sam.berry@arm.com>

Move static OID functions to x509.c

3da783b

This commit moves static functions that are necessary for mbedtls_oid_get_numeric_string and mbedtls_oid_from_numeric_string from oid.c to x509.c Signed-off-by: Sam Berry <sam.berry@arm.com>

Add ChangeLog for moving OID string conversion functions

5125a1b

Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>

Fix ChangeLog format error

e5b261f

Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>

Harry-Ramsey force-pushed the split_numeric_string_conversions_oid-development branch from d1a33b8 to e5b261f Compare September 18, 2024 20:25

davidhorstmann-arm requested changes Sep 19, 2024

View reviewed changes

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

davidhorstmann-arm previously approved these changes Sep 19, 2024

View reviewed changes

Fix x509 parse syntax typo

94c3065

Co-authored-by: David Horstmann <david.horstmann@arm.com> Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>

Harry-Ramsey force-pushed the split_numeric_string_conversions_oid-development branch from 57309c9 to 94c3065 Compare September 19, 2024 15:32

eleuzi01 self-requested a review September 23, 2024 13:10

eleuzi01 reviewed Sep 23, 2024

View reviewed changes

eleuzi01 reviewed Sep 24, 2024

View reviewed changes

Remove MBEDTLS_OID_C guard from static functions

3b71262

This commit removes the MBEDTLS_OID_C guard from the static functions in the library/x509_create.c as this function is no longer included in the oid.c file. Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>

Harry-Ramsey dismissed davidhorstmann-arm’s stale review via 3b71262 September 26, 2024 10:39

davidhorstmann-arm approved these changes Sep 26, 2024

View reviewed changes

eleuzi01 approved these changes Sep 26, 2024

View reviewed changes

davidhorstmann-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, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review labels Sep 26, 2024

davidhorstmann-arm added this pull request to the merge queue Sep 26, 2024

Merged via the queue into Mbed-TLS:development with commit b268d27 Sep 26, 2024
3 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split numeric string conversions out of the OID module #9413

Split numeric string conversions out of the OID module #9413

sezrab commented Jul 19, 2024 •

edited by davidhorstmann-arm

Loading

davidhorstmann-arm left a comment

davidhorstmann-arm left a comment

eleuzi01 Sep 23, 2024

Harry-Ramsey Sep 24, 2024

gilles-peskine-arm Sep 24, 2024

eleuzi01 Sep 24, 2024

Harry-Ramsey Sep 26, 2024

davidhorstmann-arm left a comment

eleuzi01 left a comment

Split numeric string conversions out of the OID module #9413

Split numeric string conversions out of the OID module #9413

Conversation

sezrab commented Jul 19, 2024 • edited by davidhorstmann-arm Loading

PR checklist

davidhorstmann-arm left a comment

Choose a reason for hiding this comment

davidhorstmann-arm left a comment

Choose a reason for hiding this comment

eleuzi01 Sep 23, 2024

Choose a reason for hiding this comment

Harry-Ramsey Sep 24, 2024

Choose a reason for hiding this comment

gilles-peskine-arm Sep 24, 2024

Choose a reason for hiding this comment

eleuzi01 Sep 24, 2024

Choose a reason for hiding this comment

Harry-Ramsey Sep 26, 2024

Choose a reason for hiding this comment

davidhorstmann-arm left a comment

Choose a reason for hiding this comment

eleuzi01 left a comment

Choose a reason for hiding this comment

sezrab commented Jul 19, 2024 •

edited by davidhorstmann-arm

Loading