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

Remove MBEDTLS_SSL_EXPORT_KEYS, making it always on #4989

Merged

Conversation

AndrzejKurek
Copy link
Contributor

@AndrzejKurek AndrzejKurek commented Sep 28, 2021

This option only gated an ability to set a callback, but was deemed unnecessary as it was yet another define to remember when writing tests, or test configurations.
Fixes #4653.
Fixes #3998.

Andrzej Kurek added 2 commits September 29, 2021 10:15
This option only gated an ability to set a callback,
but was deemed unnecessary as it was yet another define to
remember when writing tests, or test configurations. Fixes Mbed-TLS#4653.
Signed-off-by: Andrzej Kurek <[email protected]>
When not using DEBUG_C, but using the DTLS CID feature -
a null pointer was accessed in ssl_tls.c.
Signed-off-by: Andrzej Kurek <[email protected]>
@AndrzejKurek AndrzejKurek added Product Backlog enhancement component-tls 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 Sep 30, 2021
@AndrzejKurek AndrzejKurek added size-s Estimated task size: small (~2d) and removed size-s Estimated task size: small (~2d) labels Sep 30, 2021
@davidhorstmann-arm davidhorstmann-arm self-requested a review October 4, 2021 14:25
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-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!

Copy link
Contributor

@mprse mprse left a comment

Choose a reason for hiding this comment

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

No issues found.
LGTM

@AndrzejKurek AndrzejKurek added needs-ci Needs to pass CI tests 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 Oct 6, 2021
@davidhorstmann-arm
Copy link
Contributor

davidhorstmann-arm commented Oct 8, 2021

I believe the failures are related to the ongoing CI issue.

@davidhorstmann-arm davidhorstmann-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-ci Needs to pass CI tests labels Oct 14, 2021
@gilles-peskine-arm gilles-peskine-arm merged commit 6210320 into Mbed-TLS:development Oct 18, 2021
make

msg "test: Connection ID enabled, debug disabled"
make test
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this actually a non-regression test? In 2.28, the bug (which I'll fix in #5730) isn't triggered by unit tests, only by compat.sh.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is a non-regression test in 3.x. Without the bug fix:

Test moving clients handshake to state: CLIENT_CHANGE_CIPHER_SPEC . Segmentation fault (core dumped)

On the other hand, in 2.28, test_suite_ssl passes in this configuration, even though the corresponding test exists. I don't know why. The level of support for CID looks the same in both branches.

gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this pull request Apr 13, 2022
The fix was in Mbed-TLS#4989.
We forgot to add a changelog entry.

Signed-off-by: Gilles Peskine <[email protected]>
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-tls enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove MBEDTLS_SSL_EXPORT_KEYS Null pointer usage in ssl_tls.c in a non-default config
4 participants