Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Driver-only ECC: goals and strategy #6839

Closed
mpg opened this issue Dec 23, 2022 · 16 comments
Closed

Driver-only ECC: goals and strategy #6839

mpg opened this issue Dec 23, 2022 · 16 comments
Assignees
Labels
size-s Estimated task size: small (~2d)

Comments

@mpg
Copy link
Contributor

mpg commented Dec 23, 2022

This issue is used as a place to document the goals of the driver-only ECC EPIC and discuss the general strategy towards those goals.

Broader context: see docs/architecture/psa-migration/strategy.md; this is about implementing goal 5 "compile out our implementation when a driver is available" for elliptic-curve cryptography (ECC).

Goals

Intermediate definition: "everything works" means that in a configuration with MBEDTLS_USE_PSA_CRYPTO enabled, we can build and use crypto, X.509 and TLS, and all the tests pass, with no more tests skipped than morally justified (see "restrictions" below).

  1. TL (top-level modules): When an accelerator for one or more ECC algorithm (ECDH, ECDSA, EC J-PAKE) is present, the built-in implementation of that/those algorithms (MBEDTLS_ECDH_C, MBEDTLS_ECDSA_C, MBEDTLS_ECJPAKE_C) can be removed, and everything works. (But this might still requires the curves used to be present on both sides.)
  2. ECPa (ECP arithmetic): When the only requested ECC algorithms are provided by accelerators, we can also remove support for curve arithmetic (ecp_mul() and associated) from ecp.c and everything works. Previously: CV (curves): In the situation above, everything works even when some of the curves used are only supported on the PSA side.
  3. ECPf (ECP full): When the only requested ECC algorithms are provided by accelerators, we can also remove ecp.c fully and everything works.
  4. BN (Bignum): When the only requested ECC algorithms are provided by accelerators, and neither RSA nor (FF)DH is requested*, we can also disable MBEDTLS_BIGNUM_C and everything works.

*Note: that is, not included in the build at all, the case where they're provided by accelerators is explicitly out of scope for this EPIC which is only about taking advantage of ECC accelerators to compile things out. If the same is desired for RSA at some point, it will be a separate EPIC.

Restrictions: the following are not expected to work in driver-only builds in the immediate future:

Testing

We'll have new components in all.sh similar to component_test_psa_crypto_config_accel_hash_use_psa() materializing the completion of each goal. However, it is desirable to minimize the total number of components in the end, so we may merge the components for two successive goals, but only if there is not loss of functionality between them. All components use libtestdriver1 and have names starting with test_psa_crypto_config_accel_. Currently the testing plan looks as follows:

  1. For TL, we currently have accel_ecdsa, accel_ecdh, accel_pake each accelerating only one of the top-level ECC modules. Each running only make test (no ssl-opt.sh and based on default config (no USE_PSA, so have to disable TLS key exchanges the depend on the accelerated module) (except PAKE but is that intentional?), with no testing of coverage parity. Testing for all 3 accelerated is merged with the ECPa test below. Testing with only 2 accelerated doesn't seem worth it. (Questions: is testing with only 1 accelerated actually worth it? Is testing without USE_PSA worth it?)
  2. For ECPa, we have a component accelerating ECDSA, ECDH and ECJPAKE, with !ECP_C && ECP_LIGHT (that is, ecp.o is not empty but does not contain mbedtls_ecp_mul). Starting from full config, running ssl-opt.sh and with coverage parity. Currently named accel_all_ec_algs_use_psa, might be renamed to accel_ecc_ecp_light_only. No loss of functionality compared to TL.
  3. For ECPf, we have a component accelerating ECDSA, ECDH and ECJPAKE, with !ECP_C && !ECP_LIGHT (that is, ecp.o is empty). Starting from full config, running ssl-opt.sh (eventually) and with coverage parity. Currently named accel_all_ec_algs_use_psa, might be renamed to accel_ecc_no_ecp_at_all. (In progress, only crypto including PK so far, X.509 and TLS up next.) Loss of functionality compare to previous component: compressed points, (SpecifiedECDomain until 7628), ECC key derivation.
  4. For BN we'll have a component accelerating ECDSA, ECDH and ECJPAKE, with !ECP_C && !ECP_LIGHT && !BIGNUM_C (that is, ecp.o and bignum.o both empty). Starting from full config, running ssl-opt.sh and with coverage parity. Planned name accel_ecc_no_bignum. Not started yet. Loss of functionality compare to previous component: DHM, RSA, things that depend on it.

Testing for coverage parity means there is a companion reference component with the same configuration except nothing's accelerated, and analyze_outcomes.py will check that all tests that passed in the reference configuration also do (as opposed to being skipped) in the driver-only configuration.

Intermediate goals

We can mainly progress along two axes:

  • Depth of the modules removed: this is how goals are defined above: first, remove the top-level module (ECDH, ECDSA, ECJPAKE) when accelerators are present, then remove the intermediate ECP, then the lowest-level module, Bignum. The obvious ordering here is top-down.

  • Breadth of the features working: the goals are stated with "everything works" and there can be any number of stepping stones towards "everything":

    • all crypto except possibly PK works (in particular, for goals 2 and 3, the PSA core does not depend on ECP or Bignum);
    • PK works (with USE_PSA_CRYPTO) - possible intermediate steps for parse / write / crypto operations;
    • X.509 works - possible intermediate steps for parse/write, CRL/CSR/CRT, but probably not needed;
    • TLS works - possible intermediate steps: 1.2 vs 1.3, individual key exchanges for 1.2 / key exchange modes for 1.3.

    Here the natural ordering is generally bottom-up / lateral movement in the upper layers: we need to do PK before X.509 before the TLS key exchanges that use a certificate, which we can then do in any order, but there are exceptions: for example we can do the TLS 1.2 ECJPAKE key exchange before we did X.509 or PK.

Strategy

There are two kinds of problems in defining the strategy:

  • Define reasonably-sized steps based on the intermediate goals, with explicit dependencies and a workable ordering (no dependency cycle).
  • For some steps, design a technical solution - for example, how to make PK parsing work without ECP or Bignum, see Study: PK including parse/write in ECP-free builds #6009.

High-level dependency matrix and possible refinements

It was noted in "Intermediate goals" above that there are two axes. This can be visualized as a matrix of intermediate goals:

PSA Crypto PK X.509 TLS
w/o top-level modules TL.PSA TL.PK TL.X509 TL.TLS
w/o ECP arithmetic ECPa.PSA ECPa.PK ECPa.X509 ECPa.TLS
w/o ECP (fully removed) ECPf.PSA ECPf.PK ECPf.X509 ECP.TLS
w/o Bignum BN.PSA BN.PK ECP.X509 BN.TLS

Generally speaking, on each line each cell depends on the cells on its left, and in each column, each cell depends on the cells above it. So, G1.PSA is the root of the dependency graph.

However, this matrix is only a simplified view and we can usefully sub-divide the first line (TL) and the last column (TLS).

Indeed TL can be divided in TL-ecdh, TL-ecdsa, TL-ecjpake; then full TL means not only being able to build and work with any one of these modules disabled, but all of them disabled simultaneously. As of end of March 2023:

  • TL is achieved, as evidenced by all.sh component test_psa_crypto_config_accel_ecdh;

Note that ECPa is a stepping stone towards ECPf, which already brings a significant code size benefit, as curve arithmetic is about 5kB.

Note: as part of ECPf (previously CV), we'll probably want to define new feature macros for curves, most probably in pk.h (see this comment).

Finally, TLS can be sub-divided, first by TLS version, then by key exchange type, giving intermediate goals such as TL-ecdh.TLS.12.ECDHE-PSK which means supporting the TLS 1.2 ECDHE-PSK key exchange when MBEDTLS_ECDH_C is disabled. Interestingly, this sub-goal only depends on TL-ecdh.PSA, not TL-ecdh.PK or TL-ecdh.X509, as neither PK nor X.509 is involved in this particular TLS 1.2 key exchange.

Note that some cells (or sub-divided goals) will require specific work beyond satisfying their dependencies, while it is expected that some cells won't. For example, as far as I can see now, ECPf.X509 should be reached automatically with no additional work once ECPf.PK and TL.X509 are done; similarly TL.X509 should follow from TL.PK with no additional work.

The following sub-sections make a more detailed review of the dependencies of each TLS 1.2 and 1.3 key exchange, then of each module in general.

Dependencies of the TLS 1.2 key exchanges

  • MBEDTLS_KEY_EXCHANGE_RSA: X.509, PK (RSA)
  • MBEDTLS_KEY_EXCHANGE_DHE_RSA: X.509, PK (RSA), DHM, Bignum (see below)
  • MBEDTLS_KEY_EXCHANGE_ECDHE_RSA: X.509, PK (RSA), ECDH
  • MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA: X.509, PK (ECDSA), ECDH
  • MBEDTLS_KEY_EXCHANGE_PSK: crypto core only (not even PK)
  • MBEDTLS_KEY_EXCHANGE_DHE_PSK: DHM, Bignum (see below)
  • MBEDTLS_KEY_EXCHANGE_RSA_PSK: X.509, PK (RSA)
  • MBEDTLS_KEY_EXCHANGE_ECDHE_PSK: ECDH (no PK, no X.509)
  • MBEDTLS_KEY_EXCHANGE_ECDH_RSA: ECDH, X.509, PK (RSA), ECP (see below)
  • MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA: ECDH, X.509, PK (ECDSA), ECP (see below)
  • MBEDTLS_KEY_EXCHANGE_ECJPAKE: ECJPAKE only (no PK, no X.509)

Among those relevant for the current work (that is, excluding RSA, DHE-RSA, PSK, DHE-PSK, RSA-PSK which have no ECC component), two stand out for having fewer dependencies, in particular not depending on PK: ECJPAKE and ECDHE-PSK.

Dependencies of the TLS 1.3 key exchange modes

  • KEY_EXCHANGE_MODE_PSK: crypto core only (not even PK)
  • KEY_EXCHANGE_MODE_EPHEMERAL: ECDH, X.509, PK (RSA or ECDSA)
  • KEY_EXCHANGE_MODE_PSK_EPHEMERAL: ECDH (no PK or X.509)

Pure PSK is hardly relevant for the current work, having no ECC component; among the other two, PSK-EPHEMERAL stands out as having fewer dependencies, in particular not depending on PK or X.509.

Direct dependencies of TLS on ECP or Bignum

In principle, TLS should only depend on high-level algorithms (ECDSA, ECDH, ECJPAKE, using the PSA APIs when MBEDTLS_USE_PSA_CRYPTO is defined) and modules (X.509, PK). It should not make direct use of low-level modules such as ECP and Bignum.

However it currently directly calls the following low-level functions:

mbedtls_debug_print_mpi
    mbedtls_debug_print_ecp
        debug_print_pk
        mbedtls_debug_printf_ecdh_internal
    debug_print_pk
        mbedtls_debug_print_crt
    mbedtls_debug_printf_ecdh_internal
        mbedtls_debug_printf_ecdh

Direct dependencies of X.509 on ECP or Bignum

In principle, X.509 should only depend on PK, and not directly call any function from ECP or Bignum.

It indeed does not make any direct call to ECP, however it optionally makes direct calls to Bignum (or associated)

Dependencies of PK

Crypto computations themselves should be fine, but parsing & writing not so much - this has been addressed in #7073 and #7074 and #7682. See also #7570 for a design rationale.

Note: after #7682, if my investigations with unifdef are correct, PK no longer has any direct dependency on Bignum when ECP_C is removed (and support for ECC comes from PSA).

Dependencies of PSA crypto

As of Mbed TLS 3.3, in builds where both ECDH and ECDSA are accelerated (edit: actually when KEY_TYPE_ECC_KEY_PAIR and KEY_TYPE_ECC_PUBLIC_KEY are accelerated) we're getting failures in test_suite_psa_crypto: basically, most all cases of derive_key_type and derive_key using ECC fail while for some reason cases using derive_key_exercise pass (edit: that was a false negative which has been fixed by #6873).

This is a consequence of the fact that we haven't fully implemented driver support for cooked key derivation yet. The current plan is to accept that as a feature gap in accelerated ECC builds, until driver support for cooked key derivation is added. Note: this has been partially addressed in #7192 - the partial fix means it won't be a problem for TL or ECPa, but starting from ECP we'll have to accept a feature gap there.

@mpg mpg added the size-s Estimated task size: small (~2d) label Dec 23, 2022
@mpg mpg self-assigned this Dec 23, 2022
@gilles-peskine-arm
Copy link
Contributor

Looks like a plan.

One hurdle I'm aware of is calculating buffer sizes, where include/psa/crypto_sizes relies on MBEDTLS_ECP_DP_xxx_ENABLED macros. (And is wasteful for Montgomery-only builds.) Testing that would require a build where the largest curve is accelerated and has different cases for Curve25519 vs Curve448 (and Edwards when we implement them) vs Weierstrass curves (PSA does not support secp244k1 so we don't have to worry about its special case with larger public keys).

