Add support for GCM cipher suites from RFC 7714.

Add support for GCM cipher suites from RFC 7714.

GCM cipher suites are optional (disabled by default) and can be enabled
through "PeerConnectionFactoryInterface::Options".

If compiled with Chromium (i.e. "ENABLE_EXTERNAL_AUTH" is defined), no
GCM ciphers can be used yet (see https://crbug.com/628400).

BUG=webrtc:5222, 628400
Committed: https://crrev.com/cb56065c62a31d83919abcd4e343ea3dbe029e9f
Cr-Commit-Position: refs/heads/master@{#13635}

[joachim, 2015-12-16 01:06:20]

jbauch@webrtc.org changed reviewers:
+ juberti@webrtc.org, pthatcher@webrtc.org

[joachim, 2015-12-16 01:06:21]

This is a first version. Some areas still missing (mostly tests for the
high-level api), but the basic functionality is working.

Ptal for some early feedback.

[joachim, 2015-12-17 00:10:17]

On 2015/12/16 01:06:21, joachim wrote:
> This is a first version. Some areas still missing (mostly tests for the
> high-level api), but the basic functionality is working.
> 
> Ptal for some early feedback.

Added PeerConnection tests to make sure GCM/non-GCM is negotiated correctly, any
additional areas that should be covered?

[pthatcher1, 2015-12-18 20:31:32]

I think this is a good start, but I'd definitely like to have Justin look at it
thoroughly, since this is touches a lot of crypto code.

https://codereview.webrtc.org/1528843005/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/1528843005/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection_unittest.cc:1552: kDefaultSrtpCryptoSuite));
Can you move this common logic into a separate method that's something like:

TestGcmNegotiation(bool local_gcm_enabled, bool remote_gcm_enabled, int
expected_cipher_suite);

TestGcmNegotiation(true, true, kDefaultSrtpCryptoSuiteGcm);
TestGcmNegotiation(true, false, kDefaultSrtpCryptoSuite);
TestGcmNegotiation(false, true, kDefaultSrtpCryptoSuite);
TestGcmNegotiation(false, false, kDefaultSrtpCryptoSuite);

https://codereview.webrtc.org/1528843005/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1528843005/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface.h:552: enable_gcm_ciphers(false) {
Can you call this enable_gcm_crypto_suites to be more consistent with our
naming?

https://codereview.webrtc.org/1528843005/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface.h:569: // both endpoints will be used,
GCM must be supported by both to be used.
Might be more clear as "Enable GCM crypto suites from RFC 7714 for SRTP.  GCM
will only be used if both sides enable it."

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/channe...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/channe...
talk/session/media/channel.cc:853: }
Having the "enable gcm ciphers" passed down from the TransportController to the
Transport to the TransportChannel and then up to here seems much more complex
than necessary.  Can we not instead have WebRtcSession pass the necessary
information directly to the BaseChannel an bypass all of the complexity?

Also, I think it would be better if we had the right option pass from
WebRtcSession down to BaseChannel such that GetSrtpCryptoSuites gets the
information and selectively adds GCM rather than always adding GCM and then
removing it after the fact.

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/channe...
talk/session/media/channel.cc:885: if
(!rtc::SrtpCryptoSuiteParams(selected_crypto_suite, &key_len, &salt_len)) {
Can you call this GetSrtpKeyAndSaltLengths?

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/medias...
File talk/session/media/mediasession.cc (right):

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/medias...
talk/session/media/mediasession.cc:174: void FilterGcmCiphers(std::vector<int>*
crypto_suites) {
Can you break this up to be a bit more generic and match our naming scheme?

bool IsGcmCryptoSuite(int crypto_suite) {
  return (crypto_suite == rtc::SRTP_AEAD_AES_256_GCM ||
          crypto_suite == rtc::SRTP_AEAD_AES_128_GCM);
} 

bool IsGcmCipherName(const std::string& cipher_name) {
  return (cipher_name == rtc::CS_AEAD_AES_256_GCM ||
          cipher_name == rtc::CS_AEAD_AES_128_GCM);
}

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/medias...
talk/session/media/mediasession.cc:184: }
Then you can just remove this function and replace the existing calls with this
line:

crypto_suites.erase(std::remove_if(crypto_suites.begin(), crypto_suites.end(), 
   IsGcmCryptoSuite), crypto_suites.end());

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/medias...
talk/session/media/mediasession.cc:196: }
Similarly here:

cipher_names.erase(std::remove_if(cipher_names.begin(), cipher_names.end(), 
   IsGcmCipherName), cipher_names.end());

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/medias...
talk/session/media/mediasession.cc:262: rtc::CS_AEAD_AES_128_GCM ==
i->cipher_suite)) ||
IsGcmCipherName or IsGcmCryptoSuite would be useful here.

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/medias...
talk/session/media/mediasession.cc:265: !bundle)) {
Actually this, method seems to duplicate a lot with GetSupportXCryptoSuites. 
Could we instead of SelectCrypto pass in the supported crypto suites?  Then it's
a matter of checking if it's in the supported list rather than being hard coded.
 It will make it easier next time we add another crypto suite.

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/medias...
talk/session/media/mediasession.cc:1608: if (!options.enable_gcm_ciphers) {
Instead of filtering, can we pass options into
GetSupportedAudioCryptoSuitesNames and selectively add?

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/medias...
talk/session/media/mediasession.cc:1667: }
Instead of filtering, can we pass options into
GetSupportedAudioCryptoSuitesNames and selectively add?

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/medias...
File talk/session/media/mediasession.h (right):

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/medias...
talk/session/media/mediasession.h:107: enable_gcm_ciphers(false) {
Same here: can you change it to enable_gcm_crypto_suites?

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/srtpfi...
File talk/session/media/srtpfilter.cc (right):

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/srtpfi...
talk/session/media/srtpfilter.cc:693: expected_key_len = 32 + 12;
This seems duplicative with the new SrtpCryptoSuiteParams function you added. 
Can you use that here to avoid duplicating information in the code base?

[joachim, 2015-12-19 15:26:23]

Thanks for the detailed feedback. All changed.

https://codereview.webrtc.org/1528843005/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/1528843005/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection_unittest.cc:1552: kDefaultSrtpCryptoSuite));
On 2015/12/18 20:31:31, pthatcher1 wrote:
> Can you move this common logic into a separate method that's something like:
> 
> TestGcmNegotiation(bool local_gcm_enabled, bool remote_gcm_enabled, int
> expected_cipher_suite);
> 
> TestGcmNegotiation(true, true, kDefaultSrtpCryptoSuiteGcm);
> TestGcmNegotiation(true, false, kDefaultSrtpCryptoSuite);
> TestGcmNegotiation(false, true, kDefaultSrtpCryptoSuite);
> TestGcmNegotiation(false, false, kDefaultSrtpCryptoSuite);

Done.

https://codereview.webrtc.org/1528843005/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1528843005/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface.h:552: enable_gcm_ciphers(false) {
On 2015/12/18 20:31:31, pthatcher1 wrote:
> Can you call this enable_gcm_crypto_suites to be more consistent with our
> naming?

Done.

https://codereview.webrtc.org/1528843005/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface.h:569: // both endpoints will be used,
GCM must be supported by both to be used.
On 2015/12/18 20:31:31, pthatcher1 wrote:
> Might be more clear as "Enable GCM crypto suites from RFC 7714 for SRTP.  GCM
> will only be used if both sides enable it."

Done.

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/channe...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/channe...
talk/session/media/channel.cc:853: }
On 2015/12/18 20:31:31, pthatcher1 wrote:
> Having the "enable gcm ciphers" passed down from the TransportController to
the
> Transport to the TransportChannel and then up to here seems much more complex
> than necessary.  Can we not instead have WebRtcSession pass the necessary
> information directly to the BaseChannel an bypass all of the complexity?

Agreed. I pass the flag from the PeerConnectionFactory through the
ChannelManager to the BaseChannel now. This looks a lot cleaner.

> Also, I think it would be better if we had the right option pass from
> WebRtcSession down to BaseChannel such that GetSrtpCryptoSuites gets the
> information and selectively adds GCM rather than always adding GCM and then
> removing it after the fact.

Done

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/channe...
talk/session/media/channel.cc:885: if
(!rtc::SrtpCryptoSuiteParams(selected_crypto_suite, &key_len, &salt_len)) {
On 2015/12/18 20:31:31, pthatcher1 wrote:
> Can you call this GetSrtpKeyAndSaltLengths?

Done.

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/medias...
File talk/session/media/mediasession.cc (right):

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/medias...
talk/session/media/mediasession.cc:174: void FilterGcmCiphers(std::vector<int>*
crypto_suites) {
On 2015/12/18 20:31:31, pthatcher1 wrote:
> Can you break this up to be a bit more generic and match our naming scheme?
> 
> bool IsGcmCryptoSuite(int crypto_suite) {
>   return (crypto_suite == rtc::SRTP_AEAD_AES_256_GCM ||
>           crypto_suite == rtc::SRTP_AEAD_AES_128_GCM);
> } 
> 
> bool IsGcmCipherName(const std::string& cipher_name) {
>   return (cipher_name == rtc::CS_AEAD_AES_256_GCM ||
>           cipher_name == rtc::CS_AEAD_AES_128_GCM);
> }

Done. Implemented functions in sslstreamadapter (where the constants are
defined) and renamed second function to "IsGcmCryptoSuiteName".

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/medias...
talk/session/media/mediasession.cc:184: }
On 2015/12/18 20:31:31, pthatcher1 wrote:
> Then you can just remove this function and replace the existing calls with
this
> line:
> 
> crypto_suites.erase(std::remove_if(crypto_suites.begin(), crypto_suites.end(),

>    IsGcmCryptoSuite), crypto_suites.end());

Done.

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/medias...
talk/session/media/mediasession.cc:196: }
On 2015/12/18 20:31:32, pthatcher1 wrote:
> Similarly here:
> 
> cipher_names.erase(std::remove_if(cipher_names.begin(), cipher_names.end(), 
>    IsGcmCipherName), cipher_names.end());

Done.

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/medias...
talk/session/media/mediasession.cc:262: rtc::CS_AEAD_AES_128_GCM ==
i->cipher_suite)) ||
On 2015/12/18 20:31:31, pthatcher1 wrote:
> IsGcmCipherName or IsGcmCryptoSuite would be useful here.

