Negotiate the same SRTP crypto suites for every DTLS association formed.

Negotiate the same SRTP crypto suites for every DTLS association formed.

Before this CL, we would negotiate:
- No crypto suites for data m= sections.
- A full set for audio m= sections.
- The full set, minus SRTP_AES128_CM_SHA1_32 for video m= sections.

However, this doesn't make sense with BUNDLE, since any DTLS
association could end up being used for any type of media. If
video is "bundled on" the audio transport (which is typical), it
will actually end up using SRTP_AES128_CM_SHA1_32.

So, this CL moves the responsibility of deciding SRTP crypto suites out
of BaseChannel and into DtlsTransport. The only two possibilities are
now "normal set" or "normal set + GCM", if enabled by the PC factory
options.

This fixes an issue (see linked bug) that was occurring when audio/video
were "bundled onto" the data transport. Since the data transport
wasn't negotiating any SRTP crypto suites, none were available to use
for audio/video, so the application would get black video/no audio.

This CL doesn't affect the SDES SRTP crypto suite negotiation;
it only affects the negotiation in the DLTS handshake, through
the use_srtp extension.

BUG=chromium:711243
Review-Url: https://codereview.webrtc.org/2815513012
Cr-Commit-Position: refs/heads/master@{#17810}
Committed: https://chromium.googlesource.com/external/webrtc/+/7914b8cb4127e1bca4d288f2f53f08dc13468b6a

[Taylor Brandstetter, 2017-04-13 18:39:16]

Description was changed from

==========
Negotiate the same SRTP crypto suites for every DTLS association formed.

Before this CL, we would negotiate:
- No crypto suites for data m= sections.
- A normal set for video m= sections.
- The normal set, plus SRTP_AES128_CM_SHA1_32 for audio m= sections.

However, this doesn't make sense with BUNDLE, since any DTLS
association could end up being used for any type of media.

So, this CL moves the responsibility of deciding SRTP crypto suites out
of BaseChannel and into DtlsTransport. The only two possibilities are
now "normal set" or "normal set + GCM", if enabled by the PC factory
options.

BUG=chromium:711243
==========

to

==========
Negotiate the same SRTP crypto suites for every DTLS association formed.

Before this CL, we would negotiate:
- No crypto suites for data m= sections.
- A normal set for video m= sections.
- The normal set, plus SRTP_AES128_CM_SHA1_32 for audio m= sections.

However, this doesn't make sense with BUNDLE, since any DTLS
association could end up being used for any type of media.

So, this CL moves the responsibility of deciding SRTP crypto suites out
of BaseChannel and into DtlsTransport. The only two possibilities are
now "normal set" or "normal set + GCM", if enabled by the PC factory
options.

One side effect of this CL is that SRTP_AES128_CM_SHA1_32 will no longer
be used with DTLS, even when only an audio m= section is present. A
subsequent offer could always add a video m= section, which we want to
use an 80-bit hash for.

BUG=chromium:711243
==========

[Taylor Brandstetter, 2017-04-13 19:40:33]

Description was changed from

==========
Negotiate the same SRTP crypto suites for every DTLS association formed.

Before this CL, we would negotiate:
- No crypto suites for data m= sections.
- A normal set for video m= sections.
- The normal set, plus SRTP_AES128_CM_SHA1_32 for audio m= sections.

However, this doesn't make sense with BUNDLE, since any DTLS
association could end up being used for any type of media.

So, this CL moves the responsibility of deciding SRTP crypto suites out
of BaseChannel and into DtlsTransport. The only two possibilities are
now "normal set" or "normal set + GCM", if enabled by the PC factory
options.

One side effect of this CL is that SRTP_AES128_CM_SHA1_32 will no longer
be used with DTLS, even when only an audio m= section is present. A
subsequent offer could always add a video m= section, which we want to
use an 80-bit hash for.

BUG=chromium:711243
==========

to

==========
Negotiate the same SRTP crypto suites for every DTLS association formed.

Before this CL, we would negotiate:
- No crypto suites for data m= sections.
- A normal set for video m= sections.
- The normal set, plus SRTP_AES128_CM_SHA1_32 for audio m= sections.

However, this doesn't make sense with BUNDLE, since any DTLS
association could end up being used for any type of media.

So, this CL moves the responsibility of deciding SRTP crypto suites out
of BaseChannel and into DtlsTransport. The only two possibilities are
now "normal set" or "normal set + GCM", if enabled by the PC factory
options.

This fixes an issue (see linked bug) when audio/video are "bundled onto"
the data transport, because they need to use SRTP, but the data transport
didn't negotiate any SRTP cipher.

One side effect of this CL is that SRTP_AES128_CM_SHA1_32 will no longer
be used with DTLS, even when only an audio m= section is present. A
subsequent offer could always add a video m= section, which we want to
use an 80-bit hash for.

BUG=chromium:711243
==========

[Taylor Brandstetter, 2017-04-17 21:42:34]

Description was changed from

==========
Negotiate the same SRTP crypto suites for every DTLS association formed.

Before this CL, we would negotiate:
- No crypto suites for data m= sections.
- A normal set for video m= sections.
- The normal set, plus SRTP_AES128_CM_SHA1_32 for audio m= sections.

However, this doesn't make sense with BUNDLE, since any DTLS
association could end up being used for any type of media.

So, this CL moves the responsibility of deciding SRTP crypto suites out
of BaseChannel and into DtlsTransport. The only two possibilities are
now "normal set" or "normal set + GCM", if enabled by the PC factory
options.

This fixes an issue (see linked bug) when audio/video are "bundled onto"
the data transport, because they need to use SRTP, but the data transport
didn't negotiate any SRTP cipher.

One side effect of this CL is that SRTP_AES128_CM_SHA1_32 will no longer
be used with DTLS, even when only an audio m= section is present. A
subsequent offer could always add a video m= section, which we want to
use an 80-bit hash for.

BUG=chromium:711243
==========

to

==========
Negotiate the same SRTP crypto suites for every DTLS association formed.

Before this CL, we would negotiate:
- No crypto suites for data m= sections.
- A full set for audio m= sections.
- The full set, minus SRTP_AES128_CM_SHA1_32 for video m= sections.

However, this doesn't make sense with BUNDLE, since any DTLS
association could end up being used for any type of media. If
video is "bundled on" the audio transport (which is typical), it
will actually end up using SRTP_AES128_CM_SHA1_32.

So, this CL moves the responsibility of deciding SRTP crypto suites out
of BaseChannel and into DtlsTransport. The only two possibilities are
now "normal set" or "normal set + GCM", if enabled by the PC factory
options.

This fixes an issue (see linked bug) that was occurring when audio/video
were "bundled onto" the data transport. Since the data transport
wasn't negotiating any SRTP crypto suites, none were available to use
for audio/video, so the application would get black video/no audio.

This CL doesn't affect the SDES SRTP crypto suite negotiation;
it only affects the negotiation in the DLTS handshake, through
the use_srtp extension.

BUG=chromium:711243
==========

[Taylor Brandstetter, 2017-04-17 21:43:01]

The CQ bit was checked by deadbeef@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 21:43:07]

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

[Taylor Brandstetter, 2017-04-17 22:00:33]

deadbeef@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[Taylor Brandstetter, 2017-04-17 22:00:34]

PTAL, this is what we were talking about earlier today.

https://codereview.webrtc.org/2815513012/diff/1/webrtc/base/sslstreamadapter.h
File webrtc/base/sslstreamadapter.h (right):

https://codereview.webrtc.org/2815513012/diff/1/webrtc/base/sslstreamadapter....
webrtc/base/sslstreamadapter.h:86: std::vector<int>* crypto_suites);
Just moved from mediasession.h, since this now is needed by code in p2p/.

https://codereview.webrtc.org/2815513012/diff/20001/webrtc/p2p/base/faketrans...
File webrtc/p2p/base/faketransportcontroller.h (right):

https://codereview.webrtc.org/2815513012/diff/20001/webrtc/p2p/base/faketrans...
webrtc/p2p/base/faketransportcontroller.h:39:
/*redetermine_role_on_ice_restart=*/true,
I know we don't use this style typically, but I think we should allow it (when
sensible) since it's more consistent with Google style and it's enforceable by
clang-tidy
(https://clang.llvm.org/extra/clang-tidy/checks/misc-argument-comment.html).
WDYT?

[pthatcher1, 2017-04-17 22:12:41]

https://codereview.webrtc.org/2815513012/diff/20001/webrtc/base/sslstreamadap...
File webrtc/base/sslstreamadapter.cc (right):

https://codereview.webrtc.org/2815513012/diff/20001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter.cc:109:
crypto_suites.push_back(rtc::SRTP_AES128_CM_SHA1_80);
I'm not so sure about this being the default.  I'd feel more comfortable if this
were another crypto_options, such as
crypto_options.enable_sha1_32bit_crypto_suites, or something like that.  

But I don't see a way web apps could get to this.  Maybe it will become a
mobile-app-only thing and web apps always get 80bit?

https://codereview.webrtc.org/2815513012/diff/20001/webrtc/p2p/base/faketrans...
File webrtc/p2p/base/faketransportcontroller.h (right):

https://codereview.webrtc.org/2815513012/diff/20001/webrtc/p2p/base/faketrans...
webrtc/p2p/base/faketransportcontroller.h:39:
/*redetermine_role_on_ice_restart=*/true,
On 2017/04/17 22:00:34, Taylor Brandstetter wrote:
> I know we don't use this style typically, but I think we should allow it (when
> sensible) since it's more consistent with Google style and it's enforceable by
> clang-tidy
> (https://clang.llvm.org/extra/clang-tidy/checks/misc-argument-comment.html).
> WDYT?

I don't like the look of it, but it seems like a good choice in this instance
since it's difficult to make a named variable to pass into  the constructor.

https://codereview.webrtc.org/2815513012/diff/20001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportcontroller.h (right):

https://codereview.webrtc.org/2815513012/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportcontroller.h:266: rtc::SSLProtocolVersion
ssl_max_version_ = rtc::SSL_PROTOCOL_DTLS_12;
Should we move ssl_max_version_ into CryptoOptions?

[commit-bot: I haz the power, 2017-04-17 22:18:55]

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

[commit-bot: I haz the power, 2017-04-17 22:18:56]

Dry run: This issue passed the CQ dry run.

[Taylor Brandstetter, 2017-04-20 07:20:40]

https://codereview.webrtc.org/2815513012/diff/20001/webrtc/base/sslstreamadap...
File webrtc/base/sslstreamadapter.cc (right):

https://codereview.webrtc.org/2815513012/diff/20001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter.cc:109:
crypto_suites.push_back(rtc::SRTP_AES128_CM_SHA1_80);
On 2017/04/17 22:12:41, pthatcher1 wrote:
> I'm not so sure about this being the default.  I'd feel more comfortable if
this
> were another crypto_options, such as
> crypto_options.enable_sha1_32bit_crypto_suites, or something like that.  
> 
> But I don't see a way web apps could get to this.  Maybe it will become a
> mobile-app-only thing and web apps always get 80bit?  

It's already the default though. If we want to change the default to 80, let's
do that in a separate CL, since that's a bigger change. The main point of this
CL is to fix the "data-only to audio upgrade" regression.

https://codereview.webrtc.org/2815513012/diff/20001/webrtc/p2p/base/transport...
File webrtc/p2p/base/transportcontroller.h (right):

https://codereview.webrtc.org/2815513012/diff/20001/webrtc/p2p/base/transport...
webrtc/p2p/base/transportcontroller.h:266: rtc::SSLProtocolVersion
ssl_max_version_ = rtc::SSL_PROTOCOL_DTLS_12;
On 2017/04/17 22:12:41, pthatcher1 wrote:
> Should we move ssl_max_version_ into CryptoOptions?

Seems like a good idea (though for a separate CL).

[pthatcher1, 2017-04-21 01:12:26]

lgtm

[Taylor Brandstetter, 2017-04-21 09:59:43]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2017-04-21 09:59:44]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2815513012/#ps40001 (title:
"Merge with master")

[commit-bot: I haz the power, 2017-04-21 09:59:47]

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-04-21 10:23:33]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492768783517390,
"parent_rev": "30952b460ff6e6e0fada8015c362064fac7ba8af", "commit_rev":
"7914b8cb4127e1bca4d288f2f53f08dc13468b6a"}

[commit-bot: I haz the power, 2017-04-21 10:23:37]

Description was changed from

==========
Negotiate the same SRTP crypto suites for every DTLS association formed.

Before this CL, we would negotiate:
- No crypto suites for data m= sections.
- A full set for audio m= sections.
- The full set, minus SRTP_AES128_CM_SHA1_32 for video m= sections.

However, this doesn't make sense with BUNDLE, since any DTLS
association could end up being used for any type of media. If
video is "bundled on" the audio transport (which is typical), it
will actually end up using SRTP_AES128_CM_SHA1_32.

So, this CL moves the responsibility of deciding SRTP crypto suites out
of BaseChannel and into DtlsTransport. The only two possibilities are
now "normal set" or "normal set + GCM", if enabled by the PC factory
options.

This fixes an issue (see linked bug) that was occurring when audio/video
were "bundled onto" the data transport. Since the data transport
wasn't negotiating any SRTP crypto suites, none were available to use
for audio/video, so the application would get black video/no audio.

This CL doesn't affect the SDES SRTP crypto suite negotiation;
it only affects the negotiation in the DLTS handshake, through
the use_srtp extension.

BUG=chromium:711243
==========

to

==========
Negotiate the same SRTP crypto suites for every DTLS association formed.

Before this CL, we would negotiate:
- No crypto suites for data m= sections.
- A full set for audio m= sections.
- The full set, minus SRTP_AES128_CM_SHA1_32 for video m= sections.

However, this doesn't make sense with BUNDLE, since any DTLS
association could end up being used for any type of media. If
video is "bundled on" the audio transport (which is typical), it
will actually end up using SRTP_AES128_CM_SHA1_32.

So, this CL moves the responsibility of deciding SRTP crypto suites out
of BaseChannel and into DtlsTransport. The only two possibilities are
now "normal set" or "normal set + GCM", if enabled by the PC factory
options.

This fixes an issue (see linked bug) that was occurring when audio/video
were "bundled onto" the data transport. Since the data transport
wasn't negotiating any SRTP crypto suites, none were available to use
for audio/video, so the application would get black video/no audio.

This CL doesn't affect the SDES SRTP crypto suite negotiation;
it only affects the negotiation in the DLTS handshake, through
the use_srtp extension.

BUG=chromium:711243

Review-Url: https://codereview.webrtc.org/2815513012
Cr-Commit-Position: refs/heads/master@{#17810}
Committed:
https://chromium.googlesource.com/external/webrtc/+/7914b8cb4127e1bca4d288f2f...
==========

[commit-bot: I haz the power, 2017-04-21 10:23:38]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/7914b8cb4127e1bca4d288f2f...