Define "MD light" subset of MD #7120

mpg · 2023-02-16T21:36:24Z

Resolves #6988

Note: this PR only takes care of the functional part. A potential code size optimization is left for later: #7119

Also, currently there are no users of MD light, this will happen as part of #6989

Gatekeeper checklist

changelog not required - mostly for internal use
backport not required - new feature
tests provided

It was already marked as internal use only, and no longer used internally. Also, it won't work when we dispatch to PSA. Remove it before the MD_LIGHT split to avoid a corner case: it's technically a hashing function, no HMAC or extra metadata, but we still don't want it in MD_LIGHT really. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

See docs/architecture/psa-migration/md-cipher-dispatch.md Regarding testing, the no_md component was never very useful, as that's not something people are likely to want to do: it was mostly useful as executable documentation of what depends on MD. It's going to be even less useful when more and more modules auto-enable MD_LIGHT or even MD_C. So, recycle it to test the build with only MD_LIGHT, which is something that might happen in practice, and is necessary to ensure that the division is consistent. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg · 2023-02-21T09:19:08Z

The CI is green except for the ABI-API checker pointing out that removing md_process() is technically an API break. I think it's an acceptable one considering is was documented as /* Internal use */ but I'll ask for opinions.

mpg · 2023-02-21T10:14:18Z

