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

ssl: fix critical extension handling regression #7846

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

askourtis
Copy link

@askourtis askourtis commented Jun 27, 2023

Description

Parsing the peer certificate did not consider critical extensions and would fail by default. Using mbedtls_x509_crt_parse_der_with_ext_cb to parse the peer certificate fixes this issue.

Users can use mbedtls_ssl_set_ext_cb and mbedtls_ssl_conf_ext_cb to provide a callback and a context for handling said extensions.

Fixes #7182

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

  • changelog provided, or not required
  • backport done, or not required
  • tests provided, or not required

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

@askourtis
Copy link
Author

Did not offer tests since the change is minor, I've set up manually an HTTP server and tested that critical extensions were handled by the callback. Please tell me if the tests are necessary, for the PR to be merged.

Signed-off-by: Antonios Skourtis <[email protected]>
@yuhaoth yuhaoth added 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 priority-high High priority - will be reviewed soon labels Oct 12, 2023
@Mbed-TLS Mbed-TLS deleted a comment from yuhaoth Jan 12, 2024
@valeriosetti valeriosetti self-requested a review November 25, 2024 16:36
@valeriosetti
Copy link
Contributor

Did not offer tests since the change is minor,

I'm not an expert in X.509, but I took a look at the PR and it looks OK to me. As far as I know it would be better to test each new feature with some test, so I would add it also to this one, but this is only my opinion. Let's see what other reviewers think about it before trying to implement one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component-tls component-tls13 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 priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot use mbedtls_x509_crt_ext_cb_t (certificate extension callback) with mbedtls_ssl_conf_verify()
4 participants