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

mbedtls: define MBEDTLS_SSL_CID_TLS1_3_PAD_GRANULARITY for CID padding #12177

Closed
wants to merge 1 commit into from
Closed

mbedtls: define MBEDTLS_SSL_CID_TLS1_3_PAD_GRANULARITY for CID padding #12177

wants to merge 1 commit into from

Conversation

hasheddan
Copy link
Contributor

@hasheddan hasheddan commented Sep 1, 2023 •
edited
Loading

Updates config to define the new MBEDTLS_SSL_CID_TLS1_3_PAD_GRANULARITY
option, which replaced the previously used
MBEDTLS_SSL_CID_PADDING_GRANULARITY. The old option is continuing to be
used as the new one exceeds the maximum length for an option name in
esp-idf.

See Mbed-TLS/mbedtls#4490 for more information.

Signed-off-by: Daniel Mangum georgedanielmangum@gmail.com

Note: enabling MBEDTLS_SSL_CID_PADDING_GRANULARITY without this fix currently breaks builds.

@hasheddan hasheddan mentioned this pull request Sep 1, 2023
Merged
hasheddan
hasheddan commented Sep 1, 2023
View reviewed changes
Copy link
Contributor Author

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

Hmm seeing the following:

MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY is 42 characters long and it should be 40 at most

Anyway to override so that we can match the underlying mbedtls config?

@espressif-bot espressif-bot added the Status: Opened Issue is new label Sep 1, 2023
@github-actions github-actions bot changed the title mbedtls: use MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY for CID padding mbedtls: use MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY for CID padding (IDFGH-10989) Sep 1, 2023
@laukik-hase
Copy link
Collaborator

Hello, @hasheddan!

Thanks for the contribution!

  • You would have to shorten the Kconfig option name to 40 characters (e.g. MBEDTLS_SSLCID_TLS13_PADDING_GRANULARITY or MBEDTLS_SSL_CID_TLS1_3_PAD_GRANULARITY). The pre-commit hook does not allow Kconfig option names to be greater than 40 characters.
  • Could you also rebase the PR to the latest master branch?

@hasheddan
Copy link
Contributor Author

You would have to shorten the Kconfig option name to 40 characters

@laukik-hase is there any possibility of overriding the pre-commit hook? I think it would be unfortunate to not mirror the mbedTLS option here.

@espressif-bot espressif-bot added Status: In Progress Work is in progress and removed Status: Opened Issue is new labels Sep 5, 2023
hasheddan
hasheddan commented Sep 6, 2023
View reviewed changes
Copy link
Contributor Author

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

@laukik-hase any updates here? Is shortening the config option an absolute requirement? Would love to get this merged as Connection ID support is currently broken without it.

@igrr
Copy link
Member

igrr commented Sep 6, 2023

I'm afraid no, this limit is currently enforced globally and I would prefer not to change it just for this case. Please see if you can shorten the name.

Additionally, since you are renaming a Kconfig option, to ensure compatibility please create components/mbedtls/sdkconfig.rename and add the old and the new option name there (docs, example).

@hasheddan
Copy link
Contributor Author

@igrr in that case, would it make sense to just keep the previous option and have it select the new one?

@hasheddan
mbedtls: define MBEDTLS_SSL_CID_TLS1_3_PAD_GRANULARITY for CID padding
7c63769 
Updates config to define the new MBEDTLS_SSL_CID_TLS1_3_PAD_GRANULARITY
option, which replaced the previously used
MBEDTLS_SSL_CID_PADDING_GRANULARITY. The old option is continuing to be
used as the new one exceeds the maximum length for an option name in
esp-idf.

See Mbed-TLS/mbedtls#4490 for more information.

Signed-off-by: Daniel Mangum <georgedanielmangum@gmail.com>
@hasheddan
Copy link
Contributor Author

@igrr I have pushed an update here reflecting the use of the existing option to define the new mbedTLS option

@hasheddan hasheddan changed the title mbedtls: use MBEDTLS_SSL_CID_TLS1_3_PADDING_GRANULARITY for CID padding (IDFGH-10989) mbedtls: define MBEDTLS_SSL_CID_TLS1_3_PAD_GRANULARITY for CID padding Sep 6, 2023
@laukik-hase
Copy link
Collaborator

sha=7c63769edefda724ef5c7f7d5e20b33f9e3036ee

@laukik-hase laukik-hase added the PR-Sync-Merge Pull request sync as merge commit label Sep 7, 2023
@hasheddan
Copy link
Contributor Author

@igrr @laukik-hase anything else needed here to merge?

@laukik-hase
Copy link
Collaborator

@hasheddan, the PR has been merged internally - it will close automatically when it is synced to GitHub.

@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: In Progress Work is in progress labels Sep 11, 2023
@laukik-hase
Copy link
Collaborator

laukik-hase commented Sep 13, 2023 •
edited
Loading

Merged with a57c8dc, closing this PR!

@mahavirj mahavirj mentioned this pull request Jan 4, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@laukik-hase laukik-hase

Labels
PR-Sync-Merge Pull request sync as merge commit Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hasheddan @laukik-hase @igrr @espressif-bot