Another potential hurdle is verifying a signature before entropy is available. As this is a new feature, it can wait until we get around to implementing partial initialization.

There is a known gap in PSA that compressed points are not supported in PSA, and can't be without both an API extension and extra requirements on drivers.

@mpg
Copy link
Contributor Author

mpg commented Dec 27, 2022

As of Mbed TLS 3.3, in builds where both ECDH and ECDSA are accelerated (basically merging test_psa_crypto_config_accel_ecdsa and test_psa_crypto_config_accel_ecdh from all.sh) we're getting failures in test_suite_psa_crypto: basically, most cases of derive_key_type and derive_key using ECC fail, while for some reason cases using derive_key_exercise pass.

TODO: investigate. This sounds like a consequence of the fact that we haven't implemented driver support for key derivation yet, however it's surprising that cases use derive_key_exercise() pass, so this might be something else.

@gilles-peskine-arm @ronald-cron-arm Do you happen to have any insight on this? Note: I'm not suggesting you spend time investigating this, just wanted to check if that rings a bell before I dig further.

For reference, here's the component I added, and I'm attaching the output of test_suite_psa_crypto:
test-out.txt.

component_test_psa_crypto_config_accel_ecc () {
    msg "test: MBEDTLS_PSA_CRYPTO_CONFIG with accelerated ECC"

    # Configure and build the test driver library
    # --------------------------------------------

    # Disable ALG_STREAM_CIPHER and ALG_ECB_NO_PADDING to avoid having
    # partial support for cipher operations in the driver test library.
    scripts/config.py -f include/psa/crypto_config.h unset PSA_WANT_ALG_STREAM_CIPHER
    scripts/config.py -f include/psa/crypto_config.h unset PSA_WANT_ALG_ECB_NO_PADDING

    # SHA384 needed for some ECDSA signature tests.
    scripts/config.py -f tests/include/test/drivers/config_test_driver.h set MBEDTLS_SHA384_C
    scripts/config.py -f tests/include/test/drivers/config_test_driver.h set MBEDTLS_SHA512_C

    # TODO: add EC J-PAKE once driver dispatch is done
    loc_accel_list="ALG_ECDH ALG_ECDSA ALG_DETERMINISTIC_ECDSA KEY_TYPE_ECC_KEY_PAIR KEY_TYPE_ECC_PUBLIC_KEY"
    loc_accel_flags=$( echo "$loc_accel_list" | sed 's/[^ ]* */-DLIBTESTDRIVER1_MBEDTLS_PSA_ACCEL_&/g' )
    make -C tests libtestdriver1.a CFLAGS="$ASAN_CFLAGS $loc_accel_flags" LDFLAGS="$ASAN_CFLAGS"

    # Restore test driver base configuration
    scripts/config.py -f tests/include/test/drivers/config_test_driver.h unset MBEDTLS_SHA384_C
    scripts/config.py -f tests/include/test/drivers/config_test_driver.h unset MBEDTLS_SHA512_C

    # Configure and build the main libraries
    # ---------------------------------------

    # start with default + driver support
    scripts/config.py set MBEDTLS_PSA_CRYPTO_DRIVERS
    scripts/config.py set MBEDTLS_PSA_CRYPTO_CONFIG

    # disable modules for which we have drivers
    scripts/config.py unset MBEDTLS_ECDSA_C
    scripts/config.py unset MBEDTLS_ECDH_C
    scripts/config.py unset MBEDTLS_ECJPAKE_C

    # dependencies
    scripts/config.py unset MBEDTLS_SSL_PROTO_TLS1_3
    scripts/config.py unset MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED
    scripts/config.py unset MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED
    scripts/config.py unset MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED
    scripts/config.py unset MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED
    scripts/config.py unset MBEDTLS_KEY_EXCHANGE_ECDHE_PSK_ENABLED

    # build and link with test drivers
    loc_accel_flags="$loc_accel_flags $( echo "$loc_accel_list" | sed 's/[^ ]* */-DMBEDTLS_PSA_ACCEL_&/g' )"
    make CFLAGS="$ASAN_CFLAGS -O -Werror -I../tests/include -I../tests -I../../tests -DPSA_CRYPTO_DRIVER_TEST -DMBEDTLS_TEST_LIBTESTDRIVER1 $loc_accel_flags" LDFLAGS="-ltestdriver1 $ASAN_CFLAGS"

    # make sure these were not auto-re-enabled by accident
    not grep mbedtls_ecdsa_ library/ecdsa.o
    not grep mbedtls_ecdh_ library/ecdh.o

    # Run the tests
    # -------------

    msg "test: MBEDTLS_PSA_CRYPTO_CONFIG with accelerated ECC"
    make test
}

@gilles-peskine-arm
Copy link
Contributor

Do you happen to have any insight on this?

No, this looks weird to me. What I'd do next:

  • Look closely at the arguments passed to psa_key_derivation_output_key in passing vs failing cases. E.g. some weirdness with attributes mattering when they shouldn't, or not being checked properly?
  • Confirm that the inputs to the derivation are the same in should-pass cases.
  • Look with a debugger to see what is raising the NOT_SUPPORTED error.

@mpg
Copy link
Contributor Author

mpg commented Jan 4, 2023

It's actually quite simple: the cases that fail are exactly those that end up calling psa_generate_derived_key_internal(). The reason it return NOT_SUPPORTED is simple as well, it's structured like:

#if defined(MBEDTLS_PSA_BUILTIN_KEY_TYPE_ECC_KEY_PAIR) || \
    defined(MBEDTLS_PSA_BUILTIN_KEY_TYPE_ECC_PUBLIC_KEY) || \
    defined(MBEDTLS_PSA_BUILTIN_ALG_ECDSA) || \
    defined(MBEDTLS_PSA_BUILTIN_ALG_DETERMINISTIC_ECDSA) || \
    defined(MBEDTLS_PSA_BUILTIN_ALG_ECDH)
    if( PSA_KEY_TYPE_IS_ECC( slot->attr.type ) )
    {
        // [do it]
    } else
