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

Fix ssl alert crashes #50

Merged
merged 3 commits into from
Jun 22, 2022

Conversation

tonnenpinguin
Copy link
Contributor

@tonnenpinguin tonnenpinguin commented Jun 22, 2022

Hey all!

After upgrading to OTP 25 we noticed failing connections. The problem is that after the connection receives an SSL alert it completely stops trying to reconnect until the application is restarted.

16:11:11.635 [notice] TLS :client: In state :connection received SERVER ALERT: Fatal - Certificate required

 
16:11:11.652 [error] GenServer #PID<0.415.0> terminating
** (CaseClauseError) no case clause matching: {:error, %Mint.HTTP1{buffer: "", host: "0.0.0.0", mode: :active, port: 4001, private: %{extensions: [], scheme: :wss, sec_websocket_key: "KQvsbsvdPPlqt1e5LmN/5A=="}, proxy_headers: [], request: %{body: nil, connection: [], content_length: nil, data_buffer: [], headers_buffer: [], method: "GET", ref: #Reference<0.2518903565.3178758153.185166>, state: :status, status: nil, transfer_encoding: [], version: nil}, requests: {[], []}, scheme_as_string: "https", socket: {:sslsocket, {:gen_tcp, #Port<0.7>, :tls_connection, :undefined}, [#PID<0.426.0>, #PID<0.425.0>]}, state: :closed, streaming_request: nil, transport: Mint.Core.Transport.SSL}, %Mint.TransportError{reason: {:tls_alert, {:certificate_required, 'TLS client: In state connection received SERVER ALERT: Fatal - Certificate required\n'}}}, []}
    (slipstream 0.8.4) lib/slipstream/connection/pipeline.ex:140: Slipstream.Connection.Pipeline.decode_message/1
    (slipstream 0.8.4) lib/slipstream/connection/pipeline.ex:34: anonymous fn/1 in Slipstream.Connection.Pipeline.handle/2
    (slipstream 0.8.4) lib/slipstream/connection/telemetry.ex:29: anonymous fn/2 in Slipstream.Connection.Telemetry.span/2
    (telemetry 1.0.0) /Users/tonnenpinguin/src/poser/deps/telemetry/src/telemetry.erl:293: :telemetry.span/3
    (slipstream 0.8.4) lib/slipstream/connection/telemetry.ex:25: Slipstream.Connection.Telemetry.span/2
    (stdlib 4.0) gen_server.erl:1120: :gen_server.try_dispatch/4
    (stdlib 4.0) gen_server.erl:1197: :gen_server.handle_msg/6
    (stdlib 4.0) proc_lib.erl:240: :proc_lib.init_p_do_apply/3
Last message: {:ssl_error, {:sslsocket, {:gen_tcp, #Port<0.7>, :tls_connection, :undefined}, [#PID<0.426.0>, #PID<0.425.0>]}, {:tls_alert, {:certificate_required, 'TLS client: In state connection received SERVER ALERT: Fatal - Certificate required\n'}}}
State: %Slipstream.Connection.State{client_pid: #PID<0.414.0>, client_ref: #Reference<0.2518903565.3178758150.185680>, config: %Slipstream.Configuration{extensions: [], headers: [], heartbeat_interval_msec: 30000, json_parser: Jason, mint_opts: [protocols: [:http1], transport_opts: [cacerts: [CACERTS], keyfile: "nerves-hub/poser-key.pem", certfile: "nerves-hub/poser-cert.pem", verify: :verify_peer, server_name_indication: 'device.nerves-hub.org']], reconnect_after_msec: [1267, 2526, 4370, 11067, 23534, 40850, 64159], rejoin_after_msec: [5000], test_mode?: false, uri: %URI{authority: "0.0.0.0:4001", fragment: nil, host: "0.0.0.0", path: "/socket/websocket", port: 4001, query: nil, scheme: "wss", userinfo: nil}}, conn: %Mint.HTTP1{buffer: "", host: "0.0.0.0", mode: :active, port: 4001, private: %{extensions: [], scheme: :wss, sec_websocket_key: "KQvsbsvdPPlqt1e5LmN/5A=="}, proxy_headers: [], request: %{body: nil, connection: [], content_length: nil, data_buffer: [], headers_buffer: [], method: "GET", ref: #Reference<0.2518903565.3178758153.185166>, state: :status, status: nil, transfer_encoding: [], version: nil}, requests: {[], []}, scheme_as_string: "https", socket: {:sslsocket, {:gen_tcp, #Port<0.7>, :tls_connection, :undefined}, [#PID<0.426.0>, #PID<0.425.0>]}, state: :open, streaming_request: nil, transport: Mint.Core.Transport.SSL}, connection_id: "5e1f7e72403ec163", current_ref: 0, current_ref_str: "0", heartbeat_ref: nil, heartbeat_timer: nil, join_params: nil, joins: %{}, leaves: %{}, metadata: %{connection_id: "5e1f7e72403ec163", start_time: ~U[2022-06-22 14:11:11.417027Z], start_time_monotonic: -576460750894614603, state: %Slipstream.Connection.State{client_pid: #PID<0.414.0>, client_ref: #Reference<0.2518903565.3178758150.185680>, config: %Slipstream.Configuration{extensions: [], headers: [], heartbeat_interval_msec: 30000, json_parser: Jason, mint_opts: [protocols: [:http1], transport_opts: [cacerts: [CACERTS], keyfile: "nerves-hub/poser-key.pem", certfile: "nerves-hub/poser-cert.pem", verify: :verify_peer, server_name_indication: 'device.nerves-hub.org']], reconnect_after_msec: [1267, 2526, 4370, 11067, 23534, 40850, 64159], rejoin_after_msec: [5000], test_mode?: false, uri: %URI{authority: "0.0.0.0:4001", fragment: nil, host: "0.0.0.0", path: "/socket/websocket", port: 4001, query: nil, scheme: "wss", userinfo: nil}}, conn: nil, connection_id: "5e1f7e72403ec163", current_ref: 0, current_ref_str: "0", heartbeat_ref: nil, heartbeat_timer: nil, join_params: nil, joins: %{}, leaves: %{}, metadata: nil, request_ref: nil, status: :opened, trace_id: "3c882d640b72cecbbb740abb580d7419", websocket: nil}, trace_id: "3c882d640b72cecbbb740abb580d7419"}, request_ref: #Reference<0.2518903565.3178758153.185166>, status: :opened, trace_id: "3c882d640b72cecbbb740abb580d7419", websocket: nil}

This patch fixes this and causes slipstream to at least keep retrying.

I'm new to the project and didn't quite know where to put a test, so pointers would be highly appreciated :)

@the-mikedavis the-mikedavis self-requested a review June 22, 2022 14:24
Copy link
Collaborator

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

Hmm it might be difficult to come up with a good test case for this. I would be comfortable without a test in this case - based on the stack trace I think this looks good and it's just error handling anyways ¯\_(ツ)_/¯

lib/slipstream/connection/pipeline.ex Outdated Show resolved Hide resolved
@tonnenpinguin
Copy link
Contributor Author

tonnenpinguin commented Jun 22, 2022

Thanks for the quick review! :)

I committed your suggested change, fixed the formatting and manually double checked that everything is still working

Copy link
Collaborator

@the-mikedavis the-mikedavis 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 the fix, this is lovely! 🚀

@the-mikedavis the-mikedavis merged commit ad79bc5 into NFIBrokerage:main Jun 22, 2022
@tonnenpinguin tonnenpinguin deleted the fix_ssl_alert_crashes branch June 22, 2022 19:31
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.

None yet

2 participants