libsrtp fork review

[j1elo]

Review process of the Kurento fork.

• Upstream: https://github.com/cisco/libsrtp
• Fork: https://github.com/Kurento/libsrtp

Project: Port to Ubuntu 18.04 (Bionic): https://github.com/orgs/Kurento/projects/1

Versions

• 16.04:
  • libsrtp0, libsrtp0-dev = 1.4.5~20130609~dfsg-1.1ubuntu1
• 18.04:
  • libsrtp0, libsrtp0-dev = 1.4.5~20130609~dfsg-2ubuntu1
  • libsrtp2-1, libsrtp2-dev = 2.1.0-1
• Kurento:
  • libsrtp1, libsrtp-dev = 1.5.2.1.xenial~20181017180329.12.cf41cce

Dependencies

• Depends: -
• Depended:
  • gst-plugins-bad

Status

Several undocumented commits:\ https://github.com/Kurento/libsrtp/commits/master

• Kurento/libsrtp@7475d9b Apply debian patches

  Review action: Discard.

• Kurento/libsrtp@7c2366f Convert package in native

  Bad decision! Best is to keep upstream code and Debian patches separated.\ Review action: Convert into 'quilt' package; discard commit.

• Kurento/libsrtp@2f81496 Enable openssl

  Seems to undo 7475d9b.\ Review action: Keep.

• Kurento/libsrtp@b4613f1 Add build configuration

  Review action: Discard.

• Kurento/libsrtp@1379c27 Require ssl 1.0.2

  Review action: STUDY. Why is SSl >= 1.0.2 required? Update: Xenial comes with SSL >= 1.0.2 so this point doesn't need more consideration. Discard commit.

• Kurento/libsrtp@d66c5cd Try to fix compilation problem adding build dependencies

  Adds essential Debian packages, nothing interesting.\ Review action: Discard.

• Kurento/libsrtp@3821a88 Fix compilation on docker

  A priori no changes specific to Docker should be forced into the code.\ Review action: Discard.

• Kurento/libsrtp@b27df0c srtp: do common code of checking repeta_tx in a function

  Superfluous change, de-duplicates a bunch of lines.\ Better stick with the upstream code.\ Review action: Discard.

• Kurento/libsrtp@c697285 srtp: if allow_repeat_tx do not check neither use the window\ 55bec21 srtp: if allow_repeat_tx use the window

  WARNING These 2 commits introduce a bug

  Before, allow_repeat_tx = TRUE would disable ONLY the error err_status_replay_fail.\ After, allow_repeat_tx = TRUE disables ALL errors. That's WRONG, and very insecure.\ Review action: Discard and burn.

  Update: Also, the patch didn't actually allow repeated receptions (replay rx) because it was using "allow-repeat-tx" which is only set to TRUE by the srtpenc GStreamer element upon sending packets, but it is never set by the srtpdec element upon reception of packets. So the patch was removing error checking without providing any actual benefit.

• Kurento/libsrtp@837f48c Set development version not a release one

  Review action: Discard.

Next steps

End2End tests - Check if anything breaks for using the Ubuntu's system version instead of our own fork.

[kohenkatz]

You noted that you aren't sure "why is SSL >= 1.0.2 required?"

I don't think you need to worry about figuring that out that anymore, because the lowest supported Ubuntu version is now xenial which only ships with libssl-dev >= 1.0.2.

It might be of historical interest, but it isn't of any practical interest as far as I can tell.

[j1elo]

https://github.com/Kurento/libsrtp/commit/2d7c37e15a3175b3cdb1648889c9b94aed693a78 introduces a new patch that provides support for repeated receptions for the srtpdec element, which ultimately allows Kurento to handle packet retransmissions when there is network packet loss.