Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assert TLS configuration when booting MongoosePush #192

Merged
merged 2 commits into from
Jul 20, 2020

Conversation

leszke
Copy link
Contributor

@leszke leszke commented Jul 17, 2020

The purpose of this PR is to prevent MPush from start when APNS required ciphers (ECDHE-RSA-AES128-GCM-SHA256 and ECDHE-RSA-AES256-GCM-SHA384) are not available in runtime. In this scenario user receives error message, briefly explaining what's going on.
Use case examples:

$ _build/prod/rel/mongoose_push/bin/mongoose_push foreground
when=2020-07-17T17:36:03.985 severity=info what=init text="Starting PoolsWarden" at=Elixir.Sparrow.PoolsWarden.init/1:95 pid=#PID<0.1826.0> result=success worker=pools_warden file=lib/sparrow/pools_warden.ex application=sparrow
{"application":"mongoose_push","at":"Elixir.MongoosePush.Application.check_apns_ciphers/0:234","file":"lib/mongoose_push/application.ex","pid":"#PID<0.1880.0>","reason":"no_apns_ciphers","severity":"error","status":"error","text":"APNS required ciphers missing","what":"tls_configuration","when":"2020-07-17T17:36:04.040"}
{"at":"application_controller.info_exited/3:1943","file":"application_controller.erl","pid":"#PID<0.1660.0>","severity":"info","text":"Application mongoose_push exited: MongoosePush.Application.start(:normal, []) returned an error: :no_apns_ciphers","what":"","when":"2020-07-17T17:36:04.040"}
=SUPERVISOR REPORT==== 17-Jul-2020::17:36:04.066666 ===
    supervisor: {local,gr_counter_sup}
    errorContext: child_terminated
    reason: killed
    offender: [{pid,<0.1742.0>},
               {id,gr_lager_default_tracer_counters},
               {mfargs,{gr_counter,start_link,
                                   [gr_lager_default_tracer_counters]}},
               {restart_type,transient},
               {shutdown,brutal_kill},
               {child_type,worker}]

=SUPERVISOR REPORT==== 17-Jul-2020::17:36:04.066749 ===
    supervisor: {local,gr_param_sup}
    errorContext: child_terminated
    reason: killed
    offender: [{pid,<0.1741.0>},
               {id,gr_lager_default_tracer_params},
               {mfargs,{gr_param,start_link,[gr_lager_default_tracer_params]}},
               {restart_type,transient},
               {shutdown,brutal_kill},
               {child_type,worker}]

{"Kernel pid terminated",application_controller,"{application_start_failure,mongoose_push,{no_apns_ciphers,{'Elixir.MongoosePush.Application',start,[normal,[]]}}}"}
Kernel pid terminated (application_controller) ({application_start_failure,mongoose_push,{no_apns_ciphers,{'Elixir.MongoosePush.Application',start,[normal,[]]}}})

Crash dump is being written to: erl_crash.dump...done
$ iex -S mix
Erlang/OTP 23 [erts-11.0] [source] [64-bit] [smp:12:12] [ds:12:12:10] [async-threads:1] [hipe]

Compiling 60 files (.ex)
Compiling 1 file (.erl)
when=2020-07-17T17:40:32.855 severity=info what=init text="Starting PoolsWarden" at=Elixir.Sparrow.PoolsWarden.init/1:95 pid=#PID<0.809.0> result=success worker=pools_warden file=lib/sparrow/pools_warden.ex application=sparrow
when=2020-07-17T17:40:32.955 severity=error what=tls_configuration text="APNS required ciphers missing" at=Elixir.MongoosePush.Application.check_apns_ciphers/0:234 pid=#PID<0.841.0> reason=no_apns_ciphers status=error file=lib/mongoose_push/application.ex application=mongoose_push
** (Mix) Could not start application mongoose_push: MongoosePush.Application.start(:normal, []) returned an error: :no_apns_ciphers

Copy link
Contributor

@michalwski michalwski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the cipher suites check at application startup. This clearly defines why the app couldn't start. I had just one cosmetic suggestion, just let me know if you want to change the code or not.
The PR is ready to be merged with or without addressing my comment.

Comment on lines 216 to 227
Enum.filter(all_ciphers, fn cipher ->
case cipher do
{:ecdhe_rsa, :aes_128_gcm, _, :sha256} ->
true

{:ecdhe_rsa, :aes_256_gcm, _, :sha384} ->
true

_ ->
false
end
end)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a cosmetic thing. It looks like to me, that Enum.any? could be used instead of Enum.fiter, thanks to that we'll have the answer immediately, without the need to calculate the length of the new list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected. But in exchange we've lost the underscore and have to match to :aead directly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the previous fun used with filter would work as well. Anyway, thanks for applying my suggestion.

Copy link
Contributor

@janciesla8818 janciesla8818 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks for adding this.

@janciesla8818 janciesla8818 merged commit 9f10ae7 into master Jul 20, 2020
@janciesla8818 janciesla8818 deleted the assert_tls_configuration_on_boot branch July 20, 2020 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants