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.

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

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

Conversation

NadavTasher
Copy link

@NadavTasher NadavTasher commented Nov 23, 2024

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

Signed-off-by: Nadav Tasher <tashernadav@gmail.com>
@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
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.

@@ -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.

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.

@@ -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 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
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
Development

Successfully merging this pull request may close these issues.

3 participants