From 63ef2f9b79e21eeec2e042fe31e67b8134c533a2 Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Fri, 3 Mar 2023 11:54:22 -0300 Subject: [PATCH] feat: save uploaded OCSP issuer pem like other ssl certs --- apps/emqx/src/emqx_tls_lib.erl | 88 ++++++++++++------- apps/emqx/test/emqx_ocsp_cache_SUITE.erl | 22 ++++- apps/emqx/test/emqx_tls_lib_tests.erl | 55 +++++++++--- .../src/emqx_dashboard_listener.erl | 2 +- 4 files changed, 118 insertions(+), 49 deletions(-) diff --git a/apps/emqx/src/emqx_tls_lib.erl b/apps/emqx/src/emqx_tls_lib.erl index eb6091f29..eb8234547 100644 --- a/apps/emqx/src/emqx_tls_lib.erl +++ b/apps/emqx/src/emqx_tls_lib.erl @@ -47,8 +47,18 @@ -define(IS_TRUE(Val), ((Val =:= true) orelse (Val =:= <<"true">>))). -define(IS_FALSE(Val), ((Val =:= false) orelse (Val =:= <<"false">>))). --define(SSL_FILE_OPT_NAMES, [<<"keyfile">>, <<"certfile">>, <<"cacertfile">>]). --define(SSL_FILE_OPT_NAMES_A, [keyfile, certfile, cacertfile]). +-define(SSL_FILE_OPT_NAMES, [ + [<<"keyfile">>], + [<<"certfile">>], + [<<"cacertfile">>], + [<<"ocsp">>, <<"issuer_pem">>] +]). +-define(SSL_FILE_OPT_NAMES_A, [ + [keyfile], + [certfile], + [cacertfile], + [ocsp, issuer_pem] +]). %% non-empty string -define(IS_STRING(L), (is_list(L) andalso L =/= [] andalso is_integer(hd(L)))). @@ -298,20 +308,20 @@ ensure_ssl_files(Dir, SSL, Opts) -> RequiredKeys = maps:get(required_keys, Opts, []), case ensure_ssl_file_key(SSL, RequiredKeys) of ok -> - Keys = ?SSL_FILE_OPT_NAMES ++ ?SSL_FILE_OPT_NAMES_A, - ensure_ssl_files(Dir, SSL, Keys, Opts); + KeyPaths = ?SSL_FILE_OPT_NAMES ++ ?SSL_FILE_OPT_NAMES_A, + ensure_ssl_files(Dir, SSL, KeyPaths, Opts); {error, _} = Error -> Error end. ensure_ssl_files(_Dir, SSL, [], _Opts) -> {ok, SSL}; -ensure_ssl_files(Dir, SSL, [Key | Keys], Opts) -> - case ensure_ssl_file(Dir, Key, SSL, maps:get(Key, SSL, undefined), Opts) of +ensure_ssl_files(Dir, SSL, [KeyPath | KeyPaths], Opts) -> + case ensure_ssl_file(Dir, KeyPath, SSL, emqx_map_lib:deep_get(KeyPath, SSL, undefined), Opts) of {ok, NewSSL} -> - ensure_ssl_files(Dir, NewSSL, Keys, Opts); + ensure_ssl_files(Dir, NewSSL, KeyPaths, Opts); {error, Reason} -> - {error, Reason#{which_options => [Key]}} + {error, Reason#{which_options => [KeyPath]}} end. %% @doc Compare old and new config, delete the ones in old but not in new. @@ -321,11 +331,11 @@ delete_ssl_files(Dir, NewOpts0, OldOpts0) -> {ok, NewOpts} = ensure_ssl_files(Dir, NewOpts0, #{dry_run => DryRun}), {ok, OldOpts} = ensure_ssl_files(Dir, OldOpts0, #{dry_run => DryRun}), Get = fun - (_K, undefined) -> undefined; - (K, Opts) -> maps:get(K, Opts, undefined) + (_KP, undefined) -> undefined; + (KP, Opts) -> emqx_map_lib:deep_get(KP, Opts, undefined) end, lists:foreach( - fun(Key) -> delete_old_file(Get(Key, NewOpts), Get(Key, OldOpts)) end, + fun(KeyPath) -> delete_old_file(Get(KeyPath, NewOpts), Get(KeyPath, OldOpts)) end, ?SSL_FILE_OPT_NAMES ++ ?SSL_FILE_OPT_NAMES_A ), %% try to delete the dir if it is empty @@ -346,29 +356,33 @@ delete_old_file(_New, Old) -> ?SLOG(error, #{msg => "failed_to_delete_ssl_file", file_path => Old, reason => Reason}) end. -ensure_ssl_file(_Dir, _Key, SSL, undefined, _Opts) -> +ensure_ssl_file(_Dir, _KeyPath, SSL, undefined, _Opts) -> {ok, SSL}; -ensure_ssl_file(Dir, Key, SSL, MaybePem, Opts) -> +ensure_ssl_file(Dir, KeyPath, SSL, MaybePem, Opts) -> case is_valid_string(MaybePem) of true -> DryRun = maps:get(dry_run, Opts, false), - do_ensure_ssl_file(Dir, Key, SSL, MaybePem, DryRun); + do_ensure_ssl_file(Dir, KeyPath, SSL, MaybePem, DryRun); false -> {error, #{reason => invalid_file_path_or_pem_string}} end. -do_ensure_ssl_file(Dir, Key, SSL, MaybePem, DryRun) -> +do_ensure_ssl_file(Dir, KeyPath, SSL, MaybePem, DryRun) -> case is_pem(MaybePem) of true -> - case save_pem_file(Dir, Key, MaybePem, DryRun) of - {ok, Path} -> {ok, SSL#{Key => Path}}; - {error, Reason} -> {error, Reason} + case save_pem_file(Dir, KeyPath, MaybePem, DryRun) of + {ok, Path} -> + NewSSL = emqx_map_lib:deep_put(KeyPath, SSL, Path), + {ok, NewSSL}; + {error, Reason} -> + {error, Reason} end; false -> case is_valid_pem_file(MaybePem) of true -> {ok, SSL}; - {error, enoent} when DryRun -> {ok, SSL}; + {error, enoent} when DryRun -> + {ok, SSL}; {error, Reason} -> {error, #{ pem_check => invalid_pem, @@ -398,8 +412,8 @@ is_pem(MaybePem) -> %% To make it simple, the file is always overwritten. %% Also a potentially half-written PEM file (e.g. due to power outage) %% can be corrected with an overwrite. -save_pem_file(Dir, Key, Pem, DryRun) -> - Path = pem_file_name(Dir, Key, Pem), +save_pem_file(Dir, KeyPath, Pem, DryRun) -> + Path = pem_file_name(Dir, KeyPath, Pem), case filelib:ensure_dir(Path) of ok when DryRun -> {ok, Path}; @@ -422,11 +436,14 @@ is_generated_file(Filename) -> _ -> false end. -pem_file_name(Dir, Key, Pem) -> +pem_file_name(Dir, KeyPath, Pem) -> <> = crypto:hash(md5, Pem), Suffix = hex_str(CK), - FileName = binary:replace(ensure_bin(Key), <<"file">>, <<"-", Suffix/binary>>), - filename:join([pem_dir(Dir), FileName]). + Segments = lists:map(fun ensure_bin/1, KeyPath), + Filename0 = iolist_to_binary(lists:join(<<"_">>, Segments)), + Filename1 = binary:replace(Filename0, <<"file">>, <<>>), + Filename = <>, + filename:join([pem_dir(Dir), Filename]). pem_dir(Dir) -> filename:join([emqx:mutable_certs_dir(), Dir]). @@ -465,9 +482,9 @@ is_valid_pem_file(Path) -> %% so they are forced to upload a cert file, or use an existing file path. -spec drop_invalid_certs(map()) -> map(). drop_invalid_certs(#{enable := False} = SSL) when ?IS_FALSE(False) -> - maps:without(?SSL_FILE_OPT_NAMES_A, SSL); + lists:foldl(fun emqx_map_lib:deep_remove/2, SSL, ?SSL_FILE_OPT_NAMES_A); drop_invalid_certs(#{<<"enable">> := False} = SSL) when ?IS_FALSE(False) -> - maps:without(?SSL_FILE_OPT_NAMES, SSL); + lists:foldl(fun emqx_map_lib:deep_remove/2, SSL, ?SSL_FILE_OPT_NAMES); drop_invalid_certs(#{enable := True} = SSL) when ?IS_TRUE(True) -> do_drop_invalid_certs(?SSL_FILE_OPT_NAMES_A, SSL); drop_invalid_certs(#{<<"enable">> := True} = SSL) when ?IS_TRUE(True) -> @@ -475,14 +492,16 @@ drop_invalid_certs(#{<<"enable">> := True} = SSL) when ?IS_TRUE(True) -> do_drop_invalid_certs([], SSL) -> SSL; -do_drop_invalid_certs([Key | Keys], SSL) -> - case maps:get(Key, SSL, undefined) of +do_drop_invalid_certs([KeyPath | KeyPaths], SSL) -> + case emqx_map_lib:deep_get(KeyPath, SSL, undefined) of undefined -> - do_drop_invalid_certs(Keys, SSL); + do_drop_invalid_certs(KeyPaths, SSL); PemOrPath -> case is_pem(PemOrPath) orelse is_valid_pem_file(PemOrPath) of - true -> do_drop_invalid_certs(Keys, SSL); - {error, _} -> do_drop_invalid_certs(Keys, maps:without([Key], SSL)) + true -> + do_drop_invalid_certs(KeyPaths, SSL); + {error, _} -> + do_drop_invalid_certs(KeyPaths, emqx_map_lib:deep_remove(KeyPath, SSL)) end end. @@ -565,9 +584,10 @@ ensure_bin(A) when is_atom(A) -> atom_to_binary(A, utf8). ensure_ssl_file_key(_SSL, []) -> ok; -ensure_ssl_file_key(SSL, RequiredKeys) -> - Filter = fun(Key) -> not maps:is_key(Key, SSL) end, - case lists:filter(Filter, RequiredKeys) of +ensure_ssl_file_key(SSL, RequiredKeyPaths) -> + NotFoundRef = make_ref(), + Filter = fun(KeyPath) -> NotFoundRef =:= emqx_map_lib:deep_get(KeyPath, SSL, NotFoundRef) end, + case lists:filter(Filter, RequiredKeyPaths) of [] -> ok; Miss -> {error, #{reason => ssl_file_option_not_found, which_options => Miss}} end. diff --git a/apps/emqx/test/emqx_ocsp_cache_SUITE.erl b/apps/emqx/test/emqx_ocsp_cache_SUITE.erl index 1f5e34548..3b1c2adaf 100644 --- a/apps/emqx/test/emqx_ocsp_cache_SUITE.erl +++ b/apps/emqx/test/emqx_ocsp_cache_SUITE.erl @@ -673,7 +673,8 @@ do_t_update_listener(Config) -> Keyfile = filename:join([DataDir, "server.key"]), Certfile = filename:join([DataDir, "server.pem"]), Cacertfile = filename:join([DataDir, "ca.pem"]), - IssuerPem = filename:join([DataDir, "ocsp-issuer.pem"]), + IssuerPemPath = filename:join([DataDir, "ocsp-issuer.pem"]), + {ok, IssuerPem} = file:read_file(IssuerPemPath), %% no ocsp at first ListenerId = "ssl:default", @@ -701,6 +702,9 @@ do_t_update_listener(Config) -> <<"ocsp">> => #{ <<"enable_ocsp_stapling">> => true, + %% we use the file contents to check that + %% the API converts that to an internally + %% managed file <<"issuer_pem">> => IssuerPem, <<"responder_url">> => <<"http://localhost:9877">> } @@ -722,6 +726,22 @@ do_t_update_listener(Config) -> }, ListenerData2 ), + %% issuer pem should have been uploaded and saved to a new + %% location + ?assertNotEqual( + IssuerPemPath, + emqx_map_lib:deep_get( + [<<"ssl_options">>, <<"ocsp">>, <<"issuer_pem">>], + ListenerData2 + ) + ), + ?assertNotEqual( + IssuerPem, + emqx_map_lib:deep_get( + [<<"ssl_options">>, <<"ocsp">>, <<"issuer_pem">>], + ListenerData2 + ) + ), assert_http_get(1, 5_000), ok. diff --git a/apps/emqx/test/emqx_tls_lib_tests.erl b/apps/emqx/test/emqx_tls_lib_tests.erl index 5a81daf6a..5510e4027 100644 --- a/apps/emqx/test/emqx_tls_lib_tests.erl +++ b/apps/emqx/test/emqx_tls_lib_tests.erl @@ -117,7 +117,7 @@ ssl_files_failure_test_() -> %% empty string ?assertMatch( {error, #{ - reason := invalid_file_path_or_pem_string, which_options := [<<"keyfile">>] + reason := invalid_file_path_or_pem_string, which_options := [[<<"keyfile">>]] }}, emqx_tls_lib:ensure_ssl_files("/tmp", #{ <<"keyfile">> => <<>>, @@ -128,7 +128,7 @@ ssl_files_failure_test_() -> %% not valid unicode ?assertMatch( {error, #{ - reason := invalid_file_path_or_pem_string, which_options := [<<"keyfile">>] + reason := invalid_file_path_or_pem_string, which_options := [[<<"keyfile">>]] }}, emqx_tls_lib:ensure_ssl_files("/tmp", #{ <<"keyfile">> => <<255, 255>>, @@ -136,6 +136,18 @@ ssl_files_failure_test_() -> <<"cacertfile">> => bin(test_key()) }) ), + ?assertMatch( + {error, #{ + reason := invalid_file_path_or_pem_string, + which_options := [[<<"ocsp">>, <<"issuer_pem">>]] + }}, + emqx_tls_lib:ensure_ssl_files("/tmp", #{ + <<"keyfile">> => bin(test_key()), + <<"certfile">> => bin(test_key()), + <<"cacertfile">> => bin(test_key()), + <<"ocsp">> => #{<<"issuer_pem">> => <<255, 255>>} + }) + ), %% not printable ?assertMatch( {error, #{reason := invalid_file_path_or_pem_string}}, @@ -155,7 +167,8 @@ ssl_files_failure_test_() -> #{ <<"cacertfile">> => bin(TmpFile), <<"keyfile">> => bin(TmpFile), - <<"certfile">> => bin(TmpFile) + <<"certfile">> => bin(TmpFile), + <<"ocsp">> => #{<<"issuer_pem">> => bin(TmpFile)} } ) ) @@ -170,22 +183,29 @@ ssl_files_save_delete_test() -> SSL0 = #{ <<"keyfile">> => Key, <<"certfile">> => Key, - <<"cacertfile">> => Key + <<"cacertfile">> => Key, + <<"ocsp">> => #{<<"issuer_pem">> => Key} }, Dir = filename:join(["/tmp", "ssl-test-dir"]), {ok, SSL} = emqx_tls_lib:ensure_ssl_files(Dir, SSL0), - File = maps:get(<<"keyfile">>, SSL), - ?assertMatch(<<"/tmp/ssl-test-dir/key-", _:16/binary>>, File), - ?assertEqual({ok, bin(test_key())}, file:read_file(File)), + FileKey = maps:get(<<"keyfile">>, SSL), + ?assertMatch(<<"/tmp/ssl-test-dir/key-", _:16/binary>>, FileKey), + ?assertEqual({ok, bin(test_key())}, file:read_file(FileKey)), + FileIssuerPem = emqx_map_lib:deep_get([<<"ocsp">>, <<"issuer_pem">>], SSL), + ?assertMatch(<<"/tmp/ssl-test-dir/ocsp_issuer_pem-", _:16/binary>>, FileIssuerPem), + ?assertEqual({ok, bin(test_key())}, file:read_file(FileIssuerPem)), %% no old file to delete ok = emqx_tls_lib:delete_ssl_files(Dir, SSL, undefined), - ?assertEqual({ok, bin(test_key())}, file:read_file(File)), + ?assertEqual({ok, bin(test_key())}, file:read_file(FileKey)), + ?assertEqual({ok, bin(test_key())}, file:read_file(FileIssuerPem)), %% old and new identical, no delete ok = emqx_tls_lib:delete_ssl_files(Dir, SSL, SSL), - ?assertEqual({ok, bin(test_key())}, file:read_file(File)), + ?assertEqual({ok, bin(test_key())}, file:read_file(FileKey)), + ?assertEqual({ok, bin(test_key())}, file:read_file(FileIssuerPem)), %% new is gone, delete old ok = emqx_tls_lib:delete_ssl_files(Dir, undefined, SSL), - ?assertEqual({error, enoent}, file:read_file(File)), + ?assertEqual({error, enoent}, file:read_file(FileKey)), + ?assertEqual({error, enoent}, file:read_file(FileIssuerPem)), %% test idempotence ok = emqx_tls_lib:delete_ssl_files(Dir, undefined, SSL), ok. @@ -198,7 +218,8 @@ ssl_files_handle_non_generated_file_test() -> SSL0 = #{ <<"keyfile">> => TmpKeyFile, <<"certfile">> => TmpKeyFile, - <<"cacertfile">> => TmpKeyFile + <<"cacertfile">> => TmpKeyFile, + <<"ocsp">> => #{<<"issuer_pem">> => TmpKeyFile} }, Dir = filename:join(["/tmp", "ssl-test-dir-00"]), {ok, SSL2} = emqx_tls_lib:ensure_ssl_files(Dir, SSL0), @@ -216,24 +237,32 @@ ssl_file_replace_test() -> SSL0 = #{ <<"keyfile">> => Key1, <<"certfile">> => Key1, - <<"cacertfile">> => Key1 + <<"cacertfile">> => Key1, + <<"ocsp">> => #{<<"issuer_pem">> => Key1} }, SSL1 = #{ <<"keyfile">> => Key2, <<"certfile">> => Key2, - <<"cacertfile">> => Key2 + <<"cacertfile">> => Key2, + <<"ocsp">> => #{<<"issuer_pem">> => Key2} }, Dir = filename:join(["/tmp", "ssl-test-dir2"]), {ok, SSL2} = emqx_tls_lib:ensure_ssl_files(Dir, SSL0), {ok, SSL3} = emqx_tls_lib:ensure_ssl_files(Dir, SSL1), File1 = maps:get(<<"keyfile">>, SSL2), File2 = maps:get(<<"keyfile">>, SSL3), + IssuerPem1 = emqx_map_lib:deep_get([<<"ocsp">>, <<"issuer_pem">>], SSL2), + IssuerPem2 = emqx_map_lib:deep_get([<<"ocsp">>, <<"issuer_pem">>], SSL3), ?assert(filelib:is_regular(File1)), ?assert(filelib:is_regular(File2)), + ?assert(filelib:is_regular(IssuerPem1)), + ?assert(filelib:is_regular(IssuerPem2)), %% delete old file (File1, in SSL2) ok = emqx_tls_lib:delete_ssl_files(Dir, SSL3, SSL2), ?assertNot(filelib:is_regular(File1)), ?assert(filelib:is_regular(File2)), + ?assertNot(filelib:is_regular(IssuerPem1)), + ?assert(filelib:is_regular(IssuerPem2)), ok. bin(X) -> iolist_to_binary(X). diff --git a/apps/emqx_dashboard/src/emqx_dashboard_listener.erl b/apps/emqx_dashboard/src/emqx_dashboard_listener.erl index 112b3ad58..eac4f845f 100644 --- a/apps/emqx_dashboard/src/emqx_dashboard_listener.erl +++ b/apps/emqx_dashboard/src/emqx_dashboard_listener.erl @@ -163,7 +163,7 @@ diff_listeners(Type, Stop, Start) -> {#{Type => Stop}, #{Type => Start}}. ensure_ssl_cert(#{<<"listeners">> := #{<<"https">> := #{<<"enable">> := true}}} = Conf) -> Https = emqx_map_lib:deep_get([<<"listeners">>, <<"https">>], Conf, undefined), - Opts = #{required_keys => [<<"keyfile">>, <<"certfile">>, <<"cacertfile">>]}, + Opts = #{required_keys => [[<<"keyfile">>], [<<"certfile">>], [<<"cacertfile">>]]}, case emqx_tls_lib:ensure_ssl_files(?DIR, Https, Opts) of {ok, undefined} -> {error, <<"ssl_cert_not_found">>};