noxxi / p5-io-socket-ssl

IO::Socket::SSL Perl Module
36 stars 59 forks source link

implicit derivation of hostname for SNI should be less astonishing #16

Closed jcflack closed 10 years ago

jcflack commented 10 years ago

The addition of SNI support here: https://github.com/noxxi/p5-io-socket-ssl/commit/4f83a3cd85458bd2141f0a9f22f787174d51d587#diff-1 created one explicit way for the caller to supply the host name for which the certificate is wanted (supply it with the key SSL_hostname).

It also added a default behavior if SSL_hostname isn't specified, but that default behavior is almost surely not what the caller expects! If the caller passes both a PeerAddr and a PeerHost, the code looks at the PeerAddr first ... and then discards it if it looks like an address instead of a hostname ... and ignores the PeerHost!

It is probably better to look at PeerHost for a hostname first ... and maybe it is even best to look at both, and accept either one if it has the form of a hostname and not an address.

jcflack commented 10 years ago

I've reported closely-related issue csirtgadgets/LWPx-ParanoidAgent#14 (it breaks because it passes PeerHost to this module expecting it to work).

Naturally, the problem interaction could be fixed either by having that module pass SSL_hostname explicitly, or having this module not ignore PeerHost. I've opened issues both places because I think both changes would make sense.

noxxi commented 10 years ago

IO::Socket::SSL uses IO::Socket::INET, IO::Socket::INET6 or IO::Socket::IP as the backend and unfortunately these modules differ slightly in the use of these keys:

But, both say that PeerAddr and PeerHost are just a different name for the same thing and both ignore if you give different settings to both values (but they ignore it in a different way).

Internally INET and INET6 leave PeerHost in args, even after they've used it to set PeerAddr. IP instead removes PeerAddr if it got used to set PeerHost. In essence it should not matter if one uses PeerAddr||PeerHost or PeerHost||PeerAddr because these keys should be either the same (because they are synonyms) or one of them should be empty. But if the keys are set to different values the behavior depends on the underlying module.

In essence the only sane behavior would be to croak if both keys are set and are different.

There are some places in IO::Socket::SSL which currently only use PeerAddr. These places need to be fixed to use PeerAddr||PeerHost to work together with IO::Socket::IP too.

noxxi commented 10 years ago

There are some places in IO::Socket::SSL which currently only use PeerAddr. These places need to be fixed to use PeerAddr||PeerHost to work together with IO::Socket::IP too.

This was fixed in 68b1ba1, additionally to also accepting PeerService (used by IO::Socket::IP) instead of PeerPort. Also it was documented that PeerHost and PeerAddr are considered synonyms and thus should never be set both or at least not to different values.