Change WebRTC SslCipher to be exposed as number only.

Change WebRTC SslCipher to be exposed as number only.

This makes the SSL exposed as uint16_t which is the IANA value. GetRfcSslCipherName is introduced to handle the conversion to names from ID. IANA value will be used for UMA reporting. Names will still be used for WebRTC stats reporting.

For SRTP, currently it's still string internally but is reported as IANA number.

This is used by the ongoing CL https://codereview.chromium.org/1335023002.

BUG=523033
Committed: https://crrev.com/4fe3c9a77386598db9abd1f0d6983aefee9cc943
Cr-Commit-Position: refs/heads/master@{#10124}

[guoweis_webrtc, 2015-09-10 23:57:55]

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

[guoweis_webrtc, 2015-09-10 23:57:56]

PTAL.

[guoweis_webrtc, 2015-09-11 17:16:47]

On 2015/09/10 23:57:56, guoweis wrote:
> PTAL.

Seems that we at least use TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA as well.

Anyone knows where I could get a complete list of ciphers that we use? 

histograms.xml has this list
https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...
but it has hundreds of entries, probably overkill.

[guoweis_webrtc, 2015-09-11 17:58:06]

On 2015/09/11 17:16:47, guoweis wrote:
> On 2015/09/10 23:57:56, guoweis wrote:
> > PTAL.
> 
> Seems that we at least use TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA as well.
> 
> Anyone knows where I could get a complete list of ciphers that we use? 
> 
> histograms.xml has this list
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...
> but it has hundreds of entries, probably overkill.

When I tested this end to end with chrome by specifying --enable-webrtc-dtls12,
I could see that the ssl is using TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 which is
the default ssl cipher for SSL_PROTOCOL_DTLS_12. However, the srtp still uses
AES_CM_128_HMAC_SHA1_32, instead of AES_CM_128_HMAC_SHA1_80 which I thought it
should be.

Anything I did incorrect here?

[guoweis_webrtc, 2015-09-11 18:01:02]

On 2015/09/11 17:58:06, guoweis wrote:
> On 2015/09/11 17:16:47, guoweis wrote:
> > On 2015/09/10 23:57:56, guoweis wrote:
> > > PTAL.
> > 
> > Seems that we at least use TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA as well.
> > 
> > Anyone knows where I could get a complete list of ciphers that we use? 
> > 
> > histograms.xml has this list
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...
> > but it has hundreds of entries, probably overkill.
> 
> When I tested this end to end with chrome by specifying
--enable-webrtc-dtls12,
> I could see that the ssl is using TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 which
is
> the default ssl cipher for SSL_PROTOCOL_DTLS_12. However, the srtp still uses
> AES_CM_128_HMAC_SHA1_32, instead of AES_CM_128_HMAC_SHA1_80 which I thought it
> should be.
> 
> Anything I did incorrect here?

Btw, I was testing using apprtc.appspot.com

[joachim, 2015-09-14 09:59:02]

On 2015/09/11 17:58:06, guoweis wrote:
> On 2015/09/11 17:16:47, guoweis wrote:
> > On 2015/09/10 23:57:56, guoweis wrote:
> > > PTAL.
> > 
> > Seems that we at least use TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA as well.
> > 
> > Anyone knows where I could get a complete list of ciphers that we use? 
> > 
> > histograms.xml has this list
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...
> > but it has hundreds of entries, probably overkill.

As the cipher for DTLS is negotiated with the remote end, a lot of cipher suites
are possible.

For getting the rfc-compliant name from the internal (OpenSSL) suite id, there
is a mapping in
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

So using these suites as a starting point probably is a good idea.

> When I tested this end to end with chrome by specifying
--enable-webrtc-dtls12,
> I could see that the ssl is using TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 which
is
> the default ssl cipher for SSL_PROTOCOL_DTLS_12. However, the srtp still uses
> AES_CM_128_HMAC_SHA1_32, instead of AES_CM_128_HMAC_SHA1_80 which I thought it
> should be.
> 
> Anything I did incorrect here?

No, that's correct. Which cipher is used for SRTP is determined by the used
media. For audio-only, webrtc prefers AES_CM_128_HMAC_SHA1_32 (but supports
both), otherwise it only supports AES_CM_128_HMAC_SHA1_80.

[pthatcher1, 2015-09-16 03:52:30]

I agree with jbauch.  We should at least have the ones in there.

However, with a little macro we could probably through in all of the ones listed
in histograms.xml without that much trouble.

https://codereview.webrtc.org/1337673002/diff/80001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1337673002/diff/80001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:2067: // Since this is reported in
OnTransportCompleted
This comment doesn't make sense to me.

[pthatcher1, 2015-09-16 03:52:31]

I agree with jbauch.  We should at least have the ones in there.

However, with a little macro we could probably through in all of the ones listed
in histograms.xml without that much trouble.

[joachim, 2015-09-18 22:24:25]

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

https://codereview.webrtc.org/1337673002/diff/80001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection_unittest.cc:1354: 1);
Shouldn't this be EXPECT_EQ(1, ...) here and below?

[guoweis_webrtc, 2015-09-22 19:55:30]

Patchset #6 (id:100001) has been deleted

[guoweis_webrtc, 2015-09-22 19:58:38]

Patchset #6 (id:120001) has been deleted

[guoweis_webrtc, 2015-09-22 19:58:43]

Patchset #6 (id:140001) has been deleted

[guoweis_webrtc, 2015-09-22 19:59:39]

PTAL.

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

https://codereview.webrtc.org/1337673002/diff/80001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection_unittest.cc:1354: 1);
On 2015/09/18 22:24:25, joachim wrote:
> Shouldn't this be EXPECT_EQ(1, ...) here and below?

Done.

https://codereview.webrtc.org/1337673002/diff/80001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1337673002/diff/80001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:2067: // Since this is reported in
OnTransportCompleted
On 2015/09/16 03:52:30, pthatcher1 wrote:
> This comment doesn't make sense to me.

Done.

[Ryan Sleevi, 2015-09-22 21:42:15]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2015-09-22 21:42:15]

https://codereview.webrtc.org/1337673002/diff/160001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1337673002/diff/160001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:95: return
SrtpCipherType_AES_CM_128_HMAC_SHA1_80;
For what it's worth, this registration is also covered by IANA (
https://www.iana.org/assignments/sdp-security-descriptions/sdp-security-descr...
), but since SDP uses string identifiers, I suppose you can't escape needing to
map from string -> histogram value.

https://codereview.webrtc.org/1337673002/diff/160001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:101: if (cipher == "TLS_RSA_RC4_128_SHA")
As mentioned by davidben@, you should not design your interface to convert from
uint->string->enum->uint

Each of these ciphersuites has an IANA-assigned value (
http://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-param...
), so you can design your interface such that you expose this uint16_t value
from the socket, expose it as a *number* to the stats channel, and then
histogram that numeric value via the metrics observer.

[guoweis_webrtc, 2015-09-23 06:46:16]