#endif
    // [handle other types then eventually ]
    return( PSA_ERROR_NOT_SUPPORTED );

So, I think the problem is indeed that we are lacking driver support for cooked key derivation: the is no point in the call chain between the public function psa_key_derivation_output_key() and the code above that may call drivers. So, when we have neither built-in ECDH nor built-in ECDSA, we run straight into returning NOT_SUPPORTED.

So, I think our ability to accelerate both ECDH and ECDSA at the same time depends on implementing driver support for cooked key derivation.

Also, the part that was most surprising to me and initially made me doubt that it was about this, is that tests using derive_key_exercise() are passing. I now believe this is a bug in this test function, which is silently testing nothing: on line 7949 it should have a ! like the other function have, see lines 8017, 8029 and 8098.

Unless someone beats me to it, I'll raise a PR (if the simple fix just works without uncovering other issues) or create an issue about it tomorrow.

@mpg
Copy link
Contributor Author

mpg commented Jan 4, 2023

Unless someone beats me to it, I'll raise a PR (if the simple fix just works without uncovering other issues) or create an issue about it tomorrow.

#6873 - let's see what the CI says

@mpg
Copy link
Contributor Author

mpg commented Jan 12, 2023

So, after a first read of of #5451 and looking at subsequent issues, it looks to me that what's required before we can accelerate all ECC algorithms in a single build and have the existing tests for key derivation pass, would be #5489 - including the ability for libtestdriver1 to implement the derive_key entry point when it accelerates PSA_KEY_TYPE_ECC_KEY_PAIR.

So, basically the driver-only ECC EPIC has an external dependency on some of the "drivers for KDFs" work. Specifically, the sub-goal TL.PSA (as opposed to just TL-ecdh.PSA + TL-ecdsa.PSA + TL-ecjpake.PSA) depends on #5489 (and perhaps more depending on how we interpret the scope of that issue).

@gilles-peskine-arm
Copy link
Contributor

Deriving an ECC key from a KDF output is a PSA-only functionality that is not used by any higher-level code inside the library. It should not be part of the driver-only ECC epic, because that would block a useful feature with high demand (do TLS or other network protocol with accelerated ECC and save the code size from ecp.o and friends) on a feature with low demand (derive an ECC key pair deterministically with accelerated ECC and save the code size from ecp.o and friends) and a high implementation cost.

I think this means that when the driver-only ECC epic is complete, there will be a feature gap: ECC key derivation won't work with accelerated-only ECC until ECC key derivation works with accelerated ECC.

@gilles-peskine-arm
Copy link
Contributor

In the driver specification language, a driver can declare that it supports e.g. ECDH and ECDSA but not key derivation. The PSA_WANT/MBEDTLS_PSA_ACCEL language is not expressive enough. Maybe it should be?

Similarly the PSA_WANT language has no way to say “RSA private key support without RSA key generation” (i.e. the equivalent of disabling MBEDTLS_GENPRIME). That has been low-importance so far, but the solution is likely to be the same for both.

@mpg
Copy link
Contributor Author

mpg commented Mar 13, 2023

@mpg wrote:

As of Mbed TLS 3.3, in builds where both ECDH and ECDSA are accelerated (basically merging test_psa_crypto_config_accel_ecdsa and test_psa_crypto_config_accel_ecdh from all.sh) we're getting failures in test_suite_psa_crypto: basically, most cases of derive_key_type and derive_key using ECC fail, while for some reason cases using derive_key_exercise pass.

Ah, that was not accurate. What triggers the failures is not when both ECDH and ECDSA (and now ECJPAKE) are accelerated, what triggers the problem is when on top of that we try to accelerate KEY_TYPE_ECC_KEY_PAIR and KEY_TYPE_ECC_PUBLIC_KEY. The rest of the analysis still stands, but this is good news in terms of planning in that this will be an issue for intermediate goal ECP, but it's not an issue for the first two intermediate goals, contrary to what I've been thinking so far.

@gilles-peskine-arm wrote:

Deriving an ECC key from a KDF output is a PSA-only functionality that is not used by any higher-level code inside the library. It should not be part of the driver-only ECC epic, because that would block a useful feature with high demand (do TLS or other network protocol with accelerated ECC and save the code size from ecp.o and friends) on a feature with low demand (derive an ECC key pair deterministically with accelerated ECC and save the code size from ecp.o and friends) and a high implementation cost.

I think this means that when the driver-only ECC epic is complete, there will be a feature gap: ECC key derivation won't work with accelerated-only ECC until ECC key derivation works with accelerated ECC.

Agreed. I think this can easily be achieved by declaring that the tests for ECC key derivation depend on MBEDTLS_PSA_BUILTIN_KEY_TYPE_ECC_KEY_PAIR. Then we can remove that dependency & feature gap later as part of the EPICs on drivers for cooked key derivation.

@mpg
Copy link
Contributor Author

mpg commented Apr 11, 2023

@gilles-peskine-arm @valeriosetti I've updating the list of restrictions and indicating for each one at which level they apply:

It turns out there are three restrictions that appear when disabling ECP_LIGHT (that is, fully disabling ecp.c) that are not present when keeping ECP_LIGHT (that is, only removing part of ecp.c). This makes me question my initial decision of keeping ECP_LIGHT internal, as there might be people who find one of the restrictions unacceptable for their use case, but still want to save code size by removing part of ecp.c (the difference is about 4.5k on M0, so quite significant).

So, now I'm tempted to make ECP_LIGHT public, by having it in mbedtls_config.h with some documentation, and a warning that it is experimental and subject to change in terms of what exactly it enables.

