Support GCM ciphers even if ENABLE_EXTERNAL_AUTH is defined.

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}
Committed: https://chromium.googlesource.com/external/webrtc/+/d48f488bed37e8a5af47bd4be85d9b819fa284b4

[joachim, 2017-02-28 00:07:04]

Description was changed from

==========
Get rid of ENABLE_EXTERNAL_AUTH compile-time flag.

Instead determine support based on selected cipher (allowed for non-GCM,
now allowed for GCM).

BUG=webrtc:5222, chromium:628400
==========

to

==========
Get rid of ENABLE_EXTERNAL_AUTH compile-time flag.

Instead determine support based on selected cipher (allowed for non-GCM,
now allowed for GCM).

WORK IN PROGRESS - DON'T LAND!

BUG=webrtc:5222, chromium:628400
==========

[joachim, 2017-02-28 00:07:04]

jbauch@webrtc.org changed reviewers:
+ holmer@chromium.org

[joachim, 2017-02-28 00:11:45]

Hi Stefan, you are the current owner of crbug.com/628400, so I'm assigning this
to you - please forward if you think somebody else should get it.

I'm looking to get some feedback to see if I'm on the right track here. Also
please see my comment about how to handle/resolve the circular dependency.

https://codereview.webrtc.org/2720663003/diff/1/webrtc/p2p/base/fakeicetransp...
File webrtc/p2p/base/fakeicetransport.h (right):

https://codereview.webrtc.org/2720663003/diff/1/webrtc/p2p/base/fakeicetransp...
webrtc/p2p/base/fakeicetransport.h:18: #include "webrtc/media/base/rtputils.h"
This dependency is invalid ("media" depends on "p2p", so we can't depend the
other way around).

What is the best practice to implement this? Should I create a different CL to
move "rtputils*" to "/base" first?

https://codereview.webrtc.org/2720663003/diff/1/webrtc/p2p/base/fakepackettra...
File webrtc/p2p/base/fakepackettransport.h (right):

https://codereview.webrtc.org/2720663003/diff/1/webrtc/p2p/base/fakepackettra...
webrtc/p2p/base/fakepackettransport.h:19: #include
"webrtc/media/base/rtputils.h"
Same here.

[joachim, 2017-02-28 13:09:32]

https://codereview.webrtc.org/2720663003/diff/1/webrtc/p2p/base/fakeicetransp...
File webrtc/p2p/base/fakeicetransport.h (right):

https://codereview.webrtc.org/2720663003/diff/1/webrtc/p2p/base/fakeicetransp...
webrtc/p2p/base/fakeicetransport.h:18: #include "webrtc/media/base/rtputils.h"
On 2017/02/28 00:11:45, joachim wrote:
> This dependency is invalid ("media" depends on "p2p", so we can't depend the
> other way around).
> 
> What is the best practice to implement this? Should I create a different CL to
> move "rtputils*" to "/base" first?

That should have been "p2p/base", not "/base".

[stefan-webrtc, 2017-02-28 14:27:09]

stefan@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, stefan@webrtc.org

[stefan-webrtc, 2017-02-28 14:27:10]

Taylor, are you a good reviewer for this?

[joachim, 2017-02-28 14:29:45]

Description was changed from

==========
Get rid of ENABLE_EXTERNAL_AUTH compile-time flag.

Instead determine support based on selected cipher (allowed for non-GCM,
now allowed for GCM).

WORK IN PROGRESS - DON'T LAND!

BUG=webrtc:5222, chromium:628400
==========

to

==========
Get rid of ENABLE_EXTERNAL_AUTH compile-time flag.

Instead determine support based on selected cipher (allowed for non-GCM,
not allowed for GCM).

WORK IN PROGRESS - DON'T LAND!

BUG=webrtc:5222, chromium:628400
==========

[Taylor Brandstetter, 2017-02-28 22:53:40]

As I understand it, the intent of this CL is "Use external auth if the crypto
suite supports it." But shouldn't it be "use external auth if the crypto suite
supports it *and* the application opts in to it"? Only Chrome wants to use this;
native apps don't.

If so, we could add an "enable_external_auth" field to RTCConfiguration, which
chromium would set. Or, just leave the compile-time flag around until external
auth is completely ready for removal.

https://codereview.webrtc.org/2720663003/diff/1/webrtc/p2p/base/fakeicetransp...
File webrtc/p2p/base/fakeicetransport.h (right):

