Remove discussions of MBEDTLS_USE_PSA_CRYPTO in standalone documentation #9818
Conversation
yanesca
added
enhancement
needs-review
There was a problem hiding this comment.
This mostly looks good to me. In particular, I don't see other files that need changing.
However, there are several places where we're left with a document that isn't meaningful any longer, or at least would need a major rewrite, because it opposes a legacy world and a PSA world, and this opposition no longer exists. I lean towards just removing these documents or passages.
There are a couple of places where the text wasn't quite right for 3.6, so before we forget, let's fix it: #9823.
| @@ -3,7 +3,7 @@ PSA migration strategy for hashes and ciphers | |||
|
|
|||
There was a problem hiding this comment.
This is an architecture document focusing on how parts of the code base can accommodate both builds with PSA crypto disabled and builds with driver-only mechanisms. Going forward, this coexistence is no longer relevant. So I think we should just remove this file (like testing.md).
The document does explain why some parts of md and cipher are the way they are. In the future, we'll want to remove legacy code paths and keep only the PSA code paths. But for that, it isn't particularly useful to know how the dual code paths came about, or what constraints they had to obey. Those constraints no longer apply.
docs/use-psa-crypto.md
Outdated
| This document describes how PSA Crypto is used in the X.509 and TLS libraries | ||
| from a user's perspective. |
There was a problem hiding this comment.
Is this document still relevant? I'm on the fence. Should we keep updating it gradually or just remove it now?
The original purpose of this document was to answer the following question in more detail than the documentation of MBEDTLS_USE_PSA_CRYPTO in config.h: as a user, what do I gain and lose if I enable MBEDTLS_USE_PSA_CRYPTO? This question is no longer relevant.
In principle, everything that's relevant from a user's perspective should be listed in the API documentation (attached to the relevant configuration option or library function). And we have the maintainer's view in docs/architecture/psa-migration/psa-limitations.md. Is it still useful to have this user-facing document? At this point, it looks to me like a loose collection of facts. But maybe this loose collection is still somewhat useful. Perhaps it should become a chapter in psa-transition.md?
If we do keep this document, there are a few parts that need further work that is technically in scope here, and some where the further work will technically be in scope when we remove the legacy crypto functions from the API. But it's probably easier to do all the changes at once, so we could stop here for now and file an issue.
There was a problem hiding this comment.
Based on what you are saying and looking at the documents involved in detail, I think it is probably best to remove it:
- General considerations section: not relevant as you mentioned above
- New APIs/API extensions: these are not new or extensions anymore.
Also, for detailed information this section refers to the API
documentation, which contains all the information the user needs. - Internal changes: these are discussed in detail in docs/architecture/psa-migration/psa-limitations.md.
docs/use-psa-crypto.md
Outdated
| operations and its public part can be exported. | ||
| **API function:** `mbedtls_pk_setup_opaque()` - can be used to wrap a PSA key | ||
| pair into a PK context. The key can be used for private-key operations and its | ||
| public part can be exported. | ||
|
|
||
| **Benefits:** isolation of long-term secrets, use of PSA Crypto drivers. |
There was a problem hiding this comment.
Reading this document from the perspective of a new user, rather than someone migrating from non-PSA APIs:
The text opposes two scenarios, but what are they? “Benefits” compared to what? Below, “opt-in”, “new API”: as opposed to what?
gilles-peskine-arm
added
needs-backports
yanesca
added
needs-review
There was a problem hiding this comment.
There are still references to use-psa-crypto.md in docs/architecture/psa-migration/strategy.md and docs/driver-only-builds.md.
| As of Mbed TLS 3.2, most of (G1) and all of (G2) is implemented. For a more | ||
| detailed account of what's implemented, see `docs/use-psa-crypto.md`, where new | ||
| APIs are about (G2), and internal changes implement (G1). |
There was a problem hiding this comment.
docs/use-psa-crypto.md is now being deleted. We are effectively finalizing the transition from “all is legacy-only except what MBEDTLS_USE_PSA_CRYPTO supports” to “all is PSA except for a few limitations”. So now I think the relevant reference is docs/architecture/psa-migration/psa-limitations.md for how we've only achieved most of (G1) and not all.
docs/driver-only-builds.md
Outdated
| @@ -215,8 +200,7 @@ consequence these are not supported in builds without `MBEDTLS_ECDSA_C`. | |||
|
|
|||
| Similarly, there is no PSA support for interruptible ECDH operations so these | |||
| are not supported without `ECDH_C`. See also limitations regarding | |||
| restartable operations with `MBEDTLS_USE_PSA_CRYPTO` in [its | |||
| documentation](use-psa-crypto.md). | |||
| restartable operations in [the documentation about using PSA Crypto](use-psa-crypto.md). | |||
There was a problem hiding this comment.
With use-psa-crypto.md deleted: In the documentation of MBEDTLS_ECP_RESTARTABLE, or in docs/architecture/psa-migration/psa-limitations.md.
There was a problem hiding this comment.
There are still references to md-cipher-dispatch.md in docs/architecture/psa-migration/strategy.md. I wonder if strategy.md itself should be deleted? It's mainly about a plan that was driven by backward compatibility constraints that no longer exist in 4.0, although some of it is still of interest to explain why 4.0 is the way it is (and more complicated than one would expect based on the APIs that are left in 4.0). It isn't not referenced from anywhere except one comment in docs/architecture/psa-migration/outcome-analysis.sh.
There was a problem hiding this comment.
You are right, it is probably worth deleting. We can look up historical versions if needed, but I don't think it is worth maintaining it anymore.
gilles-peskine-arm
added
needs-work
and removed
needs-review
yanesca
force-pushed
the
remove_USE_PSA_from_standalone_doc_9632
branch
from
5c4e866 to
9655570
Compare
|
Rebased to resolve conflicts |
This is an architecture document focusing on how parts of the code base can accommodate both builds with PSA crypto disabled and builds with driver-only mechanisms. Going forward, this coexistence is no longer relevant. The document does explain why some parts of md and cipher are the way they are. In the future, we'll want to remove legacy code paths and keep only the PSA code paths. But for that, it isn't particularly useful to know how the dual code paths came about, or what constraints they had to obey. Those constraints no longer apply. Signed-off-by: Janos Follath <[email protected]>
This is an architecture document focusing on how PSA APIs can be mixed with non-PSA APIs, notably including PK (and in fact, it's mostly about PK, since we didn't identify work to be done in other areas). It is not really relevant in 4.0/1.0, where the goals will be different — to do without low-level legacy APIs. Signed-off-by: Janos Follath <[email protected]>
The original purpose of this document was to answer the following question in more detail than the documentation of MBEDTLS_USE_PSA_CRYPTO in config.h: as a user, what do I gain and lose if I enable MBEDTLS_USE_PSA_CRYPTO? This question is no longer relevant. - General considerations section: not relevant as mentioned above - New APIs/API extensions: these are not new or extensions anymore. Also, for detailed information this section refers to the API documentation, which contains all the information the user needs. - Internal changes: these are discussed in detail in docs/architecture/psa-migration/psa-limitations.md. Signed-off-by: Janos Follath <[email protected]>
Some sentences or paragraphs became confusing or meaningless after removing USE_PSA and only fixing the local context/semantics. Fix the semantics where needed and remove parts that became meaningless. Signed-off-by: Janos Follath <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
This document is mainly about a plan that was driven by backward compatibility constraints that no longer exist in 4.0. Although some of it is still of interest to explain why 4.0 is the way it is (and more complicated than one would expect based on the APIs that are left in 4.0). But for this it should suffice to consult earlier versions and does not worth to maintain it. Signed-off-by: Janos Follath <[email protected]>
yanesca
force-pushed
the
remove_USE_PSA_from_standalone_doc_9632
branch
from
a874922 to
1243896
Compare
|
Rebased to resolve conflicts. Some of the changes were on the crypto side and raised a separate PR for them: Mbed-TLS/TF-PSA-Crypto#124. |
Signed-off-by: Janos Follath <[email protected]>
yanesca
added
needs-review
There was a problem hiding this comment.
Almost LGTM based on my previous review of pr_9818-2 (a874922) and checking the differences in the files that still exist in development:
git diff pr_9818-2..upstream-public/pr/9818 -- $(comm -12 <(git diff --name-only development...pr_9818-2 | sort) <(git ls-tree -r --name-only development | sort))
The differences are in cross-references which are now good, except one.
| @@ -295,8 +295,6 @@ Arm welcomes feedback on the design of the API. If you think something could be | |||
| Mbed TLS includes a reference implementation of the PSA Cryptography API. | |||
| However, it does not aim to implement the whole specification; in particular it does not implement all the algorithms. | |||
|
|
|||
There was a problem hiding this comment.
There's still one dangling reference in ChangeLog.d/psa-always-on.txt, to use-psa-crypto.md which no longer exists.
Signed-off-by: Janos Follath <[email protected]>
yanesca
removed
the
needs-reviewer
waleed-elmelegy-arm
added
approved
Description
Remove discussions of MBEDTLS_USE_PSA_CRYPTO in API documentation. Resolves partially #9632.
PR checklist
Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.