From a819cb1633e0cf5f3916b1d660fb0c7f37d51981 Mon Sep 17 00:00:00 2001 From: Raimo Niskanen Date: Thu, 19 Apr 2018 11:48:55 +0200 Subject: Rewrite TLS dist to handle node names in certs --- lib/ssl/src/inet_tls_dist.erl | 235 +++++++++++++++++++++++++++++++--- lib/ssl/test/ssl_dist_bench_SUITE.erl | 45 +++++-- 2 files changed, 251 insertions(+), 29 deletions(-) (limited to 'lib/ssl') diff --git a/lib/ssl/src/inet_tls_dist.erl b/lib/ssl/src/inet_tls_dist.erl index 8e605bec65..affcca9095 100644 --- a/lib/ssl/src/inet_tls_dist.erl +++ b/lib/ssl/src/inet_tls_dist.erl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2011-2017. All Rights Reserved. +%% Copyright Ericsson AB 2011-2018. All Rights Reserved. %% %% Licensed under the Apache License, Version 2.0 (the "License"); %% you may not use this file except in compliance with the License. @@ -31,11 +31,14 @@ -export([split_node/1, nodelay/0]). +-export([verify_client/3, verify_server/3]). + -export([dbg/0]). % Debug -include_lib("kernel/include/net_address.hrl"). -include_lib("kernel/include/dist.hrl"). -include_lib("kernel/include/dist_util.hrl"). +-include_lib("public_key/include/public_key.hrl"). -include("ssl_api.hrl"). @@ -225,12 +228,15 @@ accept_loop(Driver, Listen, Kernel) -> case Driver:accept(Listen) of {ok, Socket} -> Opts = get_ssl_options(server), + VerifyClient = is_verify_client(Opts), wait_for_code_server(), - case ssl:ssl_accept( - Socket, [{active, false}, {packet, 4}] ++ Opts, - net_kernel:connecttime()) of + case + ssl:ssl_accept( + Socket, + trace([{active, false},{packet, 4}|Opts]), + net_kernel:connecttime()) + of {ok, #sslsocket{pid = DistCtrl} = SslSocket} -> - monitor_pid(DistCtrl), trace( Kernel ! {accept, self(), DistCtrl, @@ -239,7 +245,7 @@ accept_loop(Driver, Listen, Kernel) -> {Kernel, controller, Pid} -> ok = ssl:controlling_process(SslSocket, Pid), trace( - Pid ! {self(), controller}); + Pid ! {self(), controller, VerifyClient}); {Kernel, unsupported_protocol} -> exit(trace(unsupported_protocol)) end, @@ -250,16 +256,45 @@ accept_loop(Driver, Listen, Kernel) -> error_logger:error_msg( "Cannot accept TLS distribution connection: ~s~n", [ssl:format_error(Error)]), - _ = trace(Error), - gen_tcp:close(Socket); + gen_tcp:close(Socket), + exit(trace(Error)); Other -> - _ = trace(Other), - gen_tcp:close(Socket) + gen_tcp:close(Socket), + exit(trace(Other)) end; Error -> exit(trace(Error)) - end, - accept_loop(Driver, Listen, Kernel). + end. + +%% {verify_fun,{fun ?MODULE:verify_client/3,_}} is used +%% as a marker that the peer certificate shall be parsed +%% for node names after succesful connection +%% +is_verify_client([]) -> + false; +is_verify_client([Opt|Opts]) -> + case Opt of + {verify_fun,{Fun,_}} -> + case Fun =:= fun ?MODULE:verify_client/3 of + true -> + true; + false -> + is_verify_client(Opts) + end; + _ -> + is_verify_client(Opts) + end. + +%% Same as verify_peer - approves all valid certificates +verify_client(_, {bad_cert,_} = Reason, _) -> + {fail,Reason}; +verify_client(_, {extension,_}, S) -> + {unknown,S}; +verify_client(_, valid, S) -> + {valid,S}; +verify_client(_, valid_peer, S) -> + {valid_peer,S}. + wait_for_code_server() -> %% This is an ugly hack. Upgrading a socket to TLS requires the @@ -311,10 +346,23 @@ gen_accept_connection( do_accept(Driver, Kernel, AcceptPid, DistCtrl, MyNode, Allowed, SetupTime) -> SslSocket = ssl_connection:get_sslsocket(DistCtrl), receive - {AcceptPid, controller} -> + {AcceptPid, controller, VerifyClient} -> Timer = dist_util:start_timer(SetupTime), case check_ip(Driver, SslSocket) of true -> + NewAllowed = + case VerifyClient of + true -> + %% + %% Parse out all node names from the peer's + %% certificate and use them to create + %% a list of allowed node names + %% for the distribution handshake + %% + allowed(cert_nodes(SslSocket), Allowed); + false -> + Allowed + end, HSData0 = hs_data_common(SslSocket), HSData = HSData0#hs_data{ @@ -323,7 +371,7 @@ do_accept(Driver, Kernel, AcceptPid, DistCtrl, MyNode, Allowed, SetupTime) -> socket = DistCtrl, timer = Timer, this_flags = 0, - allowed = Allowed}, + allowed = NewAllowed}, link(DistCtrl), dist_util:handshake_other_started(trace(HSData)); {false,IP} -> @@ -334,6 +382,42 @@ do_accept(Driver, Kernel, AcceptPid, DistCtrl, MyNode, Allowed, SetupTime) -> end end. +%% Parse out all node names from the peer's certificate +%% +cert_nodes(SslSocket) -> + case ssl:peercert(SslSocket) of + {ok,PeerCert} -> + case + parse_node_names( + public_key:pkix_decode_cert(PeerCert, otp)) + of + [] -> + ?shutdown(cert_missing_node_name); + CertNodes -> + CertNodes + end; + Error -> + ?shutdown2(no_peer_cert, trace(Error)) + end. + +allowed(CertNodes, []) -> + %% All allowed -> allow only certificate nodes + CertNodes; +allowed(CertNodes, Allowed) -> + %% Find the intersection of the allowed list and certificate nodes + case + [CertNode || + CertNode <- CertNodes, + dist_util:is_allowed(CertNode, Allowed)] + of + [] -> + error_logger:error_msg( + "** Connection attempt from " + "disallowed node(s) ~p ** ~n", [CertNodes]), + ?shutdown2(CertNodes, trace({is_allowed, not_allowed})); + NewAllowed -> + NewAllowed + end. setup(Node, Type, MyNode, LongOrShortNames, SetupTime) -> @@ -361,8 +445,12 @@ do_setup(Driver, Kernel, Node, Type, MyNode, LongOrShortNames, SetupTime) -> Opts = trace( connect_options( - [{server_name_indication, atom_to_list(Node)} - |get_ssl_options(client)])), + %% + %% Use verify_server/3 to verify that + %% the server's certificate is for Node + %% + setup_verify_server( + get_ssl_options(client), Node))), dist_util:reset_timer(Timer), case ssl:connect( Address, TcpPort, @@ -370,7 +458,7 @@ do_setup(Driver, Kernel, Node, Type, MyNode, LongOrShortNames, SetupTime) -> Driver:family(), nodelay()] ++ Opts, net_kernel:connecttime()) of {ok, #sslsocket{pid = DistCtrl} = SslSocket} -> - monitor_pid(DistCtrl), + _ = monitor_pid(DistCtrl), ok = ssl:controlling_process(SslSocket, self()), HSData0 = hs_data_common(SslSocket), HSData = @@ -411,6 +499,56 @@ close(Socket) -> gen_close(Driver, Socket) -> trace(Driver:close(Socket)). +%% {verify_fun,{fun ?MODULE:verify_server/3,_}} is used +%% as a configuration marker that verify_server/3 shall be used. +%% +%% Replace the State in the first occurence of +%% {verify_fun,{fun ?MODULE:verify_server/3,State}} +%% with Node and remove the rest. Node can not be used +%% from a configuration file since it is dynamic +%% and connection dependent. +%% +setup_verify_server(Opts, Node) -> + setup_verify_server(Opts, Node, true). +%% +setup_verify_server([], _Node, _) -> + []; +setup_verify_server([Opt|Opts], Node, Once) -> + case Opt of + {verify_fun,{Fun,_}} -> + case Fun =:= fun ?MODULE:verify_server/3 of + true when Once -> + [{verify_fun,{Fun,Node}} + |setup_verify_server(Opts, Node, false)]; + true -> + setup_verify_server(Opts, Node, Once); + false -> + [Opt|setup_verify_server(Opts, Node, Once)] + end; + _ -> + [Opt|setup_verify_server(Opts, Node, Once)] + end. + +verify_server(_, {bad_cert,_} = Reason, _) -> + {fail,Reason}; +verify_server(_, {extension,_}, Node) -> + {unknown,Node}; +verify_server(_, valid, Node) -> + {valid,Node}; +verify_server(PeerCert, valid_peer, Node) -> + case parse_node_names(PeerCert) of + [] -> + {fail,cert_missing_node_name}; + CertNodes -> + case dist_util:is_allowed(Node, CertNodes) of + true -> + {valid,Node}; + false -> + {fail,wrong_nodes_in_cert} + end + end. + + %% ------------------------------------------------------------ %% Do only accept new connection attempts from nodes at our %% own LAN, if the check_ip environment parameter is true. @@ -450,6 +588,69 @@ get_ifs(#sslsocket{fd = {gen_tcp, Socket, _}}) -> end. +%% Look in Extensions, in all subjectAltName:s +%% to find node names in this certificate +%% +parse_node_names( + #'OTPCertificate'{ + tbsCertificate = #'OTPTBSCertificate'{extensions = Extensions}}) -> + parse_extensions(Extensions). +%% +parse_extensions(Extensions) when is_list(Extensions) -> + parse_extensions(Extensions, []); +parse_extensions(asn1_NOVALUE) -> + []. +%% +parse_extensions([], CertNodes) -> + CertNodes; +%% +%% XXX Why are all extnValue:s sequences? +%% Should we parse all members? +%% +parse_extensions( + [#'Extension'{ + extnID = ?'id-ce-subjectAltName', + extnValue = [{dNSName,OtherNode}|_]} + |Extensions], + CertNodes) -> + parse_extensions(Extensions, [OtherNode|CertNodes]); +parse_extensions( + [#'Extension'{ + extnID = ?'id-ce-subjectAltName', + extnValue = [{rfc822Name,OtherNode}|_]} + |Extensions], + CertNodes) -> + parse_extensions(Extensions, [OtherNode|CertNodes]); +parse_extensions( + [#'Extension'{ + extnID = ?'id-ce-subjectAltName', + extnValue = [{directoryName,{rdnSequence,[Rdn|_]}}|_]} + |Extensions], + CertNodes) -> + %% + %% XXX Why is rdnSequence a sequence? + %% Should we parse all members? + %% + case parse_rdn(Rdn) of + none -> + parse_extensions(Extensions, CertNodes); + OtherNode -> + parse_extensions(Extensions, [OtherNode|CertNodes]) + end; +parse_extensions([_|Extensions], CertNodes) -> + parse_extensions(Extensions, CertNodes). + +parse_rdn([]) -> + none; +parse_rdn( + [#'AttributeTypeAndValue'{ + type = ?'id-at-commonName', + value = {utf8String,CommonName}}|_]) -> + unicode:characters_to_list(CommonName); +parse_rdn([_|Rdn]) -> + parse_rdn(Rdn). + + %% If Node is illegal terminate the connection setup!! splitnode(Driver, Node, LongOrShortNames) -> case string:split(atom_to_list(Node), "@") of diff --git a/lib/ssl/test/ssl_dist_bench_SUITE.erl b/lib/ssl/test/ssl_dist_bench_SUITE.erl index 4d27564319..06150218ea 100644 --- a/lib/ssl/test/ssl_dist_bench_SUITE.erl +++ b/lib/ssl/test/ssl_dist_bench_SUITE.erl @@ -1,7 +1,7 @@ %%%------------------------------------------------------------------- %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2017. All Rights Reserved. +%% Copyright Ericsson AB 2017-2018. All Rights Reserved. %% %% Licensed under the Apache License, Version 2.0 (the "License"); %% you may not use this file except in compliance with the License. @@ -95,14 +95,14 @@ init_per_suite(Config) -> _ -> PrivDir = proplists:get_value(priv_dir, Config), %% - [_, HostA] = string:split(atom_to_list(Node), "@"), + [_, HostA] = split_node(Node), NodeAName = ?MODULE_STRING ++ "_node_a", NodeAString = NodeAName ++ "@" ++ HostA, NodeAConfFile = filename:join(PrivDir, NodeAString ++ ".conf"), NodeA = list_to_atom(NodeAString), %% ServerNode = ssl_bench_test_lib:setup(dist_server), - [_, HostB] = string:split(atom_to_list(ServerNode), "@"), + [_, HostB] = split_node(ServerNode), NodeBName = ?MODULE_STRING ++ "_node_b", NodeBString = NodeBName ++ "@" ++ HostB, NodeBConfFile = filename:join(PrivDir, NodeBString ++ ".conf"), @@ -116,16 +116,23 @@ init_per_suite(Config) -> ?MODULE_STRING ++ " ROOT CA", CertOptions), SSLConf = [{verify, verify_peer}, + {fail_if_no_peer_cert, true}, {versions, [TLSVersion]}, {ciphers, [TLSCipher]}], + ServerConf = + [{verify_fun, + {fun inet_tls_dist:verify_client/3, undefined}} + | SSLConf], + ClientConf = + [{verify_fun, + {fun inet_tls_dist:verify_server/3, node}} + | SSLConf], %% write_node_conf( - NodeAConfFile, NodeA, - [{fail_if_no_peer_cert, true} | SSLConf], SSLConf, + NodeAConfFile, NodeA, ServerConf, ClientConf, CertOptions, RootCert), write_node_conf( - NodeBConfFile, NodeB, - [{fail_if_no_peer_cert, true} | SSLConf], SSLConf, + NodeBConfFile, NodeB, ServerConf, ClientConf, CertOptions, RootCert), %% [{node_a_name, NodeAName}, @@ -179,8 +186,17 @@ write_node_conf( [{extensions, [#'Extension'{ extnID = ?'id-ce-subjectAltName', - extnValue = [{dNSName, atom_to_list(Node)}], - critical = false}]} | CertOptions]}), + %% extnValue = [{dNSName, atom_to_list(Node)}], + %% extnValue = [{rfc822Name, atom_to_list(Node)}], + extnValue = + [{directoryName, + {rdnSequence, + [[#'AttributeTypeAndValue'{ + type = ?'id-at-commonName', + value = + {utf8String, + atom_to_binary(Node, utf8)}}]]}}], + critical = true}]} | CertOptions]}), NodeConf = [{server, ServerConf ++ Conf}, {client, ClientConf ++ Conf}], {ok, Fd} = file:open(ConfFile, [write]), @@ -188,6 +204,8 @@ write_node_conf( io:format(Fd, "~p.~n", [NodeConf]), ok = file:close(Fd). +split_node(Node) -> + string:split(atom_to_list(Node), "@"). %%%------------------------------------------------------------------- %%% Test cases @@ -221,15 +239,18 @@ setup_loop(_A, _B, T, 0) -> T; setup_loop(A, B, T, N) -> StartTime = start_time(), - [A] = rpc:block_call(B, erlang, nodes, []), + [N,A] = [N|rpc:block_call(B, erlang, nodes, [])], Time = elapsed_time(StartTime), - [B] = erlang:nodes(), + [N,B] = [N|erlang:nodes()], Mref = erlang:monitor(process, {rex,B}), true = net_kernel:disconnect(B), receive {'DOWN',Mref,process,_,_} -> [] = erlang:nodes(), - setup_loop(A, B, Time + T, N - 1) + receive + after 500 -> + setup_loop(A, B, Time + T, N - 1) + end end. -- cgit v1.2.3 From 0edf78512f444b0029b27de1cde60c38ea388845 Mon Sep 17 00:00:00 2001 From: Raimo Niskanen Date: Fri, 20 Apr 2018 15:11:53 +0200 Subject: Create plug-in for distro cert nodes --- lib/ssl/src/inet_tls_dist.erl | 100 +++++++++++++++++----------------- lib/ssl/test/ssl_dist_bench_SUITE.erl | 6 +- 2 files changed, 55 insertions(+), 51 deletions(-) (limited to 'lib/ssl') diff --git a/lib/ssl/src/inet_tls_dist.erl b/lib/ssl/src/inet_tls_dist.erl index affcca9095..3257e3a6c2 100644 --- a/lib/ssl/src/inet_tls_dist.erl +++ b/lib/ssl/src/inet_tls_dist.erl @@ -31,7 +31,7 @@ -export([split_node/1, nodelay/0]). --export([verify_client/3, verify_server/3]). +-export([verify_client/3, verify_server/3, cert_nodes/1]). -export([dbg/0]). % Debug @@ -228,7 +228,7 @@ accept_loop(Driver, Listen, Kernel) -> case Driver:accept(Listen) of {ok, Socket} -> Opts = get_ssl_options(server), - VerifyClient = is_verify_client(Opts), + CertNodesFun = verify_client_cert_nodes_fun(Opts), wait_for_code_server(), case ssl:ssl_accept( @@ -245,7 +245,7 @@ accept_loop(Driver, Listen, Kernel) -> {Kernel, controller, Pid} -> ok = ssl:controlling_process(SslSocket, Pid), trace( - Pid ! {self(), controller, VerifyClient}); + Pid ! {self(), controller, CertNodesFun}); {Kernel, unsupported_protocol} -> exit(trace(unsupported_protocol)) end, @@ -270,30 +270,34 @@ accept_loop(Driver, Listen, Kernel) -> %% as a marker that the peer certificate shall be parsed %% for node names after succesful connection %% -is_verify_client([]) -> - false; -is_verify_client([Opt|Opts]) -> +verify_client_cert_nodes_fun([]) -> + undefined; +verify_client_cert_nodes_fun([Opt|Opts]) -> case Opt of - {verify_fun,{Fun,_}} -> + {verify_fun,{Fun,CertNodesFun}} -> case Fun =:= fun ?MODULE:verify_client/3 of + true when not is_function(CertNodesFun, 1) -> + exit( + trace( + {verify_client_bad_argument,CertNodesFun})); true -> - true; + CertNodesFun; false -> - is_verify_client(Opts) + verify_client_cert_nodes_fun(Opts) end; _ -> - is_verify_client(Opts) + verify_client_cert_nodes_fun(Opts) end. %% Same as verify_peer - approves all valid certificates verify_client(_, {bad_cert,_} = Reason, _) -> {fail,Reason}; -verify_client(_, {extension,_}, S) -> - {unknown,S}; -verify_client(_, valid, S) -> - {valid,S}; -verify_client(_, valid_peer, S) -> - {valid_peer,S}. +verify_client(_, {extension,_}, F) -> + {unknown,F}; +verify_client(_, valid, F) -> + {valid,F}; +verify_client(_, valid_peer, F) -> + {valid_peer,F}. wait_for_code_server() -> @@ -346,23 +350,12 @@ gen_accept_connection( do_accept(Driver, Kernel, AcceptPid, DistCtrl, MyNode, Allowed, SetupTime) -> SslSocket = ssl_connection:get_sslsocket(DistCtrl), receive - {AcceptPid, controller, VerifyClient} -> + {AcceptPid, controller, CertNodesFun} -> Timer = dist_util:start_timer(SetupTime), case check_ip(Driver, SslSocket) of true -> NewAllowed = - case VerifyClient of - true -> - %% - %% Parse out all node names from the peer's - %% certificate and use them to create - %% a list of allowed node names - %% for the distribution handshake - %% - allowed(cert_nodes(SslSocket), Allowed); - false -> - Allowed - end, + allowed_nodes(CertNodesFun, SslSocket, Allowed), HSData0 = hs_data_common(SslSocket), HSData = HSData0#hs_data{ @@ -382,19 +375,20 @@ do_accept(Driver, Kernel, AcceptPid, DistCtrl, MyNode, Allowed, SetupTime) -> end end. -%% Parse out all node names from the peer's certificate -%% -cert_nodes(SslSocket) -> +allowed_nodes(undefined, _SslSocket, Allowed) -> + Allowed; +allowed_nodes(CertNodesFun, SslSocket, Allowed) -> case ssl:peercert(SslSocket) of - {ok,PeerCert} -> - case - parse_node_names( - public_key:pkix_decode_cert(PeerCert, otp)) - of + {ok,PeerCertDER} -> + PeerCert = public_key:pkix_decode_cert(PeerCertDER, otp), + %% + %% Parse out all node names from the peer's certificate + %% + case CertNodesFun(PeerCert) of [] -> ?shutdown(cert_missing_node_name); CertNodes -> - CertNodes + allowed(CertNodes, Allowed) end; Error -> ?shutdown2(no_peer_cert, trace(Error)) @@ -515,10 +509,14 @@ setup_verify_server([], _Node, _) -> []; setup_verify_server([Opt|Opts], Node, Once) -> case Opt of - {verify_fun,{Fun,_}} -> + {verify_fun,{Fun,CertNodesFun}} -> case Fun =:= fun ?MODULE:verify_server/3 of + true when not is_function(CertNodesFun, 1) -> + ?shutdown2( + Node, + {verify_server_bad_argument,CertNodesFun}); true when Once -> - [{verify_fun,{Fun,Node}} + [{verify_fun,{Fun,{CertNodesFun,Node}}} |setup_verify_server(Opts, Node, false)]; true -> setup_verify_server(Opts, Node, Once); @@ -531,18 +529,21 @@ setup_verify_server([Opt|Opts], Node, Once) -> verify_server(_, {bad_cert,_} = Reason, _) -> {fail,Reason}; -verify_server(_, {extension,_}, Node) -> - {unknown,Node}; -verify_server(_, valid, Node) -> - {valid,Node}; -verify_server(PeerCert, valid_peer, Node) -> - case parse_node_names(PeerCert) of +verify_server(_, {extension,_}, S) -> + {unknown,S}; +verify_server(_, valid, S) -> + {valid,S}; +verify_server(PeerCert, valid_peer, {CertNodesFun,Node} = S) -> + %% + %% Parse out all node names from the peer's certificate + %% + case CertNodesFun(PeerCert) of [] -> {fail,cert_missing_node_name}; CertNodes -> case dist_util:is_allowed(Node, CertNodes) of true -> - {valid,Node}; + {valid,S}; false -> {fail,wrong_nodes_in_cert} end @@ -591,11 +592,12 @@ get_ifs(#sslsocket{fd = {gen_tcp, Socket, _}}) -> %% Look in Extensions, in all subjectAltName:s %% to find node names in this certificate %% -parse_node_names( +cert_nodes( #'OTPCertificate'{ tbsCertificate = #'OTPTBSCertificate'{extensions = Extensions}}) -> parse_extensions(Extensions). -%% + + parse_extensions(Extensions) when is_list(Extensions) -> parse_extensions(Extensions, []); parse_extensions(asn1_NOVALUE) -> diff --git a/lib/ssl/test/ssl_dist_bench_SUITE.erl b/lib/ssl/test/ssl_dist_bench_SUITE.erl index 06150218ea..8852b6f3c6 100644 --- a/lib/ssl/test/ssl_dist_bench_SUITE.erl +++ b/lib/ssl/test/ssl_dist_bench_SUITE.erl @@ -121,11 +121,13 @@ init_per_suite(Config) -> {ciphers, [TLSCipher]}], ServerConf = [{verify_fun, - {fun inet_tls_dist:verify_client/3, undefined}} + {fun inet_tls_dist:verify_client/3, + fun inet_tls_dist:cert_nodes/1}} | SSLConf], ClientConf = [{verify_fun, - {fun inet_tls_dist:verify_server/3, node}} + {fun inet_tls_dist:verify_server/3, + fun inet_tls_dist:cert_nodes/1}} | SSLConf], %% write_node_conf( -- cgit v1.2.3 From 4c4d861792d79ac7773548c089b7a93bc2c72a51 Mon Sep 17 00:00:00 2001 From: Raimo Niskanen Date: Fri, 20 Apr 2018 16:52:15 +0200 Subject: Open for host and node allow list --- lib/ssl/src/inet_tls_dist.erl | 55 ++++++++++++++++--------------------------- 1 file changed, 20 insertions(+), 35 deletions(-) (limited to 'lib/ssl') diff --git a/lib/ssl/src/inet_tls_dist.erl b/lib/ssl/src/inet_tls_dist.erl index 3257e3a6c2..d4215c8f83 100644 --- a/lib/ssl/src/inet_tls_dist.erl +++ b/lib/ssl/src/inet_tls_dist.erl @@ -29,7 +29,7 @@ -export([gen_listen/2, gen_accept/2, gen_accept_connection/6, gen_setup/6, gen_close/2, gen_select/2]). --export([split_node/1, nodelay/0]). +-export([nodelay/0]). -export([verify_client/3, verify_server/3, cert_nodes/1]). @@ -52,25 +52,20 @@ select(Node) -> gen_select(inet_tcp, Node). gen_select(Driver, Node) -> - case split_node(Node) of - false -> - false; - Host -> + case dist_util:split_node(Node) of + {_,Host} -> case Driver:getaddr(Host) of {ok, _} -> true; _ -> false - end + end; + _ -> + false end. %% ------------------------------------------------------------------------- is_node_name(Node) -> - case split_node(Node) of - false -> - false; - _Host -> - true - end. + dist_util:is_node_name(Node). %% ------------------------------------------------------------------------- @@ -145,13 +140,13 @@ f_getll(DistCtrl) -> f_address(SslSocket, Node) -> case ssl:peername(SslSocket) of {ok, Address} -> - case split_node(Node) of - false -> - {error, no_node}; - Host -> + case dist_util:split_node(Node) of + {_,Host} -> #net_address{ address=Address, host=Host, - protocol=tls, family=inet} + protocol=tls, family=inet}; + _ -> + {error, no_node} end end. @@ -429,7 +424,7 @@ gen_setup(Driver, Node, Type, MyNode, LongOrShortNames, SetupTime) -> [link, {priority, max}])). do_setup(Driver, Kernel, Node, Type, MyNode, LongOrShortNames, SetupTime) -> - [Name, Address] = splitnode(Driver, Node, LongOrShortNames), + {Name, Address} = split_node(Driver, Node, LongOrShortNames), case Driver:getaddr(Address) of {ok, Ip} -> Timer = trace(dist_util:start_timer(SetupTime)), @@ -654,9 +649,9 @@ parse_rdn([_|Rdn]) -> %% If Node is illegal terminate the connection setup!! -splitnode(Driver, Node, LongOrShortNames) -> - case string:split(atom_to_list(Node), "@") of - [Name, Host] when Host =/= [] -> +split_node(Driver, Node, LongOrShortNames) -> + case dist_util:split_node(Node) of + {Name, Host} -> check_node(Driver, Name, Node, Host, LongOrShortNames); [_] -> error_logger:error_msg( @@ -671,10 +666,10 @@ splitnode(Driver, Node, LongOrShortNames) -> check_node(Driver, Name, Node, Host, LongOrShortNames) -> case string:split(Host, ".") of - [_] when LongOrShortNames == longnames -> + [_] when LongOrShortNames =:= longnames -> case Driver:parse_address(Host) of {ok, _} -> - [Name, Host]; + {Name, Host}; _ -> error_logger:error_msg( "** System running to use " @@ -683,7 +678,7 @@ check_node(Driver, Name, Node, Host, LongOrShortNames) -> [Host]), ?shutdown2(Node, trace({not_longnames, Host})) end; - [_, _] when LongOrShortNames == shortnames -> + [_,_|_] when LongOrShortNames =:= shortnames -> error_logger:error_msg( "** System NOT running to use " "fully qualified hostnames **~n" @@ -691,19 +686,9 @@ check_node(Driver, Name, Node, Host, LongOrShortNames) -> [Host]), ?shutdown2(Node, trace({not_shortnames, Host})); _ -> - [Name, Host] + {Name, Host} end. -split_node(Node) when is_atom(Node) -> - case string:split(atom_to_list(Node), "@") of - [Name, Host] when Name =/= [], Host =/= [] -> - Host; - _ -> - false - end; -split_node(_) -> - false. - %% ------------------------------------------------------------------------- connect_options(Opts) -> -- cgit v1.2.3 From b5ec77bf908877d7471997527959e3d98d45bd96 Mon Sep 17 00:00:00 2001 From: Raimo Niskanen Date: Tue, 24 Apr 2018 09:34:51 +0200 Subject: Parse cert primarily for host names --- lib/ssl/src/inet_tls_dist.erl | 65 +++++++++++++++++++---------------- lib/ssl/test/ssl_dist_bench_SUITE.erl | 29 ++++++++++------ 2 files changed, 54 insertions(+), 40 deletions(-) (limited to 'lib/ssl') diff --git a/lib/ssl/src/inet_tls_dist.erl b/lib/ssl/src/inet_tls_dist.erl index d4215c8f83..3fab89fa97 100644 --- a/lib/ssl/src/inet_tls_dist.erl +++ b/lib/ssl/src/inet_tls_dist.erl @@ -585,7 +585,10 @@ get_ifs(#sslsocket{fd = {gen_tcp, Socket, _}}) -> %% Look in Extensions, in all subjectAltName:s -%% to find node names in this certificate +%% to find node names in this certificate. +%% Host names are picked up as a subjectAltName containing +%% a dNSName, and the first subjectAltName containing +%% a commonName is the node name. %% cert_nodes( #'OTPCertificate'{ @@ -594,48 +597,52 @@ cert_nodes( parse_extensions(Extensions) when is_list(Extensions) -> - parse_extensions(Extensions, []); + parse_extensions(Extensions, [], none); parse_extensions(asn1_NOVALUE) -> []. %% -parse_extensions([], CertNodes) -> - CertNodes; -%% -%% XXX Why are all extnValue:s sequences? -%% Should we parse all members? -%% -parse_extensions( - [#'Extension'{ - extnID = ?'id-ce-subjectAltName', - extnValue = [{dNSName,OtherNode}|_]} - |Extensions], - CertNodes) -> - parse_extensions(Extensions, [OtherNode|CertNodes]); -parse_extensions( - [#'Extension'{ - extnID = ?'id-ce-subjectAltName', - extnValue = [{rfc822Name,OtherNode}|_]} - |Extensions], - CertNodes) -> - parse_extensions(Extensions, [OtherNode|CertNodes]); +parse_extensions([], Hosts, none) -> + lists:reverse(Hosts); +parse_extensions([], Hosts, Name) -> + [Name ++ "@" ++ Host || Host <- lists:reverse(Hosts)]; parse_extensions( [#'Extension'{ extnID = ?'id-ce-subjectAltName', - extnValue = [{directoryName,{rdnSequence,[Rdn|_]}}|_]} + extnValue = AltNames} |Extensions], - CertNodes) -> + Hosts, Name) -> + case parse_subject_altname(AltNames) of + none -> + parse_extensions(Extensions, Hosts, Name); + {host,Host} -> + parse_extensions(Extensions, [Host|Hosts], Name); + {name,NewName} when Name =:= none -> + parse_extensions(Extensions, Hosts, NewName); + {Name,_} -> + parse_extensions(Extensions, Hosts, Name) + end; +parse_extensions([_|Extensions], Hosts, Name) -> + parse_extensions(Extensions, Hosts, Name). + +parse_subject_altname([]) -> + none; +parse_subject_altname([{dNSName,Host}|_AltNames]) -> + {host,Host}; +parse_subject_altname( + [{directoryName,{rdnSequence,[Rdn|_]}}|AltNames]) -> %% %% XXX Why is rdnSequence a sequence? %% Should we parse all members? %% case parse_rdn(Rdn) of none -> - parse_extensions(Extensions, CertNodes); - OtherNode -> - parse_extensions(Extensions, [OtherNode|CertNodes]) + parse_subject_altname(AltNames); + Name -> + {name,Name} end; -parse_extensions([_|Extensions], CertNodes) -> - parse_extensions(Extensions, CertNodes). +parse_subject_altname([_|AltNames]) -> + parse_subject_altname(AltNames). + parse_rdn([]) -> none; diff --git a/lib/ssl/test/ssl_dist_bench_SUITE.erl b/lib/ssl/test/ssl_dist_bench_SUITE.erl index 8852b6f3c6..31de0936f9 100644 --- a/lib/ssl/test/ssl_dist_bench_SUITE.erl +++ b/lib/ssl/test/ssl_dist_bench_SUITE.erl @@ -181,6 +181,7 @@ end_per_testcase(_Func, _Conf) -> write_node_conf( ConfFile, Node, ServerConf, ClientConf, CertOptions, RootCert) -> + [_Name,Host] = string:split(atom_to_list(Node), "@"), Conf = public_key:pkix_test_data( #{root => RootCert, @@ -188,17 +189,23 @@ write_node_conf( [{extensions, [#'Extension'{ extnID = ?'id-ce-subjectAltName', - %% extnValue = [{dNSName, atom_to_list(Node)}], - %% extnValue = [{rfc822Name, atom_to_list(Node)}], - extnValue = - [{directoryName, - {rdnSequence, - [[#'AttributeTypeAndValue'{ - type = ?'id-at-commonName', - value = - {utf8String, - atom_to_binary(Node, utf8)}}]]}}], - critical = true}]} | CertOptions]}), + extnValue = [{dNSName, Host}], + critical = true}%, + %% #'Extension'{ + %% extnID = ?'id-ce-subjectAltName', + %% extnValue = + %% [{directoryName, + %% {rdnSequence, + %% [[#'AttributeTypeAndValue'{ + %% type = ?'id-at-commonName', + %% value = + %% {utf8String, + %% unicode:characters_to_binary( + %% Name, utf8) + %% } + %% }]]}}], + %% critical = true} + ]} | CertOptions]}), NodeConf = [{server, ServerConf ++ Conf}, {client, ClientConf ++ Conf}], {ok, Fd} = file:open(ConfFile, [write]), -- cgit v1.2.3 From c9b2af1f2cecb6565a2c79d683d86a1d7ee746e0 Mon Sep 17 00:00:00 2001 From: Raimo Niskanen Date: Tue, 24 Apr 2018 15:19:54 +0200 Subject: Check client IP from server --- lib/ssl/src/inet_tls_dist.erl | 144 +++++++++++++++++++++++++--------- lib/ssl/test/ssl_dist_bench_SUITE.erl | 52 ++++++++---- 2 files changed, 141 insertions(+), 55 deletions(-) (limited to 'lib/ssl') diff --git a/lib/ssl/src/inet_tls_dist.erl b/lib/ssl/src/inet_tls_dist.erl index 3fab89fa97..f43a7b03e6 100644 --- a/lib/ssl/src/inet_tls_dist.erl +++ b/lib/ssl/src/inet_tls_dist.erl @@ -222,8 +222,9 @@ gen_accept(Driver, Listen) -> accept_loop(Driver, Listen, Kernel) -> case Driver:accept(Listen) of {ok, Socket} -> - Opts = get_ssl_options(server), - CertNodesFun = verify_client_cert_nodes_fun(Opts), + {Opts,CertNodesFun} = + setup_verify_client( + Driver, Socket, get_ssl_options(server)), wait_for_code_server(), case ssl:ssl_accept( @@ -262,37 +263,94 @@ accept_loop(Driver, Listen, Kernel) -> end. %% {verify_fun,{fun ?MODULE:verify_client/3,_}} is used -%% as a marker that the peer certificate shall be parsed -%% for node names after succesful connection +%% as a configuration marker that verify_client/3 shall be used. %% -verify_client_cert_nodes_fun([]) -> - undefined; -verify_client_cert_nodes_fun([Opt|Opts]) -> +%% Replace the State in the first occurence of +%% {verify_fun,{fun ?MODULE:verify_client/3,State}} and remove the rest. +%% The inserted state is not accesible from a configuration file +%% since it is dynamic and connection dependent. +%% +setup_verify_client(Driver, Socket, Opts) -> + setup_verify_client(Driver, Socket, Opts, undefined, []). +%% +setup_verify_client(_Driver, _Socket, [], CertNodesFun, OptsR) -> + {lists:reverse(OptsR),CertNodesFun}; +setup_verify_client(Driver, Socket, [Opt|Opts], CertNodesFun, OptsR) -> case Opt of - {verify_fun,{Fun,CertNodesFun}} -> + {verify_fun,{Fun,NewCertNodesFun}} -> case Fun =:= fun ?MODULE:verify_client/3 of - true when not is_function(CertNodesFun, 1) -> + true when is_function(NewCertNodesFun, 1) -> + if + CertNodesFun =:= undefined -> + case inet:peername(Socket) of + {ok,{PeerIP,_Port}} -> + setup_verify_client( + Driver, Socket, Opts, NewCertNodesFun, + [{verify_fun, + {Fun, + {NewCertNodesFun,Driver,PeerIP}}} + |OptsR]); + {error,Reason} -> + exit(trace({no_peername,Reason})) + end; + true -> + setup_verify_client( + Driver, Socket, Opts, CertNodesFun, OptsR) + end; + true -> exit( trace( {verify_client_bad_argument,CertNodesFun})); - true -> - CertNodesFun; false -> - verify_client_cert_nodes_fun(Opts) + setup_verify_client( + Driver, Socket, Opts, CertNodesFun, [Opt|OptsR]) end; _ -> - verify_client_cert_nodes_fun(Opts) + setup_verify_client( + Driver, Socket, Opts, CertNodesFun, [Opt|OptsR]) end. -%% Same as verify_peer - approves all valid certificates +%% Same as verify_peer but check cert host names for +%% peer IP address verify_client(_, {bad_cert,_} = Reason, _) -> {fail,Reason}; -verify_client(_, {extension,_}, F) -> - {unknown,F}; -verify_client(_, valid, F) -> - {valid,F}; -verify_client(_, valid_peer, F) -> - {valid_peer,F}. +verify_client(_, {extension,_}, S) -> + {unknown,S}; +verify_client(_, valid, S) -> + {valid,S}; +verify_client(PeerCert, valid_peer, {CertNodesFun,Driver,PeerIP} = S) -> + %% + %% Parse out all node names from the peer's certificate + %% + case CertNodesFun(PeerCert) of + [] -> + {fail,cert_missing_node_name}; + CertNodes -> + case filter_nodes_by_ip(CertNodes, Driver, PeerIP) of + [] -> + {fail,cert_no_host_with_peer_ip}; + _ -> + {valid,S} + end + end. + +filter_nodes_by_ip(Nodes, Driver, IP) -> + [Node || + Node <- Nodes, + case dist_util:split_node(Node) of + {_,Host} -> + filter_host_by_ip(Host, Driver, IP); + [Host] -> + filter_host_by_ip(Host, Driver, IP) + end]. + +filter_host_by_ip(Host, Driver, IP) -> + case Driver:getaddr(Host) of + {ok,IP} -> + true; + _ -> + false + end. wait_for_code_server() -> @@ -350,7 +408,8 @@ do_accept(Driver, Kernel, AcceptPid, DistCtrl, MyNode, Allowed, SetupTime) -> case check_ip(Driver, SslSocket) of true -> NewAllowed = - allowed_nodes(CertNodesFun, SslSocket, Allowed), + allowed_nodes( + Driver, CertNodesFun, SslSocket, Allowed), HSData0 = hs_data_common(SslSocket), HSData = HSData0#hs_data{ @@ -370,23 +429,31 @@ do_accept(Driver, Kernel, AcceptPid, DistCtrl, MyNode, Allowed, SetupTime) -> end end. -allowed_nodes(undefined, _SslSocket, Allowed) -> +allowed_nodes(_Driver, undefined, _SslSocket, Allowed) -> Allowed; -allowed_nodes(CertNodesFun, SslSocket, Allowed) -> +allowed_nodes(Driver, CertNodesFun, SslSocket, Allowed) -> case ssl:peercert(SslSocket) of {ok,PeerCertDER} -> - PeerCert = public_key:pkix_decode_cert(PeerCertDER, otp), - %% - %% Parse out all node names from the peer's certificate - %% - case CertNodesFun(PeerCert) of - [] -> - ?shutdown(cert_missing_node_name); - CertNodes -> - allowed(CertNodes, Allowed) + case ssl:peername(SslSocket) of + {ok,{PeerIP,_Port}} -> + PeerCert = public_key:pkix_decode_cert(PeerCertDER, otp), + %% + %% Parse out all node names from the peer's certificate + %% + case CertNodesFun(PeerCert) of + [] -> + ?shutdown(cert_missing_node_name); + CertNodes -> + allowed( + filter_nodes_by_ip( + CertNodes, Driver, PeerIP), + Allowed) + end; + Error1 -> + ?shutdown2(no_peer_ip, trace(Error1)) end; - Error -> - ?shutdown2(no_peer_cert, trace(Error)) + Error2 -> + ?shutdown2(no_peer_cert, trace(Error2)) end. allowed(CertNodes, []) -> @@ -492,10 +559,9 @@ gen_close(Driver, Socket) -> %% as a configuration marker that verify_server/3 shall be used. %% %% Replace the State in the first occurence of -%% {verify_fun,{fun ?MODULE:verify_server/3,State}} -%% with Node and remove the rest. Node can not be used -%% from a configuration file since it is dynamic -%% and connection dependent. +%% {verify_fun,{fun ?MODULE:verify_server/3,State}} and remove the rest. +%% The inserted state is not accesible from a configuration file +%% since it is dynamic and connection dependent. %% setup_verify_server(Opts, Node) -> setup_verify_server(Opts, Node, true). @@ -618,7 +684,7 @@ parse_extensions( parse_extensions(Extensions, [Host|Hosts], Name); {name,NewName} when Name =:= none -> parse_extensions(Extensions, Hosts, NewName); - {Name,_} -> + {name,_NewName} -> parse_extensions(Extensions, Hosts, Name) end; parse_extensions([_|Extensions], Hosts, Name) -> diff --git a/lib/ssl/test/ssl_dist_bench_SUITE.erl b/lib/ssl/test/ssl_dist_bench_SUITE.erl index 31de0936f9..77c42de3f2 100644 --- a/lib/ssl/test/ssl_dist_bench_SUITE.erl +++ b/lib/ssl/test/ssl_dist_bench_SUITE.erl @@ -77,6 +77,7 @@ init_per_suite(Config) -> try Node =/= nonode@nohost orelse throw({skipped,"Node not distributed"}), + verify_node_src_addr(), {supported, SSLVersions} = lists:keyfind(supported, 1, ssl:versions()), lists:member(TLSVersion, SSLVersions) orelse @@ -179,9 +180,28 @@ end_per_testcase(_Func, _Conf) -> %%%------------------------------------------------------------------- %%% CommonTest API helpers +verify_node_src_addr() -> + Msg = "Hello, world!", + {ok,Host} = inet:gethostname(), + {ok,DstAddr} = inet:getaddr(Host, inet), + {ok,Socket} = gen_udp:open(0, [{active,false}]), + {ok,Port} = inet:port(Socket), + ok = gen_udp:send(Socket, DstAddr, Port, Msg), + case gen_udp:recv(Socket, length(Msg) + 1, 1000) of + {ok,{DstAddr,Port,Msg}} -> + ok; + {ok,{SrcAddr,Port,Msg}} -> + throw({skipped, + "Src and dst address mismatch: " ++ + term_to_string(SrcAddr) ++ " =:= " ++ + term_to_string(DstAddr)}); + Weird -> + error(Weird) + end. + write_node_conf( ConfFile, Node, ServerConf, ClientConf, CertOptions, RootCert) -> - [_Name,Host] = string:split(atom_to_list(Node), "@"), + [Name,Host] = split_node(Node), Conf = public_key:pkix_test_data( #{root => RootCert, @@ -190,21 +210,21 @@ write_node_conf( [#'Extension'{ extnID = ?'id-ce-subjectAltName', extnValue = [{dNSName, Host}], - critical = true}%, - %% #'Extension'{ - %% extnID = ?'id-ce-subjectAltName', - %% extnValue = - %% [{directoryName, - %% {rdnSequence, - %% [[#'AttributeTypeAndValue'{ - %% type = ?'id-at-commonName', - %% value = - %% {utf8String, - %% unicode:characters_to_binary( - %% Name, utf8) - %% } - %% }]]}}], - %% critical = true} + critical = true}, + #'Extension'{ + extnID = ?'id-ce-subjectAltName', + extnValue = + [{directoryName, + {rdnSequence, + [[#'AttributeTypeAndValue'{ + type = ?'id-at-commonName', + value = + {utf8String, + unicode:characters_to_binary( + Name, utf8) + } + }]]}}], + critical = true} ]} | CertOptions]}), NodeConf = [{server, ServerConf ++ Conf}, {client, ClientConf ++ Conf}], -- cgit v1.2.3 From ba25ad268065f48ad7647464c8f4c237d07d18b1 Mon Sep 17 00:00:00 2001 From: Raimo Niskanen Date: Wed, 25 Apr 2018 08:51:00 +0200 Subject: Move check ip to before SSL handshake --- lib/ssl/src/inet_tls_dist.erl | 140 +++++++++++++++++++--------------- lib/ssl/test/ssl_dist_bench_SUITE.erl | 7 +- 2 files changed, 81 insertions(+), 66 deletions(-) (limited to 'lib/ssl') diff --git a/lib/ssl/src/inet_tls_dist.erl b/lib/ssl/src/inet_tls_dist.erl index f43a7b03e6..b084135d1a 100644 --- a/lib/ssl/src/inet_tls_dist.erl +++ b/lib/ssl/src/inet_tls_dist.erl @@ -222,46 +222,58 @@ gen_accept(Driver, Listen) -> accept_loop(Driver, Listen, Kernel) -> case Driver:accept(Listen) of {ok, Socket} -> - {Opts,CertNodesFun} = - setup_verify_client( - Driver, Socket, get_ssl_options(server)), - wait_for_code_server(), - case - ssl:ssl_accept( - Socket, - trace([{active, false},{packet, 4}|Opts]), - net_kernel:connecttime()) - of - {ok, #sslsocket{pid = DistCtrl} = SslSocket} -> - trace( - Kernel ! - {accept, self(), DistCtrl, - Driver:family(), tls}), - receive - {Kernel, controller, Pid} -> - ok = ssl:controlling_process(SslSocket, Pid), - trace( - Pid ! {self(), controller, CertNodesFun}); - {Kernel, unsupported_protocol} -> - exit(trace(unsupported_protocol)) - end, - accept_loop(Driver, Listen, Kernel); - {error, {options, _}} = Error -> - %% Bad options: that's probably our fault. - %% Let's log that. + case check_ip(Driver, Socket) of + true -> + accept_loop(Driver, Listen, Kernel, Socket); + {false,IP} -> error_logger:error_msg( - "Cannot accept TLS distribution connection: ~s~n", - [ssl:format_error(Error)]), - gen_tcp:close(Socket), - exit(trace(Error)); - Other -> - gen_tcp:close(Socket), - exit(trace(Other)) + "** Connection attempt from " + "disallowed IP ~w ** ~n", [IP]), + ?shutdown2(no_node, trace({disallowed, IP})) end; Error -> exit(trace(Error)) end. +accept_loop(Driver, Listen, Kernel, Socket) -> + {Opts,CertNodesFun} = + setup_verify_client( + Driver, Socket, get_ssl_options(server)), + wait_for_code_server(), + case + ssl:ssl_accept( + Socket, + trace([{active, false},{packet, 4}|Opts]), + net_kernel:connecttime()) + of + {ok, #sslsocket{pid = DistCtrl} = SslSocket} -> + trace( + Kernel ! + {accept, self(), DistCtrl, + Driver:family(), tls}), + receive + {Kernel, controller, Pid} -> + ok = ssl:controlling_process(SslSocket, Pid), + trace( + Pid ! {self(), controller, CertNodesFun}); + {Kernel, unsupported_protocol} -> + exit(trace(unsupported_protocol)) + end, + accept_loop(Driver, Listen, Kernel); + {error, {options, _}} = Error -> + %% Bad options: that's probably our fault. + %% Let's log that. + error_logger:error_msg( + "Cannot accept TLS distribution connection: ~s~n", + [ssl:format_error(Error)]), + gen_tcp:close(Socket), + exit(trace(Error)); + Other -> + gen_tcp:close(Socket), + exit(trace(Other)) + end. + + %% {verify_fun,{fun ?MODULE:verify_client/3,_}} is used %% as a configuration marker that verify_client/3 shall be used. %% @@ -326,6 +338,8 @@ verify_client(PeerCert, valid_peer, {CertNodesFun,Driver,PeerIP} = S) -> [] -> {fail,cert_missing_node_name}; CertNodes -> + %% Check if the IP address of one of the nodes + %% in the peer certificate has the peer's IP address case filter_nodes_by_ip(CertNodes, Driver, PeerIP) of [] -> {fail,cert_no_host_with_peer_ip}; @@ -334,6 +348,8 @@ verify_client(PeerCert, valid_peer, {CertNodesFun,Driver,PeerIP} = S) -> end end. +%% Filter out the nodes that has got the given IP address +%% filter_nodes_by_ip(Nodes, Driver, IP) -> [Node || Node <- Nodes, @@ -405,30 +421,25 @@ do_accept(Driver, Kernel, AcceptPid, DistCtrl, MyNode, Allowed, SetupTime) -> receive {AcceptPid, controller, CertNodesFun} -> Timer = dist_util:start_timer(SetupTime), - case check_ip(Driver, SslSocket) of - true -> - NewAllowed = - allowed_nodes( - Driver, CertNodesFun, SslSocket, Allowed), - HSData0 = hs_data_common(SslSocket), - HSData = - HSData0#hs_data{ - kernel_pid = Kernel, - this_node = MyNode, - socket = DistCtrl, - timer = Timer, - this_flags = 0, - allowed = NewAllowed}, - link(DistCtrl), - dist_util:handshake_other_started(trace(HSData)); - {false,IP} -> - error_logger:error_msg( - "** Connection attempt from " - "disallowed IP ~w ** ~n", [IP]), - ?shutdown2(no_node, trace({disallowed, IP})) - end + NewAllowed = + allowed_nodes( + Driver, CertNodesFun, SslSocket, Allowed), + HSData0 = hs_data_common(SslSocket), + HSData = + HSData0#hs_data{ + kernel_pid = Kernel, + this_node = MyNode, + socket = DistCtrl, + timer = Timer, + this_flags = 0, + allowed = NewAllowed}, + link(DistCtrl), + dist_util:handshake_other_started(trace(HSData)) end. +%% Return a list of allowed nodes according to +%% the given Allowed list that matches the peer certificate +%% allowed_nodes(_Driver, undefined, _SslSocket, Allowed) -> Allowed; allowed_nodes(Driver, CertNodesFun, SslSocket, Allowed) -> @@ -444,6 +455,10 @@ allowed_nodes(Driver, CertNodesFun, SslSocket, Allowed) -> [] -> ?shutdown(cert_missing_node_name); CertNodes -> + %% Filter out all nodes in the + %% allowed list that is in peer + %% certificate and that has got + %% the same IP address as the peer allowed( filter_nodes_by_ip( CertNodes, Driver, PeerIP), @@ -457,7 +472,8 @@ allowed_nodes(Driver, CertNodesFun, SslSocket, Allowed) -> end. allowed(CertNodes, []) -> - %% All allowed -> allow only certificate nodes + %% Empty allowed list means all allowed + %% -> allow only certificate nodes CertNodes; allowed(CertNodes, Allowed) -> %% Find the intersection of the allowed list and certificate nodes @@ -602,6 +618,8 @@ verify_server(PeerCert, valid_peer, {CertNodesFun,Node} = S) -> [] -> {fail,cert_missing_node_name}; CertNodes -> + %% Check that the node we are connecting to + %% is in the peer certificate case dist_util:is_allowed(Node, CertNodes) of true -> {valid,S}; @@ -615,15 +633,15 @@ verify_server(PeerCert, valid_peer, {CertNodesFun,Node} = S) -> %% Do only accept new connection attempts from nodes at our %% own LAN, if the check_ip environment parameter is true. %% ------------------------------------------------------------ -check_ip(Driver, SslSocket) -> +check_ip(Driver, Socket) -> case application:get_env(check_ip) of {ok, true} -> - case get_ifs(SslSocket) of + case get_ifs(Socket) of {ok, IFs, IP} -> check_ip(Driver, IFs, IP); Other -> ?shutdown2( - no_node, trace({check_ip_failed, SslSocket, Other})) + no_node, trace({check_ip_failed, Socket, Other})) end; _ -> true @@ -637,7 +655,7 @@ check_ip(Driver, [{OwnIP, _, Netmask}|IFs], PeerIP) -> check_ip(_Driver, [], PeerIP) -> {false, PeerIP}. -get_ifs(#sslsocket{fd = {gen_tcp, Socket, _}}) -> +get_ifs(Socket) -> case inet:peername(Socket) of {ok, {IP, _}} -> %% XXX this is seriously broken for IPv6 diff --git a/lib/ssl/test/ssl_dist_bench_SUITE.erl b/lib/ssl/test/ssl_dist_bench_SUITE.erl index 77c42de3f2..4ffc486fc4 100644 --- a/lib/ssl/test/ssl_dist_bench_SUITE.erl +++ b/lib/ssl/test/ssl_dist_bench_SUITE.erl @@ -246,7 +246,7 @@ setup(Config) -> run_nodepair_test(fun setup/5, Config). setup(A, B, Prefix, HA, HB) -> - Rounds = 10, + Rounds = 50, [] = ssl_apply(HA, erlang, nodes, []), [] = ssl_apply(HB, erlang, nodes, []), {SetupTime, CycleTime} = @@ -276,10 +276,7 @@ setup_loop(A, B, T, N) -> receive {'DOWN',Mref,process,_,_} -> [] = erlang:nodes(), - receive - after 500 -> - setup_loop(A, B, Time + T, N - 1) - end + setup_loop(A, B, Time + T, N - 1) end. -- cgit v1.2.3 From 5586959ed008c09141689f1e8865476150e48519 Mon Sep 17 00:00:00 2001 From: Raimo Niskanen Date: Thu, 26 Apr 2018 09:37:24 +0200 Subject: Allow check for node name --- lib/ssl/src/inet_tls_dist.erl | 172 +++++++++++++++++----------------- lib/ssl/test/ssl_dist_bench_SUITE.erl | 3 +- 2 files changed, 87 insertions(+), 88 deletions(-) (limited to 'lib/ssl') diff --git a/lib/ssl/src/inet_tls_dist.erl b/lib/ssl/src/inet_tls_dist.erl index b084135d1a..3e9828a2fe 100644 --- a/lib/ssl/src/inet_tls_dist.erl +++ b/lib/ssl/src/inet_tls_dist.erl @@ -53,7 +53,7 @@ select(Node) -> gen_select(Driver, Node) -> case dist_util:split_node(Node) of - {_,Host} -> + {node,_,Host} -> case Driver:getaddr(Host) of {ok, _} -> true; _ -> false @@ -141,7 +141,7 @@ f_address(SslSocket, Node) -> case ssl:peername(SslSocket) of {ok, Address} -> case dist_util:split_node(Node) of - {_,Host} -> + {node,_,Host} -> #net_address{ address=Address, host=Host, protocol=tls, family=inet}; @@ -335,7 +335,11 @@ verify_client(PeerCert, valid_peer, {CertNodesFun,Driver,PeerIP} = S) -> %% Parse out all node names from the peer's certificate %% case CertNodesFun(PeerCert) of + undefined -> + %% Certificate allows all nodes + {valid,S}; [] -> + %% Certificate allows no nodes {fail,cert_missing_node_name}; CertNodes -> %% Check if the IP address of one of the nodes @@ -354,10 +358,12 @@ filter_nodes_by_ip(Nodes, Driver, IP) -> [Node || Node <- Nodes, case dist_util:split_node(Node) of - {_,Host} -> + {node,_,Host} -> filter_host_by_ip(Host, Driver, IP); - [Host] -> - filter_host_by_ip(Host, Driver, IP) + {host,Host} -> + filter_host_by_ip(Host, Driver, IP); + {name,_Name} -> + true end]. filter_host_by_ip(Host, Driver, IP) -> @@ -452,7 +458,11 @@ allowed_nodes(Driver, CertNodesFun, SslSocket, Allowed) -> %% Parse out all node names from the peer's certificate %% case CertNodesFun(PeerCert) of + undefined -> + %% Certificate allows all nodes + Allowed; [] -> + %% Certificate allows no nodes ?shutdown(cert_missing_node_name); CertNodes -> %% Filter out all nodes in the @@ -615,7 +625,11 @@ verify_server(PeerCert, valid_peer, {CertNodesFun,Node} = S) -> %% Parse out all node names from the peer's certificate %% case CertNodesFun(PeerCert) of + undefined -> + %% Certificate allows all nodes + {valid,S}; [] -> + %% Certificate allows no nodes {fail,cert_missing_node_name}; CertNodes -> %% Check that the node we are connecting to @@ -681,32 +695,36 @@ cert_nodes( parse_extensions(Extensions) when is_list(Extensions) -> - parse_extensions(Extensions, [], none); + parse_extensions(Extensions, [], []); parse_extensions(asn1_NOVALUE) -> - []. + undefined. % Allow all nodes %% -parse_extensions([], Hosts, none) -> +parse_extensions([], [], []) -> + undefined; % Allow all nodes +parse_extensions([], Hosts, []) -> lists:reverse(Hosts); -parse_extensions([], Hosts, Name) -> - [Name ++ "@" ++ Host || Host <- lists:reverse(Hosts)]; +parse_extensions([], [], Names) -> + [Name ++ "@" || Name <- lists:reverse(Names)]; +parse_extensions([], Hosts, Names) -> + [Name ++ "@" ++ Host || + Host <- lists:reverse(Hosts), + Name <- lists:reverse(Names)]; parse_extensions( [#'Extension'{ extnID = ?'id-ce-subjectAltName', extnValue = AltNames} |Extensions], - Hosts, Name) -> + Hosts, Names) -> case parse_subject_altname(AltNames) of none -> - parse_extensions(Extensions, Hosts, Name); + parse_extensions(Extensions, Hosts, Names); {host,Host} -> - parse_extensions(Extensions, [Host|Hosts], Name); - {name,NewName} when Name =:= none -> - parse_extensions(Extensions, Hosts, NewName); - {name,_NewName} -> - parse_extensions(Extensions, Hosts, Name) + parse_extensions(Extensions, [Host|Hosts], Names); + {name,Name} -> + parse_extensions(Extensions, Hosts, [Name|Names]) end; -parse_extensions([_|Extensions], Hosts, Name) -> - parse_extensions(Extensions, Hosts, Name). +parse_extensions([_|Extensions], Hosts, Names) -> + parse_extensions(Extensions, Hosts, Names). parse_subject_altname([]) -> none; @@ -742,9 +760,9 @@ parse_rdn([_|Rdn]) -> %% If Node is illegal terminate the connection setup!! split_node(Driver, Node, LongOrShortNames) -> case dist_util:split_node(Node) of - {Name, Host} -> - check_node(Driver, Name, Node, Host, LongOrShortNames); - [_] -> + {node, Name, Host} -> + check_node(Driver, Node, Name, Host, LongOrShortNames); + {host, _} -> error_logger:error_msg( "** Nodename ~p illegal, no '@' character **~n", [Node]), @@ -755,8 +773,8 @@ split_node(Driver, Node, LongOrShortNames) -> ?shutdown2(Node, trace({illegal_node_name, Node})) end. -check_node(Driver, Name, Node, Host, LongOrShortNames) -> - case string:split(Host, ".") of +check_node(Driver, Node, Name, Host, LongOrShortNames) -> + case string:split(Host, ".", all) of [_] when LongOrShortNames =:= longnames -> case Driver:parse_address(Host) of {ok, _} -> @@ -824,70 +842,50 @@ get_ssl_dist_arguments(Type) -> [{erl_dist, true}] end. -ssl_options(_,[]) -> + +ssl_options(_Type, []) -> []; -ssl_options(server, ["client_" ++ _, _Value |T]) -> - ssl_options(server,T); -ssl_options(client, ["server_" ++ _, _Value|T]) -> - ssl_options(client,T); -ssl_options(server, ["server_certfile", Value|T]) -> - [{certfile, Value} | ssl_options(server,T)]; -ssl_options(client, ["client_certfile", Value | T]) -> - [{certfile, Value} | ssl_options(client,T)]; -ssl_options(server, ["server_cacertfile", Value|T]) -> - [{cacertfile, Value} | ssl_options(server,T)]; -ssl_options(client, ["client_cacertfile", Value|T]) -> - [{cacertfile, Value} | ssl_options(client,T)]; -ssl_options(server, ["server_keyfile", Value|T]) -> - [{keyfile, Value} | ssl_options(server,T)]; -ssl_options(client, ["client_keyfile", Value|T]) -> - [{keyfile, Value} | ssl_options(client,T)]; -ssl_options(server, ["server_password", Value|T]) -> - [{password, Value} | ssl_options(server,T)]; -ssl_options(client, ["client_password", Value|T]) -> - [{password, Value} | ssl_options(client,T)]; -ssl_options(server, ["server_verify", Value|T]) -> - [{verify, atomize(Value)} | ssl_options(server,T)]; -ssl_options(client, ["client_verify", Value|T]) -> - [{verify, atomize(Value)} | ssl_options(client,T)]; -ssl_options(server, ["server_verify_fun", Value|T]) -> - [{verify_fun, verify_fun(Value)} | ssl_options(server,T)]; -ssl_options(client, ["client_verify_fun", Value|T]) -> - [{verify_fun, verify_fun(Value)} | ssl_options(client,T)]; -ssl_options(server, ["server_crl_check", Value|T]) -> - [{crl_check, atomize(Value)} | ssl_options(server,T)]; -ssl_options(client, ["client_crl_check", Value|T]) -> - [{crl_check, atomize(Value)} | ssl_options(client,T)]; -ssl_options(server, ["server_crl_cache", Value|T]) -> - [{crl_cache, termify(Value)} | ssl_options(server,T)]; -ssl_options(client, ["client_crl_cache", Value|T]) -> - [{crl_cache, termify(Value)} | ssl_options(client,T)]; -ssl_options(server, ["server_reuse_sessions", Value|T]) -> - [{reuse_sessions, atomize(Value)} | ssl_options(server,T)]; -ssl_options(client, ["client_reuse_sessions", Value|T]) -> - [{reuse_sessions, atomize(Value)} | ssl_options(client,T)]; -ssl_options(server, ["server_secure_renegotiate", Value|T]) -> - [{secure_renegotiate, atomize(Value)} | ssl_options(server,T)]; -ssl_options(client, ["client_secure_renegotiate", Value|T]) -> - [{secure_renegotiate, atomize(Value)} | ssl_options(client,T)]; -ssl_options(server, ["server_depth", Value|T]) -> - [{depth, list_to_integer(Value)} | ssl_options(server,T)]; -ssl_options(client, ["client_depth", Value|T]) -> - [{depth, list_to_integer(Value)} | ssl_options(client,T)]; -ssl_options(server, ["server_hibernate_after", Value|T]) -> - [{hibernate_after, list_to_integer(Value)} | ssl_options(server,T)]; -ssl_options(client, ["client_hibernate_after", Value|T]) -> - [{hibernate_after, list_to_integer(Value)} | ssl_options(client,T)]; -ssl_options(server, ["server_ciphers", Value|T]) -> - [{ciphers, Value} | ssl_options(server,T)]; -ssl_options(client, ["client_ciphers", Value|T]) -> - [{ciphers, Value} | ssl_options(client,T)]; -ssl_options(server, ["server_dhfile", Value|T]) -> - [{dhfile, Value} | ssl_options(server,T)]; -ssl_options(server, ["server_fail_if_no_peer_cert", Value|T]) -> - [{fail_if_no_peer_cert, atomize(Value)} | ssl_options(server,T)]; -ssl_options(Type, Opts) -> - error(malformed_ssl_dist_opt, [Type, Opts]). +ssl_options(client, ["client_" ++ Opt, Value | T] = Opts) -> + ssl_options(client, T, Opts, Opt, Value); +ssl_options(server, ["server_" ++ Opt, Value | T] = Opts) -> + ssl_options(server, T, Opts, Opt, Value); +ssl_options(Type, [_Opt, _Value | T]) -> + ssl_options(Type, T). +%% +ssl_options(Type, T, Opts, Opt, Value) -> + case ssl_option(Type, Opt) of + error -> + error(malformed_ssl_dist_opt, [Type, Opts]); + Fun -> + [{list_to_atom(Opt), Fun(Value)}|ssl_options(Type, T)] + end. + +ssl_option(server, Opt) -> + case Opt of + "dhfile" -> fun listify/1; + "fail_if_no_peer_cert" -> fun atomize/1; + _ -> ssl_option(client, Opt) + end; +ssl_option(client, Opt) -> + case Opt of + "certfile" -> fun listify/1; + "cacertfile" -> fun listify/1; + "keyfile" -> fun listify/1; + "password" -> fun listify/1; + "verify" -> fun atomize/1; + "verify_fun" -> fun verify_fun/1; + "crl_check" -> fun atomize/1; + "crl_cache" -> fun termify/1; + "reuse_sessions" -> fun atomize/1; + "secure_renegotiate" -> fun atomize/1; + "depth" -> fun erlang:list_to_integer/1; + "hibernate_after" -> fun erlang:list_to_integer/1; + "ciphers" -> fun listify/1; + _ -> error + end. + +listify(List) when is_list(List) -> + List. atomize(List) when is_list(List) -> list_to_atom(List); diff --git a/lib/ssl/test/ssl_dist_bench_SUITE.erl b/lib/ssl/test/ssl_dist_bench_SUITE.erl index 4ffc486fc4..f827ea12bb 100644 --- a/lib/ssl/test/ssl_dist_bench_SUITE.erl +++ b/lib/ssl/test/ssl_dist_bench_SUITE.erl @@ -207,7 +207,8 @@ write_node_conf( #{root => RootCert, peer => [{extensions, - [#'Extension'{ + [ + #'Extension'{ extnID = ?'id-ce-subjectAltName', extnValue = [{dNSName, Host}], critical = true}, -- cgit v1.2.3 From 5803f0ad5be257b451588e8da83d1295eabea85e Mon Sep 17 00:00:00 2001 From: Raimo Niskanen Date: Fri, 27 Apr 2018 11:13:40 +0200 Subject: Fix distro CRL test cases short vs long names --- lib/ssl/test/make_certs.erl | 30 ++++++++++++++++++++---------- lib/ssl/test/ssl_dist_SUITE.erl | 7 ++++--- 2 files changed, 24 insertions(+), 13 deletions(-) (limited to 'lib/ssl') diff --git a/lib/ssl/test/make_certs.erl b/lib/ssl/test/make_certs.erl index ecbacc1590..8fe7c54549 100644 --- a/lib/ssl/test/make_certs.erl +++ b/lib/ssl/test/make_certs.erl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2007-2017. All Rights Reserved. +%% Copyright Ericsson AB 2007-2018. All Rights Reserved. %% %% Licensed under the Apache License, Version 2.0 (the "License"); %% you may not use this file except in compliance with the License. @@ -19,7 +19,7 @@ %% -module(make_certs). --compile([export_all]). +-compile([export_all, nowarn_export_all]). %-export([all/1, all/2, rootCA/2, intermediateCA/3, endusers/3, enduser/3, revoke/3, gencrl/2, verify/3]). @@ -34,14 +34,15 @@ ecc_certs = false, issuing_distribution_point = false, crl_port = 8000, - openssl_cmd = "openssl"}). + openssl_cmd = "openssl", + hostname = "host.example.com"}). default_config() -> - #config{}. + #config{hostname = net_adm:localhost()}. make_config(Args) -> - make_config(Args, #config{}). + make_config(Args, default_config()). make_config([], C) -> C; @@ -66,7 +67,9 @@ make_config([{ecc_certs, Bool}|T], C) when is_boolean(Bool) -> make_config([{issuing_distribution_point, Bool}|T], C) when is_boolean(Bool) -> make_config(T, C#config{issuing_distribution_point = Bool}); make_config([{openssl_cmd, Cmd}|T], C) when is_list(Cmd) -> - make_config(T, C#config{openssl_cmd = Cmd}). + make_config(T, C#config{openssl_cmd = Cmd}); +make_config([{hostname, Hostname}|T], C) when is_list(Hostname) -> + make_config(T, C#config{hostname = Hostname}). all([DataDir, PrivDir]) -> @@ -384,8 +387,11 @@ req_cnf(Root, C) -> "subjectKeyIdentifier = hash\n" "subjectAltName = email:copy\n"]. -ca_cnf(Root, C = #config{issuing_distribution_point = true}) -> - Hostname = net_adm:localhost(), +ca_cnf( + Root, + #config{ + issuing_distribution_point = true, + hostname = Hostname} = C) -> ["# Purpose: Configuration for CAs.\n" "\n" "ROOTDIR = " ++ Root ++ "\n" @@ -464,8 +470,12 @@ ca_cnf(Root, C = #config{issuing_distribution_point = true}) -> "crlDistributionPoints=@crl_section\n" ]; -ca_cnf(Root, C = #config{issuing_distribution_point = false}) -> - Hostname = net_adm:localhost(), +ca_cnf( + Root, + #config{ + issuing_distribution_point = false, + hostname = Hostname + } = C) -> ["# Purpose: Configuration for CAs.\n" "\n" "ROOTDIR = " ++ Root ++ "\n" diff --git a/lib/ssl/test/ssl_dist_SUITE.erl b/lib/ssl/test/ssl_dist_SUITE.erl index c822a52d1f..003e1fc448 100644 --- a/lib/ssl/test/ssl_dist_SUITE.erl +++ b/lib/ssl/test/ssl_dist_SUITE.erl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2007-2017. All Rights Reserved. +%% Copyright Ericsson AB 2007-2018. All Rights Reserved. %% %% Licensed under the Apache License, Version 2.0 (the "License"); %% you may not use this file except in compliance with the License. @@ -25,7 +25,7 @@ -include("ssl_dist_test_lib.hrl"). %% Note: This directive should only be used in test suites. --compile(export_all). +-compile([export_all, nowarn_export_all]). -define(DEFAULT_TIMETRAP_SECS, 240). @@ -724,7 +724,8 @@ setup_certs(Config) -> ok = file:make_dir(NodeDir), ok = file:make_dir(RGenDir), make_randfile(RGenDir), - {ok, _} = make_certs:all(RGenDir, NodeDir), + [Hostname|_] = string:split(net_adm:localhost(), ".", all), + {ok, _} = make_certs:all(RGenDir, NodeDir, [{hostname,Hostname}]), SDir = filename:join([NodeDir, "server"]), SC = filename:join([SDir, "cert.pem"]), SK = filename:join([SDir, "key.pem"]), -- cgit v1.2.3