ERL-169: SSL. sni_fun always proc. Partial handshake (client_hello)

[OTP-Maintainer]

Original reporter: vans163
Affected version: Not Specified
Component: ssl
Migrated from: https://bugs.erlang.org/browse/ERL-169

---

sni_fun is a little async spaghetti right now and inconsistent to use.  It does not callback when no SNI was supplied requiring a lot of coding around it to make it consistent.

This is a two part fix, the first is to make sni_fun always proc, no SNI supplied will call the sni_fun with "" (empty string). In the case of client ssl option, server_name_indication, a "" empty string is ignored and not sent as a SNI extension.  This should add some level of backwards compatibility.

The second is to extend the SSL module to allow a partial handshake, with full inspection of the client hello as a binary at the least. But I have no idea how to go about this, would really like to hear some input from developers working on the SSL module.

For a full discussion see http://erlang.org/pipermail/erlang-questions/2016-June/089551.html.

[OTP-Maintainer]

ingela said:

I am not sure I fully understand what you are getting at. Do you mean that you want to change the ssl options even though no server name indication was sent? Could you please explain your use case in more detail? 

[OTP-Maintainer]

vans163 said:

Yes always proc the callback, this would improve consistency.

{code:erlang}
SO_ORIGINAL_DST = get_so_original_dst(Socket),
SNIFun = fun("") -> get_my_certain_cert(SO_ORIGINAL_DST) end;
         fun(SNIName) ->  get_my_certain_cert(SO_ORIGINAL_DST, SNIName) end,

ssl:ssl_accept(Socket, [{sni_fun, SNIFun}])
{code}

How can we get a certain certificate based on the SO_ORIGINAL_DST if we dont proc the callback on no SNI.  We need to now craft some wonky async spaghetti logic to cover the case of SNI supplied and no SNI.

To really ring home the point.  We have a load balancer proxy.  We cannot call get_my_certain_cert(SO_ORIGINAL_DST), then find out we got SNI and call get_my_certain_cert(SO_ORIGINAL_DST, SNIName).  Calling get_my_certain_cert(SO_ORIGINAL_DST) would establish a connection to the SO_ORIGINAL_DST endpoint, the endpoint will expect SNI if the client supplied it, otherwise the endpoint will serve the wrong certificate.

[OTP-Maintainer]

vans163 said:

I am looking into this issue more now, a deeper problem I did not see initially is by making a connection to the SO_ORIGINAL_DST endpoint in the sni_fun callback the controlling_process will be incorrect for the destination socket.  Leading to the same tangled web of awkward workarounds.  

I think the only reasonable way to fix this is to allow a partial handshake using SSL, perhaps 1 step to begin with.  Client hello. something so you can write code like the following:


{code:erlang}
{ok, SSLClientSocket} = ssl:ssl_accept(Socket, [{partial, client_hello}|SSLOpts]),
{ok, NegotiationOptionsAndExtensionsIncludingSNI} = ssl:connection_information(SSLClientSocket),
NewSSLOpts = do_stuff_with_extensions(SSLClientSocket, NegotiationOptionsAndExtensionsIncludingSNI, SSLOpts),
{ok, SSLClientSocket2} = ssl:ssl_accept(SSLClientSocket, [{partial, finish}|NewSSLOpts]),
{code}


Or perhaps:


{code:erlang}
{ok, SSLClientSocket} = ssl:ssl_accept_partial(Socket, SSLOpts, client_hello),
{code}
etc.

This is quite a serious change and I would really like some input, but this behavior should be doable. This would also greatly simplify parsing other extensions, custom extensions and future proofs.



Here is some analysis with a patch for empty SNI callback:

