Verify peer cert fingerprint #22
Conversation
mickel8
force-pushed
the
verify-fingerprint
branch
from
e8fb3cc to
612dea9
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #22 +/- ##
==========================================
- Coverage 84.40% 83.85% -0.56%
==========================================
Files 10 10
Lines 449 483 +34
==========================================
+ Hits 379 405 +26
- Misses 70 78 +8
Continue to review full report in Codecov by Sentry.
|
mickel8
force-pushed
the
verify-fingerprint
branch
5 times, most recently
from
0ff209d to
4c25e71
Compare
mickel8
force-pushed
the
verify-fingerprint
branch
6 times, most recently
from
770cf3b to
788fb37
Compare
mickel8
force-pushed
the
verify-fingerprint
branch
from
788fb37 to
885f861
Compare
| state = setup_srtp(state, remote_keying_material, profile) | ||
| update_dtls_state(state, :connected) | ||
| else | ||
| Logger.debug("Non-matching peer cert fingerprint.") |
There was a problem hiding this comment.
I think this is probably a rare error, isn't it? Maybe Logger.debug is too lax for the severity? State change to :failed doesn't really tell much about the reason, so it's gonna be easy to miss.
There was a problem hiding this comment.
I don't think so. PeerConnection state failed means that either ICE failed or DTLS failed. Because user has access to ICE state it can easily know that the problem is failing DTLS.
I would say that all our logs should be on debug as we create a library. It's API role to give user sufficient information on what's going on
| @@ -408,7 +415,7 @@ defmodule ExWebRTC.PeerConnection do | |||
| dtls = | |||
| if type == :answer do | |||
| {:setup, setup} = ExSDP.Media.get_attribute(hd(sdp.media), :setup) | |||
| :ok = DTLSTransport.start_dtls(state.dtls_transport, setup) | |||
| :ok = DTLSTransport.start_dtls(state.dtls_transport, setup, state.peer_fingerprint) | |||
There was a problem hiding this comment.
Isn't it an issue that the fingerprint here might be nil as far as I understand?
There was a problem hiding this comment.
This should never happen as we can't set SDP answer before receiving SDP offer. When we receive SDP offer we save peer_fingerprint in the state
When peer cert fingerprint does not match the one provided in SDP we will move to the failed state and stop the PeerConnection process for now.
The question is if we should stick to the
permanentrestart strategy in a GenServer as it makes Supervisor always restart our GenServer, even when exited with:normal,:shutdownor{:shutdown, term}reasons.For now, I left this to be configured by a user using
Supervisor.child_spec.