Done.

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/medias...
talk/session/media/mediasession.cc:265: !bundle)) {
On 2015/12/18 20:31:32, pthatcher1 wrote:
> Actually this, method seems to duplicate a lot with GetSupportXCryptoSuites. 
> Could we instead of SelectCrypto pass in the supported crypto suites?  Then
it's
> a matter of checking if it's in the supported list rather than being hard
coded.
>  It will make it easier next time we add another crypto suite.

Done.

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/medias...
talk/session/media/mediasession.cc:1608: if (!options.enable_gcm_ciphers) {
On 2015/12/18 20:31:32, pthatcher1 wrote:
> Instead of filtering, can we pass options into
> GetSupportedAudioCryptoSuitesNames and selectively add?

Done.

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/medias...
talk/session/media/mediasession.cc:1667: }
On 2015/12/18 20:31:32, pthatcher1 wrote:
> Instead of filtering, can we pass options into
> GetSupportedAudioCryptoSuitesNames and selectively add?

Done.

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/medias...
File talk/session/media/mediasession.h (right):

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/medias...
talk/session/media/mediasession.h:107: enable_gcm_ciphers(false) {
On 2015/12/18 20:31:32, pthatcher1 wrote:
> Same here: can you change it to enable_gcm_crypto_suites?

Done.

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/srtpfi...
File talk/session/media/srtpfilter.cc (right):

https://codereview.webrtc.org/1528843005/diff/20001/talk/session/media/srtpfi...
talk/session/media/srtpfilter.cc:693: expected_key_len = 32 + 12;
On 2015/12/18 20:31:32, pthatcher1 wrote:
> This seems duplicative with the new SrtpCryptoSuiteParams function you added. 
> Can you use that here to avoid duplicating information in the code base?

Right, I wrote that code before adding the new function. Changed.

[pthatcher1, 2015-12-21 22:30:27]

Much better.  I just have one moderately large style issue left.

After that, I think it looks good, but I still want Justin to take a close look.

https://codereview.webrtc.org/1528843005/diff/40001/talk/session/media/medias...
File talk/session/media/mediasession.h (right):

https://codereview.webrtc.org/1528843005/diff/40001/talk/session/media/medias...
talk/session/media/mediasession.h:549: const MediaSessionOptions& options);
This looks a lot better, but two things:

1.  The out parameter should be last, and the options first.

2.  The MediaSessionOptions struct is huge.  Could you define a smaller struct
instead?  Perhaps something like CryptoOptions?  Then those could be passed
here.  In fact, it would be nice to pass that around and store that everywhere
you currently pass around store a bool.  So instead of 

SetEnableGcmCryptoSuites(bool enable_gcm_crypto_suites) {   
  enable_gcm_crypto_suites_ = enable_gcm_crypto_suites; 
}

And the like, we could have 

SetCryptoOptions(const CryptoOptions& crypto_options) { 
  crypto_options_ = crypto_options_; 
}.  

I think that would be cleaner and make it easier to add other controls over
crypto suites in the future.

[joachim, 2015-12-21 23:37:57]

https://codereview.webrtc.org/1528843005/diff/40001/talk/session/media/medias...
File talk/session/media/mediasession.h (right):

https://codereview.webrtc.org/1528843005/diff/40001/talk/session/media/medias...
talk/session/media/mediasession.h:549: const MediaSessionOptions& options);
On 2015/12/21 22:30:27, pthatcher1 wrote:
> This looks a lot better, but two things:
> 
> 1.  The out parameter should be last, and the options first.

