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

Tweak documentation about lifetimes #3576

Conversation

gilles-peskine-arm
Copy link
Contributor

  • Fix a mistake in the description of locations. This change is also being made in the spec draft.
  • Replace specification language discussing possible implementations by documentation of Mbed TLS discussing possible integrations and omitting some things that don't apply to Mbed TLS.

Follow-up to #3302. No behavior change, just documentation improvements.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
In the documentation of lifetimes, replace language meant for the PSA
specification by language that is specifically about Mbed TLS. Reduce
the discussion of what could happen in other implementation, and
discuss what can and cannot happen in integrations of Mbed TLS.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm added enhancement needs-review Every commit must be reviewed by at least two team members, component-crypto Crypto primitives and low-level interfaces labels Aug 18, 2020
@gilles-peskine-arm gilles-peskine-arm self-assigned this Aug 18, 2020
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

I have a few questions.

@ronald-cron-arm
Copy link
Contributor

Added the "needs: work" label as I have pending questions.

@daverodgman daverodgman added the needs-reviewer This PR needs someone to pick it up for review label Oct 2, 2020
@mpg mpg self-requested a review December 10, 2020 10:32
@mpg mpg removed the needs-reviewer This PR needs someone to pick it up for review label Dec 10, 2020
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

I'm mostly happy with this PR, except for a couple of small things already pointed by Ronald.

Also, I noticed that looking for [Ii]mplementations finds many occurrences, which should probably be changed too, but perhaps that's out of scope of this PR if you want to keep it focused on lifetimes.

@mpg mpg removed the needs-review Every commit must be reviewed by at least two team members, label Dec 11, 2020
It's about who has access to the key material in plaintext, not directly
where the operation is performed.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Some of the material was originally the PSA specification, and
discusses how different implementations might behave. Replace such
statements by a description of how Mbed TLS behaves.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Dec 11, 2020
mpg
mpg previously approved these changes Dec 14, 2020
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

This looks almost good to me. I've just spotted a typo.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Copy link
Contributor

@mpg mpg 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-review Every commit must be reviewed by at least two team members, label Dec 15, 2020
@mpg
Copy link
Contributor

mpg commented Dec 15, 2020

The CI failure in the pr-merge job on Windows is an unrelated issue with the infrastructure and can be ignored for the purposed of merging this PR.

@mpg mpg added the approved Design and code approved - may be waiting for CI or backports label Dec 15, 2020
@mpg mpg merged commit 575ece0 into Mbed-TLS:development Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-crypto Crypto primitives and low-level interfaces enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants