From da7931414d2dead6afd37531f3f19c1954d03e47 Mon Sep 17 00:00:00 2001 From: zmstone Date: Thu, 14 Nov 2024 21:56:26 +0100 Subject: [PATCH] refactor: improve ssl options validation --- src/esockd_acceptor_sup.erl | 74 +++++++++++++++++++++++++++---------- src/esockd_listener_sup.erl | 8 ++++ test/esockd_SUITE.erl | 19 ++++++++-- 3 files changed, 79 insertions(+), 22 deletions(-) diff --git a/src/esockd_acceptor_sup.erl b/src/esockd_acceptor_sup.erl index f1755c5..7893f83 100644 --- a/src/esockd_acceptor_sup.erl +++ b/src/esockd_acceptor_sup.erl @@ -24,6 +24,7 @@ -export([ start_acceptors/2 , start_acceptor/2 , count_acceptors/1 + , check_options/1 ]). %% Supervisor callbacks @@ -46,20 +47,25 @@ start_supervised(ListenerRef = {Proto, ListenOn}) -> Type = esockd_server:get_listener_prop(ListenerRef, type), Opts = esockd_server:get_listener_prop(ListenerRef, options), - TuneFun = tune_socket_fun(Opts), - UpgradeFuns = upgrade_funs(Type, Opts), - LimiterOpts = esockd_listener_sup:conn_limiter_opts(Opts, {listener, Proto, ListenOn}), - Limiter = esockd_listener_sup:conn_rate_limiter(LimiterOpts), - AcceptorMod = case Type of - dtls -> esockd_dtls_acceptor; - _ -> esockd_acceptor - end, - ConnSup = esockd_server:get_listener_prop(ListenerRef, connection_sup), - AcceptorArgs = [Proto, ListenOn, ConnSup, TuneFun, UpgradeFuns, Limiter], - case supervisor:start_link(?MODULE, {AcceptorMod, AcceptorArgs}) of - {ok, Pid} -> - _ = esockd_server:set_listener_prop(ListenerRef, acceptor_sup, Pid), - {ok, Pid}; + case check_options(Opts) of + ok -> + TuneFun = tune_socket_fun(Opts), + UpgradeFuns = upgrade_funs(Type, Opts), + LimiterOpts = esockd_listener_sup:conn_limiter_opts(Opts, {listener, Proto, ListenOn}), + Limiter = esockd_listener_sup:conn_rate_limiter(LimiterOpts), + AcceptorMod = case Type of + dtls -> esockd_dtls_acceptor; + _ -> esockd_acceptor + end, + ConnSup = esockd_server:get_listener_prop(ListenerRef, connection_sup), + AcceptorArgs = [Proto, ListenOn, ConnSup, TuneFun, UpgradeFuns, Limiter], + case supervisor:start_link(?MODULE, {AcceptorMod, AcceptorArgs}) of + {ok, Pid} -> + _ = esockd_server:set_listener_prop(ListenerRef, acceptor_sup, Pid), + {ok, Pid}; + {error, _} = Error -> + Error + end; {error, _} = Error -> Error end. @@ -139,11 +145,7 @@ ssl_upgrade_fun(Type, Opts) -> end, case proplists:get_value(Key, Opts) of undefined -> []; - SslOpts -> - %% validate ssl options and prevent the listener from starting if - %% validation failed - _ = ssl:handle_options(SslOpts, server, undefined), - [esockd_transport:ssl_upgrade_fun(SslOpts)] + SslOpts -> [esockd_transport:ssl_upgrade_fun(SslOpts)] end. tune_socket(Sock, []) -> @@ -169,3 +171,37 @@ tune_socket(Sock, [{tune_fun, {M, F, A}} | More]) -> end; tune_socket(Sock, [_|More]) -> tune_socket(Sock, More). + +-spec check_options(list()) -> ok | {error, any()}. +check_options(Opts) -> + try + ok = check_ssl_opts(ssl_options, Opts), + ok = check_ssl_opts(dtls_options, Opts) + catch + throw : Reason -> + {error, Reason} + end. + +check_ssl_opts(Key, Opts) -> + case proplists:get_value(Key, Opts) of + undefined -> + ok; + SslOpts -> + try + {ok, _} = ssl:handle_options(SslOpts, server, undefined), + ok + catch + _ : {error, Reason} -> + throw_invalid_ssl_option(Key, Reason); + _ : Wat : Stack -> + %% It's a function_clause for OTP 25 + %% And, maybe OTP decide to change some day, who knows + throw_invalid_ssl_option(Key, {Wat, Stack}) + end + end. + +-spec throw_invalid_ssl_option(_, _) -> no_return(). +throw_invalid_ssl_option(Key, Reason) -> + throw(#{error => invalid_ssl_option, + reason => Reason, + key => Key}). diff --git a/src/esockd_listener_sup.erl b/src/esockd_listener_sup.erl index efd3cad..34cf89b 100644 --- a/src/esockd_listener_sup.erl +++ b/src/esockd_listener_sup.erl @@ -97,6 +97,14 @@ get_options(ListenerRef, _Sup) -> esockd_server:get_listener_prop(ListenerRef, options). set_options(ListenerRef, Sup, Opts) -> + case esockd_acceptor_sup:check_options(Opts) of + ok -> + do_set_options(ListenerRef, Sup, Opts); + {error, Reason} -> + {error, Reason} + end. + +do_set_options(ListenerRef, Sup, Opts) -> OptsWas = esockd_server:get_listener_prop(ListenerRef, options), OptsWas = esockd_server:set_listener_prop(ListenerRef, options, esockd:merge_opts(OptsWas, Opts)), diff --git a/test/esockd_SUITE.erl b/test/esockd_SUITE.erl index 5f2b63d..23ff356 100644 --- a/test/esockd_SUITE.erl +++ b/test/esockd_SUITE.erl @@ -397,6 +397,19 @@ t_update_options_error(_) -> {ok, <<"Sock2">>} = gen_tcp:recv(Sock2, 0), ok = esockd:close(echo, 6000). +t_bad_tls_options(_Config) -> + LPort = 7001, + %% must include '{protocol, dtls}', + %% otherwise invalid because ssl lib defaults to '{protocol, ssl}' + BadOpts = [{versions, ['dtlsv1.2']}], + Res = esockd:open(echo_tls, LPort, + [{dtls_options, BadOpts}, + {connection_mfargs, echo_server}]), + ?assertMatch({error, {{shutdown, {failed_to_start_child, acceptor_sup, + #{error := invalid_ssl_option, + key := dtls_options}}}, _}}, Res), + ok. + t_update_tls_options(Config) -> LPort = 7000, SslOpts1 = [ {certfile, esockd_ct:certfile(Config)} @@ -417,9 +430,9 @@ t_update_tls_options(Config) -> [{ssl_options, SslOpts1}, {connection_mfargs, echo_server}]), {ok, Sock1} = ssl:connect("localhost", LPort, ClientSslOpts, 1000), - ?assertError( - {badmatch, _}, - esockd:set_options({echo_tls, LPort}, [{ssl_options, [{verify, verify_peer}]}]) + ?assertMatch( + {error, #{error := invalid_ssl_option}}, + esockd:set_options({echo_tls, LPort}, [{ssl_options, [{verify, verify_peer}]}]) ), ok = esockd:set_options({echo_tls, LPort}, [{ssl_options, SslOpts2}]),