Done

> 2.  The MediaSessionOptions struct is huge.  Could you define a smaller struct
> instead?  Perhaps something like CryptoOptions?  Then those could be passed
> here.  In fact, it would be nice to pass that around and store that everywhere
> you currently pass around store a bool.  So instead of 
> 
> SetEnableGcmCryptoSuites(bool enable_gcm_crypto_suites) {   
>   enable_gcm_crypto_suites_ = enable_gcm_crypto_suites; 
> }
> 
> And the like, we could have 
> 
> SetCryptoOptions(const CryptoOptions& crypto_options) { 
>   crypto_options_ = crypto_options_; 
> }.  
> 
> I think that would be cleaner and make it easier to add other controls over
> crypto suites in the future.
> 

Ok, makes sense. Added struct in sslstreamadapter.h and changed various
methods/classes.

[joachim, 2016-01-15 22:48:49]

On 2015/12/21 23:37:57, joachim wrote:
>
https://codereview.webrtc.org/1528843005/diff/40001/talk/session/media/medias...
> File talk/session/media/mediasession.h (right):
> 
>
https://codereview.webrtc.org/1528843005/diff/40001/talk/session/media/medias...
> talk/session/media/mediasession.h:549: const MediaSessionOptions& options);
> On 2015/12/21 22:30:27, pthatcher1 wrote:
> > This looks a lot better, but two things:
> > 
> > 1.  The out parameter should be last, and the options first.
> 
> Done
> 
> > 2.  The MediaSessionOptions struct is huge.  Could you define a smaller
struct
> > instead?  Perhaps something like CryptoOptions?  Then those could be passed
> > here.  In fact, it would be nice to pass that around and store that
everywhere
> > you currently pass around store a bool.  So instead of 
> > 
> > SetEnableGcmCryptoSuites(bool enable_gcm_crypto_suites) {   
> >   enable_gcm_crypto_suites_ = enable_gcm_crypto_suites; 
> > }
> > 
> > And the like, we could have 
> > 
> > SetCryptoOptions(const CryptoOptions& crypto_options) { 
> >   crypto_options_ = crypto_options_; 
> > }.  
> > 
> > I think that would be cleaner and make it easier to add other controls over
> > crypto suites in the future.
> > 
> 
> Ok, makes sense. Added struct in sslstreamadapter.h and changed various
> methods/classes.

Justin, ping?

[pthatcher1, 2016-01-30 00:54:33]

lgtm

https://codereview.chromium.org/1528843005/diff/80001/webrtc/base/sslstreamad...
File webrtc/base/sslstreamadapter.cc (right):

https://codereview.chromium.org/1528843005/diff/80001/webrtc/base/sslstreamad...
webrtc/base/sslstreamadapter.cc:76: *salt_length = 12;
Can you leave a comment saying where these values come from?

[joachim, 2016-01-31 23:39:52]

Thanks

@Justin: ptal

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

https://codereview.webrtc.org/1528843005/diff/80001/webrtc/base/sslstreamadap...
webrtc/base/sslstreamadapter.cc:76: *salt_length = 12;
On 2016/01/30 00:54:33, pthatcher1 wrote:
> Can you leave a comment saying where these values come from?

Done.

[joachim, 2016-02-04 23:22:58]

Description was changed from

==========
Add support for GCM cipher suites from RFC 7714.

GCM cipher suites are optional (disabled by default) and can be enabled
through "PeerConnectionFactoryInterface::Options".

BUG=webrtc:5222
==========

to

==========
Add support for GCM cipher suites from RFC 7714.

GCM cipher suites are optional (disabled by default) and can be enabled
through "PeerConnectionFactoryInterface::Options".

NOTE: the CL from https://codereview.webrtc.org/1607613003/ should land before
we land this!

BUG=webrtc:5222
==========

[joachim, 2016-02-04 23:24:48]

On 2016/01/31 23:39:52, joachim wrote:
> Thanks
> 
> @Justin: ptal

Ping, also for https://codereview.webrtc.org/1607613003/ which is required for
this one.

[joachim, 2016-03-17 20:31:10]

On 2016/02/04 23:24:48, joachim wrote:
> On 2016/01/31 23:39:52, joachim wrote:
> > Thanks
> > 
> > @Justin: ptal
> 
> Ping, also for https://codereview.webrtc.org/1607613003/ which is required for
> this one.

Another ping, now that https://codereview.webrtc.org/1607613003/ landed and
rolled into WebRTC...

Anything I can do to help move this forward?

[joachim, 2016-03-24 00:47:54]

Description was changed from

==========
Add support for GCM cipher suites from RFC 7714.

GCM cipher suites are optional (disabled by default) and can be enabled
through "PeerConnectionFactoryInterface::Options".

NOTE: the CL from https://codereview.webrtc.org/1607613003/ should land before
we land this!

BUG=webrtc:5222
==========

to

==========
Add support for GCM cipher suites from RFC 7714.

GCM cipher suites are optional (disabled by default) and can be enabled
through "PeerConnectionFactoryInterface::Options".

BUG=webrtc:5222
==========

[joachim, 2016-04-27 22:48:43]

jbauch@webrtc.org changed reviewers:
+ mattdr@webrtc.org

[joachim, 2016-04-27 22:48:44]

Rebased + fix in newly introduced check for SRTP-but-non-DTLS cases.

+matt for review

[mattdr, 2016-05-06 22:33:41]

I think there's a serious problem in serializing and deserializing the key as
base64 in SRTP. See my comment in mediasession.cc.

To start to address it, we'll need to remove all remaining uses of
SRTP_MASTER_KEY_LEN, of which some remain in srtpfilter.cc (see
SrtpFilter::ApplyParams).

SrtpFilter::ParseKeyParams probably shouldn't use rtc::Base64::DO_STRICT,
either, given the snippet I referenced from RFC 4568 sec 6.1.

[mattdr, 2016-05-06 22:34:12]

mattdr@google.com changed reviewers:
+ mattdr@google.com

[mattdr, 2016-05-06 22:34:14]

UNDO PREVIOUS LGTM

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/api/peerconnectio...
webrtc/api/peerconnection_unittest.cc:1758: // Test that a non-GCM cipher is
used if the initator supports GCM and the
'Test that GCM isn't used if only the initiator supports it.'

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/api/peerconnectio...
webrtc/api/peerconnection_unittest.cc:1764: // Test that a non-GCM cipher is
used if the initator supports non-GCM and the
'Test that GCM isn't used if only the receiver supports it.'

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:574:
ssl_max_version(rtc::SSL_PROTOCOL_DTLS_12) {}
Everything else here gets an explicit default. I'd like it to be more obvious
that the default options don't enable GCM.

