gh-Mbed-TLS/TF-PSA-Crypto
Watch
1
0
Fork
You've already forked TF-PSA-Crypto
 0
Code Issues 167 Pull requests 92 Releases 5 Activity

Add design assumption for the use of PSA_ITS_STORAGE_PREFIX #875

Open
bjwtaylor wants to merge 14 commits from bjwtaylor/file-backed-keystore-doc into development
pull from: bjwtaylor/file-backed-keystore-doc
merge into: gh-Mbed-TLS:development
Conversation 13 Commits 14 Files changed 6 +52 -9
bjwtaylor commented 2026-08-26 04:34:12 -05:00 (Migrated from github.com)

Description

Add design assumption for the use of PSA_ITS_STORAGE_PREFIX

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.

  • changelog provided
  • framework PR not required
  • TF-PSA-Crypto development PR provided #HERE
  • TF-PSA-Crypto 1.1 PR provided # | not required because: TODO if required
  • mbedtls development PR not required because: crypto only
  • mbedtls 4.1 PR not required because: crypto only
  • mbedtls 3.6 PRnot required because: crypto only
  • tests not required because: docs only
## Description Add design assumption for the use of PSA_ITS_STORAGE_PREFIX ## 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. - [ ] **changelog** provided - [ ] **framework PR** not required - [ ] **TF-PSA-Crypto development PR** provided #HERE - [ ] **TF-PSA-Crypto 1.1 PR** provided # | not required because: TODO if required - [ ] **mbedtls development PR** not required because: crypto only - [ ] **mbedtls 4.1 PR** not required because: crypto only - [ ] **mbedtls 3.6 PR**not required because: crypto only - **tests** not required because: docs only
gilles-peskine-arm (Migrated from github.com) reviewed 2026-08-26 05:40:26 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment
No description provided.
LGTM
gilles-peskine-arm (Migrated from github.com) reviewed 2026-08-26 07:52:20 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment
No description provided.
LGTM
bjwtaylor (Migrated from github.com) reviewed 2026-08-26 09:47:32 -05:00
include/psa/crypto_config.h
@ -1284,0 +1284,4 @@
/**
* \def MBEDTLS_PSA_ITS_STORAGE_PREFIX
*
* Set the directory for the file-based PSA ITS implementation.
bjwtaylor (Migrated from github.com) commented 2026-08-26 09:47:32 -05:00

I think this is causing a doxygen warning and the ci to fail. I've put something up that I think might resolve it, let's see what the CI does though and let me know what you think?

I think this is causing a doxygen warning and the ci to fail. I've put something up that I think might resolve it, let's see what the CI does though and let me know what you think?
gilles-peskine-arm (Migrated from github.com) reviewed 2026-08-26 09:57:19 -05:00
include/psa/crypto_config.h
@ -1284,0 +1284,4 @@
/**
* \def MBEDTLS_PSA_ITS_STORAGE_PREFIX
*
* Set the directory for the file-based PSA ITS implementation.
gilles-peskine-arm (Migrated from github.com) commented 2026-08-26 09:57:18 -05:00

Ah, my bad, I'd missed that PSA_ITS_STORAGE_PREFIX is not an official configuration option.

Since it's now referenced in public documentation, we should make it an official option. It means we commit to keep supporting it, but that's ok by me, I see no reason to retire or change it.

We should make it match our current naming pattern, however. The only PSA_xxx options we have are things that could potentially become a PSA standard. This one isn't. So we should call it either TF_PSA_CRTYPTO_ITS_STORAGE_PREFIX or MBEDTLS_PSA_ITS_STORAGE_PREFIX. I prefer MBEDTLS_PSA_ITS_STORAGE_PREFIX for consistency with the option name.

We should still recognize PSA_ITS_STORAGE_PREFIX as a fallback if MBEDTLS_PSA_ITS_STORAGE_PREFIX is not defined. Strictly speaking we could retire it at any time, but there is at least nuttx-apps using it, and I see no reason to gratuitously break them. PSA_ITS_STORAGE_PREFIX shouldn't be documented except in a comment in psa_its_file.c.

Ah, my bad, I'd missed that `PSA_ITS_STORAGE_PREFIX` is not an official configuration option. Since it's now referenced in public documentation, we should make it an official option. It means we commit to keep supporting it, but that's ok by me, I see no reason to retire or change it. We should make it match our current naming pattern, however. The only `PSA_xxx` options we have are things that could potentially become a PSA standard. This one isn't. So we should call it either `TF_PSA_CRTYPTO_ITS_STORAGE_PREFIX` or `MBEDTLS_PSA_ITS_STORAGE_PREFIX`. I prefer `MBEDTLS_PSA_ITS_STORAGE_PREFIX` for consistency with the option name. We should still recognize `PSA_ITS_STORAGE_PREFIX` as a fallback if `MBEDTLS_PSA_ITS_STORAGE_PREFIX` is not defined. Strictly speaking we could retire it at any time, but there is at least [nuttx-apps](https://github.com/open-vela/nuttx-apps) using it, and I see no reason to gratuitously break them. `PSA_ITS_STORAGE_PREFIX` shouldn't be documented except in a comment in `psa_its_file.c`.
bjwtaylor (Migrated from github.com) reviewed 2026-08-27 08:32:57 -05:00
include/psa/crypto_config.h
@ -1284,0 +1284,4 @@
/**
* \def MBEDTLS_PSA_ITS_STORAGE_PREFIX
*
* Set the directory for the file-based PSA ITS implementation.
bjwtaylor (Migrated from github.com) commented 2026-08-27 08:32:57 -05:00

I've put up some changes let's see if the CI likes them.

I've put up some changes let's see if the CI likes them.
gilles-peskine-arm (Migrated from github.com) requested changes 2026-08-27 11:18:04 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

LGTM for the code, needs work for the documentation

LGTM for the code, needs work for the documentation
ChangeLog.d/psa-its-storage-prefix.txt
@ -0,0 +1,4 @@
Features
gilles-peskine-arm (Migrated from github.com) commented 2026-08-27 11:04:35 -05:00

“where concurrent access is not supported” is ambiguous. The most natural grammatical interpretation is that “where” refers to the separate directory, and concurrent access is not supported there, with the implication that it would be supported elsewhere — although there is no semantic basis for thinking that concurrent access would be supported elsewhere, and indeed there is no other location where concurrent access is supported. It could also refer to platforms where concurrent access is not supported, as opposed to platforms where concurrent access is supported — but in fact multi-process concurrent access is not supported on any platform. What is true is that, independently of this change, multi-process concurrent access is not supported.

We don't necessarily need to mention that in the changelog entry (since that hasn't changed), but we could. Suggested wording:

Allow applications using file-based PSA ITS to configure the storage directory with MBEDTLS_PSA_ITS_STORAGE_PREFIX.

or

Allow applications using file-based PSA ITS to configure the storage directory with MBEDTLS_PSA_ITS_STORAGE_PREFIX. The default remains the current directory. Note that each concurrent process needs a separate directory.

“where concurrent access is not supported” is ambiguous. The most natural grammatical interpretation is that “where” refers to the separate directory, and concurrent access is not supported there, with the implication that it would be supported elsewhere — although there is no semantic basis for thinking that concurrent access would be supported elsewhere, and indeed there is no other location where concurrent access is supported. It could also refer to platforms where concurrent access is not supported, as opposed to platforms where concurrent access is supported — but in fact multi-process concurrent access is not supported on any platform. What is true is that, independently of this change, multi-process concurrent access is not supported. We don't necessarily need to mention that in the changelog entry (since that hasn't changed), but we could. Suggested wording: > Allow applications using file-based PSA ITS to configure the storage directory with MBEDTLS_PSA_ITS_STORAGE_PREFIX. or > Allow applications using file-based PSA ITS to configure the storage directory with MBEDTLS_PSA_ITS_STORAGE_PREFIX. The default remains the current directory. Note that each concurrent process needs a separate directory.
gilles-peskine-arm (Migrated from github.com) commented 2026-08-27 11:07:21 -05:00

“File-based PSA ITS” is correct, but it might be obsure terminology. The ostensible API feature is persistent keys combined with the compilation option MBEDTLS_PSA_ITS_FILE_C which is enabled by default. Suggested wording:

Officially provide a compile-time option MBEDTLS_PSA_ITS_STORAGE_PREFIX to select the directory for persistent keys when using the default file-based backend. …

“File-based PSA ITS” is correct, but it might be obsure terminology. The ostensible API feature is persistent keys combined with the compilation option `MBEDTLS_PSA_ITS_FILE_C` which is enabled by default. Suggested wording: > Officially provide a compile-time option MBEDTLS_PSA_ITS_STORAGE_PREFIX to select the directory for persistent keys when using the default file-based backend. …
docs/architecture/mbed-crypto-storage-specification.md
gilles-peskine-arm (Migrated from github.com) commented 2026-08-27 11:11:42 -05:00

This document keeps a historical record, because users might upgrade an existing system from an older version. The old versions referenced here did not support MBEDTLS_PSA_ITS_STORAGE_PREFIX, so these sections should not change. The information about the change needs to go into a new section.

Given that the only thing that's changing is the new option name, we don't need a full copy of the section, just the new option name and a reference to the existing section which remains fully applicable.

This document keeps a historical record, because users might upgrade an existing system from an older version. The old versions referenced here did not support `MBEDTLS_PSA_ITS_STORAGE_PREFIX`, so these sections should not change. The information about the change needs to go into a new section. Given that the only thing that's changing is the new option name, we don't need a full copy of the section, just the new option name and a reference to the existing section which remains fully applicable.
include/psa/crypto_config.h
@ -1288,2 +1301,4 @@
* Internal Trusted Storage (PSA ITS) over files.
*
* This implementation does not support concurrent access by multiple
* processes. Each process must use a separate storage directory. Configure
gilles-peskine-arm (Migrated from github.com) commented 2026-08-27 11:14:27 -05:00

We may want to mention that multithreaded access within the same process is supported.

We may want to mention that multithreaded access within the same process is supported.
bjwtaylor (Migrated from github.com) reviewed 2026-08-28 03:21:26 -05:00
docs/architecture/mbed-crypto-storage-specification.md
bjwtaylor (Migrated from github.com) commented 2026-08-28 03:21:25 -05:00

I've added a suggestion, let me know what you think.

I've added a suggestion, let me know what you think.
bjwtaylor (Migrated from github.com) reviewed 2026-08-28 03:23:16 -05:00
include/psa/crypto_config.h
@ -1288,2 +1301,4 @@
* Internal Trusted Storage (PSA ITS) over files.
*
* This implementation does not support concurrent access by multiple
* processes. Each process must use a separate storage directory. Configure
bjwtaylor (Migrated from github.com) commented 2026-08-28 03:23:16 -05:00

I've added a clarification.

I've added a clarification.
gilles-peskine-arm (Migrated from github.com) requested changes 2026-08-31 13:13:46 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

LGTM except a couple of minor documentation problems. Also the CI is unhappy.

LGTM except a couple of minor documentation problems. Also the CI is unhappy.
ChangeLog.d/psa-its-storage-prefix.txt
@ -0,0 +1,4 @@
Features
gilles-peskine-arm (Migrated from github.com) commented 2026-08-31 13:08:15 -05:00

This needs to be removed in favor of the rewritten version below.

This needs to be removed in favor of the rewritten version below.
docs/architecture/mbed-crypto-storage-specification.md
@ -470,3 +470,10 @@ The layout of a transaction file is:
## TF-PSA-Crypto 1.0
gilles-peskine-arm (Migrated from github.com) commented 2026-08-31 13:10:46 -05:00 
## TF-PSA-Crypto 1.3

### Configuration for the file namespace on stdio since TF-PSA-Crypto 1.3.0
```suggestion ## TF-PSA-Crypto 1.3 ### Configuration for the file namespace on stdio since TF-PSA-Crypto 1.3.0 ```
bjwtaylor commented 2026-09-14 04:15:36 -05:00 (Migrated from github.com)

Rebased in order to try to resolve the CI failures

Rebased in order to try to resolve the CI failures
gilles-peskine-arm (Migrated from github.com) requested changes 2026-09-17 06:07:25 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

LGTM except the changelog

LGTM except the changelog
ChangeLog.d/psa-its-storage-prefix.txt
@ -0,0 +1,4 @@
Features
gilles-peskine-arm (Migrated from github.com) commented 2026-09-17 06:06:20 -05:00

This still needs to be removed in favor of the rewritten version below.

This still needs to be removed in favor of the rewritten version below.
bjwtaylor (Migrated from github.com) reviewed 2026-09-17 09:03:23 -05:00
ChangeLog.d/psa-its-storage-prefix.txt
@ -0,0 +1,4 @@
Features
bjwtaylor (Migrated from github.com) commented 2026-09-17 09:03:23 -05:00

Apologies, I lost track of what you were after in the github thread. Hopefully I've understood it now and have uploaded the changes. Let me know what you think though?

Apologies, I lost track of what you were after in the github thread. Hopefully I've understood it now and have uploaded the changes. Let me know what you think though?
gilles-peskine-arm (Migrated from github.com) approved these changes 2026-09-17 09:31:09 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

LGTM

LGTM
This pull request is broken due to missing fork information.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 'bjwtaylor/file-backed-keystore-doc:bjwtaylor/file-backed-keystore-doc'
git switch 'bjwtaylor/file-backed-keystore-doc'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'development'
git merge --no-ff 'bjwtaylor/file-backed-keystore-doc'
git switch 'bjwtaylor/file-backed-keystore-doc'
git rebase 'development'
git switch 'development'
git merge --ff-only 'bjwtaylor/file-backed-keystore-doc'
git switch 'bjwtaylor/file-backed-keystore-doc'
git rebase 'development'
git switch 'development'
git merge --no-ff 'bjwtaylor/file-backed-keystore-doc'
git switch 'development'
git merge --squash 'bjwtaylor/file-backed-keystore-doc'
git switch 'development'
git merge --ff-only 'bjwtaylor/file-backed-keystore-doc'
git switch 'development'
git merge 'bjwtaylor/file-backed-keystore-doc'
git push origin 'development'
Sign in to join this conversation.
Reviewers
No reviewers
gilles-peskine-arm
Labels
Clear labels   
api-break
This issue/PR breaks the API and must wait for a new major version

  
approved
Design and code approved - may be waiting for CI or backports

  
bug
Something isn't working

  
component-docs
Docs / website issues filed here for tracking

  
component-platform
Portability layer and build scripts

  
component-test
Test framework and CI scripts

  
dependencies
Pull requests that update a dependency file

  
deprecation
needs deprecation announcement

  
DO-NOT-MERGE

   
enhancement
New feature or request

  
good first issue
Good for newcomers

  
help wanted
This issue is not being actively worked on, but PRs welcome.

  
needs-adoption
stalled PR that someone should pick up and complete

  
needs-backports
Backports are missing or are pending review and approval.

  
needs-ci
Needs to pass CI tests

  
needs-design-approval
Needs design discussion / approval

  
needs-info
An issue or PR which needs further info from the reporter / author

  
needs-preceding-pr
Requires another PR to be merged first

  
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

  
needs-work

   
priority-high
High priority - will be reviewed soon

  
priority-low
Low priority - this may not receive review soon

  
priority-medium
Medium priority - this can be reviewed as time permits

  
priority-scheduled
This PR is big - it will require time to be scheduled for review

  
priority-very-high
Highest priority - prioritise this over other review work

  
resolved-after-1.1
The issue is resolved in development, but still present in 1.1 LTS.

  
single-reviewer
This PR qualifies for having only one reviewer

  
size-l
Estimated task size: large (2w+)

  
size-m
Estimated task size: medium (~1w)

  
size-optimisation

   
size-s
Estimated task size: small (~2d)

  
size-xs
Estimated task size: extra small (a few hours at most)

No labels
api-break
approved
bug
component-docs
component-platform
component-test
dependencies
deprecation
DO-NOT-MERGE
enhancement
good first issue
help wanted
needs-adoption
needs-backports
needs-ci
needs-design-approval
needs-info
needs-preceding-pr
needs-review
needs-reviewer
needs-work
priority-high
priority-low
priority-medium
priority-scheduled
priority-very-high
resolved-after-1.1
single-reviewer
size-l
size-m
size-optimisation
size-s
size-xs
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-Mbed-TLS/TF-PSA-Crypto!875
No description provided.