Skip to content

Commit 1e56cfb

Browse files
MarcialRosalesmergify[bot]
authored andcommitted
Remove duplicate code
that returns an Erlang ssl options from RabbitMq Configuration (cherry picked from commit f7e25b4)
1 parent 33de933 commit 1e56cfb

File tree

3 files changed

+66
-32
lines changed

3 files changed

+66
-32
lines changed

deps/oauth2_client/src/oauth2_client.erl

Lines changed: 19 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -280,9 +280,11 @@ lookup_oauth_provider_from_keyconfig() ->
280280
ssl_options = extract_ssl_options_as_list(Map)
281281
}.
282282

283+
284+
283285
-spec extract_ssl_options_as_list(#{atom() => any()}) -> proplists:proplist().
284286
extract_ssl_options_as_list(Map) ->
285-
{Verify, CaCerts, CaCertFile} = case maps:get(peer_verification, Map, verify_peer) of
287+
{Verify, CaCerts, CaCertFile} = case get_verify_or_peer_verification(Map, verify_peer) of
286288
verify_peer ->
287289
case maps:get(cacertfile, Map, undefined) of
288290
undefined ->
@@ -323,6 +325,20 @@ extract_ssl_options_as_list(Map) ->
323325
[]
324326
end.
325327

328+
% Replace peer_verification with verify to make it more consistent with other
329+
% ssl_options in RabbitMQ and Erlang's ssl options
330+
% Eventually, peer_verification will be removed. For now, both are allowed
331+
-spec get_verify_or_peer_verification(#{atom() => any()}, any()) -> proplists:proplist().
332+
get_verify_or_peer_verification(Ssl_options, Default) ->
333+
case maps:get(verify, Ssl_options, undefined) of
334+
undefined ->
335+
case maps:get(peer_verification, Ssl_options, undefined) of
336+
undefined -> Default;
337+
PeerVerification -> PeerVerification
338+
end;
339+
Verify -> Verify
340+
end.
341+
326342
lookup_oauth_provider_config(OAuth2ProviderId) ->
327343
case application:get_env(rabbitmq_auth_backend_oauth2, oauth_providers) of
328344
undefined -> {error, oauth_providers_not_found};
@@ -427,33 +443,10 @@ map_to_oauth_provider(PropList) when is_list(PropList) ->
427443
token_endpoint = proplists:get_value(token_endpoint, PropList),
428444
authorization_endpoint = proplists:get_value(authorization_endpoint, PropList, undefined),
429445
jwks_uri = proplists:get_value(jwks_uri, PropList, undefined),
430-
ssl_options = map_ssl_options(proplists:get_value(https, PropList, undefined))
446+
ssl_options = extract_ssl_options_as_list(maps:from_list(
447+
proplists:get_value(https, PropList, [])))
431448
}.
432449

433-
map_ssl_options(undefined) ->
434-
[{verify, verify_none},
435-
{depth, 10},
436-
{fail_if_no_peer_cert, false},
437-
{crl_check, false},
438-
{crl_cache, {ssl_crl_cache, {internal, [{http, 10000}]}}}];
439-
map_ssl_options(Ssl_options) ->
440-
Ssl_options1 = [{verify, proplists:get_value(verify, Ssl_options, verify_none)},
441-
{depth, proplists:get_value(depth, Ssl_options, 10)},
442-
{fail_if_no_peer_cert, proplists:get_value(fail_if_no_peer_cert, Ssl_options, false)},
443-
{crl_check, proplists:get_value(crl_check, Ssl_options, false)},
444-
{crl_cache, {ssl_crl_cache, {internal, [{http, 10000}]}}} | cacertfile(Ssl_options)],
445-
case proplists:get_value(hostname_verification, Ssl_options, none) of
446-
wildcard ->
447-
[{customize_hostname_check, [{match_fun, public_key:pkix_verify_hostname_match_fun(https)}]} | Ssl_options1];
448-
none ->
449-
Ssl_options1
450-
end.
451-
452-
cacertfile(Ssl_options) ->
453-
case proplists:get_value(cacertfile, Ssl_options) of
454-
undefined -> [];
455-
CaCertFile -> [{cacertfile, CaCertFile}]
456-
end.
457450

458451
enrich_oauth_provider({ok, OAuthProvider}, TLSOptions) ->
459452
{ok, OAuthProvider#oauth_provider{ssl_options=TLSOptions}};

deps/oauth2_client/test/unit_SUITE.erl

Lines changed: 43 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,11 @@ groups() ->
2525
[
2626
{ssl_options, [], [
2727
no_ssl_options_triggers_verify_peer,
28-
peer_verification_verify_none,
29-
peer_verification_verify_peer_with_cacertfile
28+
choose_verify_over_peer_verification,
29+
verify_set_to_verify_none,
30+
peer_verification_set_to_verify_none,
31+
peer_verification_set_to_verify_peer_with_cacertfile,
32+
verify_set_to_verify_peer_with_cacertfile
3033
]}
3134
].
3235

@@ -39,7 +42,29 @@ no_ssl_options_triggers_verify_peer(_) ->
3942
{cacerts, _CaCerts}
4043
], oauth2_client:extract_ssl_options_as_list(#{})).
4144

42-
peer_verification_verify_none(_) ->
45+
choose_verify_over_peer_verification(_) ->
46+
Expected1 = [
47+
{verify, verify_none}
48+
],
49+
?assertEqual(Expected1, oauth2_client:extract_ssl_options_as_list(
50+
#{verify => verify_none, peer_verification => verify_peer })).
51+
52+
verify_set_to_verify_none(_) ->
53+
Expected1 = [
54+
{verify, verify_none}
55+
],
56+
?assertEqual(Expected1, oauth2_client:extract_ssl_options_as_list(#{verify => verify_none})),
57+
58+
Expected2 = [
59+
{verify, verify_none}
60+
],
61+
?assertEqual(Expected2, oauth2_client:extract_ssl_options_as_list(#{
62+
verify => verify_none,
63+
cacertfile => "/tmp"
64+
})).
65+
66+
67+
peer_verification_set_to_verify_none(_) ->
4368
Expected1 = [
4469
{verify, verify_none}
4570
],
@@ -54,7 +79,7 @@ peer_verification_verify_none(_) ->
5479
})).
5580

5681

57-
peer_verification_verify_peer_with_cacertfile(_) ->
82+
peer_verification_set_to_verify_peer_with_cacertfile(_) ->
5883
Expected = [
5984
{verify, verify_peer},
6085
{depth, 10},
@@ -66,3 +91,17 @@ peer_verification_verify_peer_with_cacertfile(_) ->
6691
cacertfile => "/tmp",
6792
peer_verification => verify_peer
6893
})).
94+
95+
96+
verify_set_to_verify_peer_with_cacertfile(_) ->
97+
Expected = [
98+
{verify, verify_peer},
99+
{depth, 10},
100+
{crl_check,false},
101+
{fail_if_no_peer_cert,false},
102+
{cacertfile, "/tmp"}
103+
],
104+
?assertEqual(Expected, oauth2_client:extract_ssl_options_as_list(#{
105+
cacertfile => "/tmp",
106+
verify => verify_peer
107+
})).

deps/rabbitmq_auth_backend_oauth2/test/jwks_SUITE.erl

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,11 +105,13 @@ init_per_group(multi_resource, Config) ->
105105
#{
106106
<<"one">> => [
107107
{issuer, strict_jwks_url(Config, "/")},
108-
{jwks_uri, strict_jwks_url(Config, "/jwks1")}
108+
{jwks_uri, strict_jwks_url(Config, "/jwks1")},
109+
{https, [{verify, verify_none}]}
109110
],
110111
<<"two">> => [
111112
{issuer, strict_jwks_url(Config, "/")},
112-
{jwks_uri, strict_jwks_url(Config, "/jwks2")}
113+
{jwks_uri, strict_jwks_url(Config, "/jwks2")},
114+
{https, [{verify, verify_none}]}
113115
]
114116
},
115117
ok = rabbit_ct_broker_helpers:rpc(Config, 0, application, set_env, [rabbitmq_auth_backend_oauth2, resource_servers, ResourceServersConfig]),

0 commit comments

Comments
 (0)