-
Notifications
You must be signed in to change notification settings - Fork 35
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
Fix ssl upgrade for regular host names #77
Conversation
Due to a buggy string match, SSL_hostname is always undef: matching ":" in host_port, which always has a colon between the host and the port. This leads to "certificate verify failed" openssl error (observed along with a proxy negotiating TLVv1.3). I suspect this is openssl trying to compare the hostname (which we do not pass) in the server certificate CN / SAN. Fixes regression introduced in v6.12: 6e9101b Making it possible to use IPv6 in https call...
I think regarding to comments @noxxi we need to keep the same logic. Maybe we can try to detect whether it has any double colons ( |
IMHO only host() should be used to check if it is an IP address, not host_port(). Checking for |
Ok, I see, let's make it |
If you add this, we can hopefully get a complete test run. diff --git a/Changes b/Changes
index 45cb4cf..d5af736 100644
--- a/Changes
+++ b/Changes
@@ -1,6 +1,7 @@
Release history for LWP-Protocol-https
{{$NEXT}}
+ - Fix ssl upgrade for regular host names (GH#77) (Axel Burri)
6.12 2024-01-22 17:51:31Z
- Enable MultiHomed for IO::Socket::SSL (GH#61) (ℕicolas ℝ.) |
@@ -98,7 +98,7 @@ if ( $Net::HTTPS::SSL_SOCKET_CLASS->can('start_SSL')) { | |||
my ($self,$sock,$url) = @_; | |||
# SNI should be passed there only if it is not an IP address. | |||
# Details: https://github.com/libwww-perl/libwww-perl/issues/449#issuecomment-1896175509 | |||
my $host = $url->host_port() =~ m/:|^[\d.]+$/s ? undef : $url->host(); | |||
my $host = $url->host() =~ m/:|^[\d.]+$/s ? undef : $url->host(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to add a regression test for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly I don't have much time for this right now. Also I don't know all the details on how the interaction with openssl works exactly, and in what situations this code is called (all I know is that it's used when talking to proxies, but I'm sure there's more to it).
I will keep this in the back of my head and try to come up with some tests in a quiet minute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, we've seen the same bug in Debian (in connection with an https_proxy), and the above change fixes it both for the reporter in his real enviroment and for me while testing.
That's https://bugs.debian.org/1061660 (a bit long-winded as it took me some time to figure out what the problem or the difference in my environment was -- the proxy).
While I don't claim to understand what's going on here on a deeper level and therefore can't offer any assistance in coming up with tests, I can at least confirm both the problem and that this proposed changes makes it better :)
Due to a buggy string match, SSL_hostname is always undef: matching ":" in host_port, which always has a colon between the host and the port. This leads to "certificate verify failed" openssl error (observed along with a proxy negotiating TLVv1.3).
I suspect this is openssl trying to compare the hostname (which we do not pass) in the server certificate CN / SAN.
Fixes regression introduced in v6.12:
6e9101b Making it possible to use IPv6 in https call...