SSL options callback sni_fun is called twice during a TLS handshake

[eseres]

Describe the bug
Erlang OTP had a change to how the sni_fun SSL option is used. This change causes the sni_fun/2 callback function to be called twice for a connection.

This change to Erlang OTP was made in commit Update server_name_indication options, hash 9e7c030 and it was released in version 26.0.

The diff causing it is:

- is_hostname_recognized(#{sni_fun := undefined,
-                          sni_hosts := SNIHosts}, Hostname) ->
-     proplists:is_defined(Hostname, SNIHosts);
- is_hostname_recognized(_, _) ->
-     true.
+ is_hostname_recognized(#{sni_fun := Fun}, Hostname) ->
+     Fun(Hostname) =:= undefined.

And because of how the call chain is:

handle_sni_extension -> check_hostname -> is_hostname_recognized -> sni_fun

... and then ...

handle_sni_extension -> handle_sni_hostname -> update_ssl_options_from_sni -> sni_fun

… sni_fun is called two times.

The fact that the sni_fun/2 function is called twice during a handshake, may not be a bug by itself. However, it is a change from previous behavior. If the particular sni_fun/2 implementation has no side effects then there is no issue. But if it has side effects or it involves heavier processing, like a database query (even without side effects), the second invocation of the function is anything but optimal.

Upon further inspection of the changes in that commit, I think there is a bug. In the ssl.erl module, there is this:

-    %% FIXME: remove sni_hosts and
-    %%   sni_fun = fun(SNIHostName) -> proplists:get_value(SNIHostname, SNIHosts) end,
-    Opts#{sni_fun => SniFun, sni_hosts => SniHosts,
-          server_name_indication => undefined  %% FIXME
-         };
+
+    SniFun = case SniFun0 =:= undefined of
+                 true -> fun(Host) -> proplists:get_value(Host, SniHosts) end;
+                 false -> SniFun0
+             end,
+
+    Opts#{sni_fun => SniFun};

If sni_fun is undefined, it will define a function that will just return SSL options for the Host from the sni_hosts field if they exist. If not, it will return undefined. The point is that with this change, sni_fun will always exist and it may now return undefined.

Now, if we look at the changes in ssl_gen_statem.erl:

- is_hostname_recognized(#{sni_fun := undefined,
-                          sni_hosts := SNIHosts}, Hostname) ->
-     proplists:is_defined(Hostname, SNIHosts);
- is_hostname_recognized(_, _) ->
-     true.
+ is_hostname_recognized(#{sni_fun := Fun}, Hostname) ->
+     Fun(Hostname) =:= undefined.

we see that the behavior has changed. Previously, if Host did not exist in sni_hosts, it would return false. Now it will return true. The line

Fun(Hostname) =:= undefined

should be

Fun(Hostname) =/= undefined
.

Expected behavior
That sni_fun be only called once during a handshake.

Affected versions
OTP 26

[dgud]

@eseres Can you review the PR #7812, your previous review was great though a bit late :-)

[dgud]

Thanks for that great bug report, fix merged.