Add SRTP for remote publishers #3449
Conversation
spscream
force-pushed
the
feature/am/remote-forwarders-srtp
branch
2 times, most recently
from
88bf274
to
dc307e9
Compare
|
@spscream thanks! So you got it working with the same SRTP crypto context for all streams, correct? |
|
no, for each sender on source side rtp forwarder helper is created, so each helper have its own srtp context, and on receiving side we have srtp context per publisher stream |
spscream
force-pushed
the
feature/am/remote-forwarders-srtp
branch
2 times, most recently
from
071ca8b
to
78951f2
Compare
There was a problem hiding this comment.
I just did a first review of the code: it looks mostly fine to me, and in fact you'll see that most comments are editorial and nitpicking. I did add a note on SRTP error logging that could be problematic the way it's done now.
I also have a doubt on just setting a stream to disabled = TRUE in case SRTP creation failed, considering there may be other things that re-enable that same stream, e.g., by subsequent API calls that set disabled back to TRUE on a stream that has a broken SRTP context at that point. Not sure if that could cause the code to crash, but it's worth investigating what could happen in that case.
| guint32 timestamp = ntohl(rtp->timestamp); | ||
| guint16 seq = ntohs(rtp->seq_number); | ||
| JANUS_LOG(LOG_ERR, "[%s] Publisher stream (#%d) SRTP unprotect error: %s (len=%d-->%d, ts=%"SCNu32", seq=%"SCNu16")\n", | ||
| publisher->user_id_str, ps->mindex, janus_srtp_error_str(res), bytes, buflen, timestamp, seq); |
There was a problem hiding this comment.
This will flood your logs in case you start getting traffic that's inconsistent with your SRTP context, since it will print a log line for every incoming RTP packet, and there may be a lot of them. In the core we try to limit that by showing an error summary every couple of seconds or so to limit the damage. An alternative might be having some sort of toggle: the first time you get an SRTP error you print the error, and then you don't anymore until you get a proper error (SRTP decoding succeeded) that resets the toggle (but this could still cause log flooding if success/error alternate often).
There was a problem hiding this comment.
Do you have any example from core, where you summarize logs? I copied this part from streaming plugin.
There was a problem hiding this comment.
I found it in ice.c, I'll make the same - write to debug log on every packet and write summary periodically.
There was a problem hiding this comment.
If it's there too then nevermind, I can look into this later on and fix it on both. I just noticed that a few of the nits I mentioned were actually in that code too (e.g., the &policy->rtp no parenthesis thing).
There was a problem hiding this comment.
I found it in ice.c, I'll make the same - write to debug log on every packet and write summary periodically.
No need, don't worry: I'll take care of that in both plugins myself so that it's done in a consistent way (I'll have to check if the same happens in SIP and NoSIP plugins too).
There was a problem hiding this comment.
I think about disabled, maybe we should add new flag for stream? like srtp_init_failed or similar and set it to true and add required checks in logic.
There was a problem hiding this comment.
so, what will be suitable decision for this?
There was a problem hiding this comment.
I think that, to keep things simple, besides setting disabled to FALSE it may be enough to simply set the stream is_srtp to FALSE as well. This way, even if a stream disabled is forcibly set to TRUE later on, there will be no SRTP code involved that may find broken SRTP structures: of course audio/video would not work at all, because we'd be relaying encoded packets, but that's to be expected, since configuring that stream failed in the first place. I only want to ensure there's not a crash when that state changes.
There was a problem hiding this comment.
btw it was is_rtp=FALSE by default, I set it to be more explicit.
spscream
force-pushed
the
feature/am/remote-forwarders-srtp
branch
4 times, most recently
from
9e86ead
to
47d63bb
Compare
|
Just as a side note, please notice that this will be merged only after #3359 is merged too (also considering I asked @fancycode to do rebases more than once already 😅 ). You may want to have a look at/review that PR as well, since it's aimed at addressing some memory leaks that apparently remote publishers cause (and since you've started using them, you'll probably be interested in that). |
Thanks 😆 happy to do more rebases if necessary - the change in my PR is not very large, so rebases are not too difficult. But even better if it would get merged at some point 👍 |
|
so what is the plan? I could rebase myself, this is not the problem |
|
I'm out of office today, there's a few PRs (including this one) that I plan to review one last time and then merge early next week. |
|
@spscream I wanted to merge this too, but I think merging your metadata PR introduced a new conflict here: could you fix this? |
spscream
force-pushed
the
feature/am/remote-forwarders-srtp
branch
3 times, most recently
from
835776a
to
22d3dd0
Compare
spscream
force-pushed
the
feature/am/remote-forwarders-srtp
branch
from
22d3dd0
to
23c3a93
Compare
|
sure, updated |
hi, I got working SRTP encryption for remote publishers, tested and worked in our environment