Skip to content

do not unconditionally override tls_options#2314

Open
krauro wants to merge 1 commit into
mojolicious:mainfrom
krauro:fix/override-tls-options
Open

do not unconditionally override tls_options#2314
krauro wants to merge 1 commit into
mojolicious:mainfrom
krauro:fix/override-tls-options

Conversation

@krauro
Copy link
Copy Markdown

@krauro krauro commented Apr 29, 2026

Summary

Only set the SSL_hostname option when it is not provided

Motivation

This respects the users' $ua->tls_options configuration

@tagg
Copy link
Copy Markdown

tagg commented Apr 29, 2026

Partly the same as #2148

Comment thread t/mojo/certs/client.key
Copy link
Copy Markdown
Author

@krauro krauro Apr 30, 2026

Choose a reason for hiding this comment

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

I have updated all certs to use 2048 bit keys. Otherwise on my system I was running into the following openssl error:

[format:PEM] error:0A00018F:SSL routines::ee key too small

While there are workarounds for most systems defaulting to openssl sec-level 2, I think its probably better to update the test certs here. I suspect @tagg was having trouble running the tests for the same reason

Comment thread t/mojo/ioloop_tls.t Outdated
@kraih
Copy link
Copy Markdown
Member

kraih commented Apr 30, 2026

Squash commits.

@krauro krauro force-pushed the fix/override-tls-options branch from c0de4bf to 31feedd Compare April 30, 2026 17:03
@krauro
Copy link
Copy Markdown
Author

krauro commented Apr 30, 2026

Squashed and now using a subtest block

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

Adjusts Mojo’s TLS option expansion to respect user-provided tls_options, and extends the TLS test suite/cert fixtures to cover the new behavior.

Changes:

  • Stop unconditionally overwriting SSL_hostname / SSL_verifycn_name defaults when user tls_options provide them.
  • Add new TLS tests for preserving SSL_hostname and SSL_verifycn_name overrides.
  • Refresh test certificates/keys (including adding mojo_server.{crt,key}) used by TLS tests.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

File Description
lib/Mojo/IOLoop/TLS.pm Changes how default SSL_hostname / SSL_verifycn_name are applied when tls_options are present.
t/mojo/ioloop_tls.t Adds regression tests for conditional TLS option defaults; updates cert regeneration notes.
t/mojo/certs/{ca,server,client,bad}.{crt,key} Updates TLS test fixtures to new cert/key material.
t/mojo/certs/mojo_server.{crt,key} Adds a dedicated server cert/key fixture for verifycn_name coverage.

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

Comment thread lib/Mojo/IOLoop/TLS.pm
Comment thread t/mojo/ioloop_tls.t Outdated
Comment thread t/mojo/ioloop_tls.t Outdated
@krauro krauro force-pushed the fix/override-tls-options branch 2 times, most recently from 139f43f to b29be3c Compare April 30, 2026 20:11
@krauro krauro requested a review from kraih May 4, 2026 12:20
@krauro
Copy link
Copy Markdown
Author

krauro commented May 4, 2026

The windows CI failure looks unrelated

@kraih
Copy link
Copy Markdown
Member

kraih commented May 4, 2026

You can rebase to fix the Windows failure.

@krauro krauro force-pushed the fix/override-tls-options branch from b29be3c to f97eee9 Compare May 4, 2026 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants