Skip to content
New issue

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

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

Already on GitHub? # to your account

Which sample programs to keep in 1.0/4.0 #9904

Open
gilles-peskine-arm opened this issue Jan 14, 2025 · 7 comments
Open

Which sample programs to keep in 1.0/4.0 #9904

gilles-peskine-arm opened this issue Jan 14, 2025 · 7 comments
Assignees
Labels
size-s Estimated task size: small (~2d)

Comments

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Jan 14, 2025

For each program under /programs in Mbed TLS 3.6, should we remove, keep, adapt or rewrite for TF-PSA-Crypto 1.0 or Mbed TLS 4.0?

This is an investigation task. The expected outcome is a list/table covering every program, with the desired outcome for each program and a justification (which I expect will usually be just a short sentence fragment). The outcomes I can think of:

  • Keep as is, e.g. x509/* — no work to be scheduled
  • Remove (because it is not really useful for anything except demonstrating the legacy crypto API), e.g. crypt_and_hashwe'll remove those in batch
  • Replace (because it has a useful purpose, but the current implementation relies heavily on the legacy crypto API), e.g. cipher_aead_demo (for which the rewriting has already been done) — we'll remove those and file issues to replace them after 1.0/4.0
  • Keep, but will need minor adjustments due to rare use of APIs that are now private, e.g. programs/fuzz/fuzz_*key — file an issue to do the minor adjustments, which will be considered for 1.0/4.0
  • Keep, but will need major adjustments due to major use of APIs that are now private, e.g. benchmark — file an issue for the rewrite, which will happen after 1.0/4.0.

Definition of done for this task: the list/table, and file issues where we identify that work needs to be done.

Reasons to keep a program:

  • If it's useful as a sample of API usage.
  • If it's useful as a test.
  • If its functionality is useful, e.g. programs/x509/* are meant to be usable to run a simple CA and work with a simple CA, and some of programs/pk/* are complementary with that.
@gilles-peskine-arm
Copy link
Contributor Author

For pk programs, it would make sense to also evaluate whether those programs should stay in Mbed TLS or move to TF-PSA-Crypto. PK is a TF-PSA-Crypto API, but if the main usefulness of the programs is to create/parse keys used by the X.509 programs, then it could make sense to keep the programs in mbedtls.

@davidhorstmann-arm davidhorstmann-arm self-assigned this Jan 23, 2025
@davidhorstmann-arm
Copy link
Contributor

davidhorstmann-arm commented Jan 30, 2025

Category Program Fate Justification Work needed (if any)
x509 load_roots.c Keep No legacy API usage
x509 req_app.c Keep No legacy API usage
x509 cert_app.c Tweak Useful for cert management Replace RNG
x509 crl_app.c Keep No legacy API usage
x509 cert_req.c Tweak Useful for generating CSRs Replace RNG
x509 cert_write.c Tweak Useful for generating certs Replace RNG
pkey rsa_genkey.c Delete Demonstrates legacy API, nonstandard key format
pkey pk_verify.c Keep PSA-ready, demonstrates PK which will be in 4.0
pkey gen_key.c Replace Needed to generate keys for X.509 programs Rewrite for PSA API
pkey pk_decrypt.c Delete PK decryption will be removed in 4.0
pkey dh_genprime.c Delete Uses low level bignum API, demonstrates custom raw network DH protocol
pkey rsa_verify.c Delete Uses legacy API, nonstandard key format
pkey mpi_demo.c Delete Demonstrates the bignum API
pkey rsa_verify_pss.c Tweak Almost PSA-ready Remove minor use of rsa and pk_rsa
pkey rsa_decrypt.c Delete Uses legacy API, nonstandard key format
pkey key_app.c Delete Uses legacy API including bignum
pkey dh_server.c Delete Uses low level bignum API, demonstrates custom raw network DH protocol
pkey ecdh_curve25519.c Delete Uses legacy API
pkey pk_encrypt.c Delete PK encryption will be removed in 4.0
pkey pk_sign.c Tweak Almost PSA-ready Replace RNG
pkey rsa_sign.c Delete Uses legacy API, nonstandard key format
pkey key_app_writer.c Delete Uses legacy API including bignum
pkey rsa_sign_pss.c Tweak Almost PSA-ready Remove minor use of rsa and pk_rsa, replace RNG
pkey dh_client.c Delete Uses low-level bignum API, demonstrates custom raw network DH protocol
pkey ecdsa.c Delete Uses legacy API
pkey rsa_encrypt.c Delete Uses legacy API, nonstandard key format
test/cmake_package_install cmake_package_install.c Keep No legacy API used
test query_included_headers.c Keep No legacy API used
test/cmake_package cmake_package.c Keep No legacy API used
test benchmark.c Replace Benchmarking is important, currently uses the legacy API Firstly, move to TF-PSA-Crypto. When possible, perhaps gradually, rewrite to use the PSA API.
test query_compile_time_config.c Keep No legacy API used
test metatest.c Keep No legacy API used
test/cmake_subproject cmake_subproject.c Keep No legacy API used
test zeroize.c Keep No legacy API used
test selftest.c Delete Uses legacy API, we can rely on existing unit tests for coverage
test dlopen.c Keep No legacy API used
test udp_proxy.c Keep No legacy API used
- wince_main.c Delete Difficult to test and no longer relevant
aes crypt_and_hash.c Delete Legacy APIs and an ad-hoc AEAD
random gen_random_ctr_drbg.c Delete Legacy API usage
random gen_entropy.c Delete Uses legacy APIs, in future entropy will be supplied by drivers
fuzz fuzz_x509crl.c Keep Used in fuzzing
fuzz fuzz_pkcs7.c Keep Used in fuzzing
fuzz fuzz_dtlsclient.c Tweak Used in fuzzing Replace RNG
fuzz fuzz_privkey.c Tweak Used in fuzzing Replace legacy crypto with PSA, replace RNG
fuzz onefile.c Keep Dependency, no legacy API usage
fuzz fuzz_server.c Tweak Used in fuzzing Replace RNG
fuzz fuzz_dtlsserver.c Tweak Used in fuzzing Replace RNG
fuzz fuzz_client.c Tweak Used in fuzzing Replace RNG
fuzz fuzz_x509csr.c Keep No legacy API usage
fuzz common.c Tweak Used in fuzzing Replace RNG
fuzz fuzz_pubkey.c Tweak Used in fuzzing Replace legacy crypto with PSA
fuzz fuzz_x509crt.c Keep No legacy crypto
hash md_hmac_demo.c Delete Uses legacy API, exists in 3.6 to demonstrate PSA migration Remove references to this program from its PSA counterpart hmac_demo.c
hash hello.c Delete Uses legacy API, uses MD5 which is a poor example of a hash function
hash generic_sum.c Delete Uses legacy API, superseded by psa_hash.c
cipher cipher_aead_demo.c Delete Uses legacy API, exists in 3.6 to demonstrate PSA migration Remove references to this program from its PSA counterpart aead_demo.c
util pem2der.c Keep Useful tool, PSA ready
util strerror.c Keep Useful tool, PSA ready
ssl ssl_client2.c Tweak Useful for CI / testing Replace restartable ECP set_max_ops with PSA (or remove the option)
ssl ssl_server.c Tweak Useful for CI / testing Replace RNG
ssl dtls_server.c Tweak Useful for CI / testing Replace RNG
ssl ssl_mail_client.c Tweak No good reason to delete, only uses legacy API for RNG Replace RNG
ssl ssl_client1.c Tweak Useful for CI / testing Replace RNG
ssl ssl_context_info.c Tweak Useful for CI / testing Replace use of cipher with PSA
ssl mini_client.c Tweak Useful for testing (used in memory testing) Replace RNG
ssl ssl_test_lib.c Tweak Dependency of other SSL programs Replace RNG
ssl ssl_test_common_source.c Keep No legacy API, dependency of other programs
ssl dtls_client.c Tweak Useful for CI / testing Replace RNG
ssl ssl_server2.c Tweak Useful for CI / testing Remove option to supply DHM parameters, change ASYNC_OP_DECRYPT to use the PSA API
ssl ssl_pthread_server.c Tweak Useful for CI / testing Replace RNG
ssl ssl_fork_server.c Tweak Useful for CI / testing Replace RNG

@gilles-peskine-arm
Copy link
Contributor Author

@davidhorstmann-arm Thanks for the analysis!

I mostly agree with your proposal, with a couple of exceptions.

gen_key

One thing I forgot to mention (sorry!): we have a demo script that illustrates a typical flow for creating certificates with a CA: cert_write_demo.sh. It's in #2698 which didn't reach 2 approvals and has bitrotted, but I think it's still relevant. It uses the following programs:

gen_key
cert_write
cert_app
cert_req
req_app

So I think we should rewrite gen_key for PSA rather than delete it. We only need the same basic functionality, not e.g. the hex dumps.

Benchmark

We can keep the benchmark as a benchmark until we rewrite it.
We might rewrite it gradually.

gen_random_ctr_drbg

No point in a demo just for that, because in PSA it's a single function, unlike legacy.

gen_entropy

There will not be a new entropy API, only a driver interface. So nothing to demo. A demo for the driver entry points would be useful but would look completely different.

md_hmac_demo, cipher_aead_demo

Minor task: remove references to those programs from their PSA counterparts.

ssl_mail_client

Why no reason to keep? I don't think that demo is particularly useful, but on the other hand I don't see how it's affected by the evolution of the library.

@davidhorstmann-arm
Copy link
Contributor

davidhorstmann-arm commented Feb 6, 2025

I've made the updates to the table above. A couple of notes:

Re ssl_mail_client.c: I couldn't think of a reason to justify keeping it but I'm happy for us to do that, it just needs random generation replacing as with many of the SSL programs.

Re benchmark.c: it seems fine to keep it as-is for now, but I've added a note that we should move it to TF-PSA-Crypto so that it can continue using the legacy API without problems.

@mpg
Copy link
Contributor

mpg commented Feb 20, 2025

I just went over the table (just the table, I didn't check the programs themselves and assumed that assertions like "does not use legacy APIs" are correct) and it all looks good to me. Just two thoughts:

fuzz/fuzz_pubkey.c - Tweak - Used in fuzzing - Replace legacy crypto with PSA

Just to be sure: I assume you mean keep mbedtls_pk_parse_public_key (not replace it with psa_import()), as is supports more formats.

Replace restartable ECP set_max_ops with PSA (or remove the option)

I think for now it will have to be "remove the option", as the PSA interruptible API is not used by TLS yet.

@gilles-peskine-arm
Copy link
Contributor Author

Replace restartable ECP set_max_ops with PSA (or remove the option)

I think for now it will have to be "remove the option", as the PSA interruptible API is not used by TLS yet.

The PSA equivalent is psa_interruptible_set_max_ops(), which just calls mbedtls_ecp_set_max_ops(). So it doesn't matter which crypto API is used in TLS: both APIs use the same global variable.

@mpg
Copy link
Contributor

mpg commented Feb 21, 2025

So it doesn't matter which crypto API is used in TLS: both APIs use the same global variable.

Ah, good to know, thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
size-s Estimated task size: small (~2d)
Projects
Status: DIs for 1.0 MVP
Development

No branches or pull requests

3 participants