ENABLE_EXTERNAL_AUTH is *not* set for a WebRTC isolated build, so WebRTC appears to test *WILDLY DIFFERENT* security code than ends up compiled and run in Chromium.

So I see at least two problems:
1. WebRTC and Chromium see wildly different code. It's not obvious which code is tested in which scenarios.
2. As written, this code breaks when we want to use anything other than SHA1-HMAC. Thanks to (1), it's not obvious whether tests of code adding (e.g.) AES-GCM are actually compiling in the behavior that would break them.

Hopefully, we can eliminate any code that expects to be able to modify the packet after encryption and authentication have occurred, since such code (as demonstrated here) seems incredibly fragile.

Sorry for the thrashing; I'm late to the party and learning about this as I go along.

Chromium relies on WebRTC's ENABLE_EXTERNAL_AUTH codepaths so it can stamp a timestamp on outgoing packets as close as possible to when they hit the wire. The goal is to facilitate receiver-estimated maximum bitrate (REMB).

References:
https://webrtc.org/experiments/rtp-hdrext/abs-send-time/
https://tools.ietf.org/html/draft-alvestrand-rmcat-remb-03#section-3
https://groups.google.com/d/msg/discuss-webrtc/9AlQQY5lNgU/-FJll3OAbc8J

Standalone WebRTC does not build with ENABLE_EXTERNAL_AUTH.

However, juberti@ pointed me to this bug to *obsolete* receive-side bandwidth estimation:
https://bugs.chromium.org/p/webrtc/issues/detail?id=4173

As I understand it, once we move bandwidth estimation to the sender, we can remove these codepaths.

As I mentioned in the other bug, it gets a bit more complicated due to interop with Firefox etc, which also use REMB and don't support the send-side version. We'll have to figure out a plan for getting rid of receive-side BWE once the send-side has launched, but it may not be possible to do it quickly.

Once we have send-side enabled, is there any way of quantifying how bad it would be to remove this packet-restamping code? Receive-side bandwidth estimation will still *work*, obviously, but we will (re-)introduce jitter due to local CPU load. What effect is that likely to have on calls?

As long as we require restamping for REMB, we can't negotiate AES-GCM and REMB at the same time. So I think our options are:

1) Get rid of restamping entirely.

2) Disable restamping if using AES-GCM. (Adds special cases, but probably least disruptive. What clients support AES-GCM but don't support send-side bandwidth estimation?)

3) Forbid negotiating AES-GCM if using REMB. (Would it even be possible to express this in an SDP offer?)

For completeness, I looked and there is no good/supported way in OpenSSL (even less so in BoringSSL) to recompute the GHASH of an AES-GCM packet. The best one could do is probably CRYPTO_gcm128_init with a no-op block transform.

3) would be my preference. We can't express it in SDP, but we could fail if we get a request to do AES-GCM with recv-side BWE.

Probably not a good idea since it would violate any standards, but another option would be to insert a random value V in the encrypted packet in place of the timestamp, and then use that random value later in Chromium to encrypt the packet timestamp at send time.

Another potential option, if I understand RTCP correctly -- can the timestamp go in the header and thus unencrypted?

The timestamp is in the clear, but SRTP authenticates the entire packet including the header. https://tools.ietf.org/html/rfc3711#section-3.1

But AES-GCM uses null auth, right? So the contents of the packet are integrity protected because they're GCM-encrypted, but the header isn't covered by that.

The header, etc. are included as AAD. https://tools.ietf.org/html/rfc7714

File another bug to do 3) in comment 6 and assign to mattdr@

Shouldn't that be done as part of https://codereview.webrtc.org/1528843005/ which adds support for GCM ciphers? I'm assuming you don't want to have broken GCM support added first and then fix it in a later CL.

I think we can separate the work into a few steps:

1) Implement AES-GCM in WebRTC and make it available to callers that opt in. Opt-in will be an error if ENABLE_EXTERNAL_AUTH is defined.

2) Refactor EXTERNAL_AUTH code in Chromium to remove conditional compilation and provide real APIs.

3) Add code in Chromium to enforce the REMB-or-GCM constraint, then allow Chromium to opt in to GCM.

The key here, to me, is that standalone WebRTC can offer AES-GCM to clients without these, er, unique requirements; then, when Chromium has adapted, it can benefit as well. This seems like the best way to avoid a priority inversion between WebRTC and Chromium.

Meanwhile, https://codereview.webrtc.org/1528843005/ only needs to be changed to implement (1). If ENABLE_EXTERNAL_AUTH is set, AES-GCM is not an option.

