-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Comments
Ping @dave-rodgman - I think you took notes during the meeting, please correct my list if it's not in line with those notes. |
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 Similar considerations might apply to other modules, I've not tried making a complete list yet. (Note: currently |
Ah, actually there's a reason, it's that Everest dispatch is implemented in |
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:
|
Definitely not.
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 |
I've just created the issue for removing DHM module -> #9956 |
Hi all, I'm a maintainer of Godot Engine which, as some of you may know, uses mbedTLS as the cryptographic library. Prompted by the recent email about an upcoming mbedTLS 4.0 release I ended up finding this issue and went through the list to see how this would affect our integration. As far as I can tell, we should be able to migrate almost all API usage to the PSA counterparts (we also have a 'lite embed' option which only bundle a very restricted set of files, but we can probably handle that ad hoc). One thing I noticed though, is:
As far as I can tell, there's no PSA counterpart to do base64, and we do use the mbedTLS version for doing base64. While we could pull the internal header when bundling mbedTLS in the engine, we wouldn't be able to use it when mbedTLS is provided by the OS as a shared library (and would likely have to still bundle that source file directly in such scenario). Is there any chance that base64 functions could be left exposed, or maybe exposed via PSA crypto? |
@Faless Thank you very much for the feedback! I'm glad that you found a way to migrate most of your code. About the “lite embed” option, please note that if you rely on low-level crypto APIs, many of those will probably be still be around in 4.0, but with declarations only in private headers, and they're very likely to change without notice in subsequent releases. About Base64, we've reevaluated whether PEM and Base64 should be public and we've decided to keep both public. (I'll update the document in that pull request shortly.) |
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:
aes.h
,aria.h
,block_cipher.h
,camellia.h
,ccm.h
,chacha20.h
,chachapoly.h
,cmac.h
,des.h
,gcm.h
,poly1305.h
,cipher.h
Remove cipher.h in 4.0 #8451nist_kw.h
(note: includescipher.h
! If we removecipher.h
we'll need to change types innist_kw.h
- onlymbedtls_cipher_id_t
is used AFAICS) → Move NIST_KW to PSA API #9382.Hashes & related:
md5.h
,ripemd160.h
,sha1.h
,sha256.h
,sha3.h
,sha512.h
,md.h
Evolution of md.h in 4.0 #8450hkdf.h
Randomness:
entropy.h
,ctr_drbg.h
,hmac_drbg.h
- requireStudy: PSA random generation drivers for 4.0 (replace entropy.h) #8150 to avoid functionality lossMBEDTLS_ENTROPY_HARDWARE_ALT in 4.0 #9618 to avoid critical functionality lossAsymmetric crypto:
bignum.h
,dhm.h
,ecdh.h
,ecdsa.h
,ecjpake.h
,ecp.h
,rsa.h
- note: potential feature loss by removingecp.h
andrsa.h
(andbignum.h
) but came to an agreement accepting it. Note: important feature loss (restartable/interruptible) unless [DI] Use interruptible sign/verify in X.509 & TLS #7292, [DI] Add interruptible APIs to PSA for ECDH #7293, [DI] Use interruptible ECDH in TLS #7294 and resulting EPIC(s) are done first. Note impact on fuzzers.pk.h
Evolution of pk.h in 4.0 #8452lms.h
(note: does not include other headers)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 #9380platform.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 internalizeprivate_access.h
-> keepmemory_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.
The text was updated successfully, but these errors were encountered: