Skip to content

Commit 3a4e005

Browse files
kinyoklionkinyoklion
authored
fix: Redact SDK in gen_server descriptions and network errors. (#166)
There are two categories of issues addressed in this PR. The first is that there are situations where an SDK key can be logged if it is part of process state. So this PR introduces format_status callbacks which control how that formatting is done, and redacts the SDK key. The second is there are some network errors that could log the headers which include authorization. This PR adds some extra formatting to network errors to reduce the detail contained in the error. A side-effect will be that in some situations logs will contain less useful data for troubleshooting.
1 parent 9dbe853 commit 3a4e005

7 files changed

+437
-11
lines changed

‎src/ldclient_event_dispatch_httpc.erl‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,10 +60,10 @@ send(State, JsonEvents, PayloadId, Uri) ->
6060

6161
-type http_request() :: {ok, {{string(), integer(), string()}, [{string(), string()}], string() | binary()}}.
6262

63-
-spec process_request({error, term()} | http_request())
63+
-spec process_request({error, term()} | http_request())
6464
-> {ok, integer()} | {error, temporary, string()} | {error, permanent, string()}.
6565
process_request({error, Reason}) ->
66-
{error, temporary, Reason};
66+
{error, temporary, ldclient_key_redaction:format_httpc_error(Reason)};
6767
process_request({ok, {{_Version, StatusCode, _ReasonPhrase}, Headers, _Body}}) when StatusCode < 400 ->
6868
{ok, get_server_time(Headers)};
6969
process_request({ok, {{Version, StatusCode, ReasonPhrase}, _Headers, _Body}}) ->

‎src/ldclient_event_process_server.erl‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
-export([start_link/1, init/1]).
1313

1414
%% Behavior callbacks
15-
-export([code_change/3, handle_call/3, handle_cast/2, handle_info/2, terminate/2]).
15+
-export([code_change/3, handle_call/3, handle_cast/2, handle_info/2, terminate/2, format_status/1]).
1616

1717
%% API
1818
-export([
@@ -152,6 +152,15 @@ terminate(_Reason, _State) ->
152152
code_change(_OldVsn, State, _Extra) ->
153153
{ok, State}.
154154

155+
%% @doc Redact SDK key from state for logging
156+
%% @private
157+
%%
158+
%% @end
159+
format_status(#{state := State}) ->
160+
#{state => State#{sdk_key => "[REDACTED]"}};
161+
format_status(Other) ->
162+
Other.
163+
155164
%%===================================================================
156165
%% Internal functions
157166
%%===================================================================

‎src/ldclient_key_redaction.erl‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
%%-------------------------------------------------------------------
2+
%% @doc SDK key redaction utilities
3+
%% @private
4+
%% Provides functions to safely format error reasons and other data
5+
%% to prevent SDK keys from appearing in logs.
6+
%% @end
7+
%%-------------------------------------------------------------------
8+
9+
-module(ldclient_key_redaction).
10+
11+
%% API
12+
-export([format_httpc_error/1]).
13+
-export([format_shotgun_error/1]).
14+
15+
%%===================================================================
16+
%% API
17+
%%===================================================================
18+
19+
%% @doc Format httpc error for safe logging
20+
%%
21+
%% This function provides an abstract representation of httpc error reasons
22+
%% that is safe to log. It only formats known error types. Unknown
23+
%% error types are represented as "unknown error" to prevent accidental
24+
%% exposure of SDK keys, especially in cases where the SDK key might
25+
%% contain special characters like newlines.
26+
%% @end
27+
-spec format_httpc_error(Reason :: term()) -> string().
28+
%% HTTP status codes or other integers are safe to log
29+
format_httpc_error(StatusCode) when is_integer(StatusCode) ->
30+
lists:flatten(io_lib:format("~b", [StatusCode]));
31+
%% Common httpc connection errors
32+
format_httpc_error({failed_connect, _}) ->
33+
"failed to connect";
34+
format_httpc_error(timeout) ->
35+
"timeout opening connection";
36+
format_httpc_error(etimedout) ->
37+
"connection timeout";
38+
format_httpc_error(econnrefused) ->
39+
"connection refused";
40+
format_httpc_error(enetunreach) ->
41+
"network unreachable";
42+
format_httpc_error(ehostunreach) ->
43+
"host unreachable";
44+
format_httpc_error(nxdomain) ->
45+
"domain name not found";
46+
format_httpc_error({tls_alert, {_, Description}}) when is_atom(Description) ->
47+
lists:flatten(io_lib:format("tls_alert: ~p", [Description]));
48+
format_httpc_error({tls_alert, Alert}) ->
49+
lists:flatten(io_lib:format("tls_alert: ~p", [Alert]));
50+
%% Socket errors
51+
format_httpc_error({socket_closed_remotely, _, _}) ->
52+
"socket closed remotely";
53+
format_httpc_error(closed) ->
54+
"connection closed";
55+
format_httpc_error(enotconn) ->
56+
"socket not connected";
57+
%% Known atom errors
58+
format_httpc_error(Reason) when is_atom(Reason) ->
59+
atom_to_list(Reason);
60+
%% For any unknown error type, do not expose details
61+
format_httpc_error(_Reason) ->
62+
"unknown error".
63+
64+
%% @doc Format shotgun/gun error for safe logging
65+
%%
66+
%% This function provides an abstract representation of shotgun error reasons
67+
%% that is safe to log. Shotgun uses gun underneath, so errors can come from gun.
68+
%% Unknown error types are represented as "unknown error".
69+
%% @end
70+
-spec format_shotgun_error(Reason :: term()) -> string().
71+
%% HTTP status codes or other integers are safe to log
72+
format_shotgun_error(StatusCode) when is_integer(StatusCode) ->
73+
lists:flatten(io_lib:format("~b", [StatusCode]));
74+
%% Known shotgun errors from open
75+
format_shotgun_error(gun_open_failed) ->
76+
"connection failed to open";
77+
format_shotgun_error(gun_open_timeout) ->
78+
"timeout opening connection";
79+
%% Gun/socket errors
80+
format_shotgun_error(timeout) ->
81+
"connection timeout";
82+
format_shotgun_error(econnrefused) ->
83+
"connection refused";
84+
format_shotgun_error(enetunreach) ->
85+
"network unreachable";
86+
format_shotgun_error(ehostunreach) ->
87+
"host unreachable";
88+
format_shotgun_error(nxdomain) ->
89+
"domain name not found";
90+
format_shotgun_error({shutdown, _}) ->
91+
"connection shutdown";
92+
format_shotgun_error(normal) ->
93+
"connection closed normally";
94+
format_shotgun_error(closed) ->
95+
"connection closed";
96+
%% Known atom errors
97+
format_shotgun_error(Reason) when is_atom(Reason) ->
98+
atom_to_list(Reason);
99+
%% For any unknown error type, do not expose details
100+
format_shotgun_error(_Reason) ->
101+
"unknown error".

‎src/ldclient_update_poll_server.erl‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
-export([start_link/1, init/1]).
1313

1414
%% Behavior callbacks
15-
-export([code_change/3, handle_call/3, handle_cast/2, handle_info/2, terminate/2]).
15+
-export([code_change/3, handle_call/3, handle_cast/2, handle_info/2, terminate/2, format_status/1]).
1616

1717
-type state() :: #{
1818
sdk_key := string(),
@@ -107,6 +107,15 @@ terminate(_Reason, _State) ->
107107
code_change(_OldVsn, State, _Extra) ->
108108
{ok, State}.
109109

110+
%% @doc Redact SDK key from state for logging
111+
%% @private
112+
%%
113+
%% @end
114+
format_status(#{state := State}) ->
115+
#{state => State#{sdk_key => "[REDACTED]"}};
116+
format_status(Other) ->
117+
Other.
118+
110119
%%===================================================================
111120
%% Internal functions
112121
%%===================================================================

‎src/ldclient_update_stream_server.erl‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,9 @@ handle_info({listen}, #{stream_uri := Uri} = State) ->
9292
handle_info({'DOWN', _Mref, process, ShotgunPid, Reason}, #{conn := ShotgunPid, backoff := Backoff} = State) ->
9393
NewBackoff = ldclient_backoff:fail(Backoff),
9494
_ = ldclient_backoff:fire(NewBackoff),
95-
error_logger:warning_msg("Got DOWN message from shotgun pid with reason: ~p, will retry in ~p ms~n", [Reason, maps:get(current, NewBackoff)]),
95+
% Reason from DOWN message could contain connection details with headers/SDK keys
96+
SafeReason = ldclient_key_redaction:format_shotgun_error(Reason),
97+
error_logger:warning_msg("Got DOWN message from shotgun pid with reason: ~s, will retry in ~p ms~n", [SafeReason, maps:get(current, NewBackoff)]),
9698
{noreply, State#{conn := undefined, backoff := NewBackoff}};
9799
handle_info({timeout, _TimerRef, listen}, State) ->
98100
error_logger:info_msg("Reconnecting streaming connection...~n"),
@@ -132,13 +134,16 @@ do_listen(#{
132134
NewBackoff = do_listen_fail_backoff(Backoff, temporary, Reason),
133135
State#{backoff := NewBackoff};
134136
{error, permanent, Reason} ->
137+
% Reason here is already safe: either a sanitized string from format_shotgun_error
138+
% or an integer status code from the do_listen/5 method.
135139
error_logger:error_msg("Stream encountered permanent error ~p, giving up~n", [Reason]),
136140
State;
137141
{ok, Pid} ->
138142
NewBackoff = ldclient_backoff:succeed(Backoff),
139143
State#{conn := Pid, backoff := NewBackoff}
140-
catch Code:Reason ->
141-
NewBackoff = do_listen_fail_backoff(Backoff, Code, Reason),
144+
catch Code:_Reason ->
145+
% Don't pass raw exception reason as it could contain unsafe data
146+
NewBackoff = do_listen_fail_backoff(Backoff, Code, "unexpected exception"),
142147
State#{backoff := NewBackoff}
143148
end.
144149

@@ -171,9 +176,10 @@ do_listen(Uri, FeatureStore, Tag, GunOpts, Headers) ->
171176
F = fun(nofin, _Ref, Bin) ->
172177
try
173178
process_event(parse_shotgun_event(Bin), FeatureStore, Tag)
174-
catch Code:Reason ->
175-
% Exception when processing event, log error, close connection
176-
error_logger:warning_msg("Invalid SSE event error (~p): ~p", [Code, Reason]),
179+
catch Code:_Reason ->
180+
% Exception when processing event - don't log exception details
181+
% as they could theoretically contain sensitive data
182+
error_logger:warning_msg("Invalid SSE event error (~p)", [Code]),
177183
shotgun:close(Pid)
178184
end;
179185
(fin, _Ref, _Bin) ->
@@ -185,7 +191,8 @@ do_listen(Uri, FeatureStore, Tag, GunOpts, Headers) ->
185191
case shotgun:get(Pid, Path ++ Query, Headers, Options) of
186192
{error, Reason} ->
187193
shotgun:close(Pid),
188-
{error, temporary, Reason};
194+
SafeReason = ldclient_key_redaction:format_shotgun_error(Reason),
195+
{error, temporary, SafeReason};
189196
{ok, #{status_code := StatusCode}} when StatusCode >= 400 ->
190197
{error, ldclient_http:is_http_error_code_recoverable(StatusCode), StatusCode};
191198
{ok, _Ref} ->

‎test/ldclient_key_redaction_SUITE.erl‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
-module(ldclient_key_redaction_SUITE).
2+
3+
-include_lib("common_test/include/ct.hrl").
4+
5+
%% ct functions
6+
-export([all/0]).
7+
-export([init_per_suite/1]).
8+
-export([end_per_suite/1]).
9+
10+
%% Tests
11+
-export([
12+
format_httpc_error_atom/1,
13+
format_httpc_error_integer/1,
14+
format_httpc_error_tuple/1,
15+
format_httpc_error_unknown/1,
16+
format_shotgun_error_atom/1,
17+
format_shotgun_error_integer/1,
18+
format_shotgun_error_unknown/1
19+
]).
20+
21+
%%====================================================================
22+
%% ct functions
23+
%%====================================================================
24+
25+
all() ->
26+
[
27+
format_httpc_error_atom,
28+
format_httpc_error_integer,
29+
format_httpc_error_tuple,
30+
format_httpc_error_unknown,
31+
format_shotgun_error_atom,
32+
format_shotgun_error_integer,
33+
format_shotgun_error_unknown
34+
].
35+
36+
init_per_suite(Config) ->
37+
Config.
38+
39+
end_per_suite(_) ->
40+
ok.
41+
42+
%%====================================================================
43+
%% Tests
44+
%%====================================================================
45+
46+
format_httpc_error_atom(_) ->
47+
% Known atom errors should be formatted safely
48+
"timeout opening connection" = ldclient_key_redaction:format_httpc_error(timeout),
49+
"connection refused" = ldclient_key_redaction:format_httpc_error(econnrefused),
50+
"connection timeout" = ldclient_key_redaction:format_httpc_error(etimedout).
51+
52+
format_httpc_error_integer(_) ->
53+
% HTTP status codes should be formatted as integers
54+
"404" = ldclient_key_redaction:format_httpc_error(404),
55+
"500" = ldclient_key_redaction:format_httpc_error(500),
56+
"200" = ldclient_key_redaction:format_httpc_error(200).
57+
58+
format_httpc_error_tuple(_) ->
59+
% Known tuple errors should be formatted safely
60+
"failed to connect" = ldclient_key_redaction:format_httpc_error({failed_connect, []}),
61+
"connection closed" = ldclient_key_redaction:format_httpc_error(closed).
62+
63+
format_httpc_error_unknown(_) ->
64+
% Unknown error types, including those that might contain SDK keys, should be redacted
65+
"unknown error" = ldclient_key_redaction:format_httpc_error({http_error, "sdk-12345"}),
66+
"unknown error" = ldclient_key_redaction:format_httpc_error(["Authorization: sdk-test\n"]),
67+
"unknown error" = ldclient_key_redaction:format_httpc_error({request, [{headers, [{"Authorization", "sdk-key-with-newline\n"}]}]}).
68+
69+
format_shotgun_error_atom(_) ->
70+
% Known atom errors should be formatted safely
71+
"connection timeout" = ldclient_key_redaction:format_shotgun_error(timeout),
72+
"connection failed to open" = ldclient_key_redaction:format_shotgun_error(gun_open_failed),
73+
"timeout opening connection" = ldclient_key_redaction:format_shotgun_error(gun_open_timeout),
74+
"connection refused" = ldclient_key_redaction:format_shotgun_error(econnrefused).
75+
76+
format_shotgun_error_integer(_) ->
77+
% HTTP status codes should be formatted as integers
78+
"401" = ldclient_key_redaction:format_shotgun_error(401),
79+
"500" = ldclient_key_redaction:format_shotgun_error(500).
80+
81+
format_shotgun_error_unknown(_) ->
82+
% Unknown error types, including those that might contain SDK keys, should be redacted
83+
"unknown error" = ldclient_key_redaction:format_shotgun_error({gun_error, "sdk-12345"}),
84+
"unknown error" = ldclient_key_redaction:format_shotgun_error(["Headers: sdk-test\n"]),
85+
"unknown error" = ldclient_key_redaction:format_shotgun_error({connection_error, [{auth, "sdk-malformed\nkey"}]}).

0 commit comments

Comments
 (0)