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

Remove des from tf-psa-crypto #475

Merged
bjwtaylor merged 22 commits from remove-des into development 2025-09-29 08:27:01 -05:00
Conversation 15 Commits 22 Files changed 43 +102 -3451
bjwtaylor commented 2025-09-24 06:52:40 -05:00 (Migrated from github.com)

Description

Remove des from tf-psa-crypto contributes to https://github.com/Mbed-TLS/mbedtls/issues/9164

PR checklist

## Description Remove des from tf-psa-crypto contributes to https://github.com/Mbed-TLS/mbedtls/issues/9164 ## PR checklist - [ ] **changelog** provided - [ ] **framework PR** not required - [ ] **mbedtls development PR** provided Mbed-TLS/mbedtls# https://github.com/Mbed-TLS/mbedtls/pull/10413 - [ ] **mbedtls 3.6 PR** not required because: No backports - **tests** provided
gilles-peskine-arm (Migrated from github.com) requested changes 2025-09-26 04:35:46 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

LGTM except for a couple of documentation issues.

LGTM except for a couple of documentation issues.
ChangeLog.d/remove-des.txt
@ -0,0 +1,2 @@
Removals
gilles-peskine-arm (Migrated from github.com) commented 2025-09-26 04:34:56 -05:00

Spelling: “it's” → “its”, although I don't see what “and all its references” adds to the sentence.

Also, I'm not sure the fact that it's no longer allowed by NIST is really important here (after all, we still support SHA-1, and Camellia which has never been allowed by NIST). I think I'd just say “Removed DES (including 3DES).”

Spelling: “it's” → “its”, although I don't see what “and all its references” adds to the sentence. Also, I'm not sure the fact that it's no longer allowed by NIST is really important here (after all, we still support SHA-1, and Camellia which has never been allowed by NIST). I think I'd just say “Removed DES (including 3DES).”
gilles-peskine-arm (Migrated from github.com) commented 2025-09-26 04:35:18 -05:00

Please remove the mentions of DES in docs/driver-only-builds.md.

Please remove the mentions of DES in `docs/driver-only-builds.md`.
include/psa/crypto_compat.h
@ -39,0 +40,4 @@
* tests. Without it building the tests will fail. To remove it we would
* need to change the tests to remove all references to this symbol.
*/
#define PSA_KEY_TYPE_DES ((psa_key_type_t) 0x2301)
gilles-peskine-arm (Migrated from github.com) commented 2025-09-26 03:55:21 -05:00

Please add a comment explaining why this is here and what it would take to remove it.

Please add a comment explaining why this is here and what it would take to remove it.
tests/suites/test_suite_pkcs5.data
@ -169,3 +191,3 @@
PBES2 Decrypt (KDF != PBKDF2)
depends_on:PSA_WANT_ALG_SHA_1:MBEDTLS_DES_C
depends_on:PSA_WANT_ALG_SHA_1:MBEDTLS_AES_C
pbes2_decrypt:MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE:"300B06092A864886F70D01050D":"":"":0:MBEDTLS_ERR_PKCS5_FEATURE_UNAVAILABLE:""
gilles-peskine-arm (Migrated from github.com) commented 2025-09-26 04:30:50 -05:00

I wish there were explanations of how the test data was constructed, but that's preexisting, so ok. I checked in a debugger that the changed test cases still seem to hit the intended error detection point.

I wish there were explanations of how the test data was constructed, but that's preexisting, so ok. I checked in a debugger that the changed test cases still seem to hit the intended error detection point.
bjwtaylor (Migrated from github.com) reviewed 2025-09-26 04:40:30 -05:00
include/psa/crypto_compat.h
@ -39,0 +40,4 @@
* tests. Without it building the tests will fail. To remove it we would
* need to change the tests to remove all references to this symbol.
*/
#define PSA_KEY_TYPE_DES ((psa_key_type_t) 0x2301)
bjwtaylor (Migrated from github.com) commented 2025-09-26 04:40:30 -05:00

Fair point is it also worth raising this as a bug, or is this overkill?

Fair point is it also worth raising this as a bug, or is this overkill?
bjwtaylor (Migrated from github.com) reviewed 2025-09-26 04:46:22 -05:00
include/psa/crypto_compat.h
@ -39,0 +40,4 @@
* tests. Without it building the tests will fail. To remove it we would
* need to change the tests to remove all references to this symbol.
*/
#define PSA_KEY_TYPE_DES ((psa_key_type_t) 0x2301)
bjwtaylor (Migrated from github.com) commented 2025-09-26 04:46:22 -05:00

I've added a comment, let me know what you think and if we need a bug report.

I've added a comment, let me know what you think and if we need a bug report.
bjwtaylor (Migrated from github.com) reviewed 2025-09-26 04:49:01 -05:00
tests/suites/test_suite_pkcs5.data
@ -169,3 +191,3 @@
PBES2 Decrypt (KDF != PBKDF2)
depends_on:PSA_WANT_ALG_SHA_1:MBEDTLS_DES_C
depends_on:PSA_WANT_ALG_SHA_1:MBEDTLS_AES_C
pbes2_decrypt:MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE:"300B06092A864886F70D01050D":"":"":0:MBEDTLS_ERR_PKCS5_FEATURE_UNAVAILABLE:""
bjwtaylor (Migrated from github.com) commented 2025-09-26 04:49:01 -05:00

Now you mention it, I also would have found this useful at the beginning of this task. So I'll add a description of how I generated the strings as it maybe useful for someone in the future.

