GNOME Bugzilla – Bug 730539
sdp: add support for multiple crypto sessions
Last modified: 2016-11-21 12:59:18 UTC
Created attachment 276947 [details] [review] add multiple crypto sessions for all SSRC senders This patch adds support for multiple SSRC senders. It creates a new crypto session map for each sender SSRC and its rollover counter.
Any chance this and the related bug 730540 will make it for 1.4.0?
(In reply to comment #1) > Any chance this and the related bug 730540 will make it for 1.4.0? Unlikely, I would like to rework the SRTP caps properties and try to implement the ROC counter without a signal.
(In reply to comment #2) > (In reply to comment #1) > > Any chance this and the related bug 730540 will make it for 1.4.0? > > Unlikely, I would like to rework the SRTP caps properties and try to implement > the ROC counter without a signal. Does that mean the "roc" field in the caps will dynamically change? With a shared media, new clients can connect and the ROC might have changed. That's why I used a signal, so it's dynamic. But I'm sure I miss something if you say it's possible.
(In reply to comment #3) > (In reply to comment #2) > > (In reply to comment #1) > > > Any chance this and the related bug 730540 will make it for 1.4.0? > > > > Unlikely, I would like to rework the SRTP caps properties and try to implement > > the ROC counter without a signal. > > Does that mean the "roc" field in the caps will dynamically change? With a > shared media, new clients can connect and the ROC might have changed. That's > why I used a signal, so it's dynamic. But I'm sure I miss something if you say > it's possible. Actually, the "setter" part is done by passing roc in the caps.
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > (In reply to comment #1) > > > > Any chance this and the related bug 730540 will make it for 1.4.0? > > > > > > Unlikely, I would like to rework the SRTP caps properties and try to implement > > > the ROC counter without a signal. > > > > Does that mean the "roc" field in the caps will dynamically change? With a > > shared media, new clients can connect and the ROC might have changed. That's > > why I used a signal, so it's dynamic. But I'm sure I miss something if you say > > it's possible. > > Actually, the "setter" part is done by passing roc in the caps. We currently have something similar in the payloaders. In the caps we find the seqnum and timestamp of the first packet but any time you want to get the current values, you use the stats property to get a GstStructure of values. It used to be done with properties but it was hard to get an atomic snapshot of the seqnum and timestamp and possibly other fields. So maybe we can do something similar for the ROC and other crypto params? I'm thinking that you get the stats to get all the fields in a consistent state to make the MIKEY message.
(In reply to comment #5) > > We currently have something similar in the payloaders. In the caps we find the > seqnum and timestamp of the first packet but any time you want to get the > current values, you use the stats property to get a GstStructure of values. It > used to be done with properties but it was hard to get an atomic snapshot of > the seqnum and timestamp and possibly other fields. > > So maybe we can do something similar for the ROC and other crypto params? I'm > thinking that you get the stats to get all the fields in a consistent state to > make the MIKEY message. Got it. Yes, I think that would make sense and would be cleaner. But now, I remember. Another problem was that you need to pass an SSRC to obtain its ROC, that's why I didn't use a property which would be better than a signal. How would you solve this?
you would probably need to put an array of SSRC and ROC pairs in the stats?
(In reply to comment #7) > you would probably need to put an array of SSRC and ROC pairs in the stats? Right. Instead of asking for one ROC you just return all of them. That shouldn't be a lot of changes from my patches. We would only need to add the stats property in the srtp elements and make a few changes on this bug and bug 730540 patches. Is that right? When is 1.4.0 scheduled? I'm starting my days off soonish and I'll be out for a couple of weeks (more or less). And I'm doing completely unrelated GStreamer work these days (sort of). Were you willing to make the changes or you wanted me to?
Created attachment 280872 [details] [review] add support for rollover counters Following patch for bug 733265, this uses the new "stats" property from the srtpenc element.
Created attachment 289704 [details] [review] add support for rollover counters fixup Uses boxed types properly after bug 733265 comment 2.
Created attachment 326869 [details] [review] add support for rollover counters (1.8.0)
*** Bug 765071 has been marked as a duplicate of this bug. ***
Created attachment 329596 [details] [review] add support for rollover counters (1.8.0, revised)
Review of attachment 329596 [details] [review]: Generally looks good, just some comments ::: gst/rtsp-server/rtsp-sdp.c @@ +104,3 @@ + break; + } else { + GST_WARNING ("unable to obtain ROC for stream with SSRC %u", ssrc); That seems at the wrong place. You would warn now if the first ssrc is not what you want, but the second is @@ +131,3 @@ + + g_object_get (session, "sources", &sources, NULL); + for (i = 0; i < sources->n_values; i++) { Could this be NULL? @@ +140,3 @@ + source = (GObject *) g_value_get_object (val); + + g_object_get (source, "ssrc", &ssrc, "is-sender", &is_sender, NULL); Maybe for efficiency, is-sender should be inside the stats structs too? That way you would only have to iterate that one, right? @@ +149,3 @@ + + if (stats) { + roc = get_roc_from_stats (stats, ssrc); Should we fail if this returns -1? Probably?
(In reply to Sebastian Dröge (slomo) from comment #14) > Review of attachment 329596 [details] [review] [review]: > > Generally looks good, just some comments > > ::: gst/rtsp-server/rtsp-sdp.c > @@ +104,3 @@ > + break; > + } else { > + GST_WARNING ("unable to obtain ROC for stream with SSRC %u", ssrc); > > That seems at the wrong place. You would warn now if the first ssrc is not > what you want, but the second is > Oooops, yes, this goes outside. > @@ +131,3 @@ > + > + g_object_get (session, "sources", &sources, NULL); > + for (i = 0; i < sources->n_values; i++) { > > Could this be NULL? > Added check. > @@ +140,3 @@ > + source = (GObject *) g_value_get_object (val); > + > + g_object_get (source, "ssrc", &ssrc, "is-sender", &is_sender, NULL); > > Maybe for efficiency, is-sender should be inside the stats structs too? That > way you would only have to iterate that one, right? > Yes, but I don't think we have access to the source in the srtpenc element. > @@ +149,3 @@ > + > + if (stats) { > + roc = get_roc_from_stats (stats, ssrc); > > Should we fail if this returns -1? Probably? Added the warning from above instead.
Created attachment 329661 [details] [review] add support for rollover counters (1.8.0, revised 2)
Review of attachment 329661 [details] [review]: Looks mostly good now, thanks ::: gst/rtsp-server/rtsp-sdp.c @@ +156,3 @@ + } + + gst_mikey_message_add_cs_srtp (msg, 0, ssrc, roc); If roc == -1, shouldn't we error out completely instead of adding -1 to the SDP?
(In reply to Sebastian Dröge (slomo) from comment #17) > Review of attachment 329661 [details] [review] [review]: > > Looks mostly good now, thanks > > ::: gst/rtsp-server/rtsp-sdp.c > @@ +156,3 @@ > + } > + > + gst_mikey_message_add_cs_srtp (msg, 0, ssrc, roc); > > If roc == -1, shouldn't we error out completely instead of adding -1 to the > SDP? Do you mean propagating the error or just GST_ERROR? For error propagation I'll have to change a few more functions.
Well, what would be the effect if we just ignore the error and generate the SDP as if nothing is wrong? :)
(In reply to Sebastian Dröge (slomo) from comment #19) > Well, what would be the effect if we just ignore the error and generate the > SDP as if nothing is wrong? :) The effect would be that the client can't decode the stream. I'll propagate the error. It will end up here in rtsp-client.c:handle_describe_request no_sdp: { GST_ERROR ("client %p: can't create SDP", client); send_generic_response (client, GST_RTSP_STS_SERVICE_UNAVAILABLE, ctx); g_free (path); g_object_unref (media); return FALSE; }
Created attachment 329736 [details] [review] add support for rollover counters (1.8.0, revised 3) Let's try one more time. I have changed the code so any error from gst_rtsp_sdp_from_media will be returned. Before, in some cases, errors where ignored and so was the stream that caused it. I'm not sure if this was on purpose. With this patch errors are not ignored.
Review of attachment 329736 [details] [review]: Looks good to me
Comment on attachment 329736 [details] [review] add support for rollover counters (1.8.0, revised 3) commit 85c52e194bcb81928b96614be0ae47d59eccb1ce Author: Aleix Conchillo Flaqué <aleix@oblong.com> Date: Thu Apr 14 22:56:11 2016 -0700 sdp: add rollover counters for all sender SSRC We add different crypto sessions in MIKEY, one for each sender SSRC. Currently, all of them will have the same security policy, 0. The rollover counters are obtained from the srtpenc element using the "stats" property.