Maybe add a static method to CryptoOptions, CryptoOptions::NoGcm(), which
returns an instance with GCM disabled, then use it here?

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/base/sslstreamada...
File webrtc/base/sslstreamadapter.h (right):

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:73: CryptoOptions() :
The initializer will fit on one line.

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/base/sslstreamada...
File webrtc/base/sslstreamadapter_unittest.cc (right):

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter_unittest.cc:1064: rtc::GetSrtpKeyAndSaltLengths(
belongs with previous line, I think, here and below

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:327: rtc::CryptoOptions crypto_options_;
Should we make this available as a public or protected getter, and then avoid
having to plumb crypto_options through the GetSrtpCryptoSuites functions?

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/channel_unitte...
File webrtc/pc/channel_unittest.cc (right):

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/channel_unitte...
webrtc/pc/channel_unittest.cc:389: bool CheckGcmCipher(typename T::Channel*
channel, int flags) {
A quick comment might be useful:
// Checks that the channel is using GCM iff GCM_CIPHER is set in flags.
// Returns true if so.

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/channel_unitte...
webrtc/pc/channel_unittest.cc:1350: int common_gcm_flags = (flags1 & GCM_CIPHER)
& (flags2 & GCM_CIPHER);
flags1 & flags2 & GCM_CIPHER

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/channelmanager.cc
File webrtc/pc/channelmanager.cc (right):

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/channelmanager...
webrtc/pc/channelmanager.cc:104: crypto_options_ = crypto_options;
Maybe this should fail or log if there are active channels? API consumers might
imagine this will change the crypto on existing channels, instead of them
keeping a copy of this state.

Could also consider renaming this to, say, SetCryptoSetupOptions.

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/channelmanager.h
File webrtc/pc/channelmanager.h (right):

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/channelmanager...
webrtc/pc/channelmanager.h:178: rtc::CryptoOptions crypto_options_;
Again, I'd prefer to have a more explicit default set in the constructor here.

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:94: int master_key_base64_len = (key_len + salt_len) *
4 / 3;
This is bad.

E.g.
SRTP_AEAD_AES_256_GCM has key_len = 32 bytes and salt_len = 12 bytes. Total is
44 bytes, which requires at least 59 characters to encode in base64 (not
including padding), but we round down to 58.

https://tools.ietf.org/html/rfc4568#section-6.1

   If the length (after being decoded
   from base64) does not match that specified for the crypto-suite, the
   crypto attribute in question MUST be considered invalid.  Each master
   key and salt MUST be a cryptographically random number and MUST be
   unique to the entire SDP message.  When base64 decoding the key and
   salt, padding characters (i.e., one or two "=" at the end of the
   base64-encoded data) are discarded (see [RFC3548] for details).
   Base64 encoding assumes that the base64 encoding input is an integral
   number of octets.  If a given crypto-suite requires the use of a
   concatenated key and salt with a length that is not an integral
   number of octets, said crypto-suite MUST define a padding scheme that
   results in the base64 input being an integral number of octets.  For
   example, if the length defined were 250 bits, then 6 padding bits
   would be needed, which could be defined to be the last 6 bits in a
   256 bit input.

Much more troubling -- the unit tests *cover* AES-256-GCM but *pass*. Something
is wrong with the parsing code (SrtpFilter::ParseKeyParams) or its callers if
this didn't blow up.

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:232: rtc::IsGcmCryptoSuiteName(i->cipher_suite)) ||
one more space on this line to align with the begin of the paren expression
above

[pthatcher1, 2016-05-06 22:37:44]

no lgtm

?

[pthatcher1, 2016-05-06 22:37:56]

On 2016/05/06 22:37:44, pthatcher1 wrote:
> no lgtm
> 
> ?

-lgtm

?

[mattdr, 2016-05-06 22:39:45]

Sorry, new to Reitveld. not lgtm.

[pthatcher1, 2016-05-06 22:40:34]

On 2016/05/06 22:37:56, pthatcher1 wrote:
> On 2016/05/06 22:37:44, pthatcher1 wrote:
> > no lgtm
> > 
> > ?
> 
> -lgtm
> 
> ?

not lgtm

[mattdr, 2016-05-06 22:47:52]

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:165:
crypto_suites->push_back(rtc::SRTP_AEAD_AES_256_GCM);
Judging by the comments above, I'm inferring these ciphers are listed in
decreasing order of priority, i.e. we'll prefer AES-256-GCM to AES-128-GCM.

Is there an RFC or other explicit recommendation on which we should prefer?

[joachim, 2016-05-09 23:21:41]

Thanks for your thorough review and detailed feedback!

I addressed all comments in the latest patch, ptal.

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/api/peerconnectio...
webrtc/api/peerconnection_unittest.cc:1758: // Test that a non-GCM cipher is
used if the initator supports GCM and the
On 2016/05/06 22:34:13, mattdr wrote:
> 'Test that GCM isn't used if only the initiator supports it.'

Done.

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/api/peerconnectio...
webrtc/api/peerconnection_unittest.cc:1764: // Test that a non-GCM cipher is
used if the initator supports non-GCM and the
On 2016/05/06 22:34:13, mattdr wrote:
> 'Test that GCM isn't used if only the receiver supports it.'

Done.

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:574:
ssl_max_version(rtc::SSL_PROTOCOL_DTLS_12) {}
On 2016/05/06 22:34:13, mattdr wrote:
> Everything else here gets an explicit default. I'd like it to be more obvious
> that the default options don't enable GCM.
> 
> Maybe add a static method to CryptoOptions, CryptoOptions::NoGcm(), which
> returns an instance with GCM disabled, then use it here?

Right, that's a good idea. Done.

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/base/sslstreamada...
File webrtc/base/sslstreamadapter.h (right):

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:73: CryptoOptions() :
On 2016/05/06 22:34:13, mattdr wrote:
> The initializer will fit on one line.

Done.

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/base/sslstreamada...
File webrtc/base/sslstreamadapter_unittest.cc (right):

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter_unittest.cc:1064: rtc::GetSrtpKeyAndSaltLengths(
On 2016/05/06 22:34:13, mattdr wrote:
> belongs with previous line, I think, here and below

Done.

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:327: rtc::CryptoOptions crypto_options_;
On 2016/05/06 22:34:13, mattdr wrote:
> Should we make this available as a public or protected getter, and then avoid
> having to plumb crypto_options through the GetSrtpCryptoSuites functions?

Done.

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/channel_unitte...
File webrtc/pc/channel_unittest.cc (right):

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/channel_unitte...
webrtc/pc/channel_unittest.cc:389: bool CheckGcmCipher(typename T::Channel*
channel, int flags) {
On 2016/05/06 22:34:14, mattdr wrote:
> A quick comment might be useful:
> // Checks that the channel is using GCM iff GCM_CIPHER is set in flags.
> // Returns true if so.

Done.

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/channel_unitte...
webrtc/pc/channel_unittest.cc:1350: int common_gcm_flags = (flags1 & GCM_CIPHER)
& (flags2 & GCM_CIPHER);
On 2016/05/06 22:34:14, mattdr wrote:
> flags1 & flags2 & GCM_CIPHER

Done.

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/channelmanager.cc
File webrtc/pc/channelmanager.cc (right):

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/channelmanager...
webrtc/pc/channelmanager.cc:104: crypto_options_ = crypto_options;
On 2016/05/06 22:34:14, mattdr wrote:
> Maybe this should fail or log if there are active channels? API consumers
might
> imagine this will change the crypto on existing channels, instead of them
> keeping a copy of this state.
> 
> Could also consider renaming this to, say, SetCryptoSetupOptions.

Done (added warning log and comment in header but didn't rename).

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/channelmanager.h
File webrtc/pc/channelmanager.h (right):

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/channelmanager...
webrtc/pc/channelmanager.h:178: rtc::CryptoOptions crypto_options_;
On 2016/05/06 22:34:14, mattdr wrote:
> Again, I'd prefer to have a more explicit default set in the constructor here.

Done.

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:94: int master_key_base64_len = (key_len + salt_len) *
4 / 3;
On 2016/05/06 22:34:14, mattdr wrote:
> This is bad.

Right, and sorry for missing this :-( I now changed it to generate binary random
data with the required length and then encode to base64.

> E.g.
> SRTP_AEAD_AES_256_GCM has key_len = 32 bytes and salt_len = 12 bytes. Total is
> 44 bytes, which requires at least 59 characters to encode in base64 (not
> including padding), but we round down to 58.
> 
> https://tools.ietf.org/html/rfc4568#section-6.1
> 
>    If the length (after being decoded
>    from base64) does not match that specified for the crypto-suite, the
>    crypto attribute in question MUST be considered invalid.  Each master
>    key and salt MUST be a cryptographically random number and MUST be
>    unique to the entire SDP message.  When base64 decoding the key and
>    salt, padding characters (i.e., one or two "=" at the end of the
>    base64-encoded data) are discarded (see [RFC3548] for details).
>    Base64 encoding assumes that the base64 encoding input is an integral
>    number of octets.  If a given crypto-suite requires the use of a
>    concatenated key and salt with a length that is not an integral
>    number of octets, said crypto-suite MUST define a padding scheme that
>    results in the base64 input being an integral number of octets.  For
>    example, if the length defined were 250 bits, then 6 padding bits
>    would be needed, which could be defined to be the last 6 bits in a
>    256 bit input.
> 
> Much more troubling -- the unit tests *cover* AES-256-GCM but *pass*.
Something
> is wrong with the parsing code (SrtpFilter::ParseKeyParams) or its callers if
> this didn't blow up.

The reason why this was not caught by my added tests is that for DTLS-SRTP the
key is extracted from the underlying channel (see "BaseChannel::SetupDtlsSrtp")
which directly returns the binary key.

Also none of my added tests enabled Gcm for cases where keys are read from a SDP
(this codepath is only executed for such keys).

I added "WebRtcSessionTest.VerifyCryptoParamsInSDPGcm" to check the length of
encoded keys in the SDP but I'm not sure if this is the best place to verify
this or if another test should be added somewhere else.

Other tests were added to srtpfilter_unittest.cc together with a fix in
"SrtpFilter::ApplyParams".

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:165:
crypto_suites->push_back(rtc::SRTP_AEAD_AES_256_GCM);
On 2016/05/06 22:47:52, mattdr wrote:
> Judging by the comments above, I'm inferring these ciphers are listed in
> decreasing order of priority, i.e. we'll prefer AES-256-GCM to AES-128-GCM.
> 
> Is there an RFC or other explicit recommendation on which we should prefer?

No particular reason from my side. I didn't find anything special recommendation
regarding SRTP and GCM, but Mozilla prioritizes AES-256 over -128 for the
webserver setup
(https://wiki.mozilla.org/Security/Server_Side_TLS#Modern_compatibility), so I
decided to use the same ordering here.

https://codereview.webrtc.org/1528843005/diff/180001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:232: rtc::IsGcmCryptoSuiteName(i->cipher_suite)) ||
On 2016/05/06 22:34:14, mattdr wrote:
> one more space on this line to align with the begin of the paren expression
> above

Done.

[mattdr, 2016-05-10 22:29:13]

Thanks for your work and for the quick turnaround!

This lgtm, modulo a nit about the new Random() function.

https://codereview.webrtc.org/1528843005/diff/200001/webrtc/base/helpers.h
File webrtc/base/helpers.h (right):

https://codereview.webrtc.org/1528843005/diff/200001/webrtc/base/helpers.h#ne...
webrtc/base/helpers.h:44: bool CreateRandomData(size_t length, void* data);
Let's avoid the buffer pointer and use a vector<char> or string.

e.g.

bool CreateRandomBytes(size_t length, string* bytes);

https://codereview.webrtc.org/1528843005/diff/200001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/1528843005/diff/200001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:103: rtc::Base64::EncodeFromArray(master_key.data(),
master_key_len, &key);
if we use a string buffer, we can use key = rtc::Base64::Encode(keyBytes) -- a
small convenience.

[joachim, 2016-05-10 23:09:45]

All changed, thanks.

https://codereview.webrtc.org/1528843005/diff/200001/webrtc/base/helpers.h
File webrtc/base/helpers.h (right):

https://codereview.webrtc.org/1528843005/diff/200001/webrtc/base/helpers.h#ne...
webrtc/base/helpers.h:44: bool CreateRandomData(size_t length, void* data);
On 2016/05/10 22:29:13, mattdr wrote:
> Let's avoid the buffer pointer and use a vector<char> or string.
> 
> e.g.
> 
> bool CreateRandomBytes(size_t length, string* bytes);
> 
> 

Done (used std::string).

https://codereview.webrtc.org/1528843005/diff/200001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/1528843005/diff/200001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:103: rtc::Base64::EncodeFromArray(master_key.data(),
master_key_len, &key);
On 2016/05/10 22:29:13, mattdr wrote:
> if we use a string buffer, we can use key = rtc::Base64::Encode(keyBytes) -- a
> small convenience.

Done.

https://codereview.webrtc.org/1528843005/diff/220001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/1528843005/diff/220001/webrtc/pc/mediasession.c...
webrtc/pc/mediasession.cc:101: RTC_CHECK_EQ(static_cast<size_t>(master_key_len),
master_key.size());
This should never trigger (correct behaviour of CreateRandomData is verified by
tests), but serves as additional sanity check here.

[joachim, 2016-05-17 20:41:03]

@Peter and Justin: now that Matt is happy with the CL, could you ptal my latest
changes?

I think this anyway has to wait for whatever downstream project you have that is
using an rather old version of libsrtp. See
https://bugs.chromium.org/p/webrtc/issues/detail?id=5133 for another change by
me using a more recent libsrtp API that got reverted. Most likely that projects
libsrtp also doesn't support the GCM ciphers API...

[joachim, 2016-06-29 23:48:36]

On 2016/05/17 20:41:03, joachim wrote:
> @Peter and Justin: now that Matt is happy with the CL, could you ptal my
latest
> changes?
> 
> I think this anyway has to wait for whatever downstream project you have that
is
> using an rather old version of libsrtp. See
> https://bugs.chromium.org/p/webrtc/issues/detail?id=5133 for another change by
> me using a more recent libsrtp API that got reverted. Most likely that
projects
> libsrtp also doesn't support the GCM ciphers API...

Rebased to current master and updated to match WebRTC changes since my last
rebase.

@Peter/Justin: could you please take another look?

Matt has updated your internal libsrtp dependency (thanks again), so this is no
longer blocking the CL.

[pthatcher1, 2016-06-30 21:45:56]

lgtm

https://codereview.webrtc.org/1528843005/diff/240001/webrtc/base/sslstreamada...
File webrtc/base/sslstreamadapter.h (right):

https://codereview.webrtc.org/1528843005/diff/240001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:82: bool enable_gcm_crypto_suites;
Putting "= false;" on here would we slightly nicer.

[joachim, 2016-06-30 22:21:43]

https://codereview.webrtc.org/1528843005/diff/240001/webrtc/base/sslstreamada...
File webrtc/base/sslstreamadapter.h (right):

https://codereview.webrtc.org/1528843005/diff/240001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:82: bool enable_gcm_crypto_suites;
On 2016/06/30 21:45:56, pthatcher1 wrote:
> Putting "= false;" on here would we slightly nicer.

Done.

[joachim, 2016-07-05 12:53:40]

On 2016/06/29 23:48:36, joachim wrote:
> On 2016/05/17 20:41:03, joachim wrote:
> > @Peter and Justin: now that Matt is happy with the CL, could you ptal my
> latest
> > changes?
> > 
> > I think this anyway has to wait for whatever downstream project you have
that
> is
> > using an rather old version of libsrtp. See
> > https://bugs.chromium.org/p/webrtc/issues/detail?id=5133 for another change
by
> > me using a more recent libsrtp API that got reverted. Most likely that
> projects
> > libsrtp also doesn't support the GCM ciphers API...
> 
> Rebased to current master and updated to match WebRTC changes since my last
> rebase.
> 
> @Peter/Justin: could you please take another look?

@Justin: ping - do you also want to take a look, or are you fine with Matt and
Peter having reviewed this?

> Matt has updated your internal libsrtp dependency (thanks again), so this is
no
> longer blocking the CL.

[mattdr-at-webrtc.org, 2016-07-06 17:54:04]

Justin's on vacation this week. Go ahead and submit, and he can yell at me if
I'm wrong. :)

[joachim, 2016-07-06 18:34:05]

On 2016/07/06 17:54:04, mattdr1 wrote:
> Justin's on vacation this week. Go ahead and submit, and he can yell at me if
> I'm wrong. :)

:) ok thanks. So let's see what the CQ thinks of this...

[joachim, 2016-07-06 18:34:16]

The CQ bit was checked by jbauch@webrtc.org

[joachim, 2016-07-06 18:34:17]

The patchset sent to the CQ was uploaded after l-g-t-m from mattdr@google.com,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1528843005/#ps250001 (title:
"Feedback from Peter")

[commit-bot: I haz the power, 2016-07-06 18:34:31]

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, 2016-07-06 18:38:37]

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

[commit-bot: I haz the power, 2016-07-06 18:38:38]

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/16008)

[joachim, 2016-07-06 20:02:47]

A CL to fix the asan error pending here:
https://codereview.webrtc.org/2129753002/

[mattdr-at-webrtc.org, 2016-07-14 20:39:51]

not lgtm

Sorry, something new's come up. :(

https://codereview.webrtc.org/1528843005/diff/290001/webrtc/pc/srtpfilter.cc
File webrtc/pc/srtpfilter.cc (right):

https://codereview.webrtc.org/1528843005/diff/290001/webrtc/pc/srtpfilter.cc#...
webrtc/pc/srtpfilter.cc:729: policy.rtp.auth_type = EXTERNAL_HMAC_SHA1;
I had to read code near here for some other purpose, and thought of this CL.

This code summarily overrides the cipher policy's auth mechanism and replaces it
with this "external" HMAC-SHA1 implementation. Looking at the implementation
(look for external_hmac_compute, UpdateRtpAuthTag) I expect this does something
aggressively bad in the face of a different auth mechanism.

It also hints at code that rewrites packets after libsrtp has seen them (see
ApplyPacketOptions in rtputils.h).

... does any of that code actually run?

[mattdr-at-webrtc.org, 2016-07-14 20:43:22]

https://codereview.webrtc.org/1528843005/diff/290001/webrtc/pc/srtpfilter.cc
File webrtc/pc/srtpfilter.cc (right):

https://codereview.webrtc.org/1528843005/diff/290001/webrtc/pc/srtpfilter.cc#...
webrtc/pc/srtpfilter.cc:729: policy.rtp.auth_type = EXTERNAL_HMAC_SHA1;
... and should this have come up in testing?

note that libsrtp uses "null auth" for aes-gcm:
https://github.com/cisco/libsrtp/blob/f97b18d7686b6e9a750e55f93dc4bc1153ddee5...

[mattdr-at-webrtc.org, 2016-07-14 22:14:37]

see also:
https://bugs.chromium.org/p/chromium/issues/detail?id=628400

[mattdr-at-webrtc.org, 2016-07-15 00:52:33]

I've put what I've learned so far into
https://bugs.chromium.org/p/chromium/issues/detail?id=628400.

It occurs to me now I probably should have filed that on WebRTC -- sorry, I'm
new to juggling several different bug trackers. Let me know if it'd be more
convenient for you in WebRTC.

I think the takeaway here is: AES-GCM is fine for WebRTC standalone (not
compiled into Chromium, or equivalently where ENABLE_EXTERNAL_AUTH is not set),
but we need to make AES-GCM and ENABLE_EXTERNAL_AUTH aggressively incompatible.
Otherwise, some poor fool will try to use AES-GCM and externalhmac.cc will
overwrite the AES-GCM auth tag with a SHA1-HMAC.

And, yes: this doesn't seem great to me either.

[joachim, 2016-07-18 20:55:54]

On 2016/07/15 00:52:33, mattdr-at-webrtc.org wrote:
> I've put what I've learned so far into
> https://bugs.chromium.org/p/chromium/issues/detail?id=628400.
> 
> It occurs to me now I probably should have filed that on WebRTC -- sorry, I'm
> new to juggling several different bug trackers. Let me know if it'd be more
> convenient for you in WebRTC.
> 
> I think the takeaway here is: AES-GCM is fine for WebRTC standalone (not
> compiled into Chromium, or equivalently where ENABLE_EXTERNAL_AUTH is not
set),
> but we need to make AES-GCM and ENABLE_EXTERNAL_AUTH aggressively
incompatible.
> Otherwise, some poor fool will try to use AES-GCM and externalhmac.cc will
> overwrite the AES-GCM auth tag with a SHA1-HMAC.
> 
> And, yes: this doesn't seem great to me either.

Thanks for the heads up and detailed explanations in the referenced issue. I'm
mostly using standalone WebRTC right now, so completely missed this :-/

Please let me know how you want this to proceed, I also commented on the issue.

[mattdr-at-webrtc.org, 2016-07-18 21:18:26]

On 2016/07/18 20:55:54, joachim wrote:
> On 2016/07/15 00:52:33, http://mattdr-at-webrtc.org wrote:
> > I've put what I've learned so far into
> > https://bugs.chromium.org/p/chromium/issues/detail?id=628400.
> > 
> > It occurs to me now I probably should have filed that on WebRTC -- sorry,
I'm
> > new to juggling several different bug trackers. Let me know if it'd be more
> > convenient for you in WebRTC.
> > 
> > I think the takeaway here is: AES-GCM is fine for WebRTC standalone (not
> > compiled into Chromium, or equivalently where ENABLE_EXTERNAL_AUTH is not
> set),
> > but we need to make AES-GCM and ENABLE_EXTERNAL_AUTH aggressively
> incompatible.
> > Otherwise, some poor fool will try to use AES-GCM and externalhmac.cc will
> > overwrite the AES-GCM auth tag with a SHA1-HMAC.
> > 
> > And, yes: this doesn't seem great to me either.
> 
> Thanks for the heads up and detailed explanations in the referenced issue. I'm
> mostly using standalone WebRTC right now, so completely missed this :-/
> 
> Please let me know how you want this to proceed, I also commented on the
issue.

