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

Make legacy crypto APIs internal #8663

Open
mpg opened this issue Dec 28, 2023 · 5 comments
Open

Make legacy crypto APIs internal #8663

mpg opened this issue Dec 28, 2023 · 5 comments
Labels
api-break This issue/PR breaks the API and must wait for a new major version component-crypto Crypto primitives and low-level interfaces deprecation needs deprecation announcement size-m Estimated task size: medium (~1w)

Comments

@mpg
Copy link
Contributor

mpg commented Dec 28, 2023

As part of making PSA Crypto the main crypto API in 4.0, we're making (most of) the legacy crypto API internal (that is, headers would be visible to other crypto modules but not to applications).

This issue is an attempt at summarizing the consensus that was reached last time we discussed this.

Categorized list of crypto headers

Cipher & related:

  • internalize: aes.h, aria.h, block_cipher.h, camellia.h, ccm.h, chacha20.h, chachapoly.h, cmac.h, des.h, gcm.h, poly1305.h,
  • under discussion: cipher.h Remove cipher.h in 4.0 #8451
  • keep for now: nist_kw.h (note: includes cipher.h! If we remove cipher.h we'll need to change types in nist_kw.h - only mbedtls_cipher_id_t is used AFAICS).

Hashes & related:

Randomness:

Asymmetric crypto:

Supporting modules:

  • pkcs12.h, pkcs5.h -> internalize (used by PK parse and soon extended PSA key import)
  • base64.h -> internalize (used by PEM only)
  • pem.h -> semi-internal? (used by X.509, PK parse/write and soon extended PSA key import/export)
  • asn1.h, asn1write.h -> semi-internal? (used by X.509, PK parse/write and soon extended PSA key import/export, but also basic RSA import/export)
  • oid.h -> split and redesign, partly internalized: Split numeric string conversions out of the OID module #9379, Redesign OID API for smaller code size #9380
  • platform.h, platform_time.h, platform_util.h, threading.h -> keep?
  • constant_time.h -> ?? (only one function now)
  • build_info.h error.h, version.h, -> keep but split crypto vs non-crypto I guess?
  • psa_util.h -> keep if it helps the transition, or internalize
  • private_access.h -> keep
  • memory_buffer_alloc.h -> ??
  • timing.h -> ??

Notes

Above, by "crypto headers" I mean all headers that are part of libmbedcrypto - that includes things that are does not actually provide crypto functionality, like platform support.

There seems to be a need to a special "semi-internal" status for header that should be visible to both crypto modules and X.509, but perhaps not to applications (pem.h, asn1.h, asn1write.h).

This issue it mainly to record what we've discussed, and host further discussion if needed. For execution, we probably want to split this in a series of task, for example per area as listed above.

There will probably be interactions with splitting out PSA crypto to its own repo too.

@mpg mpg added the size-l Estimated task size: large (2w+) label Dec 28, 2023
@mpg
Copy link
Contributor Author

mpg commented Dec 28, 2023

Ping @dave-rodgman - I think you took notes during the meeting, please correct my list if it's not in line with those notes.

@mpg mpg added component-crypto Crypto primitives and low-level interfaces api-break This issue/PR breaks the API and must wait for a new major version deprecation needs deprecation announcement labels Dec 28, 2023
@mpg
Copy link
Contributor Author

mpg commented Jan 25, 2024

Note: some legacy modules, when made internal, can actually be removed as well - when the PSA implementation of that feature doesn't actually use the legacy module.

For example for FFDH, when making dhm.h internal, we can remove dhm.c entirely, as psa_crypto_ffdh.c does not depend on it. We can also move the definition of the constants for the domain parameters to psa_crypto_ffdh.c (currently taken from dhm.h) and then remove dhm.h as well.

Similar considerations might apply to other modules, I've not tried making a complete list yet.

(Note: currently psa_crypto_ecp.c calls functions from ecdh.c but I don't think there's a good reason for that, IMO it should be calling mbedtls_ecp_mul() directly instead.)

@mpg
Copy link
Contributor Author

mpg commented Jan 25, 2024

(Note: currently psa_crypto_ecp.c calls functions from ecdh.c but I don't think there's a good reason for that, IMO it should be calling mbedtls_ecp_mul() directly instead.)

Ah, actually there's a reason, it's that Everest dispatch is implemented in ecdh.c not ecp.c. IMO that's not a good reason in the long term: Everest should be a PSA driver like everything else instead of having its own special dispatch mechanism in ecdh.c.

@yanesca
Copy link
Contributor

yanesca commented Sep 20, 2024

Almost everything here (that is not covered by a separate issue) will be done as part of the repo-split. What remains here to do is to document it clearly. The lack of interface stability must be clearly explained and the affected APIs marked as such.

Open questions:

  • Do we want internal/non-stable APIs to show up on readthedocs?
  • Do we want to mark individual functions as internal or header placement/comment in the header should be enough?

@yanesca yanesca added size-m Estimated task size: medium (~1w) and removed size-l Estimated task size: large (2w+) labels Sep 20, 2024
@gilles-peskine-arm
Copy link
Contributor

Do we want internal/non-stable APIs to show up on readthedocs?

Definitely not.

Do we want to mark individual functions as internal or header placement/comment in the header should be enough?

Separate headers. In the past we've gotten pretty loud complaints when we moved a function declaration from a public header with a comment “internal use only” to no longer being in a private header.

Any legacy function that we don't want to promise keeping for as long as 4.x is around should be in a separate header. I propose to create a subdirectory include/mbedtls/unstable for such headers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-break This issue/PR breaks the API and must wait for a new major version component-crypto Crypto primitives and low-level interfaces deprecation needs deprecation announcement size-m Estimated task size: medium (~1w)
Projects
Status: Mbed TLS 4.0 MUST
Status: Planning needed
Status: 4.0 - Hide low level crypto
Development

No branches or pull requests

3 participants