Skip to content

mbedtls: split mbed_connect_step1 into sub functions#20689

Closed
bagder wants to merge 6 commits intomasterfrom
bagder/mbed-split
Closed

mbedtls: split mbed_connect_step1 into sub functions#20689
bagder wants to merge 6 commits intomasterfrom
bagder/mbed-split

Conversation

@bagder
Copy link
Copy Markdown
Member

@bagder bagder commented Feb 23, 2026

Increase readability. Reduce complexity.

@bagder bagder added the TLS label Feb 23, 2026
@bagder bagder marked this pull request as ready for review February 23, 2026 15:09
@bagder bagder requested a review from Copilot February 23, 2026 15:09
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors the mbedTLS TLS connection setup to improve readability by splitting mbed_connect_step1 into smaller, focused helper functions while preserving the overall connection flow.

Changes:

  • Extracted CA cert, client cert, private key, CRL loading, and SSL configuration into separate helper functions.
  • Reorganized mbed_connect_step1 to sequence these helpers and centralize error propagation.
  • Minor formatting/line-wrapping adjustments for comments and long calls.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@testclutch
Copy link
Copy Markdown

Analysis of PR #20689 at 8edf4f78:

Test ../../tests/http/test_07_upload.py::TestUpload::test_07_13_upload_seq_large[h2] failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them).

Generated by Testclutch

@bagder
Copy link
Copy Markdown
Member Author

bagder commented Feb 23, 2026

augment review

@augmentcode
Copy link
Copy Markdown

augmentcode bot commented Feb 23, 2026

🤖 Augment PR Summary

Summary: Refactors the mbedTLS backend’s connection setup by splitting mbed_connect_step1() into smaller helper functions to improve readability and reduce complexity.

Changes:

  • Extracted CA trust store loading into mbed_load_cacert()
  • Extracted client certificate handling into mbed_load_clicert()
  • Extracted private key parsing into mbed_load_privkey()
  • Extracted optional CRL loading into mbed_load_crl()
  • Moved SSL config/setup and session-cache reuse logic into mbed_configure_ssl()
  • Adjusted the error message for client certificate blob parsing and did minor formatting/variable cleanups

Technical Notes: The refactor keeps the same high-level connect state machine, but touches the ordering/propagation of return codes around session cache reuse and the optional fsslctx callback.

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

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

Review completed. 1 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

@bagder
Copy link
Copy Markdown
Member Author

bagder commented Feb 24, 2026

@aisle-analyzer

@aisle-research-bot
Copy link
Copy Markdown

aisle-research-bot bot commented Feb 24, 2026

🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

# Severity Title
1 🔵 Low Out-of-bounds read on zero-length certificate blobs in mbedTLS backend

See details in the comment below.


Analyzed PR: #20689 at commit c7e4514

bagder added a commit that referenced this pull request Feb 24, 2026
A blob must have a length or it will not be accepted. By enforcing the
check here, TLS backend code can rely on the fact that the length is
always non-zero from this point.

Reported-by: aisle-research-bot
URL: #20689 (review)
bagder added a commit that referenced this pull request Feb 24, 2026
A blob must have a length or it will not be accepted. By enforcing the
check here, TLS backend code can rely on the fact that the length is
always non-zero from this point.

Reported-by: aisle-research-bot
URL: #20689 (review)
Closes #20705
@bagder bagder closed this in 7981594 Feb 24, 2026
@bagder bagder deleted the bagder/mbed-split branch February 24, 2026 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

3 participants