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

Added debug print in tls13 ssl_tls13_write_key_share_ext #9798

Open
wants to merge 1 commit into
base: development
Choose a base branch
from
Open

Added debug print in tls13 ssl_tls13_write_key_share_ext #9798

wants to merge 1 commit into from

Conversation

NadavTasher
Copy link

@NadavTasher NadavTasher commented Nov 23, 2024 •
edited by gilles-peskine-arm
Loading

Description

Added a useful debug print in tls13 client hello (helped me debug a faulty config)

PR checklist

@tom-daubney-arm
Copy link
Contributor

Hi @NadavTasher , the CI is failing because the DCO check is not passing. Can you please resubmit with a signed off commit? e.g. git commit -s. Thanks

@NadavTasher NadavTasher force-pushed the feature/more-debug-prints branch from b9a0cd1 to 8bfa04a Compare November 25, 2024 22:51
@NadavTasher
Copy link
Author

@tom-daubney-arm Done.

@tom-daubney-arm tom-daubney-arm added enhancement 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-tls13 priority-medium Medium priority - this can be reviewed as time permits labels Nov 27, 2024
@tom-daubney-arm tom-daubney-arm self-requested a review November 27, 2024 09:04
tom-daubney-arm
tom-daubney-arm approved these changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@tom-daubney-arm tom-daubney-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 - thanks for your contribution.

gilles-peskine-arm
gilles-peskine-arm reviewed Nov 29, 2024
View reviewed changes
library/ssl_tls13_client.c
@@ -318,6 +318,7 @@ static int ssl_tls13_write_key_share_ext(mbedtls_ssl_context *ssl,
ssl, group_id, p, end, &key_exchange_len);
p += key_exchange_len;
if (ret != 0) {
MBEDTLS_SSL_DEBUG_MSG(1, ("client hello: failed generating xxdh key exchange"));
Copy link
Contributor

Choose a reason for hiding this comment

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

It's usually useful to log the error code too, we have a macro for that. Also, when debugging the library, it's convenient to have the function name.

Suggested change
MBEDTLS_SSL_DEBUG_MSG(1, ("client hello: failed generating xxdh key exchange"));
MBEDTLS_SSL_DEBUG_RET(1, "mbedtls_ssl_tls13_generate_and_write_xxdh_key_exchange", ret);

Copy link
Author

Choose a reason for hiding this comment

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

I agree that the MBEDTLS_SSL_DEBUG_RET should be used, but isn't it better to keep the text as is?
Seems like it gives a more literal description of the error - when someone will debug the library they will end up Ctrl + Fing the string anyway...

Copy link
Contributor

Choose a reason for hiding this comment

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

MBEDTLS_SSL_DEBUG_RET would be consistent with the rest of the code base, and gives the location of the failure plus the error code. But that's no big deal.

gilles-peskine-arm
gilles-peskine-arm approved these changes Dec 9, 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 — should be backported to 3.6 for consistency, I'll go ahead and do that.

library/ssl_tls13_client.c
@@ -318,6 +318,7 @@ static int ssl_tls13_write_key_share_ext(mbedtls_ssl_context *ssl,
ssl, group_id, p, end, &key_exchange_len);
p += key_exchange_len;
if (ret != 0) {
MBEDTLS_SSL_DEBUG_MSG(1, ("client hello: failed generating xxdh key exchange"));
Copy link
Contributor

Choose a reason for hiding this comment

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

MBEDTLS_SSL_DEBUG_RET would be consistent with the rest of the code base, and gives the location of the failure plus the error code. But that's no big deal.

@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Dec 9, 2024
Open
5 tasks
@gilles-peskine-arm gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports needs-backports Backports are missing or are pending review and approval. 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 Dec 9, 2024
@NadavTasher
Copy link
Author

Can anyone merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tom-daubney-arm tom-daubney-arm tom-daubney-arm approved these changes

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports component-tls13 enhancement needs-backports Backports are missing or are pending review and approval. priority-medium Medium priority - this can be reviewed as time permits
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NadavTasher @tom-daubney-arm @gilles-peskine-arm