diff --git a/src/credentials_obfuscation.erl b/src/credentials_obfuscation.erl index a8c5ca5..c95d668 100644 --- a/src/credentials_obfuscation.erl +++ b/src/credentials_obfuscation.erl @@ -56,6 +56,6 @@ decrypt(undefined) -> undefined; decrypt(Term) -> credentials_obfuscation_svc:decrypt(Term). --spec refresh_config() -> ok. +-spec refresh_config() -> ok | {error, invalid_config}. refresh_config() -> credentials_obfuscation_svc:refresh_config(). diff --git a/src/credentials_obfuscation_svc.erl b/src/credentials_obfuscation_svc.erl index 7236ce5..c59ba50 100644 --- a/src/credentials_obfuscation_svc.erl +++ b/src/credentials_obfuscation_svc.erl @@ -49,7 +49,7 @@ start_link() -> get_config(Config) -> gen_server:call(?MODULE, {get_config, Config}). --spec refresh_config() -> ok. +-spec refresh_config() -> ok | {error, invalid_config}. refresh_config() -> gen_server:call(?MODULE, refresh_config). @@ -63,19 +63,20 @@ set_fallback_secret(Secret) when is_binary(Secret) -> -spec encrypt(iodata()) -> {plaintext, binary()} | {encrypted, binary()} | binary(). -encrypt(Term) when is_binary(Term); is_list(Term) -> +encrypt(Term) -> + Bin = to_binary(Term), try - gen_server:call(?MODULE, {encrypt, Term}, ?TIMEOUT) + gen_server:call(?MODULE, {encrypt, Bin}, ?TIMEOUT) catch exit:{timeout, _} -> %% We treat timeouts the same way we do other "encryption is impossible" %% scenarios: return the original value. This won't be acceptable to every user %% but might be to some. There is no right or wrong answer to whether %% availability or security are more important, so the users have to decide %% whether using {plaintext, Term} results is appropriate in their specific case. - {plaintext, to_binary(Term)}; + {plaintext, Bin}; _:_ -> %% see above - {plaintext, to_binary(Term)} + {plaintext, Bin} end. -spec decrypt({plaintext, binary()} | {encrypted, binary()}) -> binary(). @@ -100,10 +101,14 @@ handle_call({get_config, iterations}, _From, #state{iterations=Iterations}=State handle_call({get_config, secret}, _From, #state{secret=Secret}=State) -> {reply, Secret, State}; handle_call(refresh_config, _From, State0) -> - {ok, State1} = refresh_config(State0), - {reply, ok, State1}; + try refresh_config(State0) of + State1 -> + {reply, ok, State1} + catch _:_ -> + {reply, {error, invalid_config}, State0} + end; handle_call({encrypt, Term}, _From, #state{enabled=false}=State) -> - {reply, to_binary(Term), State}; + {reply, Term, State}; handle_call({encrypt, Term}, _From, #state{cipher=Cipher, hash=Hash, iterations=Iterations, @@ -111,11 +116,11 @@ handle_call({encrypt, Term}, _From, #state{cipher=Cipher, % We need to wrap the data in a tuple to be able to say if the decryption was % successful or not. We may just receive junk data if the secret is incorrect % upon decryption. - ClearText = {?VALUE_TAG, to_binary(Term)}, + ClearText = {?VALUE_TAG, Term}, Encrypted = credentials_obfuscation_pbe:encrypt_term(Cipher, Hash, Iterations, Secret, ClearText), case Encrypted of - {plaintext, {?VALUE_TAG, Bin}} -> - {reply, {plaintext, Bin}, State}; + {plaintext, {?VALUE_TAG, Term}} -> + {reply, {plaintext, Term}, State}; _ -> {reply, Encrypted, State} end; handle_call({decrypt, Term}, _From, #state{enabled=false}=State) -> @@ -167,16 +172,15 @@ init_state() -> {ok, State}. -spec refresh_config(#state{enabled::boolean(), cipher::atom(), hash::atom(), iterations::non_neg_integer(), secret::'$pending-secret' | binary()}) -> - {'ok', #state{enabled::boolean(), cipher::atom(), hash::atom(), iterations::non_neg_integer(), secret::'$pending-secret' | binary()}}. + #state{enabled::boolean(), cipher::atom(), hash::atom(), iterations::non_neg_integer(), secret::'$pending-secret' | binary()}. refresh_config(#state{secret=Secret}=State0) -> {ok, Enabled, Cipher, Hash, Iterations} = get_config_values(), ok = case Enabled of true -> check(Cipher, Hash, Iterations); false -> ok end, - State1 = State0#state{enabled = Enabled, cipher = Cipher, hash = Hash, - iterations = Iterations, secret = Secret}, - {ok, State1}. + State0#state{enabled = Enabled, cipher = Cipher, hash = Hash, + iterations = Iterations, secret = Secret}. get_config_values() -> Enabled = application:get_env(credentials_obfuscation, enabled, true), @@ -206,5 +210,11 @@ try_decrypt(Cipher, Hash, Iterations, Secret, Term) -> end. % currently the callers may rely on this process converting strings to binary -to_binary(Term) when is_list(Term) -> erlang:list_to_binary(Term); -to_binary(Term) when is_binary(Term) -> Term. +to_binary(Term) -> + try + iolist_to_binary(Term) + catch + _:_ -> + %% `none' prevents the argument from appearing in the stackstrace + erlang:error(badarg, none) + end. diff --git a/test/credentials_obfuscation_SUITE.erl b/test/credentials_obfuscation_SUITE.erl index 3253a09..a1fc538 100644 --- a/test/credentials_obfuscation_SUITE.erl +++ b/test/credentials_obfuscation_SUITE.erl @@ -13,6 +13,7 @@ all() -> [encrypt_decrypt, encrypt_decrypt_char_list_value, + encrypt_decrypt_invalid_char_list_value, use_predefined_secret, use_cookie_as_secret, change_of_secret_returns_passed_in_data, @@ -21,6 +22,7 @@ all() -> [encrypt_decrypt, change_default_cipher, disabled, refresh_configuration, + refresh_configuration_invalid_cipher, application_failure_for_invalid_cipher]. init_per_testcase(disabled, Config) -> @@ -81,6 +83,25 @@ encrypt_decrypt_char_list_value(_Config) -> ?assertEqual(Expected, credentials_obfuscation:decrypt(Encrypted)), ok. +encrypt_decrypt_invalid_char_list_value(_Config) -> + InvalidCredentials = "guest " ++ [128557], + Secret = credentials_obfuscation:secret(), + ?assert(is_binary(Secret)), + + Result = + try + credentials_obfuscation:encrypt(InvalidCredentials), + ok + catch + C:E:ST -> + {C, E, ST} + end, + %% bad argument is not present in stacktrace + ?assertMatch({error, badarg, [{credentials_obfuscation_svc, to_binary, 1, _}|_]}, Result), + %% ensure the server did not crash and preserved original secret + ?assertEqual(Secret, credentials_obfuscation:secret()), + ok. + use_predefined_secret(_Config) -> Secret = crypto:strong_rand_bytes(128), ok = credentials_obfuscation:set_secret(Secret), @@ -204,6 +225,26 @@ refresh_configuration(_Config) -> ?assertEqual(Value, credentials_obfuscation:decrypt(Value)), ok. +refresh_configuration_invalid_cipher(_Config) -> + ?assert(credentials_obfuscation:enabled()), + + Cipher = credentials_obfuscation:cipher(), + + Credentials = <<"guest">>, + Encrypted = credentials_obfuscation:encrypt(Credentials), + ?assertNotEqual(Credentials, Encrypted), + ?assertMatch({encrypted, _}, Encrypted), + ?assertEqual(Credentials, credentials_obfuscation:decrypt(Encrypted)), + + %% try to load invalid config + ok = application:set_env(credentials_obfuscation, cipher, dummy_cipher), + ?assertEqual({error, invalid_config}, credentials_obfuscation:refresh_config()), + + %% cipher is unchanged and encrypting still works + ?assertEqual(Cipher, credentials_obfuscation:cipher()), + ?assertMatch({encrypted, _}, credentials_obfuscation:encrypt(Credentials)), + ok. + application_failure_for_invalid_cipher(_Config) -> {error, _} = application:ensure_all_started(credentials_obfuscation), ok.