Now you mention it, I also would have found this useful at the beginning of this task. So I'll add a description of how I generated the strings as it maybe useful for someone in the future.
gilles-peskine-arm (Migrated from github.com) reviewed 2025-09-26 05:00:09 -05:00
include/psa/crypto_compat.h
@ -39,0 +40,4 @@
* tests. Without it building the tests will fail. To remove it we would
* need to change the tests to remove all references to this symbol.
*/
#define PSA_KEY_TYPE_DES ((psa_key_type_t) 0x2301)
gilles-peskine-arm (Migrated from github.com) commented 2025-09-26 05:00:09 -05:00

In principle we should file an issue. For only one mostly harmless line, it's not very important. Also I've made a note at https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/453/files#r2381753897, if that PR gets rebased then I can take the opportunity to remove the line.

In principle we should file an issue. For only one mostly harmless line, it's not very important. Also I've made a note at https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/453/files#r2381753897, if that PR gets rebased then I can take the opportunity to remove the line.
bjwtaylor (Migrated from github.com) reviewed 2025-09-26 05:16:31 -05:00
tests/suites/test_suite_pkcs5.data
@ -169,3 +191,3 @@
PBES2 Decrypt (KDF != PBKDF2)
depends_on:PSA_WANT_ALG_SHA_1:MBEDTLS_DES_C
depends_on:PSA_WANT_ALG_SHA_1:MBEDTLS_AES_C
pbes2_decrypt:MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE:"300B06092A864886F70D01050D":"":"":0:MBEDTLS_ERR_PKCS5_FEATURE_UNAVAILABLE:""
bjwtaylor (Migrated from github.com) commented 2025-09-26 05:16:31 -05:00

I've added some documentation, let me know what you think.

I've added some documentation, let me know what you think.
gilles-peskine-arm (Migrated from github.com) reviewed 2025-09-26 05:55:53 -05:00
tests/suites/test_suite_pkcs5.data
@ -169,3 +191,3 @@
PBES2 Decrypt (KDF != PBKDF2)
depends_on:PSA_WANT_ALG_SHA_1:MBEDTLS_DES_C
depends_on:PSA_WANT_ALG_SHA_1:MBEDTLS_AES_C
pbes2_decrypt:MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE:"300B06092A864886F70D01050D":"":"":0:MBEDTLS_ERR_PKCS5_FEATURE_UNAVAILABLE:""
gilles-peskine-arm (Migrated from github.com) commented 2025-09-26 05:55:52 -05:00

Thanks for writing this up!

Thanks for writing this up!
gilles-peskine-arm (Migrated from github.com) reviewed 2025-09-26 05:55:59 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment
No description provided.
LGTM
gilles-peskine-arm (Migrated from github.com) reviewed 2025-09-26 07:41:20 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment
No description provided.
LGTM
valeriosetti (Migrated from github.com) requested changes 2025-09-26 10:30:55 -05:00
valeriosetti (Migrated from github.com) left a comment

I also see that the framework reference is updated in this PR, but I don't see any PR in framework being linked in description. Perhaps this was intentional and you did that to fix some problem with the CI, but I just want to be sure this was not done by accident :)

I also see that the `framework` reference is updated in this PR, but I don't see any PR in `framework` being linked in description. Perhaps this was intentional and you did that to fix some problem with the CI, but I just want to be sure this was not done by accident :)
tests/suites/test_suite_pkcs5.function
valeriosetti (Migrated from github.com) commented 2025-09-26 10:29:02 -05:00

Is this something related to the test function parsing/integration mechanism (that I'm not aware of)? Or is it a leftover from debug session?

Is this something related to the test function parsing/integration mechanism (that I'm not aware of)? Or is it a leftover from debug session?
bjwtaylor (Migrated from github.com) reviewed 2025-09-29 01:38:01 -05:00
tests/suites/test_suite_pkcs5.function
bjwtaylor (Migrated from github.com) commented 2025-09-29 01:38:01 -05:00

Suspect it's leftover debug, it should now be removed.

Suspect it's leftover debug, it should now be removed.
bjwtaylor commented 2025-09-29 01:47:25 -05:00 (Migrated from github.com)

@valeriosetti, the framework update was done intentionally as I was seeing some build errors that the update resolved. I am now unable to recreate them, so it's possible a rebase has fixed them. Not sure if updating it is doing any harm and if it needs removal or if it can be kept?

@valeriosetti, the framework update was done intentionally as I was seeing some build errors that the update resolved. I am now unable to recreate them, so it's possible a rebase has fixed them. Not sure if updating it is doing any harm and if it needs removal or if it can be kept?
valeriosetti commented 2025-09-29 04:10:25 -05:00 (Migrated from github.com)

@valeriosetti, the framework update was done intentionally as I was seeing some build errors that the update resolved. I am now unable to recreate them, so it's possible a rebase has fixed them. Not sure if updating it is doing any harm and if it needs removal or if it can be kept?

I checked and, unless I'm mistaken, I would say that:

  • the framework hash pointed by this PR is ab4d9cee6
  • the ones pointed by development is 0bfaf0ed9

and the reference in this PR is newer than the one in development so this change is OK to me.

> @valeriosetti, the framework update was done intentionally as I was seeing some build errors that the update resolved. I am now unable to recreate them, so it's possible a rebase has fixed them. Not sure if updating it is doing any harm and if it needs removal or if it can be kept? I checked and, unless I'm mistaken, I would say that: - the `framework` hash pointed by this PR is ab4d9cee6 - the ones pointed by `development` is 0bfaf0ed9 and the reference in this PR is newer than the one in `development` so this change is OK to me.
🚀 1
valeriosetti (Migrated from github.com) approved these changes 2025-09-29 04:11:23 -05:00
gilles-peskine-arm (Migrated from github.com) approved these changes 2025-09-29 06:45:56 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

LGTM

LGTM
Sign in to join this conversation.
Reviewers
No reviewers
valeriosetti
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!475
No description provided.