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

Put the id field at the end of the psa_key_attributes_s structure #8745

Merged
merged 3 commits into from
Feb 6, 2024
Merged

Put the id field at the end of the psa_key_attributes_s structure #8745

merged 3 commits into from
Feb 6, 2024

Conversation

adeaarm
Copy link
Contributor

@adeaarm adeaarm commented Jan 24, 2024

Description

Putting the id at the of the psa_key_attributes_s structure allows for a more efficient marshalling of the parameters around a transport channel which provides separation between a client view and a service view of the key parameters.

PR checklist

  • changelog NOT REQUIRED
  • backport NOT REQUIRED
  • tests NOT REQUIRED

@adeaarm
Put the id field at the end of the psa_key_attributes_s structure
667cad5 
Putting the id at the of the psa_key_attributes_s structure allows
for a more efficient marshalling of the parameters around a transport
channel which provides separation between a client view and a service
view of the key parameters.

Signed-off-by: Antonio de Angelis <antonio.deangelis@arm.com>
@paul-elliott-arm paul-elliott-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 component-psa PSA keystore/dispatch layer (storage, drivers, …) size-xs Estimated task size: extra small (a few hours at most) labels Jan 24, 2024
@adeaarm
Copy link
Contributor Author

adeaarm commented Jan 25, 2024

This breaks ABI as it's re-arranging the fields of the structures. The new layout could be put under MBEDTLS_PSA_CRYPTO_CLIENT && !MBEDTLS_PSA_CRYPTO_C if preserving the ABI compatibility in 3.6 is important for the default case.

@davidhorstmann-arm
Copy link
Contributor

We have previously broken the ABI for struct rearrangements, e.g. in #5189 / #5268, but I'm unsure if this is a similar case.

@daverodgman daverodgman added the priority-high High priority - will be reviewed soon label Jan 29, 2024
@daverodgman
Copy link
Contributor

The ABI change is fine outside of an LTS. We explicitly mention re-ordering fields as OK in BRANCHES.md.

daverodgman
daverodgman previously approved these changes Jan 29, 2024
View reviewed changes
Copy link
Contributor

@daverodgman daverodgman left a comment

Choose a reason for hiding this comment

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

LGTM

gilles-peskine-arm
gilles-peskine-arm requested changes Jan 31, 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.

Ok but needs comments.

include/psa/crypto_struct.h Show resolved Hide resolved
@gilles-peskine-arm gilles-peskine-arm added needs-work 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 Jan 31, 2024
@adeaarm
Add comments in psa/crypto_struct.h for id layout
6fb1be6 
Make sure the layout of psa_key_attributes_s is commented
enough so that it does not accidentally get reorganized
by mistake in the future.

Signed-off-by: Antonio de Angelis <antonio.deangelis@arm.com>
daverodgman
daverodgman previously approved these changes Feb 2, 2024
View reviewed changes
@daverodgman daverodgman self-requested a review February 2, 2024 15:12
gilles-peskine-arm
gilles-peskine-arm previously approved these changes Feb 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.

LGTM apart from the macro name

@adeaarm
Correct the ENCODES_OWNER macro name in comment
6932e29 
Signed-off-by: Antonio de Angelis <antonio.deangelis@arm.com>
@daverodgman daverodgman added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests and removed needs-work labels Feb 5, 2024
@adeaarm
Copy link
Contributor Author

adeaarm commented Feb 5, 2024

LGTM apart from the macro name

Fixed.

gilles-peskine-arm
gilles-peskine-arm approved these changes Feb 6, 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

@gilles-peskine-arm gilles-peskine-arm 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 Feb 6, 2024
@daverodgman daverodgman added this pull request to the merge queue Feb 6, 2024
Merged via the queue into Mbed-TLS:development with commit 91d5fde Feb 6, 2024
4 of 6 checks passed
@adeaarm adeaarm deleted the trail_key_id_field branch February 7, 2024 09:07
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

@daverodgman daverodgman daverodgman approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports component-psa PSA keystore/dispatch layer (storage, drivers, …) priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most)
Projects
Past EPICs
Status: Mbed TLS 3.6 release
Roadmap Board for Mbed TLS
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@adeaarm @davidhorstmann-arm @daverodgman @gilles-peskine-arm @paul-elliott-arm