search

ninenines / gun

HTTP/1.1, HTTP/2, Websocket client (and more) for Erlang/OTP.
ISC License
891 stars 232 forks source link

Fixed problem with multiple server_name_indication in tls opts. Havin… #264

Closed jbevemyr closed 2 years ago

jbevemyr commented 3 years ago

…g multiple server_name_indication entries triggers a problem in OTP 23.2.7.

essen commented 3 years ago

How do you use Gun, that you have to set SNI manually?

jbevemyr commented 3 years ago

We use gun to talk to a server that doesn't have a hostname that matches the certificate, so instead we set the SNI. We have a cluster of nodes where each node runs a cowboy server with the same certificate. We can connect to them with different host names but still verify them with the SNI. A workaround would be to issue separate certificates for each host and have dns alt names, of course.

jbevemyr commented 3 years ago

Same fix, just slightly differently coded.

michaelklishin commented 3 years ago

This looks reasonable to me. Most users won't need it but the risk of accepting this PR seem low.

essen commented 3 years ago

Sure. Could you please add a test though? Something similar to https://github.com/ninenines/gun/blob/master/test/gun_SUITE.erl#L81 except you'll want an event that occurs after ensure_alpn_sni.

essen commented 2 years ago

It appears that since https://github.com/erlang/otp/commit/e9b0dbb4a95 (OTP-20, Gun is 22+) the ssl application already sets SNI if none is provided. Therefore I think the best course of action here is to simply remove this code. I will add a test to confirm that SNI is sent and that a custom SNI can be set, regardless.

essen commented 2 years ago

OK the automatic SNI applies to directly connecting using ssl. It does not apply to upgrading a TCP socket. For that we must specify SNI (there's a note about this in https://www.erlang.org/doc/man/ssl.html#connect-3). So the patch is still necessary.

essen commented 2 years ago

Cherry-picked the relevant commit. Thanks!

The tests I've added can't catch the original issue, but they will be good to have for the future.