Updated the issue. Short answer: let's get AES-GCM into standalone WebRTC, but
do what we can to prevent it from being selected or used if ENABLE_EXTERNAL_AUTH
is set.

[joachim, 2016-07-20 21:24:34]

Description was changed from

==========
Add support for GCM cipher suites from RFC 7714.

GCM cipher suites are optional (disabled by default) and can be enabled
through "PeerConnectionFactoryInterface::Options".

BUG=webrtc:5222
==========

to

==========
Add support for GCM cipher suites from RFC 7714.

GCM cipher suites are optional (disabled by default) and can be enabled
through "PeerConnectionFactoryInterface::Options".

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

[joachim, 2016-07-20 21:26:49]

Changed to disable GCM if ENABLE_EXTERNAL_AUTH is defined (logs a warning if
requested and disables it).

I'm not sure how to test this from here, probably must be done in Chromium.

[joachim, 2016-07-20 21:28:55]

Description was changed from

==========
Add support for GCM cipher suites from RFC 7714.

GCM cipher suites are optional (disabled by default) and can be enabled
through "PeerConnectionFactoryInterface::Options".

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

to

==========
Add support for GCM cipher suites from RFC 7714.

GCM cipher suites are optional (disabled by default) and can be enabled
through "PeerConnectionFactoryInterface::Options".

If compiled with Chromium (i.e. "ENABLE_EXTERNAL_AUTH" is defined), no
GCM ciphers can be used yet (see https://crbug.com/628400).

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

[mattdr-at-webrtc.org, 2016-07-22 18:31:31]

On 2016/07/20 21:26:49, joachim wrote:
> Changed to disable GCM if ENABLE_EXTERNAL_AUTH is defined (logs a warning if
> requested and disables it).
> 
> I'm not sure how to test this from here, probably must be done in Chromium.

lgtm. Thanks for accommodating this wonderful wacky flag.

[joachim, 2016-08-04 09:25:17]

On 2016/07/22 18:31:31, mattdr-at-webrtc.org wrote:
> On 2016/07/20 21:26:49, joachim wrote:
> > Changed to disable GCM if ENABLE_EXTERNAL_AUTH is defined (logs a warning if
> > requested and disables it).
> > 
> > I'm not sure how to test this from here, probably must be done in Chromium.
> 
> lgtm. Thanks for accommodating this wonderful wacky flag.

The libsrtp changes have rolled into WebRTC in
https://codereview.webrtc.org/2215633002
so this should be good to CQ now (I ran a couple of trybots to verify).

Still ok from your side?

[mattdr-at-webrtc.org, 2016-08-04 10:45:42]

On 2016/08/04 09:25:17, joachim wrote:
> On 2016/07/22 18:31:31, http://mattdr-at-webrtc.org wrote:
> > On 2016/07/20 21:26:49, joachim wrote:
> > > Changed to disable GCM if ENABLE_EXTERNAL_AUTH is defined (logs a warning
if
> > > requested and disables it).
> > > 
> > > I'm not sure how to test this from here, probably must be done in
Chromium.
> > 
> > lgtm. Thanks for accommodating this wonderful wacky flag.
> 
> The libsrtp changes have rolled into WebRTC in
> https://codereview.webrtc.org/2215633002
> so this should be good to CQ now (I ran a couple of trybots to verify).
> 
> Still ok from your side?

still lgtm

[joachim, 2016-08-04 10:59:26]

The CQ bit was checked by jbauch@webrtc.org

[joachim, 2016-08-04 10:59:27]

The patchset sent to the CQ was uploaded after l-g-t-m from mattdr@google.com,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1528843005/#ps310001 (title:
"Disable GCM if ENABLE_EXTERNAL_AUTH is defined.")

[commit-bot: I haz the power, 2016-08-04 10:59:37]

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, 2016-08-04 11:14:06]

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

[commit-bot: I haz the power, 2016-08-04 11:14:08]

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

[joachim, 2016-08-04 11:51:08]

The CQ bit was checked by jbauch@webrtc.org

[commit-bot: I haz the power, 2016-08-04 11:51:21]

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, 2016-08-04 12:20:38]

Description was changed from

==========
Add support for GCM cipher suites from RFC 7714.

GCM cipher suites are optional (disabled by default) and can be enabled
through "PeerConnectionFactoryInterface::Options".

If compiled with Chromium (i.e. "ENABLE_EXTERNAL_AUTH" is defined), no
GCM ciphers can be used yet (see https://crbug.com/628400).

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

to

==========
Add support for GCM cipher suites from RFC 7714.

GCM cipher suites are optional (disabled by default) and can be enabled
through "PeerConnectionFactoryInterface::Options".

If compiled with Chromium (i.e. "ENABLE_EXTERNAL_AUTH" is defined), no
GCM ciphers can be used yet (see https://crbug.com/628400).

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

[commit-bot: I haz the power, 2016-08-04 12:20:40]

Committed patchset #17 (id:310001)

[commit-bot: I haz the power, 2016-08-04 12:20:47]

Description was changed from

==========
Add support for GCM cipher suites from RFC 7714.

GCM cipher suites are optional (disabled by default) and can be enabled
through "PeerConnectionFactoryInterface::Options".

If compiled with Chromium (i.e. "ENABLE_EXTERNAL_AUTH" is defined), no
GCM ciphers can be used yet (see https://crbug.com/628400).

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

to

==========
Add support for GCM cipher suites from RFC 7714.

GCM cipher suites are optional (disabled by default) and can be enabled
through "PeerConnectionFactoryInterface::Options".

If compiled with Chromium (i.e. "ENABLE_EXTERNAL_AUTH" is defined), no
GCM ciphers can be used yet (see https://crbug.com/628400).

BUG=webrtc:5222, 628400

Committed: https://crrev.com/cb56065c62a31d83919abcd4e343ea3dbe029e9f
Cr-Commit-Position: refs/heads/master@{#13635}
==========

[commit-bot: I haz the power, 2016-08-04 12:20:49]

Patchset 17 (id:??) landed as
https://crrev.com/cb56065c62a31d83919abcd4e343ea3dbe029e9f
Cr-Commit-Position: refs/heads/master@{#13635}