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

[3.6 only] Document transitional feature macros / guards #9161

Merged
merged 13 commits into from
Oct 9, 2024
Merged

[3.6 only] Document transitional feature macros / guards #9161

merged 13 commits into from
Oct 9, 2024

Conversation

mpg
Copy link
Contributor

@mpg mpg commented May 22, 2024 •
edited
Loading

Description

Resolves #8736

PR checklist

  • changelog not required because: documentation
  • development PR not required because: 4.0 will complete the transition and have much simpler guards
  • framework PR not required
  • 3.6 PR here
  • 2.28 PR not required because: 2.28 does not have those guards that were added for driver-only support
  • tests not required: documentation

@mpg mpg self-assigned this May 22, 2024
@gilles-peskine-arm gilles-peskine-arm self-requested a review May 22, 2024 11:06
gilles-peskine-arm
gilles-peskine-arm requested changes May 22, 2024
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks correct to me apart from some minor niggles. I haven't reviewed for completeness.

However, this does not address #8736.

@mpg
Copy link
Contributor Author

mpg commented May 23, 2024

However, this does not address #8736.

Aw, I forgot to add the new file, how embarrassing. Sorry.

gilles-peskine-arm
gilles-peskine-arm requested changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

Thank you very much for writing this up! This is a great guide overall. I've commented on some typos and imprecisions.

docs/architecture/psa-migration/transition-guards.md
- transitional `MBEDTLS_xxx` macros that stem from the desire to be able to
use crypto mechanisms that are only provided by a driver (G5 in
`strategy.md`).

Copy link
Contributor

Choose a reason for hiding this comment

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

Macros defined in include/mbedtls/config_adjust_legacy_*.h only, used outside of include/mbedtls/*.h and not mentioned here:

  • MBEDTLS_BLOCK_CIPHER_xxx_VIA_xxx (used in library, test dependencies)
  • MBEDTLS_BLOCK_CIPHER_SOME_PSA (used in library, test helpers, test code)
  • MBEDTLS_MD_xxx_VIA_PSA (used in library, test dependencies)
  • MBEDTLS_PSA_UTIL_HAVE_ECDSA (used in library, test dependencies)

We should document them as well, but that's not a blocker for this pull request.

@gilles-peskine-arm
Copy link
Contributor

This should be on the 3.6 branch, since it won't be relevant in 4.0.

mpg and others added 9 commits September 10, 2024 10:58
@mpg
Update psa-limitations.md and add summary
42a1453 
Just reflecting recent/on-going work.

Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Update psa-migration/strategy.md
7237563 
Just reflecting recent/on-going work.

Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Add transition-guards.md
06adca4 
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Minor updates in doc/comments/debug
32bdf19 
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Add links and missing )
4837f72 
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg @gilles-peskine-arm
Fix some typos & markdown
3342e80 
Co-authored-by: Gilles Peskine <[email protected]>
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Clarify a comment in all.sh
b50b638 
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Misc minor clarifications in transition-guards.md
4975232 
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Clarify summary of PSA limitations
a046577 
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg mpg changed the base branch from development to mbedtls-3.6 September 10, 2024 09:12
@mpg mpg changed the title Doc guards [3.6 only] Document transitional feature macros / guards Sep 10, 2024
@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 size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon priority-medium Medium priority - this can be reviewed as time permits and removed priority-high High priority - will be reviewed soon labels Sep 10, 2024
@mpg
Copy link
Contributor Author

mpg commented Sep 10, 2024

@gilles-peskine-arm I added commits addressing your feedback, and then rebased to resolve conflicts and more importantly target the correct branch. Please feel free to review again. Note: I wasn't sure if this is priority medium of high, please feel free to adjust.

@gilles-peskine-arm
Copy link
Contributor

@mpg 1. So this shouldn't be a draft PR anymore? 2. The CI isn't happy.

@mpg mpg marked this pull request as ready for review September 10, 2024 10:10
@mpg
Copy link
Contributor Author

mpg commented Sep 11, 2024

@mpg 1. So this shouldn't be a draft PR anymore? 2. The CI isn't happy.

Yeah, I meant to un-draft it but forgot, thanks for the reminder. Regarding the CI, I'm not sure what happened but it's looking happy now.

gilles-peskine-arm
gilles-peskine-arm requested changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! This mostly looks good to me, but there's one 4.0 plan that just changed, and a few requests for clarifications from my previous review that you haven't addressed and I think at least some of them should be addressed.

@gilles-peskine-arm gilles-peskine-arm removed the needs-review Every commit must be reviewed by at least two team members, label Sep 25, 2024
@gilles-peskine-arm gilles-peskine-arm added needs-work and removed needs-reviewer This PR needs someone to pick it up for review labels Sep 25, 2024
mpg added 4 commits September 26, 2024 09:45
@mpg
Update and refine notes on restartable ECC and 4.0
4ceb71f 
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Clarify the superset rule
6076976 
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Expand on key derivations
83f62ff 
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Expand on block cipher modes/derivatives
9fc3b7d 
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
@mpg
Copy link
Contributor Author

mpg commented Sep 26, 2024

@gilles-peskine-arm Thanks for your review and sorry for forgetting to address some of your comments. I believe I've now addressed all of them except for #9161 (comment) which I'd rather leave for later.

gilles-peskine-arm
gilles-peskine-arm approved these changes Oct 3, 2024
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

LGTM

docs/architecture/psa-migration/transition-guards.md

**Legacy and `USE_PSA` domains:** for hashes, `MBEDTLS_MD_CAN_xxx` (where
`xxx` is the legacy name of the hash) can be used everywhere (except in the
PSA domain which should use `PSA_WANT` as usual). No special include is
required, `build_info.h` or `common.h` is enough.

**Pure TLS 1.3 domain:** it is not easy to know which uses of hashes fall in
this domain as opposed to the `USE_PSA` domain which looking at the code.
this domain as opposed to the `USE_PSA` domain whithout looking at the code.
Copy link
Contributor

Choose a reason for hiding this comment

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

Trivial:

Suggested change
this domain as opposed to the `USE_PSA` domain whithout looking at the code.
this domain as opposed to the `USE_PSA` domain without looking at the code.

@gilles-peskine-arm gilles-peskine-arm 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 and removed needs-work labels Oct 3, 2024
Harry-Ramsey
Harry-Ramsey approved these changes Oct 9, 2024
View reviewed changes
Copy link
Contributor

@Harry-Ramsey Harry-Ramsey left a comment

Choose a reason for hiding this comment

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

LGTM. I don't have prior experience with 3.6 drivers but this information seems vastly useful for developing ontop of Mbed TLS. The transition guards section will be useful for developing ontop of Mbed TLS or contributing to the library as a whole.

@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-reviewer This PR needs someone to pick it up for review labels Oct 9, 2024
@mpg mpg added this pull request to the merge queue Oct 9, 2024
Merged via the queue into Mbed-TLS:mbedtls-3.6 with commit 8536c3c Oct 9, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

@Harry-Ramsey Harry-Ramsey Harry-Ramsey approved these changes

Assignees

@mpg mpg

Labels
approved Design and code approved - may be waiting for CI or backports priority-medium Medium priority - this can be reviewed as time permits size-s Estimated task size: small (~2d)
Projects
Roadmap pull requests (new board)
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Document compilation guards for stacks
3 participants
@mpg @gilles-peskine-arm @Harry-Ramsey