Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years ago by joachim Modified:
4 years, 4 months ago Reviewers:
mattdr, mattdr-at-webrtc.org, juberti1, pthatcher1 CC:
tterriberry_mozilla.com, webrtc-reviews_webrtc.org Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd 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}
Patch Set 1 #Patch Set 2 : Added PeerConnection tests using GCM ciphers, fixed passing of flag through DtlsTransportChannel. #
Total comments: 28
Patch Set 3 : Updates after feedback from Peter #
Total comments: 2
Patch Set 4 : Add CryptoOptions struct #Patch Set 5 : Rebased #
Total comments: 2
Patch Set 6 : Reference RFCs for key/salt lengths. #Patch Set 7 : Rebased #Patch Set 8 : Rebased #Patch Set 9 : Don't use hardcoded crypto suite names in channel_unittest.cc #Patch Set 10 : Fix failing SRTP-but-no-DTLS tests. #
Total comments: 26
Patch Set 11 : Feedback from Matt #
Total comments: 4
Patch Set 12 : More feedback from Matt #
Total comments: 1
Patch Set 13 : Rebased #
Total comments: 2
Patch Set 14 : Feedback from Peter #Patch Set 15 : Attempt to fix compilation on Windows. #Patch Set 16 : Fix compilation on tsan2. #
Total comments: 2
Patch Set 17 : Disable GCM if ENABLE_EXTERNAL_AUTH is defined. #
Created: 4 years, 5 months ago
Messages
Total messages: 63 (16 generated)
jbauch@webrtc.org changed reviewers: + juberti@webrtc.org, pthatcher@webrtc.org
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.
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?
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?
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.
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.
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.
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?
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?
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.
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 ==========
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.
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?
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 ==========
jbauch@webrtc.org changed reviewers: + mattdr@webrtc.org
Rebased + fix in newly introduced check for SRTP-but-non-DTLS cases. +matt for review
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@google.com changed reviewers: + mattdr@google.com
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
no lgtm ?
On 2016/05/06 22:37:44, pthatcher1 wrote: > no lgtm > > ? -lgtm ?
Sorry, new to Reitveld. not lgtm.
On 2016/05/06 22:37:56, pthatcher1 wrote: > On 2016/05/06 22:37:44, pthatcher1 wrote: > > no lgtm > > > > ? > > -lgtm > > ? not lgtm
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?
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.
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.
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.
@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...
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.
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.
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.
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.
Justin's on vacation this week. Go ahead and submit, and he can yell at me if I'm wrong. :)
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...
The CQ bit was checked by jbauch@webrtc.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
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)
A CL to fix the asan error pending here: https://codereview.webrtc.org/2129753002/
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?
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...
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.
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.
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.
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 ==========
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.
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 ==========
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.
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?
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
The CQ bit was checked by jbauch@webrtc.org
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.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by jbauch@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:310001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/cb56065c62a31d83919abcd4e343ea3dbe029e9f Cr-Commit-Position: refs/heads/master@{#13635} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