We had a discussion on slack and there seems to be consensus that removing md_process() is acceptable. (See also #4657 where the opinion that it was OK to remove md_process() in a minor version was already expressed - there were questions about the underlying functions but this PR is not touching them, just the generic wrapper md_process().)

AndrzejKurek · 2023-02-22T13:35:22Z

include/mbedtls/mbedtls_config.h

+ * meta-data.
+ *
+ * This is automatically enabled whenever MBEDTLS_MD_C is enabled, but it is
+ * possible to enable this with MBEDTLS_MD_C if support for HMAC or extra


Suggested change

* possible to enable this with MBEDTLS_MD_C if support for HMAC or extra

* possible to enable this without MBEDTLS_MD_C if support for HMAC or extra

AndrzejKurek · 2023-02-22T13:36:34Z

include/mbedtls/mbedtls_config.h

@@ -2643,7 +2643,7 @@
 /**
 * \def MBEDTLS_MD_C
 *
- * Enable the generic message digest layer.
+ * Enable the generic layer for message digest (hashing) and HMAC.


Suggested change

* Enable the generic layer for message digest (hashing) and HMAC.

* Enable the generic layer for message digest (hashing) and HMAC.

* This will automatically enable `MBEDTLS_MD_LIGHT`.

AndrzejKurek · 2023-02-22T13:36:50Z

include/mbedtls/mbedtls_config.h

+ *
+ * Uncomment to enabled the "light" subsect of MD.
+ */
+#define MBEDTLS_MD_LIGHT


Shouldn't this be named MBEDTLS_MD_LIGHT_C?

I don't know, I think both make sense. There's no file md_light.c which is why I went without _C. OTOH, we have names with _C for things that don't correspond to the name of a file, like SHA224_C and SHA384_C - but there I think it's for uniformity with the other SHAxxx_C. Here it's a bit of a special case: unlike the various SHAxxx_C that are now independent of each other, here MD_LIGHT is explicitly a subset of MD_C and the later auto-enables the former.

Unless you and/or the other reviewer feel strongly about this, I'm inclined to keep the current name just to avoid changes.

The historical convention is _C for features that are implemented by a separate .c file. But that doesn't make a lot of sense since the fact that the implementation is a separate .c file is irrelevant to the programming interface. And so MBEDTLS_SHA224_C and MBEDTLS_SHA384_C departed from this historical convention because as an interface, it makes sense to make them similar to the other hashes.

PSA and TLS have been deviating from this convention since we refactored the code to split it into more files, but didn't make an API change just for that. {md,pk,cipher}_wrap.c also deviate: they don't have their own enablement macro because they aren't useful on their own.

Usually MBEDTLS_XXX_YYY is a subset of MBEDTLS_XXX_C which either has no effect if MBEDTLS_XXX_C is disabled, or requires MBEDTLS_XXX_C to be enabled. This is an interface consideration which MBEDTLS_MD_LIGHT would deviate from. We have at least one precedent: MBEDTLS_PSA_CRYPTO_CLIENT is a subset of MBEDTLS_PSA_CRYPTO_C.

When looking at md alone, MBEDTLS_MD_LIGHT seems right to me. When looking at the bigger picture, MBEDTLS_MD_LIGHT_C (considering it to be a separate module, which MBEDTLS_MD_C happens to require) is more uniform. But MBEDTLS_MD_LIGHT is essentially an internal symbol, so I don't think interface uniformity matters much.

In fact, should we even expose MBEDTLS_MD_LIGHT in mbedtls_config.h, as opposed to just automatically enabling it when other modules need it? Do we really want to promise this as a stable interface?

In fact, should we even expose MBEDTLS_MD_LIGHT in mbedtls_config.h, as opposed to just automatically enabling it when other modules need it? Do we really want to promise this as a stable interface?

Indeed initially I was planning on keeping it internal (that's what I did in the prototype). Then I thought it might be useful for people who just need hashing but not HMAC, they can save a bit of code size this way. (Actually you can use all of X.509, as well as AEAD ciphersuites for TLS 1.2 without HMAC, so that might be a number of users.)

Also, it feels a bit simpler for all build options to be in mbedtls_config.h (though that's already not the case for some options, like those related to driver testing).

Wdyt?

I'm a bit wary of introducing a new interface and declaring it as stable straight away. In this case, it's constrained to be a subset of the existing MD interface, and we've already pretty much reduced it to the minimum, so we're unlikely to make incompatible changes. But maybe we'll want to make some optimizations that break the API, e.g. changing the behavior on invalid arguments?

Ah, good point. Should we add a warning that it's experimental (the scope of the subset may vary, as well as the behaviour of the functions), or just make it internal for now to avoid the issue?

I'd prefer to make it internal to start with, if only to reduce the potential testing burden. Then once the dust has settled, maybe expose it — depending partly on whether the dust does settle quickly.

Sounds like a plan. Let's do that.

AndrzejKurek · 2023-02-22T13:38:13Z

include/mbedtls/md.h

+ *          feature macros:
+ *          - MBEDTLS_MD_C enables the whole module;
+ *          - MBEDTLS_MD_LIGHT enables only functions for hashing an accessing
+ *          some hash metadata; is it automatically set whenever MBEDTLS_MD_C


some hash metadata is pretty broad. How do we know what metadata is accessible, and what isn't?

AndrzejKurek · 2023-02-22T13:38:19Z

include/mbedtls/md.h

+ *          Availability of function in this modules is controled by two
+ *          feature macros:
+ *          - MBEDTLS_MD_C enables the whole module;
+ *          - MBEDTLS_MD_LIGHT enables only functions for hashing an accessing


Suggested change

* - MBEDTLS_MD_LIGHT enables only functions for hashing an accessing

* - MBEDTLS_MD_LIGHT enables only functions for hashing and accessing

AndrzejKurek · 2023-02-22T13:39:44Z

include/mbedtls/md.h

@@ -130,6 +139,7 @@ const int *mbedtls_md_list(void);
 * \return          NULL if the associated message-digest information is not found.
 */
 const mbedtls_md_info_t *mbedtls_md_info_from_string(const char *md_name);
+#endif /* MBEDTLS_MD_C */


Suggestion: Maybe we could group functions that need MBEDTLS_MD_C in one place?

AndrzejKurek · 2023-02-22T13:45:13Z

include/mbedtls/md.h

@@ -107,6 +115,7 @@ typedef struct mbedtls_md_context_t {
    void *MBEDTLS_PRIVATE(hmac_ctx);
 } mbedtls_md_context_t;



Unrelated to this line, but rather about this file:
Don't we want to guard the function declarations with MBEDTLS_MD_LIGHT?

We usually don't guard function declarations in header files with the corresponding implementation macro. For example, before this PR they were not guarded with MBEDTLS_MD_C. Technically, the guards I'm adding wouldn't be necessary, but I think they're helpful to document what's part of MD-light and what's not.

AndrzejKurek

Looks good, only minor points raised and questions asked.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

AndrzejKurek · 2023-02-23T13:29:13Z

tests/scripts/all.sh

    if [ "$DRIVER_ONLY" -eq 1 ]; then
        scripts/config.py unset MBEDTLS_MD_C
+        scripts/config.py unset MBEDTLS_MD_LIGHT


this will probably fail, since there's no MBEDTLS_MD_LIGHT define in the config now.

this will probably fail, since there's no MBEDTLS_MD_LIGHT define in the config now.

I tested it and config.py doesn't complain if there's a call to unset a define that doesn't exist, only set complains. Still, there's no MBEDTLS_MD_LIGHT in the config file anymore.

config.py unset FOO doesn't require FOO to exist (originally for backward compatibility with config.pl; I don't know if our test scripts currently depend on it).

If we want to ensure that a particular build does not enable MBEDTLS_MD_LIGHT, we can grep for an mbedtls_md_xxx symbol in libmbedcrypto.a.

Agreed, even if config.py happens not to complain, this command is confusing, I'll remove it. (I was planning to but apparently forgot at some point.)

If we want to ensure that a particular build does not enable MBEDTLS_MD_LIGHT, we can grep for an mbedtls_md_xxx symbol in libmbedcrypto.a.

Yes, that's what we're doing in this component (which is temporary, we're going to have MD_LIGHT enabled there at some point in the future).

AndrzejKurek

Probably one issue in the all.sh tests. It would be also great to group functions in md.c in a similar way to md.h to use one #if defined(MBEDTLS_MD_C) instead of separate ones for each function (but that's just a minor).

It turns out config.py wouldn't complain, but it's still confusing. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

valeriosetti

LGTM

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

daverodgman · 2023-03-03T14:29:03Z

Manually fixed a trivial merge conflict in build_info.h

valeriosetti

LGTM

mpg added 2 commits February 16, 2023 18:44

mpg added enhancement 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 size-s Estimated task size: small (~2d) labels Feb 16, 2023

mpg self-assigned this Feb 16, 2023

mpg added this to To Do in Roadmap Board for Mbed TLS via automation Feb 16, 2023

Disable MD-light in accel_hash_use_psa

9e04b5b

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

AndrzejKurek self-requested a review February 21, 2023 09:39

AndrzejKurek reviewed Feb 22, 2023

View reviewed changes

AndrzejKurek suggested changes Feb 22, 2023

View reviewed changes

Roadmap Board for Mbed TLS automation moved this from To Do to In Development Feb 22, 2023

mpg added 5 commits February 23, 2023 09:36

Make it clearer what's part of MD-light or not

82a4394

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Clarify relationship between MD_C and MD_LIGHT

f3953c8

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Add links for macros in doxygen documentation

39a4f42

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Fix a couple more typos

cacc0ea

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Make MBEDTLS_MD_LIGHT private for now.

0d41521

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg requested a review from AndrzejKurek February 23, 2023 12:16

AndrzejKurek reviewed Feb 23, 2023

View reviewed changes

AndrzejKurek suggested changes Feb 23, 2023

View reviewed changes

Remove config.py call on now-internal option

623c73b

It turns out config.py wouldn't complain, but it's still confusing. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

bensze01 moved this from In Development to Has Approval in Roadmap Board for Mbed TLS Feb 24, 2023

mpg mentioned this pull request Feb 24, 2023

Use MD (not low-level hash interface) in X.509 and TLS #7168

Merged

3 tasks

mpg added the priority-high High priority - will be reviewed soon label Feb 24, 2023

gilles-peskine-arm mentioned this pull request Mar 2, 2023

ssl_tls12_populate_transform optim #6072

Closed

1 task

valeriosetti previously approved these changes Mar 2, 2023

View reviewed changes

daverodgman added approved Design and code approved - may be waiting for CI or backports and removed needs-reviewer This PR needs someone to pick it up for review needs-review Every commit must be reviewed by at least two team members, labels Mar 3, 2023

Merge branch 'development' into md-light

45cef61

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

daverodgman dismissed stale reviews from valeriosetti and AndrzejKurek via 45cef61 March 3, 2023 14:28

Roadmap Board for Mbed TLS automation moved this from Has Approval to In Development Mar 3, 2023

daverodgman added the needs-ci Needs to pass CI tests label Mar 3, 2023

daverodgman requested review from AndrzejKurek and valeriosetti March 3, 2023 14:30

daverodgman added needs-review Every commit must be reviewed by at least two team members, and removed needs-ci Needs to pass CI tests approved Design and code approved - may be waiting for CI or backports labels Mar 3, 2023

valeriosetti approved these changes Mar 6, 2023

View reviewed changes

AndrzejKurek approved these changes Mar 6, 2023

View reviewed changes

Roadmap Board for Mbed TLS automation moved this from In Development to Has Approval Mar 6, 2023

mpg 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 Mar 6, 2023

mpg merged commit 228a30d into Mbed-TLS:development Mar 6, 2023

Roadmap Board for Mbed TLS automation moved this from Has Approval to Done Mar 6, 2023

mpg mentioned this pull request Mar 9, 2023

Implement MD dispatch to PSA #7242

Merged

3 tasks

daverodgman removed this from Done in Roadmap Board for Mbed TLS Apr 2, 2023

mpg mentioned this pull request Aug 11, 2023

[G2] Create Cipher light #8058

Closed

daverodgman mentioned this pull request Aug 30, 2023

Remove mbedtls_md_process and underlying functions #4657

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define "MD light" subset of MD #7120

Define "MD light" subset of MD #7120

mpg commented Feb 16, 2023 •

edited

Loading

mpg commented Feb 21, 2023

mpg commented Feb 21, 2023

AndrzejKurek Feb 22, 2023

AndrzejKurek Feb 22, 2023

AndrzejKurek Feb 22, 2023

mpg Feb 23, 2023

gilles-peskine-arm Feb 23, 2023

mpg Feb 23, 2023

gilles-peskine-arm Feb 23, 2023

mpg Feb 23, 2023

gilles-peskine-arm Feb 23, 2023

mpg Feb 23, 2023

AndrzejKurek Feb 22, 2023

AndrzejKurek Feb 22, 2023

AndrzejKurek Feb 22, 2023

AndrzejKurek Feb 22, 2023

mpg Feb 23, 2023

AndrzejKurek left a comment

AndrzejKurek Feb 23, 2023

AndrzejKurek Feb 23, 2023

gilles-peskine-arm Feb 23, 2023

mpg Feb 23, 2023

AndrzejKurek left a comment

valeriosetti left a comment

daverodgman commented Mar 3, 2023

valeriosetti left a comment

	* possible to enable this with MBEDTLS_MD_C if support for HMAC or extra
	* possible to enable this without MBEDTLS_MD_C if support for HMAC or extra

	* Enable the generic layer for message digest (hashing) and HMAC.
	* Enable the generic layer for message digest (hashing) and HMAC.
	* This will automatically enable `MBEDTLS_MD_LIGHT`.

	* - MBEDTLS_MD_LIGHT enables only functions for hashing an accessing
	* - MBEDTLS_MD_LIGHT enables only functions for hashing and accessing

		@@ -107,6 +115,7 @@ typedef struct mbedtls_md_context_t {
		void *MBEDTLS_PRIVATE(hmac_ctx);
		} mbedtls_md_context_t;

Define "MD light" subset of MD #7120

Define "MD light" subset of MD #7120

Conversation

mpg commented Feb 16, 2023 • edited Loading

Gatekeeper checklist

mpg commented Feb 21, 2023

mpg commented Feb 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrzejKurek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrzejKurek left a comment

Choose a reason for hiding this comment

valeriosetti left a comment

Choose a reason for hiding this comment

daverodgman commented Mar 3, 2023

valeriosetti left a comment

Choose a reason for hiding this comment

mpg commented Feb 16, 2023 •

edited

Loading