One more question. 

At
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...,
there are many "default" ciphers for TLS1.0, 1.2, etc. Can they be found
anywhere in the boringssl library? Does WebRTC really need to specify them here?

https://codereview.webrtc.org/1337673002/diff/160001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1337673002/diff/160001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:95: return
SrtpCipherType_AES_CM_128_HMAC_SHA1_80;
On 2015/09/22 21:42:15, Ryan Sleevi wrote:
> For what it's worth, this registration is also covered by IANA (
>
https://www.iana.org/assignments/sdp-security-descriptions/sdp-security-descr...
> ), but since SDP uses string identifiers, I suppose you can't escape needing
to
> map from string -> histogram value.

Even though these are also defined in IANA, do you know why SRTP Crypto doesn't
have value defined as TLS cipher?

[guoweis_webrtc, 2015-09-23 06:47:57]

On 2015/09/23 06:46:16, guoweis wrote:
> One more question. 
> 
> At
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...,
> there are many "default" ciphers for TLS1.0, 1.2, etc. Can they be found
> anywhere in the boringssl library? Does WebRTC really need to specify them
here?
> 
>
https://codereview.webrtc.org/1337673002/diff/160001/talk/app/webrtc/webrtcse...
> File talk/app/webrtc/webrtcsession.cc (right):
> 
>
https://codereview.webrtc.org/1337673002/diff/160001/talk/app/webrtc/webrtcse...
> talk/app/webrtc/webrtcsession.cc:95: return
> SrtpCipherType_AES_CM_128_HMAC_SHA1_80;
> On 2015/09/22 21:42:15, Ryan Sleevi wrote:
> > For what it's worth, this registration is also covered by IANA (
> >
>
https://www.iana.org/assignments/sdp-security-descriptions/sdp-security-descr...
> > ), but since SDP uses string identifiers, I suppose you can't escape needing
> to
> > map from string -> histogram value.
> 
> Even though these are also defined in IANA, do you know why SRTP Crypto
doesn't
> have value defined as TLS cipher?

Never mind about my question for "default" cipher. It seems only existing for
WebRTC test purpose.

[guoweis_webrtc, 2015-09-24 05:39:27]

Patchset #7 (id:180001) has been deleted

[guoweis_webrtc, 2015-09-24 05:39:34]

Patchset #7 (id:200001) has been deleted

[juberti, 2015-09-24 13:41:14]

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

[juberti, 2015-09-24 13:41:15]

https://codereview.webrtc.org/1337673002/diff/220001/talk/app/webrtc/fakemetr...
File talk/app/webrtc/fakemetricsobserver.h (left):

https://codereview.webrtc.org/1337673002/diff/220001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.h:63: // This is a 2 dimension array. The
first index is the enum counter type. The
This comment could use some clarification. It's not obvious what this
vector/map<int/int> thing is trying to do.

https://codereview.webrtc.org/1337673002/diff/220001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1337673002/diff/220001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:92: if (cipher == "AES_CM_128_HMAC_SHA1_32")
is there some pre-existing constant somewhere we can use, instead of duplicating
it here?

https://codereview.webrtc.org/1337673002/diff/220001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:96: return SrtpCipherType_Unknown;
Usually we don't replicate 'Type' in enum values.

https://codereview.webrtc.org/1337673002/diff/220001/webrtc/base/opensslstrea...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/1337673002/diff/220001/webrtc/base/opensslstrea...
webrtc/base/opensslstreamadapter.cc:144: 0xC014,
The fact we need to get the id and name right here seems overly complex to me.
The tests should be able to get by with just the name.

https://codereview.webrtc.org/1337673002/diff/220001/webrtc/base/sslstreamada...
File webrtc/base/sslstreamadapter.cc (right):

https://codereview.webrtc.org/1337673002/diff/220001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.cc:72: return kNullSslCipher;
Using the null cipher here seems dangerous. We certainly don't want to ever
expect to use the null cipher!

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

https://codereview.webrtc.org/1337673002/diff/220001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:52: uint16_t ssl_id;
prefer plain |int| to a sized type.

https://codereview.webrtc.org/1337673002/diff/220001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:53: std::string rfc_name;
I would just call this |name|

[charlieloveeric3, 2015-09-24 13:44:27]

charlieloveeric3@gmail.com changed reviewers:
+ charlieloveeric3@gmail.com
- jbauch@webrtc.org, juberti@google.com, juberti@webrtc.org,
pthatcher@webrtc.org, rsleevi@chromium.org

[charlieloveeric3, 2015-09-24 13:44:27]

<groupId>org.slf4j</groupId>
    <artifactId>slf4j-api</artifactId>
</dependency>
<dependency>
    <groupId>ch.qos.logback</groupId>
    <artifactId>logback-core</artifactId>
</dependency>
<dependency>
    <groupId>ch.qos.logback</groupId>
    <artifactId>logback-classic</artifactId>
</dependency>
<dependency>
    <groupId>org.slf4j</groupId>
    <artifactId>jcl-over-slf4j</artifactId>
</dependency>

[guoweis_webrtc, 2015-09-24 16:42:20]

Patchset #8 (id:240001) has been deleted

[guoweis_webrtc, 2015-09-24 17:57:30]

Patchset #8 (id:260001) has been deleted

[guoweis_webrtc, 2015-09-24 17:59:14]

guoweis@webrtc.org changed reviewers:
+ juberti@webrtc.org, pthatcher@webrtc.org, rsleevi@chromium.org
- charlieloveeric3@gmail.com

[guoweis_webrtc, 2015-09-24 18:06:27]

Patchset #8 (id:280001) has been deleted

[guoweis_webrtc, 2015-09-24 18:27:13]

PTAL.

https://codereview.webrtc.org/1337673002/diff/220001/talk/app/webrtc/fakemetr...
File talk/app/webrtc/fakemetricsobserver.h (left):

https://codereview.webrtc.org/1337673002/diff/220001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.h:63: // This is a 2 dimension array. The
first index is the enum counter type. The
On 2015/09/24 13:41:15, juberti wrote:
> This comment could use some clarification. It's not obvious what this
> vector/map<int/int> thing is trying to do.

Done.

https://codereview.webrtc.org/1337673002/diff/220001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1337673002/diff/220001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:92: if (cipher == "AES_CM_128_HMAC_SHA1_32")
On 2015/09/24 13:41:15, juberti wrote:
> is there some pre-existing constant somewhere we can use, instead of
duplicating
> it here?

Done.

https://codereview.webrtc.org/1337673002/diff/220001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:96: return SrtpCipherType_Unknown;
On 2015/09/24 13:41:15, juberti wrote:
> Usually we don't replicate 'Type' in enum values.

Done.

https://codereview.webrtc.org/1337673002/diff/220001/webrtc/base/opensslstrea...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/1337673002/diff/220001/webrtc/base/opensslstrea...
webrtc/base/opensslstreamadapter.cc:144: 0xC014,
On 2015/09/24 13:41:15, juberti wrote:
> The fact we need to get the id and name right here seems overly complex to me.
> The tests should be able to get by with just the name.

Problem is that when doing the verification of UMA, it needs ID and since there
is no name to ID mapping, we have to provide the ID together.

https://codereview.webrtc.org/1337673002/diff/220001/webrtc/base/sslstreamada...
File webrtc/base/sslstreamadapter.cc (right):

https://codereview.webrtc.org/1337673002/diff/220001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.cc:72: return kNullSslCipher;
On 2015/09/24 13:41:15, juberti wrote:
> Using the null cipher here seems dangerous. We certainly don't want to ever
> expect to use the null cipher!

This function is only used by tests. We should just rename it as
GetDefaultSslCipherForTest. and/or changing the signature to return bool as
GetSslCipher().

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

https://codereview.webrtc.org/1337673002/diff/220001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:52: uint16_t ssl_id;
On 2015/09/24 13:41:15, juberti wrote:
> prefer plain |int| to a sized type.

Done.

https://codereview.webrtc.org/1337673002/diff/220001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:53: std::string rfc_name;
On 2015/09/24 13:41:15, juberti wrote:
> I would just call this |name|

Done.

[Ryan Sleevi, 2015-09-24 21:09:58]

rsleevi@chromium.org changed reviewers:
+ davidben@chromium.org

[Ryan Sleevi, 2015-09-24 21:09:58]

I don't believe this does what you want, and it also suffers from necessitating
static initializers, which is a no-go.

Is there a reason you cannot update the interface to consistently use the
integer value for rtc::SslCipher, and for the place where it needs to be a
string, have a function to convert the IANA-assigned value to a string
(although, to reiterate, you shouldn't be using the string at all)

https://codereview.chromium.org/1337673002/diff/320001/talk/app/webrtc/peerco...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.chromium.org/1337673002/diff/320001/talk/app/webrtc/peerco...
talk/app/webrtc/peerconnectioninterface.h:141: // the code gets into chromium.
This seems like a bad comment.

It discusses Chromium code in third-party code, which we strongly discourage. It
also describes concrete implementation usage in a header, which seems like a
layering violation.

https://codereview.chromium.org/1337673002/diff/320001/webrtc/base/opensslstr...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.chromium.org/1337673002/diff/320001/webrtc/base/opensslstr...
webrtc/base/opensslstreamadapter.cc:151: static const SslCipher
kDefaultSslCipher10 = {
Each of these is a static initializer, which is forbidden

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Static_and_Gl...

https://codereview.chromium.org/1337673002/diff/320001/webrtc/base/sslstreama...
File webrtc/base/sslstreamadapter.h (right):

https://codereview.chromium.org/1337673002/diff/320001/webrtc/base/sslstreama...
webrtc/base/sslstreamadapter.h:181: KeyType key_type = KT_DEFAULT);
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argum...

[juberti, 2015-09-24 21:37:31]

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

[juberti, 2015-09-24 21:37:32]

https://codereview.webrtc.org/1337673002/diff/220001/webrtc/base/opensslstrea...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/1337673002/diff/220001/webrtc/base/opensslstrea...
webrtc/base/opensslstreamadapter.cc:144: 0xC014,
On 2015/09/24 18:27:13, guoweis wrote:
> On 2015/09/24 13:41:15, juberti wrote:
> > The fact we need to get the id and name right here seems overly complex to
me.
> > The tests should be able to get by with just the name.
> 
> Problem is that when doing the verification of UMA, it needs ID and since
there
> is no name to ID mapping, we have to provide the ID together.

This still seems like a bad path. I could totally see us somehow mixing up the
wrong IDs and strings here.

https://codereview.webrtc.org/1337673002/diff/220001/webrtc/base/sslstreamada...
File webrtc/base/sslstreamadapter.cc (right):

https://codereview.webrtc.org/1337673002/diff/220001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.cc:72: return kNullSslCipher;
On 2015/09/24 18:27:13, guoweis wrote:
> On 2015/09/24 13:41:15, juberti wrote:
> > Using the null cipher here seems dangerous. We certainly don't want to ever
> > expect to use the null cipher!
> 
> This function is only used by tests. We should just rename it as
> GetDefaultSslCipherForTest. and/or changing the signature to return bool as
> GetSslCipher().

The function name wasn't the problem. See my comment about null vs invalid
elsewhere.

https://codereview.webrtc.org/1337673002/diff/320001/talk/app/webrtc/fakemetr...
File talk/app/webrtc/fakemetricsobserver.cc (right):

https://codereview.webrtc.org/1337673002/diff/320001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.cc:67: if (it != counters_[type].end()) {
prefer to return early on failure, not success

https://codereview.webrtc.org/1337673002/diff/320001/talk/app/webrtc/fakemetr...
File talk/app/webrtc/fakemetricsobserver.h (right):

https://codereview.webrtc.org/1337673002/diff/320001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.h:61: // is to work with sparse enum type
which might span a large number space.
An example of how this works would also be helpful. Is there any limit to the
size of the array?

https://codereview.webrtc.org/1337673002/diff/320001/talk/app/webrtc/umametri...
File talk/app/webrtc/umametrics.h (right):

https://codereview.webrtc.org/1337673002/diff/320001/talk/app/webrtc/umametri...
talk/app/webrtc/umametrics.h:126: // The same list as SrtpCipherMap from
opensslstsreamadapter.cc. Must be kept
We should use the IDs from https://tools.ietf.org/html/rfc5764#section-4.1.2
instead. But this should go down into srtpfilter.h or similar.

https://codereview.webrtc.org/1337673002/diff/320001/talk/app/webrtc/umametri...
talk/app/webrtc/umametrics.h:132: SrtpCipher_Max,
Why do we have these _Max values?

https://codereview.webrtc.org/1337673002/diff/320001/webrtc/base/opensslstrea...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/1337673002/diff/320001/webrtc/base/opensslstrea...
webrtc/base/opensslstreamadapter.cc:146: static const SslCipher kNullSslCipher =
{0, ""};
Null cipher is a thing, and so this is confusing. You want something like
kInvalidSslCipher.

https://codereview.webrtc.org/1337673002/diff/320001/webrtc/base/sslstrings.h
File webrtc/base/sslstrings.h (right):

https://codereview.webrtc.org/1337673002/diff/320001/webrtc/base/sslstrings.h...
webrtc/base/sslstrings.h:15: extern const char AES_CM_128_HMAC_SHA1_80_NAME[];
We already have these defs in srtpfilter.h. Do they need to move down? If so,
they should probably go into sslstreamadapter.h

https://codereview.webrtc.org/1337673002/diff/320001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportchannel.h (left):

https://codereview.webrtc.org/1337673002/diff/320001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportchannel.h:109: virtual bool GetSrtpCipher(std::string*
cipher) = 0;
As discussed in the other thread, I think we need to emit ID and name out of
these APIs as well.

[guoweis_webrtc, 2015-09-25 16:54:29]