https://codereview.webrtc.org/2720663003/diff/1/webrtc/p2p/base/fakeicetransp...
webrtc/p2p/base/fakeicetransport.h:18: #include "webrtc/media/base/rtputils.h"
On 2017/02/28 00:11:45, joachim wrote:
> This dependency is invalid ("media" depends on "p2p", so we can't depend the
> other way around).
> 
> What is the best practice to implement this? Should I create a different CL to
> move "rtputils*" to "/base" first?

I don't quite understand why this dependency is needed; could you explain? Why
does the fake transport need to handle external auth?

https://codereview.webrtc.org/2720663003/diff/1/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (left):

https://codereview.webrtc.org/2720663003/diff/1/webrtc/pc/channel.cc#oldcode1463
webrtc/pc/channel.cc:1463: #if defined(ENABLE_EXTERNAL_AUTH)
Replace #ifdef with "if (external_auth_enabled_)"?

https://codereview.webrtc.org/2720663003/diff/1/webrtc/pc/srtpfilter.cc
File webrtc/pc/srtpfilter.cc (right):

https://codereview.webrtc.org/2720663003/diff/1/webrtc/pc/srtpfilter.cc#newco...
webrtc/pc/srtpfilter.cc:210: return false;
Could DCHECK here.

https://codereview.webrtc.org/2720663003/diff/1/webrtc/pc/srtpfilter.cc#newco...
webrtc/pc/srtpfilter.cc:612: bool SrtpSession::AllowExternalAuth() {
"AllowExternalAuth" implies some action, but this just returns a flag. Maybe
rename to "ExternalAuthAllowed"?

https://codereview.webrtc.org/2720663003/diff/1/webrtc/pc/srtpfilter.cc#newco...
webrtc/pc/srtpfilter.cc:625: if (!AllowExternalAuth()) {
DCHECK?

https://codereview.webrtc.org/2720663003/diff/1/webrtc/pc/srtpfilter_unittest.cc
File webrtc/pc/srtpfilter_unittest.cc (right):

https://codereview.webrtc.org/2720663003/diff/1/webrtc/pc/srtpfilter_unittest...
webrtc/pc/srtpfilter_unittest.cc:93: f1_.DisableAllowExternalAuthForTests(true);
Can you explain why this is needed in a comment?

https://codereview.webrtc.org/2720663003/diff/1/webrtc/pc/srtpfilter_unittest...
webrtc/pc/srtpfilter_unittest.cc:586: // Test directly setting the params with
SRTP_AEAD_AES_128_GCM
nit: Period at end of comment.

https://codereview.webrtc.org/2720663003/diff/1/webrtc/pc/srtpfilter_unittest...
webrtc/pc/srtpfilter_unittest.cc:642:
f1_.DisableAllowExternalAuthForTests(false);
Let's see, this is "don't disable allowing external auth"? Would be nice to
avoid a double negative.

[joachim, 2017-03-01 00:43:46]

Thanks for your review. You're right, I changed too much code that is not really
necessary for the intended scope.

I uploaded a modified version that should be much cleaner.

Ptal.

https://codereview.webrtc.org/2720663003/diff/1/webrtc/pc/srtpfilter.cc
File webrtc/pc/srtpfilter.cc (right):

https://codereview.webrtc.org/2720663003/diff/1/webrtc/pc/srtpfilter.cc#newco...
webrtc/pc/srtpfilter.cc:210: return false;
On 2017/02/28 22:53:40, Taylor Brandstetter wrote:
> Could DCHECK here.

I didn't DCHECK to stay consistent with the existing methods that only log a
warning.

https://codereview.webrtc.org/2720663003/diff/1/webrtc/pc/srtpfilter.cc#newco...
webrtc/pc/srtpfilter.cc:612: bool SrtpSession::AllowExternalAuth() {
On 2017/02/28 22:53:40, Taylor Brandstetter wrote:
> "AllowExternalAuth" implies some action, but this just returns a flag. Maybe
> rename to "ExternalAuthAllowed"?

Renamed to "IsExternalAuthActive".

https://codereview.webrtc.org/2720663003/diff/1/webrtc/pc/srtpfilter.cc#newco...
webrtc/pc/srtpfilter.cc:625: if (!AllowExternalAuth()) {
On 2017/02/28 22:53:40, Taylor Brandstetter wrote:
> DCHECK?

Done.

https://codereview.webrtc.org/2720663003/diff/1/webrtc/pc/srtpfilter_unittest.cc
File webrtc/pc/srtpfilter_unittest.cc (right):

https://codereview.webrtc.org/2720663003/diff/1/webrtc/pc/srtpfilter_unittest...
webrtc/pc/srtpfilter_unittest.cc:586: // Test directly setting the params with
SRTP_AEAD_AES_128_GCM
On 2017/02/28 22:53:40, Taylor Brandstetter wrote:
> nit: Period at end of comment.

Done.

[joachim, 2017-03-01 00:47:00]

Description was changed from

==========
Get rid of ENABLE_EXTERNAL_AUTH compile-time flag.

Instead determine support based on selected cipher (allowed for non-GCM,
not allowed for GCM).

WORK IN PROGRESS - DON'T LAND!

BUG=webrtc:5222, chromium:628400
==========

to

==========
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
==========

[Taylor Brandstetter, 2017-03-01 01:45:32]

lgtm with minor comments

https://codereview.webrtc.org/2720663003/diff/1/webrtc/pc/srtpfilter.cc
File webrtc/pc/srtpfilter.cc (right):

https://codereview.webrtc.org/2720663003/diff/1/webrtc/pc/srtpfilter.cc#newco...
webrtc/pc/srtpfilter.cc:210: return false;
On 2017/03/01 00:43:46, joachim wrote:
> On 2017/02/28 22:53:40, Taylor Brandstetter wrote:
> > Could DCHECK here.
> 
> I didn't DCHECK to stay consistent with the existing methods that only log a
> warning.

Acknowledged.

https://codereview.webrtc.org/2720663003/diff/20001/webrtc/pc/srtpfilter_unit...
File webrtc/pc/srtpfilter_unittest.cc (right):

https://codereview.webrtc.org/2720663003/diff/20001/webrtc/pc/srtpfilter_unit...
webrtc/pc/srtpfilter_unittest.cc:586: // GCM should not allow external auth
anyway.
Can remove this comment now.

https://codereview.webrtc.org/2720663003/diff/20001/webrtc/pc/srtpfilter_unit...
webrtc/pc/srtpfilter_unittest.cc:598: kTestKeyGcm128_1, kTestKeyGcm128Len));
Would it make sense to do "EXPECT_FALSE(f1_.IsExternalAuthActive())" here?

[joachim, 2017-03-01 17:28:28]

Thanks, uploaded changes based on your feedback. Also changed the BUILD.gn to
use the same variable for building "externalhmac.*" that is used to set the
compiler define.

Ptal

https://codereview.webrtc.org/2720663003/diff/20001/webrtc/pc/srtpfilter_unit...
File webrtc/pc/srtpfilter_unittest.cc (right):

https://codereview.webrtc.org/2720663003/diff/20001/webrtc/pc/srtpfilter_unit...
webrtc/pc/srtpfilter_unittest.cc:586: // GCM should not allow external auth
anyway.
On 2017/03/01 01:45:32, Taylor Brandstetter wrote:
> Can remove this comment now.

Done.

https://codereview.webrtc.org/2720663003/diff/20001/webrtc/pc/srtpfilter_unit...
webrtc/pc/srtpfilter_unittest.cc:598: kTestKeyGcm128_1, kTestKeyGcm128Len));
On 2017/03/01 01:45:32, Taylor Brandstetter wrote:
> Would it make sense to do "EXPECT_FALSE(f1_.IsExternalAuthActive())" here?

Done here and in the other similar tests.

[Taylor Brandstetter, 2017-03-01 22:28:18]

lgtm

[joachim, 2017-03-01 22:43:28]

The CQ bit was checked by jbauch@webrtc.org

[commit-bot: I haz the power, 2017-03-01 22:43:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-01 22:52:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 22:52:44]

Try jobs failed on following builders:
  win_x64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/23294)

[joachim, 2017-03-01 23:11:49]

The CQ bit was checked by jbauch@webrtc.org

[joachim, 2017-03-01 23:11:50]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2720663003/#ps60001 (title:
"Fixed win_x64 compile errors (added explicit casts).")

[commit-bot: I haz the power, 2017-03-01 23:11:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-01 23:34:36]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488409909986440,
"parent_rev": "0c003776a9b89c2c35282ae37b8c5bd2b88ef0ca", "commit_rev":
"d48f488bed37e8a5af47bd4be85d9b819fa284b4"}

[commit-bot: I haz the power, 2017-03-01 23:34:39]

Description was changed from

==========
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
==========

to

==========
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}
Committed:
https://chromium.googlesource.com/external/webrtc/+/d48f488bed37e8a5af47bd4be...
==========

[commit-bot: I haz the power, 2017-03-01 23:34:40]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/d48f488bed37e8a5af47bd4be...