Fix module configuration options in mbedtls_config.h #8161
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Boolean options that modify the behavior of a module are supposed to be in the "feature support" section, not in the "configuration options" support: that section is documented to contain commented-out definitions with a value, for which the comment contains the default version. In particular, merely uncommenting a definition in the "configuration options" section is not supposed to change anything. Move the offending boolean options to the proper section. This causes those options to be enabled by `config.py full` unless explicitly excluded. For all the offending options, this is undesirable, so make sure those options are indeed excluded. Signed-off-by: Gilles Peskine <[email protected]>
In particular, pre-shared keys are supported. Signed-off-by: Gilles Peskine <[email protected]>
Mbed TLS can be configured by writing a configuration file from scratch, without copying mbedtls_config.h. As a consequence, all the macro definitions in mbedtls_config.h must be optional. This was not the case for some MBEDTLS_SSL_TLS1_3_xxx macros with numerical values related to session tickets. Fix that. Signed-off-by: Gilles Peskine <[email protected]>
Fix unused variable when MBEDTLS_SSL_PROTO_TLS1_3 and MBEDTLS_SSL_SESSION_TICKETS are enabled but not MBEDTLS_DEBUG_C. Signed-off-by: Gilles Peskine <[email protected]>
This is not required (it's ok to define the default in mbedtls_config and skip the definition in rsa.h), but comment it out for uniformity with all the other options in this section. Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm
added
bug
needs-backports
gilles-peskine-arm
added
needs-review
daverodgman
reviewed
Sep 7, 2023
daverodgman
previously approved these changes
Sep 7, 2023
daverodgman
added
the
single-reviewer
If that bothers you enough to do another review, I'll rebase #8034 once this one is merged. |
The documentation was not updated when we started detecting memset_s() and such. Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm
force-pushed
the
config-boolean-options-wrong-section-202309
branch
from
8ca7f87 to
8673383
Compare
|
I amended the last commit to fix a doxygen syntax error. |
daverodgman
approved these changes
Sep 7, 2023
3 tasks
| #endif | ||
|
|
||
| #if !defined(MBEDTLS_SSL_TLS1_3_TICKET_NONCE_LENGTH) | ||
| #define MBEDTLS_SSL_TLS1_3_TICKET_NONCE_LENGTH 32 |
There was a problem hiding this comment.
The disadvantage of doing it this way is to change the default one now needs to make identical changes in two places
There was a problem hiding this comment.
Indeed. But in practice, I don't recall us changing the default for any of thse options in the past 6½ years.
Anyway, if we changed the organization, this pull request wouldn't be the place to do it. It would require some thought about the new design and backward compatibility.
tom-cosgrove-arm
added
approved
tom-cosgrove-arm
added a commit
to tom-cosgrove-arm/mbedtls
that referenced
this pull request
Sep 14, 2023
…t in config Numeric options should be commented out with their default values in the config file, and a separate header file should set the default value if necessary. This was done for most other options in Mbed-TLS#8161; do it here for MBEDTLS_SSL_MAX_EARLY_DATA_SIZE. Signed-off-by: Tom Cosgrove <[email protected]>
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix several issues in the “Module configuration options” section of
mbedtls_config.h:MBEDTLS_SSL_PROTO_TLS1_3andMBEDTLS_SSL_SESSION_TICKETS)?Priority: I discovered this during the p256-m review and it's a bug in the p256-m integration, so I'm rating this priority-high, but it isn't a release blocker, and I could bend this to priority-medium since the only consequence is some mild user confusion.
PR checklist
Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")