Patchset #8 (id:300001) has been deleted

[guoweis_webrtc, 2015-09-25 17:44:00]

Patchset #9 (id:340001) has been deleted

[guoweis_webrtc, 2015-09-25 17:54:19]

Patchset #9 (id:360001) has been deleted

[guoweis_webrtc, 2015-09-25 18:16:46]

Patchset #9 (id:380001) has been deleted

[guoweis_webrtc, 2015-09-25 18:16:54]

Patchset #9 (id:400001) has been deleted

[guoweis_webrtc, 2015-09-25 18:19:38]

Patchset #9 (id:360002) has been deleted

[guoweis_webrtc, 2015-09-25 18:30:32]

PTAL.

https://codereview.webrtc.org/1337673002/diff/320001/talk/app/webrtc/fakemetr...
File talk/app/webrtc/fakemetricsobserver.cc (right):

https://codereview.webrtc.org/1337673002/diff/320001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.cc:67: if (it != counters_[type].end()) {
On 2015/09/24 21:37:32, juberti wrote:
> prefer to return early on failure, not success

Done.

https://codereview.webrtc.org/1337673002/diff/320001/talk/app/webrtc/fakemetr...
File talk/app/webrtc/fakemetricsobserver.h (right):

https://codereview.webrtc.org/1337673002/diff/320001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.h:61: // is to work with sparse enum type
which might span a large number space.
On 2015/09/24 21:37:32, juberti wrote:
> An example of how this works would also be helpful. Is there any limit to the
> size of the array?

Done.

https://codereview.webrtc.org/1337673002/diff/320001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1337673002/diff/320001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface.h:141: // the code gets into chromium.
On 2015/09/24 21:09:58, Ryan Sleevi wrote:
> This seems like a bad comment.
> 
> It discusses Chromium code in third-party code, which we strongly discourage.
It
> also describes concrete implementation usage in a header, which seems like a
> layering violation.

Done.

https://codereview.webrtc.org/1337673002/diff/320001/talk/app/webrtc/umametri...
File talk/app/webrtc/umametrics.h (right):

https://codereview.webrtc.org/1337673002/diff/320001/talk/app/webrtc/umametri...
talk/app/webrtc/umametrics.h:126: // The same list as SrtpCipherMap from
opensslstsreamadapter.cc. Must be kept
On 2015/09/24 21:37:32, juberti wrote:
> We should use the IDs from https://tools.ietf.org/html/rfc5764#section-4.1.2
> instead. But this should go down into srtpfilter.h or similar.

Done.

https://codereview.webrtc.org/1337673002/diff/320001/talk/app/webrtc/umametri...
talk/app/webrtc/umametrics.h:132: SrtpCipher_Max,
On 2015/09/24 21:37:32, juberti wrote:
> Why do we have these _Max values?

Done.

https://codereview.webrtc.org/1337673002/diff/320001/webrtc/base/opensslstrea...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/1337673002/diff/320001/webrtc/base/opensslstrea...
webrtc/base/opensslstreamadapter.cc:146: static const SslCipher kNullSslCipher =
{0, ""};
On 2015/09/24 21:37:32, juberti wrote:
> Null cipher is a thing, and so this is confusing. You want something like
> kInvalidSslCipher.

Done.

https://codereview.webrtc.org/1337673002/diff/320001/webrtc/base/opensslstrea...
webrtc/base/opensslstreamadapter.cc:151: static const SslCipher
kDefaultSslCipher10 = {
On 2015/09/24 21:09:58, Ryan Sleevi wrote:
> Each of these is a static initializer, which is forbidden
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Static_and_Gl...

Done.

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

https://codereview.webrtc.org/1337673002/diff/320001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:181: KeyType key_type = KT_DEFAULT);
On 2015/09/24 21:09:58, Ryan Sleevi wrote:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argum...

Done.

https://codereview.webrtc.org/1337673002/diff/320001/webrtc/base/sslstrings.h
File webrtc/base/sslstrings.h (right):

https://codereview.webrtc.org/1337673002/diff/320001/webrtc/base/sslstrings.h...
webrtc/base/sslstrings.h:15: extern const char AES_CM_128_HMAC_SHA1_80_NAME[];
On 2015/09/24 21:37:32, juberti wrote:
> We already have these defs in srtpfilter.h. Do they need to move down? If so,
> they should probably go into sslstreamadapter.h

Done.

https://codereview.webrtc.org/1337673002/diff/320001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportchannel.h (left):

https://codereview.webrtc.org/1337673002/diff/320001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportchannel.h:109: virtual bool GetSrtpCipher(std::string*
cipher) = 0;
On 2015/09/24 21:37:32, juberti wrote:
> As discussed in the other thread, I think we need to emit ID and name out of
> these APIs as well.

Sounds like that we're heading toward ID only.

[davidben_webrtc, 2015-09-25 19:23:12]

davidben@webrtc.org changed reviewers:
+ davidben@webrtc.org

[davidben_webrtc, 2015-09-25 19:23:13]

lgtm with comments, though presumably you want to wait for a WebRTC review too.

https://codereview.webrtc.org/1337673002/diff/450001/talk/app/webrtc/fakemetr...
File talk/app/webrtc/fakemetricsobserver.cc (right):

https://codereview.webrtc.org/1337673002/diff/450001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.cc:71: ;
Stay semicolon?

https://codereview.webrtc.org/1337673002/diff/450001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1337673002/diff/450001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface.h:139: // TODO(guoweis): Remove the
implmenetation once the dependency's interface
Nit: implementation :-)

https://codereview.webrtc.org/1337673002/diff/450001/webrtc/base/opensslstrea...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/1337673002/diff/450001/webrtc/base/opensslstrea...
webrtc/base/opensslstreamadapter.cc:341: const std::string
OpenSSLStreamAdapter::GetRfcSslCipherName(uint16_t cipher) {
(Ditto on const std::string)

https://codereview.webrtc.org/1337673002/diff/450001/webrtc/base/opensslstrea...
webrtc/base/opensslstreamadapter.cc:363: bool
OpenSSLStreamAdapter::GetSslCipher(uint16* cipher) {
Nit: uint16_t.

https://codereview.webrtc.org/1337673002/diff/450001/webrtc/base/opensslstrea...
File webrtc/base/opensslstreamadapter.h (right):

https://codereview.webrtc.org/1337673002/diff/450001/webrtc/base/opensslstrea...
webrtc/base/opensslstreamadapter.h:90: static const std::string
GetRfcSslCipherName(uint16_t cipher);
(Ditto on const std::string)

https://codereview.webrtc.org/1337673002/diff/450001/webrtc/base/sslstreamada...
File webrtc/base/sslstreamadapter.cc (right):

https://codereview.webrtc.org/1337673002/diff/450001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.cc:101: const std::string
SSLStreamAdapter::GetRfcSslCipherName(uint16_t cipher) {
(Ditto on const std::string)

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

https://codereview.webrtc.org/1337673002/diff/450001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:194: static const std::string
GetRfcSslCipherName(uint16_t cipher);
const std::string -> std::string

const return types don't really do anything useful. (const references do, but I
don't think you want a const reference here.)

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

https://codereview.webrtc.org/1337673002/diff/450001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter_unittest.cc:423: return ret;
Nit: Any reason not to just write this as:

  if (client)
    return client_ssl_->GetSslCipher(retval);
  else
    return server_ssl_->GetSslCipher(retval);

[guoweis_webrtc, 2015-09-25 21:20:26]

Thanks. Issues addressed.

juberti/pthatcher, PTAL.

[guoweis_webrtc, 2015-09-28 17:13:42]

On 2015/09/25 21:20:26, guoweis wrote:
> Thanks. Issues addressed.
> 
> juberti/pthatcher, PTAL.

juberti/pthatcher, Ping.

[Ryan Sleevi, 2015-09-28 17:24:17]

At this point, there's a lot going on in this CL, so I don't want to make it
even more complicated than it is.

Most of my suggestions relate to comment and naming nits, so hopefully those are
easier to fix. I also suggest a number of design considerations - I think you
should use your best judgement as whether to fix these now or defer for
follow-up CLs.

Given the subtlety of the relationships between the SDP crypto-suite and the
DTLS-SRTP protection profile and the [D]TLS ciphersuite, when possible, it's
good to be clear on the documentation, without detailing too much of the
implementation. That's where most of the comment nits go.

Otherwise, LGTM

https://codereview.webrtc.org/1337673002/diff/470001/talk/app/webrtc/statscol...
File talk/app/webrtc/statscollector_unittest.cc (right):

https://codereview.webrtc.org/1337673002/diff/470001/talk/app/webrtc/statscol...
talk/app/webrtc/statscollector_unittest.cc:654: channel_stats.ssl_cipher =
0xC014;
Perhaps a comment explaining this magic value.

https://codereview.webrtc.org/1337673002/diff/470001/talk/session/media/chann...
File talk/session/media/channel_unittest.cc (right):

https://codereview.webrtc.org/1337673002/diff/470001/talk/session/media/chann...
talk/session/media/channel_unittest.cc:1775: "inline:" +
rtc::CreateRandomString(40), ""));
nit: std::string() will be slightly more efficient here...

https://codereview.webrtc.org/1337673002/diff/470001/talk/session/media/chann...
talk/session/media/channel_unittest.cc:1842: "inline:" +
rtc::CreateRandomString(40), ""));
same throughout

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/opensslstrea...
File webrtc/base/opensslstreamadapter.h (right):

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/opensslstrea...
webrtc/base/opensslstreamadapter.h:90: static std::string
GetRfcSslCipherName(uint16_t cipher);
Does this need to be exposed as a public static? Couldn't you keep it class
local? Alternatively, should it be parse of the base Stream adapter?

I mention this because any caller ends up tightly coupled to the
OpenSSLStreamAdapter (by virtue of needing to call the static method), and that
seems to defeat the encapsulation goal of having an SSLStreamAdapter.

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

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:22: // This follows
https://tools.ietf.org/html/rfc5764#section-4.1.2.
// DTLS-SRTP protection profiles, as described in
https://tools.ietf.org/html/rfc5764#section-4.1.2

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:23: enum SrtpCipherType {
Is this really a Cipher type? According to the RFC, it's a protection profile,
which itself matches an SDP Crypto Suite.

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:32: // there is existing definition already.
This comment is quite confusing. There's lots of RFCs at play, as I called out
earlier.

Perhaps this might be better stated

// Returns the DTLS-SRTP protection profile ID, as defined in
https://tools.ietf.org/html/rfc5764#section-4.1.2,
// for the given SDP Crypto-suite, as defined in
https://tools.ietf.org/html/rfc4568#section-6.2

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:36: // in applications (voice) where the
additional bandwidth may be significant.
The comment starting with "typically" seems to be describing too much of the
implementation

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:194: static std::string
GetRfcSslCipherName(uint16_t cipher);
DESIGN nit: This is not really for this CL, but this sort of design makes things
really weird. It 'seems' as if the idea of the SSLStreamAdapter base class is
meant to make inheritance easier and abstract away the platform implementation,
but as seen in the .cc file, you end up coupling tightly to the derived classes
by virtue of needing to use #ifdefs.

Presumably you'd want this to be non-static - indeed, having static methods on a
virtual base class really strikes as weird design and violation of
encapsulation.

This stood out in this CL, but I'd suggest it's more something to consider for
the future rather than something that requires immediate attention.

[pthatcher1, 2015-09-29 22:25:17]

https://codereview.webrtc.org/1337673002/diff/470001/talk/app/webrtc/fakemetr...
File talk/app/webrtc/fakemetricsobserver.h (right):

https://codereview.webrtc.org/1337673002/diff/470001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.h:61: // comes to sparse enum type, like SSL
cipher IANA registry.
"spare enum type" => "sparse enum types" ?
"like SSL cipher IANA registry" => "like the SSL ciphers in the IANA registry" ?

https://codereview.webrtc.org/1337673002/diff/470001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1337673002/diff/470001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface.h:138: // This is used to handle sparse
counters like SSL cipher suite.
cipher suite => cipher suites

https://codereview.webrtc.org/1337673002/diff/470001/talk/app/webrtc/statscol...
File talk/app/webrtc/statscollector_unittest.cc (right):

https://codereview.webrtc.org/1337673002/diff/470001/talk/app/webrtc/statscol...
talk/app/webrtc/statscollector_unittest.cc:654: channel_stats.ssl_cipher =
0xC014;
On 2015/09/28 17:24:17, Ryan Sleevi wrote:
> Perhaps a comment explaining this magic value.

+1 

Or better yet, make a named constant and then use that.

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/opensslstrea...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/opensslstrea...
webrtc/base/opensslstreamadapter.cc:147: 0xC00A;  //
TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA
Can we have a some named constants rather than magic numbers + comments?

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
File webrtc/base/sslstreamadapter.cc (right):

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.cc:33: const char CS_AES_CM_128_HMAC_SHA1_32[] =
"AES_CM_128_HMAC_SHA1_32";
Is this used anywhere that isn't PeerConnection or SDP specific?  If not, can we
move it up into the PeerConnection/SDP layer of the code?

That goes for the function below as well.

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

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:23: enum SrtpCipherType {
On 2015/09/28 17:24:17, Ryan Sleevi wrote:
> Is this really a Cipher type? According to the RFC, it's a protection profile,
> which itself matches an SDP Crypto Suite.

In https://tools.ietf.org/html/rfc4568#section-6.2, it's called "SRTP crypto
suites".  I think it would make sense to call the enum "SrtpCryptoSuite" (no
need for Type on the end) with prefix "SRTP_".  Then the names would match the
constants in https://tools.ietf.org/html/rfc5764#section-4.1.2.

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:28: SrtpCipher_NULL_HMAC_SHA1_32 = 6,
Do we actually use these anywhere?  If not, can we remove them?

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:32: // there is existing definition already.
On 2015/09/28 17:24:17, Ryan Sleevi wrote:
> This comment is quite confusing. There's lots of RFCs at play, as I called out
> earlier.
> 
> Perhaps this might be better stated
> 
> // Returns the DTLS-SRTP protection profile ID, as defined in
> https://tools.ietf.org/html/rfc5764#section-4.1.2,
> // for the given SDP Crypto-suite, as defined in
> https://tools.ietf.org/html/rfc4568#section-6.2 

I agree we should mention both RFCs in the comment.  But 4568 calls them "SRTP
crypto suites", not "SDP crypto suites", which happens to be expressed in SDP as
an SDP attribute called the "SDP crypto attribute".

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:36: // in applications (voice) where the
additional bandwidth may be significant.
On 2015/09/28 17:24:17, Ryan Sleevi wrote:
> The comment starting with "typically" seems to be describing too much of the
> implementation

I think it's more saying how these are typically used in the industry.  Plus, I
think he just moved this comment from the previous location.  I think it's still
accurate.

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.h:114: bool GetSrtpCipher(std::string*
cipher) override { return false; }
If we're going to change the SslCipher to an int rather than a string, we should
probably do the same for SrtpCipher.

If it's not critical for the DTLS 1.2 work you're trying to land, can you put a
TODO and then have a follow-up CL after you land this one?

[guoweis_webrtc, 2015-09-30 04:02:30]

Patchset #12 (id:490001) has been deleted

[guoweis_webrtc, 2015-09-30 04:09:47]

https://codereview.webrtc.org/1337673002/diff/470001/talk/app/webrtc/fakemetr...
File talk/app/webrtc/fakemetricsobserver.h (right):

https://codereview.webrtc.org/1337673002/diff/470001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.h:61: // comes to sparse enum type, like SSL
cipher IANA registry.
On 2015/09/29 22:25:16, pthatcher1 wrote:
> "spare enum type" => "sparse enum types" ?
> "like SSL cipher IANA registry" => "like the SSL ciphers in the IANA registry"
?

Done.

https://codereview.webrtc.org/1337673002/diff/470001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1337673002/diff/470001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface.h:138: // This is used to handle sparse
counters like SSL cipher suite.
On 2015/09/29 22:25:16, pthatcher1 wrote:
> cipher suite => cipher suites

Done.

https://codereview.webrtc.org/1337673002/diff/470001/talk/app/webrtc/statscol...
File talk/app/webrtc/statscollector_unittest.cc (right):

https://codereview.webrtc.org/1337673002/diff/470001/talk/app/webrtc/statscol...
talk/app/webrtc/statscollector_unittest.cc:654: channel_stats.ssl_cipher =
0xC014;
On 2015/09/29 22:25:16, pthatcher1 wrote:
> On 2015/09/28 17:24:17, Ryan Sleevi wrote:
> > Perhaps a comment explaining this magic value.
> 
> +1 
> 
> Or better yet, make a named constant and then use that.

Done.

https://codereview.webrtc.org/1337673002/diff/470001/talk/session/media/chann...
File talk/session/media/channel_unittest.cc (right):

https://codereview.webrtc.org/1337673002/diff/470001/talk/session/media/chann...
talk/session/media/channel_unittest.cc:1775: "inline:" +
rtc::CreateRandomString(40), ""));
On 2015/09/28 17:24:17, Ryan Sleevi wrote:
> nit: std::string() will be slightly more efficient here...

Done.

https://codereview.webrtc.org/1337673002/diff/470001/talk/session/media/chann...
talk/session/media/channel_unittest.cc:1842: "inline:" +
rtc::CreateRandomString(40), ""));
On 2015/09/28 17:24:17, Ryan Sleevi wrote:
> same throughout

Done.

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/opensslstrea...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/opensslstrea...
webrtc/base/opensslstreamadapter.cc:147: 0xC00A;  //
TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA
On 2015/09/29 22:25:16, pthatcher1 wrote:
> Can we have a some named constants rather than magic numbers + comments?

Do you mean simply change "static unit16_t" to #define?

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/opensslstrea...
File webrtc/base/opensslstreamadapter.h (right):

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/opensslstrea...
webrtc/base/opensslstreamadapter.h:90: static std::string
GetRfcSslCipherName(uint16_t cipher);
On 2015/09/28 17:24:17, Ryan Sleevi wrote:
> Does this need to be exposed as a public static? Couldn't you keep it class
> local? Alternatively, should it be parse of the base Stream adapter?
> 
> I mention this because any caller ends up tightly coupled to the
> OpenSSLStreamAdapter (by virtue of needing to call the static method), and
that
> seems to defeat the encapsulation goal of having an SSLStreamAdapter.

I agree that static method doesn't make a lot of sense here. But it's not true
that any callers have to be coupled with opensslstreamadapater. SSLStreamAdapter
forwards its own static class method over here. Yes, I know, another not so
great design here.

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
File webrtc/base/sslstreamadapter.cc (right):

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.cc:33: const char CS_AES_CM_128_HMAC_SHA1_32[] =
"AES_CM_128_HMAC_SHA1_32";
On 2015/09/29 22:25:16, pthatcher1 wrote:
> Is this used anywhere that isn't PeerConnection or SDP specific?  If not, can
we
> move it up into the PeerConnection/SDP layer of the code?
> 
> That goes for the function below as well.

If I don't misunderstand, I was following Justin's comment in previous review.
talk/ uses a lot of them in media sessions.

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

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:22: // This follows
https://tools.ietf.org/html/rfc5764#section-4.1.2.
On 2015/09/28 17:24:17, Ryan Sleevi wrote:
> // DTLS-SRTP protection profiles, as described in
> https://tools.ietf.org/html/rfc5764#section-4.1.2

Done.

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:23: enum SrtpCipherType {
On 2015/09/29 22:25:17, pthatcher1 wrote:
> On 2015/09/28 17:24:17, Ryan Sleevi wrote:
> > Is this really a Cipher type? According to the RFC, it's a protection
profile,
> > which itself matches an SDP Crypto Suite.
> 
> In https://tools.ietf.org/html/rfc4568#section-6.2, it's called "SRTP crypto
> suites".  I think it would make sense to call the enum "SrtpCryptoSuite" (no
> need for Type on the end) with prefix "SRTP_".  Then the names would match the
> constants in https://tools.ietf.org/html/rfc5764#section-4.1.2.

Yes, it should be named cryptos. However, it's consistently misused as Cipher
across webrtc code. I will put a TODO there.

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:28: SrtpCipher_NULL_HMAC_SHA1_32 = 6,
On 2015/09/29 22:25:16, pthatcher1 wrote:
> Do we actually use these anywhere?  If not, can we remove them?