Matt, do you want to own this bug?  I can't think of anyone better.

By the way, should we right down our idea for how to refactor the Chromium code, as we discussed at lunch?  I believe it was something along the lines of "send the real send timestamp of the current packet in the *next* packet so we don't have to munged and re-auth this packet".

Yes, I'll own this.

Your summary is spot on. We want to consider solutions that allow us to put information in the *next* packet instead of modifying the current packet. I'll need to learn more about how bandwidth estimation works to understand if such a solution is feasible.

How would that work though? If we're going to invent a new mechanism, it doesn't solve the problem with the old mechanism (REMB).

I expect the answer is "it can't", given my very superficial understanding of how REMB operates. Since Peter brought it up, though, I was hoping I might have missed something. :)

FYI, https://codereview.webrtc.org/1528843005/ with (1) implemented from #c14 has landed as https://crrev.com/cb56065c62a31d83919abcd4e343ea3dbe029e9f

In response to #18: It might have to be an "REMB 2" or "REMB 1.1".  

What's the long-term plan for REMB?  Is it worth updating to work with GCM, or is the plan to just have it be gone in some time frame?

REMB will likely be around in one form or another. We will however likely get rid of abs-send-time, which is the header extensions causing problems here. toffset may still have to be kept though, but I don't think we update that in the socket?

Getting rid of the abs-send-time header extension would be *wonderful*. As far as I can tell it's the only thing we go back and twiddle after calling into libsrtp.

So REMB would either 
a) operate in concert with send-side BWE (for new clients)
b) be used as part of recv-side BWE, but with toffset (for old clients)

a) requires no updates, and b) could skip rewriting (we would encourage customers to use a) instead). Is there any third case (e.g. Firefox?)

We plan to use some variant of REMB in combination with send-side BWE to make it possible for a middlebox to notify the sender on the first hop about the bandwidth it has available on the second hop.

Agree, b) can skip rewriting to encourage that send-side BWE should be used, but we'll keep it there as long as we have significant usage. Once a new feedback format has been standardized it should be easier to also drop support for toffset.

ENABLE_EXTERNAL_AUTH is causing me significant problems in trying to update libsrtp in Chromium/WebRTC. I have to hack the build in order to test if these externalhmac.h/.cc files will *build* when they get imported to Chromium, because they live in WebRTC but don't compile as part of WebRTC standalone.

I see multiple options here:

- Make GCM and REMB mutually exclusive. This can be done by:
  (assuming that they're both features offered in the SDP blob):
  - If we offer GCM, don't offer REMB
  - If we get an offer that offers both GCM and REMB, don't accept both
- Fix REMB so that we don't need the later-patching when GCM is in use.
  This can be done by setting the timestamp before encryption, and not
  twiddling it later if GCM is in use. This would reduce the accuracy of
  the REMB prediction to what it was before we added EXTERNAL_AUTH

The latter path will impact bandwidth estimation on interworking with Edge and Firefox, but only if they implement GCM. What are the signals there?

Can we go further and eliminate restamping *entirely*, even when we're using AES+SHA1?

It seems like we'd be considering the same signals there as we would for disabling this only for GCM. How do we measure how much "better" this makes REMB?

We'd have to create an field trial and look at UMA for video send bitrate etc.

The problem that this solved was that packets were timestamped (abs-send-time header extension) before they passed the IPC from the render process to the browser process, which on slower machines caused significant jitter. Because of this the jitter was interpreted as network congestion by the bandwidth estimation which caused less reliable and lower video send bitrate and quality.

At this point the abs-send-time header extension is not the default in Chrome (REMB is not really the issue here, the timestamping is). However, many sessions in Chrome still have it as their defaults, so I think we should be careful with making it worse.

@juberti We are seeing all new business coming to use mandating use of AES 256 for SRTP, so this is getting up to the critical stage. Lack of AES 256 is being used as a WebRTC objection by traditional VC vendors in high compliance requirements. 

I can't see a mutually exclusive trade off between REMB and GCM working, as REMB is the only available bandwidth control we have at our disposal.

A marginally compromised REMB may be the only acceptable path. 

I think the right path is to live with the inaccuracy in REMB that occurs when using GCM/not restamping. I'd like to kill off REMB eventually, so this could be a carrot, and it won't affect existing (e.g. non-GCM) users.

Agree. abs-send-time is getting less and less usage at the expense of transport-sequence-number, so I'd be comfortable making this change at this point.

Choosing a name from the hat, as I'm leaving Google. Please reassign as appropriate.

Work in progress: https://codereview.webrtc.org/2720663003/

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/d48f488bed37e8a5af47bd4be85d9b819fa284b4

commit d48f488bed37e8a5af47bd4be85d9b819fa284b4
Author: jbauch <jbauch@webrtc.org>
Date: Wed Mar 01 23:34:36 2017

Support GCM ciphers even if ENABLE_EXTERNAL_AUTH is defined.

With ENABLE_EXTERNAL_AUTH, external auth will only be used depending
on the selected cipher (allowed for non-GCM, not allowed for GCM).

BUG= webrtc:5222 , chromium:628400

Review-Url: https://codereview.webrtc.org/2720663003
Cr-Commit-Position: refs/heads/master@{#16955}

[modify] https://crrev.com/d48f488bed37e8a5af47bd4be85d9b819fa284b4/webrtc/pc/BUILD.gn
[modify] https://crrev.com/d48f488bed37e8a5af47bd4be85d9b819fa284b4/webrtc/pc/channel.cc
[modify] https://crrev.com/d48f488bed37e8a5af47bd4be85d9b819fa284b4/webrtc/pc/channelmanager.cc
[modify] https://crrev.com/d48f488bed37e8a5af47bd4be85d9b819fa284b4/webrtc/pc/srtpfilter.cc
[modify] https://crrev.com/d48f488bed37e8a5af47bd4be85d9b819fa284b4/webrtc/pc/srtpfilter.h
[modify] https://crrev.com/d48f488bed37e8a5af47bd4be85d9b819fa284b4/webrtc/pc/srtpfilter_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/ac170d5c214e1f487ba82b888e5c3d5000118de0

commit ac170d5c214e1f487ba82b888e5c3d5000118de0
Author: jbauch <jbauch@webrtc.org>
Date: Sat Mar 04 08:54:45 2017

Improve testing of SRTP external auth code paths.

Previously code behind ENABLE_EXTERNAL_AUTH was only compiled with Chromium
but developed in WebRTC, which made testing rather complicated. This caused
some trouble in the past (e.g. https://crbug.com/628400#c1)

This CL helps in that the external auth code is now compiled with WebRTC
and the srtpfilter integration gets tested.

BUG=chromium:628400

Review-Url: https://codereview.webrtc.org/2722423003
Cr-Commit-Position: refs/heads/master@{#17030}

[modify] https://crrev.com/ac170d5c214e1f487ba82b888e5c3d5000118de0/webrtc/pc/BUILD.gn
[modify] https://crrev.com/ac170d5c214e1f487ba82b888e5c3d5000118de0/webrtc/pc/channel.cc
[modify] https://crrev.com/ac170d5c214e1f487ba82b888e5c3d5000118de0/webrtc/pc/externalhmac.cc
[modify] https://crrev.com/ac170d5c214e1f487ba82b888e5c3d5000118de0/webrtc/pc/externalhmac.h
[modify] https://crrev.com/ac170d5c214e1f487ba82b888e5c3d5000118de0/webrtc/pc/srtpfilter.cc
[modify] https://crrev.com/ac170d5c214e1f487ba82b888e5c3d5000118de0/webrtc/pc/srtpfilter.h
[modify] https://crrev.com/ac170d5c214e1f487ba82b888e5c3d5000118de0/webrtc/pc/srtpfilter_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/d81f121cfa7d7ad5d66f461f97d647e1540bd0ac

commit d81f121cfa7d7ad5d66f461f97d647e1540bd0ac
Author: jbauch <jbauch@webrtc.org>
Date: Sat Mar 04 09:37:28 2017

Revert of Improve testing of SRTP external auth code paths. (patchset #2 id:20001 of https://codereview.webrtc.org/2722423003/ )

Reason for revert:
Breaks compilation in FYI bots, e.g. here:
http://build.chromium.org/p/chromium.webrtc.fyi/builders/Win%20Builder/builds/9314

FAILED: obj/third_party/webrtc/pc/rtc_pc/channel.obj
ninja -t msvc -e environment.x86 -- E:\b\c\goma_client/gomacc.exe "E:\b\depot_tools\win_toolchain\vs_files\d3cb0e37bdd120ad0ac4650b674b09e81be45616\VC\bin\amd64_x86/cl.exe" /nologo /showIncludes /FC @obj/third_party/webrtc/pc/rtc_pc/channel.obj.rsp /c ../../third_party/webrtc/pc/channel.cc /Foobj/third_party/webrtc/pc/rtc_pc/channel.obj /Fd"obj/third_party/webrtc/pc/rtc_pc_cc.pdb"
e:\b\c\b\win_builder\src\third_party\webrtc\pc\channel.cc(176): error C2819: type 'cricket::SrtpFilter' does not have an overloaded member 'operator ->'
e:\b\c\b\win_builder\src\third_party\webrtc\pc\srtpfilter.h(45): note: see declaration of 'cricket::SrtpFilter'
e:\b\c\b\win_builder\src\third_party\webrtc\pc\channel.cc(176): note: did you intend to use '.' instead?
e:\b\c\b\win_builder\src\third_party\webrtc\pc\channel.cc(176): error C2232: '->cricket::SrtpFilter::EnableExternalAuth': left operand has 'class' type, use '.'

Original issue's description:
> Improve testing of SRTP external auth code paths.
>
> Previously code behind ENABLE_EXTERNAL_AUTH was only compiled with Chromium
> but developed in WebRTC, which made testing rather complicated. This caused
> some trouble in the past (e.g. https://crbug.com/628400#c1)
>
> This CL helps in that the external auth code is now compiled with WebRTC
> and the srtpfilter integration gets tested.
>
> BUG=chromium:628400
>
> Review-Url: https://codereview.webrtc.org/2722423003
> Cr-Commit-Position: refs/heads/master@{#17030}
> Committed: https://chromium.googlesource.com/external/webrtc/+/ac170d5c214e1f487ba82b888e5c3d5000118de0

TBR=deadbeef@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=chromium:628400

Review-Url: https://codereview.webrtc.org/2734643002
Cr-Commit-Position: refs/heads/master@{#17031}

[modify] https://crrev.com/d81f121cfa7d7ad5d66f461f97d647e1540bd0ac/webrtc/pc/BUILD.gn
[modify] https://crrev.com/d81f121cfa7d7ad5d66f461f97d647e1540bd0ac/webrtc/pc/channel.cc
[modify] https://crrev.com/d81f121cfa7d7ad5d66f461f97d647e1540bd0ac/webrtc/pc/externalhmac.cc
[modify] https://crrev.com/d81f121cfa7d7ad5d66f461f97d647e1540bd0ac/webrtc/pc/externalhmac.h
[modify] https://crrev.com/d81f121cfa7d7ad5d66f461f97d647e1540bd0ac/webrtc/pc/srtpfilter.cc
[modify] https://crrev.com/d81f121cfa7d7ad5d66f461f97d647e1540bd0ac/webrtc/pc/srtpfilter.h
[modify] https://crrev.com/d81f121cfa7d7ad5d66f461f97d647e1540bd0ac/webrtc/pc/srtpfilter_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/dfcab728b21cec2994131a449c6c1e38018c264a

commit dfcab728b21cec2994131a449c6c1e38018c264a
Author: jbauch <jbauch@webrtc.org>
Date: Mon Mar 06 08:14:10 2017

Reland: Improve testing of SRTP external auth code paths.

This CL is a reland of https://codereview.webrtc.org/2722423003 which got
reverted due to compile errors when rolling into Chromium.

Original CL description:

Improve testing of SRTP external auth code paths.

Previously code behind ENABLE_EXTERNAL_AUTH was only compiled with Chromium
but developed in WebRTC, which made testing rather complicated. This caused
some trouble in the past (e.g. https://crbug.com/628400#c1)

This CL helps in that the external auth code is now compiled with WebRTC
and the srtpfilter integration gets tested.

BUG=chromium:628400

Review-Url: https://codereview.webrtc.org/2735613002
Cr-Commit-Position: refs/heads/master@{#17052}

[modify] https://crrev.com/dfcab728b21cec2994131a449c6c1e38018c264a/webrtc/pc/BUILD.gn
[modify] https://crrev.com/dfcab728b21cec2994131a449c6c1e38018c264a/webrtc/pc/channel.cc
[modify] https://crrev.com/dfcab728b21cec2994131a449c6c1e38018c264a/webrtc/pc/externalhmac.cc
[modify] https://crrev.com/dfcab728b21cec2994131a449c6c1e38018c264a/webrtc/pc/externalhmac.h
[modify] https://crrev.com/dfcab728b21cec2994131a449c6c1e38018c264a/webrtc/pc/srtpfilter.cc
[modify] https://crrev.com/dfcab728b21cec2994131a449c6c1e38018c264a/webrtc/pc/srtpfilter.h
[modify] https://crrev.com/dfcab728b21cec2994131a449c6c1e38018c264a/webrtc/pc/srtpfilter_unittest.cc

Removing owner and setting status to Available, since the issue isn't currently being worked on.