{code:diff}
+++ tls_connection.erl	2016-07-02 14:02:00.000000000 -0400
@@ -1013,29 +1013,29 @@
 
 handle_sni_extension(#client_hello{extensions = HelloExtensions}, State0) ->
     case HelloExtensions#hello_extensions.sni of
-    undefined ->
-        State0;
-    #sni{hostname = Hostname} ->
-        NewOptions = update_ssl_options_from_sni(State0#state.ssl_options, Hostname),
-        case NewOptions of
-        undefined ->
-            State0;
-        _ ->
-            {ok, Ref, CertDbHandle, FileRefHandle, CacheHandle, CRLDbHandle, OwnCert, Key, DHParams} = 
-            ssl_config:init(NewOptions, State0#state.role),
-            State0#state{
-              session = State0#state.session#session{own_certificate = OwnCert},
-              file_ref_db = FileRefHandle,
-              cert_db_ref = Ref,
-              cert_db = CertDbHandle,
-              crl_db = CRLDbHandle,
-              session_cache = CacheHandle,
-              private_key = Key,
-              diffie_hellman_params = DHParams,
-              ssl_options = NewOptions,
-              sni_hostname = Hostname
-             }
-        end
+        undefined -> handle_sni_extension_1("", State0);
+        #sni{hostname = Hostname} -> handle_sni_extension_1(Hostname, State0)
     end;
-handle_sni_extension(_, State) ->
-    State.
\ No newline at end of file
+handle_sni_extension(_, State) -> handle_sni_extension_1("", State).
+
+handle_sni_extension_1(Hostname, State) ->
+    NewOptions = update_ssl_options_from_sni(State#state.ssl_options, Hostname),
+    case NewOptions of
+    undefined ->
+        State;
+    _ ->
+        {ok, Ref, CertDbHandle, FileRefHandle, CacheHandle, CRLDbHandle, OwnCert, Key, DHParams} = 
+        ssl_config:init(NewOptions, State#state.role),
+        State#state{
+          session = State#state.session#session{own_certificate = OwnCert},
+          file_ref_db = FileRefHandle,
+          cert_db_ref = Ref,
+          cert_db = CertDbHandle,
+          crl_db = CRLDbHandle,
+          session_cache = CacheHandle,
+          private_key = Key,
+          diffie_hellman_params = DHParams,
+          ssl_options = NewOptions,
+          sni_hostname = Hostname
+         }
+    end.
\ No newline at end of file
{code}

The problematic use case (with the above patch):

{code:erlang}
handle_ssl_http_accept({pass_socket, ClientSocket}, S) ->
    {ok, [{raw,0,80,Info}]} = inet:getopts(ClientSocket,[{raw, 0, 80, 16}]),
    <<_:16, DestPort:16/big, A:8, B:8, C:8, D:8, _/binary>> = Info,
    DestAddr = {A,B,C,D},

    SNIHACK_ETS = ets:new(sni_hack, [public]),
    ParentPID = self(),

    {ok, SSLClientSocket} = ssl:ssl_accept(ClientSocket, 
        [
            {versions, [sslv3, tlsv1, 'tlsv1.1', 'tlsv1.2']},
            {ciphers, ssl:cipher_suites(all)},
            {sni_fun, fun(ServerName) ->
               %ServerName == "" if no SNI passed
               %We cannot code the logic otherwise. sni_fun must proc every time

               %If we do not pass the correct SNI the destination will serve us the
               %wrong ssl certificate
               {DestSocket, DestKey, DestCert} 
                   = router_connect_ssl(DestAddr, DestPort, ServerName)

               %We must hack around sni_fun :(
               ssl:controlling_process(DestSocket, ParentPID),
               ets:insert(SNIHACK_ETS, {ssl_dest_sock, DestSocket}),

               [{key, {'PrivateKeyInfo', DestKey}}, {cert, DestCert}]
            end}
        ],
        10000
    ),
    %Hack around sni_fun to get DestSocket back
    [{ssl_dest_sock, DestSocket}] = ets:lookup(SNIHACK_ETS, ssl_dest_sock),

    ssl:setopts(SSLClientSocket, [{active, once}, {packet, http_bin}]),
    ssl:setopts(DestSocket, [{active, once}, {packet, http_bin}]),

    {noreply, S#{type=> https, sourceSocket=> SSLClientSocket, destSocket=> DestSocket}};
{code}

[OTP-Maintainer]

ingela said:

The  options of the listen socket, even the ssl options  are inherited by the accept socket. However the ssl options may be changed in the ssl_accept-call and currently when the sni-extension is sent. In the case of an upgrade from a tcp-socket all options must be supplied to ssl_accept and possible updated when handling sni. 

There are two options to handle sni that are mutally exculsive sni_hosts and sni_fun. Calling sni_fun with ""
would be like having an entry for hostname "" meaning default, but then those options could be supplied as
default to the listen socket or ssl_accept upgrade call. 

What does router_connect_ssl do? I feel like I am missing something.

[OTP-Maintainer]

vans163 said:

router_connect_ssl connects to another server, grabs that servers certificate, checks if we have a cached version of it, if not it signs that certificate with the trusted CA and serves it.

Serving default when hostname is "" seems quite reasonable to me.  Since you need a default certificate anyways if you don't want to crash when no SNI is supplied.  If you have a partial hello you can gracefully close the connection if you only want clients that passed SNI.

Because what is the other option, adding yet another SSL option to gracefully close the connection if no SNI is supplied?

[OTP-Maintainer]

ingela said:

Maybe the way to go is to let the callback return an alert or a new configuration and make it always be called.
Hence going on with the new configuration or closing the connection with the supplied alert.
I think it could be  ok to make sni_fun more powerful than sni_hosts.
It could possible be a new callback that could handle more extensions. But a lot of extensions are handled by ssl without any need for user interaction, and maybe we do not want to let the user have free access to all extensions.
We could considering having the fun being called with undefined or hostname, would not be backwards compatible, but nither is always calling the sni_fun and then we probably would need some kind of fallback to not break old code. 

[OTP-Maintainer]

vans163 said:

Yea it would not be fully backwards compatible.  I think the sni_fun part can be ignored really. Its a semi specific use case, not worth breaking backwards compatibility for,  the partial handshake is really what I am getting at.

The SSL module already gets the entire handshake parsed into a record with all the extensions at some point, just making the gen_statem pause, return this record.  Then resume with more sslopts being passed seems quite simple to add, and it would be very powerful to the user.  Giving them much more control over how they handle their clients.

[OTP-Maintainer]

ingela said:

I am not convinced that we need new event and creating a new API function. It could be possible to have a more general callback that could always be called if it exists.
In general callback for handling extensions might be desirable, but there are some thinking about what it should be allowed to do and how it would work with already implemented options.

[OTP-Maintainer]

vans163 said:

The problem with having callbacks like this is it leads to spaghetti code and scope creep for certain use cases (like I demonstrated in the example). In a language like Erlang with the way BEAM handles things you should not have a need to write a callback, ever.  Even if you add more callbacks, those callbacks are still being called from the ssl gen_statem process leading to possible weird issues in the future such as if you open a socket in the callback the controlling_process will be wrong.

Anyways I spoke on IRC about this issue of the partial handshake and others seems to be on the side that it is unnecessary for the common use case.  You just don't need it.  Adding callbacks/handlers for every future extension that could be added to SSL seems to be the way to go.  

Perhaps once QUIC and HTTP2 start being more widely used this issue will be revisited.

[OTP-Maintainer]

ingela said:

Well maybe one of the problems is that extensions can be so very different things. I do not agree that there should be no callbacks ever. It can be so that you do not want 
the execution to be in the users process. I am not totally ruling out your idea, but it needs maturing.  Maybe extension handling will need a new approach in the future and maybe SNI handling needs special treatment anyhow. We will keep it in mind.

[OTP-Maintainer]

vans163 said:

Thank you.  If I get to implementing QUIC in Erlang I will see if any new issues arise.  Keep in mind if you ever write a Quic/HTTP2 to HTTP1.x forward facing proxy, these exact issues I brought up will be ran into.

[OTP-Maintainer]

ingela said:

https://github.com/erlang/otp/pull/1779

[OTP-Maintainer]

ingela said:

Merged PR1779