Done.

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:32: // there is existing definition already.
On 2015/09/28 17:24:17, Ryan Sleevi wrote:
> This comment is quite confusing. There's lots of RFCs at play, as I called out
> earlier.
> 
> Perhaps this might be better stated
> 
> // Returns the DTLS-SRTP protection profile ID, as defined in
> https://tools.ietf.org/html/rfc5764#section-4.1.2,
> // for the given SDP Crypto-suite, as defined in
> https://tools.ietf.org/html/rfc4568#section-6.2 

Done.

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:194: static std::string
GetRfcSslCipherName(uint16_t cipher);
On 2015/09/28 17:24:17, Ryan Sleevi wrote:
> DESIGN nit: This is not really for this CL, but this sort of design makes
things
> really weird. It 'seems' as if the idea of the SSLStreamAdapter base class is
> meant to make inheritance easier and abstract away the platform
implementation,
> but as seen in the .cc file, you end up coupling tightly to the derived
classes
> by virtue of needing to use #ifdefs.
> 
> Presumably you'd want this to be non-static - indeed, having static methods on
a
> virtual base class really strikes as weird design and violation of
> encapsulation.
> 
> This stood out in this CL, but I'd suggest it's more something to consider for
> the future rather than something that requires immediate attention.

added a TODO.

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/p2p/base/p2ptrans...
File webrtc/p2p/base/p2ptransportchannel.h (right):

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/p2p/base/p2ptrans...
webrtc/p2p/base/p2ptransportchannel.h:114: bool GetSrtpCipher(std::string*
cipher) override { return false; }
Changing srtp is probably more work. GetSrtpCipher is used in for example
talk/session/media/channel.h as string already.

BoringSSL defines these integers already so it's easy for us to reuse instead of
redefining our own. I don't know if it's the case for SRTP. I could be wrong but
I don't think it's the case. It's just where we want the integer version
ourselves internally. I agree this is more consistent but not sure how much it
buys for us.

[pthatcher1, 2015-09-30 05:46:22]

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/opensslstrea...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/opensslstrea...
webrtc/base/opensslstreamadapter.cc:147: 0xC00A;  //
TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA
On 2015/09/30 04:09:46, guoweis wrote:
> On 2015/09/29 22:25:16, pthatcher1 wrote:
> > Can we have a some named constants rather than magic numbers + comments?
> 
> Do you mean simply change "static unit16_t" to #define? 

I mean like

static uint16_t kDefaultSslCipher10 = TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA;
static uint16_t kDefaultSslEcCipher10 = TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA;
...

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
File webrtc/base/sslstreamadapter.cc (right):

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.cc:33: const char CS_AES_CM_128_HMAC_SHA1_32[] =
"AES_CM_128_HMAC_SHA1_32";
On 2015/09/30 04:09:47, guoweis wrote:
> On 2015/09/29 22:25:16, pthatcher1 wrote:
> > Is this used anywhere that isn't PeerConnection or SDP specific?  If not,
can
> we
> > move it up into the PeerConnection/SDP layer of the code?
> > 
> > That goes for the function below as well.
> 
> If I don't misunderstand, I was following Justin's comment in previous review.
> talk/ uses a lot of them in media sessions.

The constant in abstract form (which could be an int or enum) is used in two
places outside of tests:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...

We could convert both of those to use int/enum values rather than strings.


Only in SDP do we convert to/from a string:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...


So, we could convert to/from a string just in the SDP layer and use an enum/int
everywhere else.

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

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:23: enum SrtpCipherType {
On 2015/09/30 04:09:47, guoweis wrote:
> On 2015/09/29 22:25:17, pthatcher1 wrote:
> > On 2015/09/28 17:24:17, Ryan Sleevi wrote:
> > > Is this really a Cipher type? According to the RFC, it's a protection
> profile,
> > > which itself matches an SDP Crypto Suite.
> > 
> > In https://tools.ietf.org/html/rfc4568#section-6.2, it's called "SRTP crypto
> > suites".  I think it would make sense to call the enum "SrtpCryptoSuite" (no
> > need for Type on the end) with prefix "SRTP_".  Then the names would match
the
> > constants in https://tools.ietf.org/html/rfc5764#section-4.1.2.
> 
> Yes, it should be named cryptos. However, it's consistently misused as Cipher
> across webrtc code. I will put a TODO there.

Can you at least name the new enum type you are adding to what we want it to be
long-term?  I still would prefer SrtpCryptoSuite with SRTP_ as the prefix rather
than SrtpCipher_.

[guoweis_webrtc, 2015-09-30 17:40:25]

Patchset #13 (id:530001) has been deleted

[guoweis_webrtc, 2015-09-30 17:41:55]

PTAL.

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/opensslstrea...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/opensslstrea...
webrtc/base/opensslstreamadapter.cc:147: 0xC00A;  //
TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA
On 2015/09/30 05:46:22, pthatcher1 wrote:
> On 2015/09/30 04:09:46, guoweis wrote:
> > On 2015/09/29 22:25:16, pthatcher1 wrote:
> > > Can we have a some named constants rather than magic numbers + comments?
> > 
> > Do you mean simply change "static unit16_t" to #define? 
> 
> I mean like
> 
> static uint16_t kDefaultSslCipher10 = TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA;
> static uint16_t kDefaultSslEcCipher10 = TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA;
> ...
> 
> 

Done.

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
File webrtc/base/sslstreamadapter.cc (right):

https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.cc:33: const char CS_AES_CM_128_HMAC_SHA1_32[] =
"AES_CM_128_HMAC_SHA1_32";
On 2015/09/30 05:46:22, pthatcher1 wrote:
> On 2015/09/30 04:09:47, guoweis wrote:
> > On 2015/09/29 22:25:16, pthatcher1 wrote:
> > > Is this used anywhere that isn't PeerConnection or SDP specific?  If not,
> can
> > we
> > > move it up into the PeerConnection/SDP layer of the code?
> > > 
> > > That goes for the function below as well.
> > 
> > If I don't misunderstand, I was following Justin's comment in previous
review.
> > talk/ uses a lot of them in media sessions.
> 
> The constant in abstract form (which could be an int or enum) is used in two
> places outside of tests:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...
> 
> We could convert both of those to use int/enum values rather than strings.
> 
> 
> Only in SDP do we convert to/from a string:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...
> 
> 
> So, we could convert to/from a string just in the SDP layer and use an
enum/int
> everywhere else.

This makes sense. I'd like to do this in a separate CL.

[pthatcher1, 2015-09-30 18:39:35]

