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

Correctly override connection state using sslContextCallback #499

Merged
merged 6 commits into from
Dec 18, 2024

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Dec 17, 2024

Motivation:

The sslContextCallback overrides modify the SSL_CTX object itself. That's not useful: in addition to having a global effect, which is rarely what we want, certain modifications don't apply that late, and setting the credentials is one of them. The result is that the callback didn't actually do anything. Not that useful.

Modifications:

  • Extract the cert chain setting functions from NIOSSLContext and move them to general-purpose functions.
  • Add support there for calling them on a SSL * as well as on a SSL_CTX *
  • Call them in an override function on SSLConnection
  • Call that!
  • Add a bunch of tests.

Result:

The callback actually does what it should.

Motivation:

The sslContextCallback overrides modify the SSL_CTX object
itself. That's not useful: in addition to having a global effect,
which is rarely what we want, certain modifications don't apply
that late, and setting the credentials is one of them. The result
is that the callback didn't actually do anything. Not that useful.

Modifications:

- Extract the cert chain setting functions from NIOSSLContext and
    move them to general-purpose functions.
- Add support there for calling them on a SSL * as well as on a
    SSL_CTX *
- Call them in an override function on SSLConnection
- Call that!
- Add a bunch of tests.

Result:

The callback actually does what it should.
@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Dec 17, 2024
Copy link

@zg zg left a comment

Choose a reason for hiding this comment

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

👍

@Lukasa Lukasa merged commit 3cb4d5a into apple:main Dec 18, 2024
32 of 36 checks passed
@Lukasa Lukasa deleted the cb-fix-up-cert-override branch December 18, 2024 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants