Skip to content

Remove use of pk_debug() #10517

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
Open
valeriosetti wants to merge 8 commits into
base: development
Choose a base branch
from
Open

Remove use of pk_debug() #10517

valeriosetti wants to merge 8 commits into from
+63 −375

Conversation

@valeriosetti
Copy link
Contributor

@valeriosetti valeriosetti commented Nov 19, 2025 •
edited
Loading

Description

Resolves #10460.
Depends on Mbed-TLS/TF-PSA-Crypto#577.
Follow-up is Mbed-TLS/TF-PSA-Crypto#617

PR checklist

@valeriosetti valeriosetti added needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first 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 labels Nov 19, 2025
mpg
mpg requested changes Dec 11, 2025
View reviewed changes
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.

Looks pretty good to me. Just one question and one request:

  1. Can you clarify the motivation for the last-but-one commit?
  2. Looking at the test data changes in the last commit, I realise the format I was suggesting is missing some info that could be useful: I think printing the type and bitsize of the key, in addition to the key material, would make the output more helpful. (That info was present in the old output and is now much less apparent.)

@valeriosetti
tf-psa-crypto: update reference
8b63241 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
library: debug: remove mbedtls_debug_print_mpi()
c47389e 
This function is no more used anywhere and can be safely removed.

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
library: debug: use mbedtls_pk_write_pubkey_psa() to write public key
6b9d94a 
Remove usage of mbedtls_pk_debug stuff and related functions
(mbedtls_debug_print_psa_rsa(), mbedtls_debug_print_psa_ec(),
mbedtls_debug_print_integer() and debug_count_valid_bits()) and use
mbedtls_pk_write_pubkey_psa() to get the public key from the PK context.

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
library: debug: improve mbedtls_debug_print_buf()
b4eb6b4 
Move single line printing to a separate function named
mbedtls_debug_print_buf_one_line(). This accepts one extra parameter
'add_text' to tell if the final text chars are to be printed at the end
of the line or not.

Add also mbedtls_debug_print_buf_ext() as a generalized version of
mbedtls_debug_print_buf() by adding the extra 'add_text' param.

debug_print_pk() will now use mbedtls_debug_print_buf_ext() in order not
to print chars while dumping the buffer.

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
tests: scripts: adjust CRT dump test data
292df93 
Adjust dumping format of public keys following recent updates to
mbedtls_debug_print_crt() and debug_print_pk()

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
Copy link
Contributor Author

valeriosetti commented Dec 15, 2025 •
edited
Loading

2. Looking at the test data changes in the last commit, I realise the format I was suggesting is missing some info that could be useful: I think printing the type and bitsize of the key, in addition to the key material, would make the output more helpful. (That info was present in the old output and is now much less apparent.)

I was working to fix this when I realized that actually the information is already available. For example in test case Debug print certificate #1 (RSA) the following lines are being printed:

MyFile(0999): signed using      : RSA with SHA1
MyFile(0999): RSA key size      : 2048 bits

while in Debug print certificate #2 (EC) the following is printed:

MyFile(0999): signed using      : ECDSA with SHA256
MyFile(0999): EC key size       : 384 bits

Isn't this a bit redundant?

Edit: as far as I can tell, these info are printed for every certificate on the chain, so we are not missing anything also in the current state.

@valeriosetti
Copy link
Contributor Author

valeriosetti commented Dec 15, 2025

The last rebase was to include recent changes in tf-psa-crypto (addition of mbedtls_pk_write_public_key) and to make the CI happy.

@valeriosetti valeriosetti requested a review from mpg December 15, 2025 17:17
@valeriosetti valeriosetti added needs-review Every commit must be reviewed by at least two team members, and removed needs-reviewer This PR needs someone to pick it up for review needs-preceding-pr Requires another PR to be merged first labels Dec 15, 2025
@mpg
Copy link
Contributor

mpg commented Dec 16, 2025

Edit: as far as I can tell, these info are printed for every certificate on the chain, so we are not missing anything also in the current state.

Ah yes, good point, I completely missed that the info is already available, just 2 lines above. I withdraw my request for adding it then, indeed it would be redundant.

mpg
mpg reviewed Dec 16, 2025
View reviewed changes
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 on code inspection but there are CI failures which look real and related to this PR.

@valeriosetti
Copy link
Contributor Author

valeriosetti commented Dec 16, 2025 •
edited
Loading

LGTM on code inspection but there are CI failures which look real and related to this PR.

Ouch, the problem is that mbedtls_pk_write_pubkey_psa is guarded by MBEDTLS_PK_WRITE_C, but this is not set in suite-b :(

@valeriosetti
library: debug: add PK_WRITE_C guard to mbedtls_debug_print_crt()
d2a36da 
In tf-psa-crypto "mbedtls_pk_write_pubkey_psa()" is only available when
MBEDTLS_PK_WRITE_C is defined. Therefore we need to add this guard also
in mbedtls to "debug_print_pk" (and indirectly to
"mbedtls_debug_print_crt") and the corresponding tests using it.

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
mpg
mpg previously approved these changes Dec 16, 2025
View reviewed changes
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.

Fixed dependencies look good to me, let's hope the CI agrees :)

@valeriosetti
include: debug: fix guards for MBEDTLS_SSL_DEBUG_CRT
6464fd2 
Guards for "mbedtls_debug_print_crt()" were updated in previous commit,
but those changes were not applied to MBEDTLS_SSL_DEBUG_CRT therefore
causing build failures in the CI. This commit fixes the problem.

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti valeriosetti requested a review from mpg December 16, 2025 13:44
@valeriosetti
Copy link
Contributor Author

valeriosetti commented Dec 16, 2025

Fixed dependencies look good to me, let's hope the CI agrees :)

Sorry the previous last commit didn't properly fix all the guards. I added a new one which hopefully should make the CI fully happy.

@valeriosetti
library: debug: fix print format in mbedtls_debug_print_buf_ext()
48af8f0 
%zu creates problem in MinGW testing. Use MBEDTLS_PRINTF_SIZET intead.

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti valeriosetti force-pushed the issue10460 branch 2 times, most recently from 7148c6a to 48af8f0 Compare December 18, 2025 17:36
@valeriosetti valeriosetti removed the needs-ci Needs to pass CI tests label Dec 19, 2025
@valeriosetti
Copy link
Contributor Author

valeriosetti commented Dec 19, 2025

Thanks to @gilles-peskine-arm help I was able to fix remaining issues with MinGW testing (see this commit).

@mpg @davidhorstmann-arm since testing is fine and there is no conflict, I think this is finally ready for another round of reviews

@valeriosetti valeriosetti mentioned this pull request Dec 19, 2025
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidhorstmann-arm davidhorstmann-arm Awaiting requested review from davidhorstmann-arm

@mpg mpg Awaiting requested review from mpg

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

needs-review Every commit must be reviewed by at least two team members, priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)

Projects

Roadmap pull requests (new board)
Status: In Development

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove use of pk_debug()

2 participants

@valeriosetti @mpg