https://codereview.webrtc.org/1337673002/diff/570001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/1337673002/diff/570001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection_unittest.cc:1345:
EXPECT_EQ_WAIT(rtc::SSLStreamAdapter::GetSslCipherRfcNameById(
I think a good pair of names for these would be:

GetSslCipherName
GetSslCipherFromName

https://codereview.webrtc.org/1337673002/diff/570001/talk/app/webrtc/statscol...
File talk/app/webrtc/statscollector_unittest.cc (right):

https://codereview.webrtc.org/1337673002/diff/570001/talk/app/webrtc/statscol...
talk/app/webrtc/statscollector_unittest.cc:63: const uint16_t
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA = 0xC014;
Can you put a comment explaining where the value comes from?

https://codereview.webrtc.org/1337673002/diff/570001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1337673002/diff/570001/talk/session/media/chann...
talk/session/media/channel.cc:821: GetSupportedDefaultCryptoSuites(&ciphers);
And this should be GetDefaultSrtpCryptoSuiteNames

https://codereview.webrtc.org/1337673002/diff/570001/talk/session/media/chann...
File talk/session/media/channel.h (right):

https://codereview.webrtc.org/1337673002/diff/570001/talk/session/media/chann...
talk/session/media/channel.h:288: std::vector<std::string>* ciphers) const = 0;
I think a good name here would be GetSrtpCryptoSuiteNames

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

https://codereview.webrtc.org/1337673002/diff/570001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:37: uint16_t GetSrtpCipherByRfcName(const
std::string& cipher_rfc_name);
I think a good name here would be GetSrtpCrypoSuiteFromName

[guoweis_webrtc, 2015-09-30 19:49:13]

Patchset #15 (id:590001) has been deleted

[guoweis_webrtc, 2015-09-30 20:09:00]

Patchset #15 (id:610001) has been deleted

[guoweis_webrtc, 2015-09-30 20:14:10]

Patchset #15 (id:630001) has been deleted

[guoweis_webrtc, 2015-09-30 20:14:21]

PTAL.

https://codereview.webrtc.org/1337673002/diff/570001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/1337673002/diff/570001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection_unittest.cc:1345:
EXPECT_EQ_WAIT(rtc::SSLStreamAdapter::GetSslCipherRfcNameById(
On 2015/09/30 18:39:35, pthatcher1 wrote:
> I think a good pair of names for these would be:
> 
> GetSslCipherName
> GetSslCipherFromName

There are 2 types of functions for SSL
1. GetSslCipher() ==> get the current cipher on the channel as an id
2. GetSslCipherName(id) ==> translate from id to name

There are 2 types of functions for SRTP
1. GetSrtpCryptoSuite(str*) ==> get the current crypto on the channel as an
string
2. GetSrtpCryptoSuiteFromName(str) ==> translate from name to id

https://codereview.webrtc.org/1337673002/diff/570001/talk/app/webrtc/statscol...
File talk/app/webrtc/statscollector_unittest.cc (right):

https://codereview.webrtc.org/1337673002/diff/570001/talk/app/webrtc/statscol...
talk/app/webrtc/statscollector_unittest.cc:63: const uint16_t
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA = 0xC014;
On 2015/09/30 18:39:35, pthatcher1 wrote:
> Can you put a comment explaining where the value comes from?

Done.

https://codereview.webrtc.org/1337673002/diff/570001/talk/session/media/chann...
File talk/session/media/channel.cc (right):

https://codereview.webrtc.org/1337673002/diff/570001/talk/session/media/chann...
talk/session/media/channel.cc:821: GetSupportedDefaultCryptoSuites(&ciphers);
On 2015/09/30 18:39:35, pthatcher1 wrote:
> And this should be GetDefaultSrtpCryptoSuiteNames

Done.

https://codereview.webrtc.org/1337673002/diff/570001/talk/session/media/chann...
File talk/session/media/channel.h (right):

https://codereview.webrtc.org/1337673002/diff/570001/talk/session/media/chann...
talk/session/media/channel.h:288: std::vector<std::string>* ciphers) const = 0;
On 2015/09/30 18:39:35, pthatcher1 wrote:
> I think a good name here would be GetSrtpCryptoSuiteNames

Done.

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

https://codereview.webrtc.org/1337673002/diff/570001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter.h:37: uint16_t GetSrtpCipherByRfcName(const
std::string& cipher_rfc_name);
On 2015/09/30 18:39:35, pthatcher1 wrote:
> I think a good name here would be GetSrtpCrypoSuiteFromName

Done.

[pthatcher1, 2015-09-30 20:35:50]

lgtm

[guoweis_webrtc, 2015-09-30 20:37:41]

The CQ bit was checked by guoweis@webrtc.org

[guoweis_webrtc, 2015-09-30 20:37:42]

The patchset sent to the CQ was uploaded after l-g-t-m from davidben@webrtc.org,
rsleevi@chromium.org
Link to the patchset: https://codereview.webrtc.org/1337673002/#ps650001 (title:
" ")

[commit-bot: I haz the power, 2015-09-30 20:37:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1337673002/650001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1337673002/650001

[commit-bot: I haz the power, 2015-09-30 20:38:59]

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

[commit-bot: I haz the power, 2015-09-30 20:39:00]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...)
  android_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_dbg/builds/5977)
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/4680)
  linux_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...)
  linux_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_rel/builds/5837)
  linux_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/9779)
  linux_tsan2 on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/7070)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/210)
  mac_compile_x64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_x64_dbg/bui...)
  mac_x64_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_dbg/builds/4343)

[guoweis_webrtc, 2015-09-30 20:46:30]

The CQ bit was checked by guoweis@webrtc.org

[guoweis_webrtc, 2015-09-30 20:46:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
rsleevi@chromium.org, pthatcher@webrtc.org, davidben@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1337673002/#ps670001 (title:
" ")

[commit-bot: I haz the power, 2015-09-30 20:46:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1337673002/670001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1337673002/670001

[commit-bot: I haz the power, 2015-09-30 20:49:28]

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

[commit-bot: I haz the power, 2015-09-30 20:49:29]

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

[guoweis_webrtc, 2015-10-01 01:47:17]

The CQ bit was checked by guoweis@webrtc.org

[guoweis_webrtc, 2015-10-01 01:47:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
rsleevi@chromium.org, pthatcher@webrtc.org, davidben@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1337673002/#ps750001 (title:
" ")

[guoweis_webrtc, 2015-10-01 01:47:22]

The CQ bit was checked by guoweis@webrtc.org

[commit-bot: I haz the power, 2015-10-01 01:47:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1337673002/750001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1337673002/750001

[commit-bot: I haz the power, 2015-10-01 01:49:14]

Committed patchset #20 (id:750001)

[commit-bot: I haz the power, 2015-10-01 01:49:29]

Patchset 20 (id:??) landed as
https://crrev.com/4fe3c9a77386598db9abd1f0d6983aefee9cc943
Cr-Commit-Position: refs/heads/master@{#10124}

[guoweis_webrtc, 2015-10-01 02:22:49]

A revert of this CL (patchset #20 id:750001) has been created in
https://codereview.webrtc.org/1380603005/ by guoweis@webrtc.org.

The reason for reverting is: This broke chromium.fyi bot..

[juberti, 2015-10-01 04:57:51]

let's talk about this CL tomorrow. 

An API like this:
bool SetSrtpCiphers(const std::vector<std::string>& ciphers) override;
bool GetSrtpCryptoSuite(std::string* cipher) override;
bool GetSslCipherSuite(uint16_t* cipher) override;

feels entirely inconsistent.