Wdyt?

@mpg
Copy link
Contributor Author

mpg commented Apr 11, 2023

Note: as @valeriosetti mentions here, a middle ground could be to not document it publicly in mbedtls_config.h, but only in the ChangeLog.

@gilles-peskine-arm
Copy link
Contributor

I'm reluctant to make ECP_LIGHT a public interface because it's very likely that its user-facing semantics (i.e. what you can't do without it — as opposed to which bits of the code base it includes) will change over time. Each of the limitations you list will be resolved independently.

I'd prefer to introduce feature-specific configuration options (where they don't already exist) and automatically enable ECP_LIGHT based on those. So based on your list:

  • MBEDTLS_ECP_RESTARTABLE && MBEDTLS_X509_CRT_C implies MBEDTLS_ECP_LIGHT
  • new option for compressed points, implied by MBEDTLS_ECP_C, implies MBEDTLS_ECP_LIGHT, default off (but would be implicitly enabled by default due to ECP_C)
  • MBEDTLS_PK_PARSE_EC_EXTENDED implies MBEDTLS_ECP_LIGHT
  • For derivation of ECP keys, I'm more and more thinking we need an official PSA_WANT_ symbol for that (and similarly for RSA key generation)

@valeriosetti
Copy link
Contributor

@gilles-peskine-arm thanks for your feedback!
After a quick discussion with @mpg we decided to proceed more gradually:

  • keep the ECP_LIGHT symbol private for now and try to merge PR Define (private) "light" subset of ECP #7410 without adding too many changes because on one hand it would make the PR a little mess and on the other one that PR is blocking further work
  • once this is done we can think about new symbols and how to add auto-enable rules. Doing this in a separate PR also avoids to create new symbols in a rush without a proper strategy

@mpg
Copy link
Contributor Author

mpg commented Apr 13, 2023

@gilles-peskine-arm Thanks for your input! One minor point first:

MBEDTLS_ECP_RESTARTABLE && MBEDTLS_X509_CRT_C implies MBEDTLS_ECP_LIGHT

That is actually unrelated to ECP_LIGHT, what we'd need for X.509 is ECDSA_C, and for TLS ECDH_C && ECDSA_C. It's only the other three points in the list that relate to ECP_LIGHT. (That what I meant by "at TL and beyond" vs "at ECPf and beyond", which I agree is not clear outside the context of the issue description.)

Other than that, I tend to agree. Initially, I wanted ECP_LIGHT to be private and auto-enabled when needed, and actually hoped it would be very short-lived and could be removed very soon as we make it possible to remove all of ecp.c. Then I started noticing those three restrictions, so I thought some users might still want to remove most but not quite all of ecp.c for some time until we lift those restrictions.

Then I noticed we didn't have the proper feature macros for users to express "I want to read compress points" or "I want to derive ECC keys", so I thought instead of adding two public options for that, we could add just one, namely ECP_LIGHT, as we don't want too many options, and 1 is less than 2 so it's better, right? But now that you mention it, adding 2 options with well-defined, stable semantics is actually much better than adding a single option with ill-defined semantics that will change over time.

Regarding compressed points, I'm thinking MBEDTLS_PK_PARSE_EC_COMPRESSED for uniformity with the existing MBEDTLS_PK_PARSE_EC_EXTENDED.

Regarding PSA derivation of ECP keys, what to you think would be an appropriate name / granularity? I was thinking PSA_WANT_DERIVE_KEY_TYPE_xxx for each of the existing non-raw key types? (If we wanted optimal dependency management, we'd need to distinguish by curve, as I think Short Weierstrass curves need ECP_LIGHT while Montgomery only need BIGNUM_C. But I think that would make the scheme overly complicated for little gain, so it's probably better to keep only key type-level granularity).

Wdyt?

@gilles-peskine-arm
Copy link
Contributor

About new PSA_WANT symbols: I've created #7439 to discuss this.

@mpg
Copy link
Contributor Author

mpg commented Sep 29, 2023

Closing as the work has been completed.

@mpg mpg closed this as completed Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size-s Estimated task size: small (~2d)
Projects
None yet
Development

No branches or pull requests

3 participants