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.

Sign up for GitHub

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

Jump to bottom

Update "use PSA" documentation (inc. strategy) #5757

Merged
merged 16 commits into from
Jul 4, 2022
Merged

Update "use PSA" documentation (inc. strategy) #5757

merged 16 commits into from
Jul 4, 2022

Conversation

mpg
Copy link
Contributor

@mpg mpg commented Apr 20, 2022 •
edited
Loading

Description

Fixes #5621

Status

IN DEVELOPMENT - but what's there can already be reviewed. Will be finalized when we stabilize in preparation for 3.2.

Should be fully up to date as of 271c305 - just anticipates a bit on #5831 and #5844 being closed before the release.

Possible changes that may or may not happen before the release, and will require an update if they do:

Requires Backporting

NO - 3.x only

@mpg mpg added the needs-work label Apr 20, 2022
@mpg mpg self-assigned this Apr 20, 2022
@mpg mpg changed the title Update "use PSA" document (inc. strategy) Update "use PSA" documentation (inc. strategy) Apr 20, 2022
@mpg mpg mentioned this pull request Apr 21, 2022
Closed
3 tasks
gilles-peskine-arm
gilles-peskine-arm reviewed Apr 27, 2022
View reviewed changes
docs/architecture/psa-migration/psa-limitations.md Outdated
of RSA decryption would be still checking that is has the correct format:
48 bytes, the first two matching the TLS version - note that this is timing
sensitive.)

HKDF: Expand not exposed on its own (TLS 1.3)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#5784

@mpg mpg force-pushed the update-doc-use-psa branch from f617329 to 4a270f1 Compare June 7, 2022 08:30
@mpg
Copy link
Contributor Author

mpg commented Jun 7, 2022

I updated the second paragraph of "PSA implementation in Mbed TLS" in the README but the third paragraph needs updating too (outdated link), and perhaps the first as well (do we still consider that PSA code is not as well reviewed as the rest?). @gilles-peskine-arm could you suggest a new version? Then I'd include it in this PR, to avoid the trouble of an extra PR with contextual conflicts.

@mpg mpg marked this pull request as ready for review June 7, 2022 11:37
@mpg mpg added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Jun 7, 2022
@mpg mpg requested a review from gilles-peskine-arm June 7, 2022 11:45
@gilles-peskine-arm
Copy link
Contributor

First paragraph: I think we can drop the maturity bit. We haven't done the systematic quality bar that I wanted to do, but we've rewritten pretty much all the original code and test coverage, so I think we've reached similar quality to the old code.

Third paragraph: we've passed compliance with 1.0. We can just remove this paragraph. Although we might tweak the first paragraph to say that while we're a reference implementation, we don't aim to implement the whole thing, in particular we don't implement all the algorithms.

@mpg
Copy link
Contributor Author

mpg commented Jun 10, 2022

Self-reminder: add ChangeLog entries too.

@mpg mpg removed the needs-work label Jun 20, 2022
@daverodgman daverodgman added the priority-high High priority - will be reviewed soon label Jun 21, 2022
minosgalanakis
minosgalanakis previously approved these changes Jun 27, 2022
View reviewed changes
Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mpg mpg removed the needs-reviewer This PR needs someone to pick it up for review label Jun 28, 2022
wernerlewis
wernerlewis previously approved these changes Jul 4, 2022
View reviewed changes
Copy link
Contributor

@wernerlewis wernerlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mpg added 8 commits July 4, 2022 12:38
@mpg
Rm tasks-g2.md
2ffb93a 
That document was always temporary (said so at the top). Now superseded
by https://github.com/orgs/Mbed-TLS/projects/1#column-18338322

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg
Update testing.md
b8a6c23 
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg
Update psa-limitations
83c5388 
- misc updates about on-going/recent work
- removal of the section about mixed-PSK: being done in Mbed-TLS#5762
- clarifications in some places
- some typo fixes

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg
Update strategy.md
2a47d23 
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg
Clarify effect of USE_PSA on TLS 1.3
97ec0b7 
The previous wording was wrong, there are parts that are affected.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg
Remove stability waiver from USE_PSA
ff43ff6 
It was initially motivated by the fact that the PSA Crypto APIs
themselves were not stable. In the meantime, PSA Crypto has reached
1.0.0 so this no longer applies.

If we want user to be able to fully benefit from PSA in order to
isolate long-term secrets, they need to be able to use the new APIs with
confidence. There is no reason to think those APIs are any more likely
to change than any of our other APIs, and if they do, we'll follow the
normal process (deprecated in favour of a new variant).

For reference, the APIs in question are:

mbedtls_pk_setup_opaque() // to use PSA-held ECDSA/RSA keys in TLS

mbedtls_ssl_conf_psk_opaque()   // for PSA-held PSKs in TLS
mbedtls_ssl_set_hs_psk_opaque() // for PSA-held PSKs in TLS

mbedtls_cipher_setup_psa() (deprecated in 3.2)
mbedtls_pk_wrap_as_opaque() (documented internal, to be removed in 3.2)

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg
Update docs/use-psa-crypto.md
b2bd34e 
The scope of the option has been expanded, now it makes more sense to
describe it as "everything except ...".

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg
Remove HKDF-Extract/Expand
103b992 
Being resolved in Mbed-TLS#5784

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg mpg dismissed stale reviews from wernerlewis and minosgalanakis via 553f997 July 4, 2022 10:40
@mpg mpg force-pushed the update-doc-use-psa branch from 44dc1bf to 553f997 Compare July 4, 2022 10:40
mpg added 8 commits July 4, 2022 12:44
@mpg
Clarify the TLS 1.3 situation
3e83098 
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg
Now compatible with MBEDTLS_PSA_CRYPTO_KEY_ID_ENCODES_OWNER
f3f79a0 
Also make a few general clarifications/improvements while at it.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg
Update README about USE_PSA_CRYPTO
cbc03f5 
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg
Link to restartable ECC EPIC
9bf9b9e 
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg
Fix list of what's common to TLS 1.2 and 1.3
0dba51c 
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg
Tune description of PSA crypto implementation
2dc436d 
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg
Add ChangeLog entries for general Use PSA improvements
465341f 
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg
Misc clean-ups in docs/use-psa-crypto.md
b5b27c1 
Note: limitations of opaque PSKs changed from "TLS 1.2 only" to "none"
since TLS 1.3 does not support PSK at all so far, and it is expected to
support opaque PSKs as soon as it gains PSK support, it will be just a
matter of selecting between psa_key_derivation_input_bytes() and
psa_key_derivation_input_key() - and testing obviously.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
@mpg mpg force-pushed the update-doc-use-psa branch from 553f997 to b5b27c1 Compare July 4, 2022 10:44
@mpg
Copy link
Contributor Author

mpg commented Jul 4, 2022

There was a conflict in mbedtls_config.h so I rebased and force-pushed in order to resolve it. @minosgalanakis @wernerlewis please review. (I took the opportunity to mention the running handshake hash as shared between 1.2 and 1.3, which I had forgotten before - other than that there should be not change.)

@mpg mpg requested review from wernerlewis and minosgalanakis July 4, 2022 10:46
@paul-elliott-arm paul-elliott-arm added the needs-ci Needs to pass CI tests label Jul 4, 2022
@mpg mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests labels Jul 4, 2022
@mpg mpg merged commit 0358597 into Mbed-TLS:development Jul 4, 2022
@mpg mpg deleted the update-doc-use-psa branch July 5, 2022 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@minosgalanakis minosgalanakis minosgalanakis approved these changes

@wernerlewis wernerlewis wernerlewis approved these changes

@gilles-peskine-arm gilles-peskine-arm Awaiting requested review from gilles-peskine-arm

Assignees

@mpg mpg

Labels
approved Design and code approved - may be waiting for CI or backports priority-high High priority - will be reviewed soon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update "use PSA" documentation
6 participants
@mpg @gilles-peskine-arm @wernerlewis @minosgalanakis @daverodgman @paul-elliott-arm