Another thing that struck me is that while we agree the SNI host is without the trailing dot, but how about the name we use to verify the server cert? At least one TLS library won't let us use different names for those two different uses. Does it mean we should use the SNI name there or the resolve host name?

RFC 6125 doesn't seem to say

The SNI requirement leaves no room for other interpretations:

"HostName" contains the fully qualified DNS hostname of the server,
as understood by the client. The hostname is represented as a byte
string using ASCII encoding without a trailing dot.

RFC 6066 section 3

Yes that part is clear. Are there any server certificates that have the trailing dot in the altname? I don't know of any.

Do you know any HTTPS site that cares about the trailing dot in a Host: header? I don't.

I don't either but the reported issue looks legit.

But yes, browsers seem to use the SNI name too (no dot) for checking the server cert.

@bagder (not nearly having as much experience, but maybe this is helpful:)

At one point, I had to hand-implement the hostname-matching part for a programming language VM's SSL plugin and tired to make it as similar behaving as other clients I had at hand. So I toyed around: https://github.com/krono/x509hostmatch

And to make that work, had to dot-drop both the cert-sAN and the incoming hostname…
https://github.com/krono/x509hostmatch/blob/master/x509hostmatch.c#L69

HTH

I think more changes will be needed for hostname verification. For some backends we call Curl_cert_hostcheck (which strips the trailing dot) and that should be fine. For others though we call a backend specific function which may or may not handle the trailing dot. Tests of the backends I build running curld https://httpbin.org./:

OpenSSL: Works. Done by Curl_cert_hostcheck.
Schannel in manual verify mode (ie ca bundle specified): Works. Done by Curl_cert_hostcheck.
Schannel in auto verify mode (ie no ca bundle, use os certs): Works. Done by Schannel's InitializeSecurityContext.
wolfSSL: Doesn't work. Done by wolfSSL's wolfSSL_check_domain_name.

Another thing, the commit squashme: more sni conversions for schannel adds two more SNI conversions for other InitializeSecurityContext calls. The problem is those calls are not necessarily done at the beginning, and currently Curl_ssl_snihost uses data->state.buffer to store the SNI temporarily, however it relies on data->state.buffer not actually being used for something else during this time. I don't know if a separate buffer is needed to avoid overwriting data->state.buffer.

The problem is those calls are not necessarily done at the beginning

Then I think the name needs to be copied from when it is called in the beginning. I mean, we don't necessarily have to make the function do that generically for all backends since SNI sort of implies it is done before the transfer starts (and the buffer is used for download data).

Then I think the name needs to be copied from when it is called in the beginning.

Ok redid the schannel sni code so that now Curl_ssl_snihost is only called once, right after schannel_acquire_credential_handle in step 1, and it copies the SNI hostname needed by the credential.

I still don't like how Curl_ssl_snihost uses the download buffer as the temporary space, I think that could lead to a bug one day. For example I have the sni hostname retrieved separately from schannel_acquire_credential_handle but it really is needed for any Schannel credential handle, and what if a credential handle needs to be created at some point other than step 1? Who is going to remember that Curl_ssl_snihost can only be called from step 1?

I suggest adding a dedicated buffer to an ssl struct, which shouldn't be that large for example as fqdn 255 max so char snihost[255+1].

I suggest adding a dedicated buffer to an ssl struct, which shouldn't be that large for example as fqdn 255 max so char snihost[255+1].

I checked the spec. The SNI field can be 64K. I don't think it is always limited to the max DNS name size. Name resolving does not always mean DNS and there's no 255 byte limit in host names in URLs.

It feels wrong to me to "waste" such a large data field in a long-living struct for something that is only needed when setting up the connection and then isn't needed anymore. The download buffer has been used for this purpose for well over seven years now without problems, which could be an indicator that it might be okay.

One additional detail struck me with the kept trailing dot. A connection to example.com. (with a trailing dot) will not be reused/shared for example.com (without a trailing dot). But I also don't think we should change that.

I checked the spec. The SNI field can be 64K. I don't think it is always limited to the max DNS name size. Name resolving does not always mean DNS and there's no 255 byte limit in host names in URLs.

Yes but currently and practically we are only sending the DNS hostname, which is all that is allowed.

One additional detail struck me with the kept trailing dot. A connection to example.com. (with a trailing dot) will not be reused/shared for example.com (without a trailing dot). But I also don't think we should change that.

agree

Yes but currently and practically we are only sending the DNS hostname, which is all that is allowed.

curl doesn't have any such limit and can certainly use a longer name than what DNS supports. If you use something else than DNS to resolve (like /etc/hosts for example), the name can be longer for TLS. Practically that might of course be extremely rare. But you can bet that someone will run into that limit if we would impose it! 😀

This causes a problem for two appveyor builds:

schannel.c(1035,16): warning C4189: 'hostname': local variable is initialized but not referenced 

I'm getting ready to merge. How do you feel about this @jay ?

'hostname': local variable is initialized but not referenced

Sorry did not see this until after you merged. The debug statement should be left in, it is part of a series of debug messages schannel_connect_step{1,2,3} and so it is problematic to take out just step 2. I will open a PR to readd it, with a (void)hostname to rid of the warning for release builds.

I figured they're "just debug messages" and as such they tend to linger around for longer than necessary and they're very easy to put back in there when you actually need them for debugging... but if you think it's better to keep it there, I'm fine with it.