Enable SCTP in libjingle.

Enable SCTP target in libjingle.gyp. SCTP was already existed in Chrome third_party for a while but never used. Libjingle is trying use it for DataChannels.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=205864

[Sergey Ulanov, 2013-05-31 19:45:29]

lgtm. Please mention usrsctp updates in the CL description.

https://codereview.chromium.org/16196007/diff/1/third_party/libjingle/libjing...
File third_party/libjingle/libjingle.gyp (right):

https://codereview.chromium.org/16196007/diff/1/third_party/libjingle/libjing...
third_party/libjingle/libjingle.gyp:774: #TODO(mallinath) - Investigate why this
section can't be moved below.
nit: add space before TODO. 80 chars lines. I don't understand this comment -
what you mean by moving it below? might be worth clarifying.

[wjia(left Chromium), 2013-05-31 21:16:52]

lgtm on content/renderer/media/.

[Mallinath (Gone from Chromium), 2013-05-31 21:17:30]

Hi,

Please review the CL, which is trying add SCTP to third_party library.

Thanks,
Mallinath

https://codereview.chromium.org/16196007/diff/1/third_party/libjingle/libjing...
File third_party/libjingle/libjingle.gyp (right):

https://codereview.chromium.org/16196007/diff/1/third_party/libjingle/libjing...
third_party/libjingle/libjingle.gyp:774: #TODO(mallinath) - Investigate why this
section can't be moved below.
Never mind, i was planning to insert these line @ line 558. But now we have iOS
as well. So its better to keep this section separately.
On 2013/05/31 19:45:30, Sergey Ulanov wrote:
> nit: add space before TODO. 80 chars lines. I don't understand this comment -
> what you mean by moving it below? might be worth clarifying.

[Daniel Berlin, 2013-05-31 21:52:43]

It looks like sctp is already in third_party, and this is only adding/changing
some of the overrides.
That seems fine to me from a third party perspective.

[Mallinath (Gone from Chromium), 2013-05-31 21:55:22]

On 2013/05/31 21:52:43, Daniel Berlin wrote:
> It looks like sctp is already in third_party, and this is only adding/changing
> some of the overrides.
> That seems fine to me from a third party perspective.

Updated README.chromium which describes local modifications.

[Mallinath (Gone from Chromium), 2013-05-31 22:16:43]

On 2013/05/31 21:52:43, Daniel Berlin wrote:
> It looks like sctp is already in third_party, and this is only adding/changing
> some of the overrides.
> That seems fine to me from a third party perspective.

Daniel, FYI even though SCTP was in third_party no one was using it. This is the
first time we are trying to use it.

[Daniel Berlin, 2013-06-01 00:06:12]

Still okay by me then :)

On Fri, May 31, 2013 at 3:16 PM,  <mallinath@chromium.org> wrote:
> On 2013/05/31 21:52:43, Daniel Berlin wrote:
>>
>> It looks like sctp is already in third_party, and this is only
>> adding/changing
>> some of the overrides.
>> That seems fine to me from a third party perspective.
>
>
> Daniel, FYI even though SCTP was in third_party no one was using it. This is
> the
> first time we are trying to use it.
>
> https://codereview.chromium.org/16196007/

[Mallinath (Gone from Chromium), 2013-06-03 14:50:18]

On 2013/06/01 00:06:12, Daniel Berlin wrote:
> Still okay by me then :)
> 
> On Fri, May 31, 2013 at 3:16 PM,  <mailto:mallinath@chromium.org> wrote:
> > On 2013/05/31 21:52:43, Daniel Berlin wrote:
> >>
> >> It looks like sctp is already in third_party, and this is only
> >> adding/changing
> >> some of the overrides.
> >> That seems fine to me from a third party perspective.
> >
> >
> > Daniel, FYI even though SCTP was in third_party no one was using it. This is
> > the
> > first time we are trying to use it.
> >
> > https://codereview.chromium.org/16196007/

Pinging @security

[parisa, 2013-06-03 22:25:10]

On 2013/06/03 22:09:30, mallinath2 wrote:

Thanks for adding an OWNERS file :)

I think we (security) just found out this was checked in. Whoops!

comex@ said he may have already stumbled onto a few security bugs, but IIUC, a
larger concern with usrsctp is that it introduces its own separate (likely
buggy) implementation of SHA1. I'm going to replace myself with sleevi as a
reviewer, since I'm not sure if this concerning enough to block the CL.

[juberti, 2013-06-03 22:28:14]

The SHA situation is suboptimal here, but the effect is extremely minor.
SHA is only used to compute the SCTP handshake cookie, which has no meaning
in a WebRTC session where SCTP runs atop a secure transport (DTLS).

Also, the use of SCTP is currently gated by a command-line flag.


On Mon, Jun 3, 2013 at 3:25 PM, <parisa@chromium.org> wrote:

> On 2013/06/03 22:09:30, mallinath2 wrote:
>
> Thanks for adding an OWNERS file :)
>
> I think we (security) just found out this was checked in. Whoops!
>
> comex@ said he may have already stumbled onto a few security bugs, but
> IIUC, a
> larger concern with usrsctp is that it introduces its own separate (likely
> buggy) implementation of SHA1. I'm going to replace myself with sleevi as a
> reviewer, since I'm not sure if this concerning enough to block the CL.
>
>
https://codereview.chromium.**org/16196007/<https://codereview.chromium.org/1...
>

[Ryan Sleevi, 2013-06-05 00:00:30]

https://codereview.chromium.org/16196007/diff/29001/third_party/usrsctp/overr...
File third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.c (right):

https://codereview.chromium.org/16196007/diff/29001/third_party/usrsctp/overr...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.c:52:
SCTP_SHA1_Init(struct sha1_context *ctx)
ctx = PK11_CreateDigestContext(SEC_OID_SHA1);
PK11_DigestBegin(ctx);

https://codereview.chromium.org/16196007/diff/29001/third_party/usrsctp/overr...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.c:138:
SCTP_SHA1_Update(struct sha1_context *ctx, const unsigned char *ptr, int siz)
PK11_DigestOp(ctx, data, data_len)

https://codereview.chromium.org/16196007/diff/29001/third_party/usrsctp/overr...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.c:168:
SCTP_SHA1_Final(unsigned char *digest, struct sha1_context *ctx)
PK11_DigestFinal(ctx, digest, &output_len, digest_len);
PK11_DestroyContext(ctx);

https://codereview.chromium.org/16196007/diff/29001/third_party/usrsctp/overr...
File third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h (right):

https://codereview.chromium.org/16196007/diff/29001/third_party/usrsctp/overr...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h:44: struct
sha1_context {
We really shouldn't be rolling in yet-another-SHA1 implementation.

It seems if you're going the overrides/ path (entirely reasonable), then we
should be treating sha1_context as entirely opaque, and under the hood treating
it as an NSS type whose allocation is handled in _Init() and deallocation
handled in _Final().

[Mallinath (Gone from Chromium), 2013-06-06 14:55:50]

Removed libjingle roll from this issue as that handled separately.

https://codereview.chromium.org/16196007/diff/29001/third_party/usrsctp/overr...
File third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.c (right):

https://codereview.chromium.org/16196007/diff/29001/third_party/usrsctp/overr...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.c:52:
SCTP_SHA1_Init(struct sha1_context *ctx)
On 2013/06/05 00:00:31, Ryan Sleevi wrote:
> ctx = PK11_CreateDigestContext(SEC_OID_SHA1);
> PK11_DigestBegin(ctx);

Done.

https://codereview.chromium.org/16196007/diff/29001/third_party/usrsctp/overr...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.c:138:
SCTP_SHA1_Update(struct sha1_context *ctx, const unsigned char *ptr, int siz)
On 2013/06/05 00:00:31, Ryan Sleevi wrote:
> PK11_DigestOp(ctx, data, data_len)

Done.

https://codereview.chromium.org/16196007/diff/29001/third_party/usrsctp/overr...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.c:168:
SCTP_SHA1_Final(unsigned char *digest, struct sha1_context *ctx)
On 2013/06/05 00:00:31, Ryan Sleevi wrote:
> PK11_DigestFinal(ctx, digest, &output_len, digest_len);
> PK11_DestroyContext(ctx);

Done.

https://codereview.chromium.org/16196007/diff/29001/third_party/usrsctp/overr...
File third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h (right):

https://codereview.chromium.org/16196007/diff/29001/third_party/usrsctp/overr...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h:44: struct
sha1_context {
On 2013/06/05 00:00:31, Ryan Sleevi wrote:
> We really shouldn't be rolling in yet-another-SHA1 implementation.
> 
> It seems if you're going the overrides/ path (entirely reasonable), then we
> should be treating sha1_context as entirely opaque, and under the hood
treating
> it as an NSS type whose allocation is handled in _Init() and deallocation
> handled in _Final().

Done.

[juberti, 2013-06-06 15:14:39]

https://codereview.chromium.org/16196007/diff/120001/third_party/usrsctp/over...
File third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h (right):

https://codereview.chromium.org/16196007/diff/120001/third_party/usrsctp/over...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h:46: struct
PK11Context *pk11_ctx;
Ryan, will this work on Android? Or do we need to define an OpenSSL variant as
well?

[Ryan Sleevi, 2013-06-06 21:05:19]

NSS isn't used on Android, so you'll still have to have the OpenSSL path.

https://codereview.chromium.org/16196007/diff/120001/third_party/usrsctp/over...
File third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h (right):

https://codereview.chromium.org/16196007/diff/120001/third_party/usrsctp/over...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h:46: struct
PK11Context *pk11_ctx;
On 2013/06/06 15:14:40, juberti wrote:
> Ryan, will this work on Android? Or do we need to define an OpenSSL variant as
> well?

usrsctp already has an OpenSSL implementation, hidden behind some #ifdefs -
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/usrsct...

With the right header flipping, you should be able to pull in OpenSSL's version
with minimal changes.

[Mallinath (Gone from Chromium), 2013-06-06 21:27:08]

https://codereview.chromium.org/16196007/diff/120001/third_party/usrsctp/over...
File third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h (right):

https://codereview.chromium.org/16196007/diff/120001/third_party/usrsctp/over...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h:46: struct
PK11Context *pk11_ctx;
Didn't we go this route because depending on OpenSSL was bad idea for Chrome.
On 2013/06/06 21:05:19, Ryan Sleevi wrote:
> On 2013/06/06 15:14:40, juberti wrote:
> > Ryan, will this work on Android? Or do we need to define an OpenSSL variant
as
> > well?
> 
> usrsctp already has an OpenSSL implementation, hidden behind some #ifdefs -
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/usrsct...
> 
> With the right header flipping, you should be able to pull in OpenSSL's
version
> with minimal changes.

[Ryan Sleevi, 2013-06-06 21:29:47]

On 2013/06/06 21:27:08, mallinath2 wrote:
>
https://codereview.chromium.org/16196007/diff/120001/third_party/usrsctp/over...
> File third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h (right):
> 
>
https://codereview.chromium.org/16196007/diff/120001/third_party/usrsctp/over...
> third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h:46: struct
> PK11Context *pk11_ctx;
> Didn't we go this route because depending on OpenSSL was bad idea for Chrome.
> On 2013/06/06 21:05:19, Ryan Sleevi wrote:
> > On 2013/06/06 15:14:40, juberti wrote:
> > > Ryan, will this work on Android? Or do we need to define an OpenSSL
variant
> as
> > > well?
> > 
> > usrsctp already has an OpenSSL implementation, hidden behind some #ifdefs -
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/usrsct...
> > 
> > With the right header flipping, you should be able to pull in OpenSSL's
> version
> > with minimal changes.

Like I said, "with the right header flipping"

I'm not trying to be obtuse here - the NSS code is not going to work on Android,
so you're going to need to do an OpenSSL implementation as well for Android, and
the OpenSSL implementation is trivial - as shown through the headers.

With an NSS + OpenSSL implementation (eg: not shipping yet-another-SHA1), then
you'll have no problems rolling forward.

[juberti, 2013-06-06 23:25:21]

On 2013/06/06 21:29:47, Ryan Sleevi wrote:
> On 2013/06/06 21:27:08, mallinath2 wrote:
> >
>
https://codereview.chromium.org/16196007/diff/120001/third_party/usrsctp/over...
> > File third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h (right):
> > 
> >
>
https://codereview.chromium.org/16196007/diff/120001/third_party/usrsctp/over...
> > third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h:46: struct
> > PK11Context *pk11_ctx;
> > Didn't we go this route because depending on OpenSSL was bad idea for
Chrome.
> > On 2013/06/06 21:05:19, Ryan Sleevi wrote:
> > > On 2013/06/06 15:14:40, juberti wrote:
> > > > Ryan, will this work on Android? Or do we need to define an OpenSSL
> variant
> > as
> > > > well?
> > > 
> > > usrsctp already has an OpenSSL implementation, hidden behind some #ifdefs
-
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/usrsct...
> > > 
> > > With the right header flipping, you should be able to pull in OpenSSL's
> > version
> > > with minimal changes.
> 
> Like I said, "with the right header flipping"
> 
> I'm not trying to be obtuse here - the NSS code is not going to work on
Android,
> so you're going to need to do an OpenSSL implementation as well for Android,
and
> the OpenSSL implementation is trivial - as shown through the headers.
> 
> With an NSS + OpenSSL implementation (eg: not shipping yet-another-SHA1), then
> you'll have no problems rolling forward.

We've got that abstraction in webrtc - the MessageDigest class will thunk to
OpenSSL or NSS depending on how it's compiled. 

My preference here would be to allow the SHA impl to be injected at runtime - in
essence, the SCTP SHA entry points would just be thunks to the supplied function
pointers - and then we can inject MessageDigest from the sctpdataengine.cc code.

[Ryan Sleevi, 2013-06-06 23:34:46]

On 2013/06/06 23:25:21, juberti wrote:
> On 2013/06/06 21:29:47, Ryan Sleevi wrote:
> > On 2013/06/06 21:27:08, mallinath2 wrote:
> > >
> >
>
https://codereview.chromium.org/16196007/diff/120001/third_party/usrsctp/over...
> > > File third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/16196007/diff/120001/third_party/usrsctp/over...
> > > third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h:46: struct
> > > PK11Context *pk11_ctx;
> > > Didn't we go this route because depending on OpenSSL was bad idea for
> Chrome.
> > > On 2013/06/06 21:05:19, Ryan Sleevi wrote:
> > > > On 2013/06/06 15:14:40, juberti wrote:
> > > > > Ryan, will this work on Android? Or do we need to define an OpenSSL
> > variant
> > > as
> > > > > well?
> > > > 
> > > > usrsctp already has an OpenSSL implementation, hidden behind some
#ifdefs
> -
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/usrsct...
> > > > 
> > > > With the right header flipping, you should be able to pull in OpenSSL's
> > > version
> > > > with minimal changes.
> > 
> > Like I said, "with the right header flipping"
> > 
> > I'm not trying to be obtuse here - the NSS code is not going to work on
> Android,
> > so you're going to need to do an OpenSSL implementation as well for Android,
> and
> > the OpenSSL implementation is trivial - as shown through the headers.
> > 
> > With an NSS + OpenSSL implementation (eg: not shipping yet-another-SHA1),
then
> > you'll have no problems rolling forward.
> 
> We've got that abstraction in webrtc - the MessageDigest class will thunk to
> OpenSSL or NSS depending on how it's compiled. 
> 
> My preference here would be to allow the SHA impl to be injected at runtime -
in
> essence, the SCTP SHA entry points would just be thunks to the supplied
function
> pointers - and then we can inject MessageDigest from the sctpdataengine.cc
code.

Yup, I have no complaints with that approach either.

I just want to make sure we're not introducing yet-another-SHA1, and instead
relying on the OpenSSL and NSS bits that we already have. The pointers I gave
were for if you wanted to do it directly in this code. I have no personal stake
in you thunking in WebRTC code here, other than it seems to just be adding an
extra layer of indirection, but that's neither here nor there.

Just no new SHA1 :)

[juberti, 2013-06-06 23:42:12]

Agree on the end goal.

Mallinath, alternately, we could have
SCTP_USE_NSS_SHA1/SCTP_USE_OPENSSL_SHA1 #defines, and each of those would
compile in the right thunks to NSS or OpenSSL SHA-1 code. If that's a lot
easier, we can just do that.


On Thu, Jun 6, 2013 at 4:34 PM, <rsleevi@chromium.org> wrote:

> On 2013/06/06 23:25:21, juberti wrote:
>
>> On 2013/06/06 21:29:47, Ryan Sleevi wrote:
>> > On 2013/06/06 21:27:08, mallinath2 wrote:
>> > >
>> >
>>
>
> https://codereview.chromium.**org/16196007/diff/120001/**
>
third_party/usrsctp/overrides/**usrsctplib/netinet/sctp_sha1.h<https://codereview.chromium.org/16196007/diff/120001/third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h>
>
>> > > File third_party/usrsctp/overrides/**usrsctplib/netinet/sctp_sha1.h
>> (right):
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.**org/16196007/diff/120001/**
>
third_party/usrsctp/overrides/**usrsctplib/netinet/sctp_sha1.**h#newcode46<https://codereview.chromium.org/16196007/diff/120001/third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h#newcode46>
>
>> > > third_party/usrsctp/overrides/**usrsctplib/netinet/sctp_sha1.**h:46:
>> struct
>> > > PK11Context *pk11_ctx;
>> > > Didn't we go this route because depending on OpenSSL was bad idea for
>> Chrome.
>> > > On 2013/06/06 21:05:19, Ryan Sleevi wrote:
>> > > > On 2013/06/06 15:14:40, juberti wrote:
>> > > > > Ryan, will this work on Android? Or do we need to define an
>> OpenSSL
>> > variant
>> > > as
>> > > > > well?
>> > > >
>> > > > usrsctp already has an OpenSSL implementation, hidden behind some
>>
> #ifdefs
>
>> -
>> > > >
>> > >
>> >
>>
>
> https://code.google.com/p/**chromium/codesearch#chromium/**
> src/third_party/usrsctp/**usrsctplib/netinet/sctp_os_**
> userspace.h&sq=package:**chromium&q=SHA1_Final%**2525252520file:src/third_
>
**party/usrsctp&type=cs&l=1010<https://code.google.com/p/chromium/codesearch#chromium/src/third_party/usrsctp/usrsctplib/netinet/sctp_os_userspace.h&sq=package:chromium&q=SHA1_Final%2525252520file:src/third_party/usrsctp&type=cs&l=1010>
>
>  > > >
>> > > > With the right header flipping, you should be able to pull in
>> OpenSSL's
>> > > version
>> > > > with minimal changes.
>> >
>> > Like I said, "with the right header flipping"
>> >
>> > I'm not trying to be obtuse here - the NSS code is not going to work on
>> Android,
>> > so you're going to need to do an OpenSSL implementation as well for
>> Android,
>> and
>> > the OpenSSL implementation is trivial - as shown through the headers.
>> >
>> > With an NSS + OpenSSL implementation (eg: not shipping
>> yet-another-SHA1),
>>
> then
>
>> > you'll have no problems rolling forward.
>>
>
>  We've got that abstraction in webrtc - the MessageDigest class will thunk
>> to
>> OpenSSL or NSS depending on how it's compiled.
>>
>
>  My preference here would be to allow the SHA impl to be injected at
>> runtime -
>>
> in
>
>> essence, the SCTP SHA entry points would just be thunks to the supplied
>>
> function
>
>> pointers - and then we can inject MessageDigest from the sctpdataengine.cc
>>
> code.
>
> Yup, I have no complaints with that approach either.
>
> I just want to make sure we're not introducing yet-another-SHA1, and
> instead
> relying on the OpenSSL and NSS bits that we already have. The pointers I
> gave
> were for if you wanted to do it directly in this code. I have no personal
> stake
> in you thunking in WebRTC code here, other than it seems to just be adding
> an
> extra layer of indirection, but that's neither here nor there.
>
> Just no new SHA1 :)
>
>
https://codereview.chromium.**org/16196007/<https://codereview.chromium.org/1...
>

[Mallinath (Gone from Chromium), 2013-06-07 14:45:10]

Went with #defines approach. Ready for review.

On 2013/06/06 23:42:12, juberti wrote:
> Agree on the end goal.
> 
> Mallinath, alternately, we could have
> SCTP_USE_NSS_SHA1/SCTP_USE_OPENSSL_SHA1 #defines, and each of those would
> compile in the right thunks to NSS or OpenSSL SHA-1 code. If that's a lot
> easier, we can just do that.
> 
> 
> On Thu, Jun 6, 2013 at 4:34 PM, <mailto:rsleevi@chromium.org> wrote:
> 
> > On 2013/06/06 23:25:21, juberti wrote:
> >
> >> On 2013/06/06 21:29:47, Ryan Sleevi wrote:
> >> > On 2013/06/06 21:27:08, mallinath2 wrote:
> >> > >
> >> >
> >>
> >
> > https://codereview.chromium.**org/16196007/diff/120001/**
> >
>
third_party/usrsctp/overrides/**usrsctplib/netinet/sctp_sha1.h<https://codereview.chromium.org/16196007/diff/120001/third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h>
> >
> >> > > File third_party/usrsctp/overrides/**usrsctplib/netinet/sctp_sha1.h
> >> (right):
> >> > >
> >> > >
> >> >
> >>
> >
> > https://codereview.chromium.**org/16196007/diff/120001/**
> >
>
third_party/usrsctp/overrides/**usrsctplib/netinet/sctp_sha1.**h#newcode46<https://codereview.chromium.org/16196007/diff/120001/third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h#newcode46>
> >
> >> > > third_party/usrsctp/overrides/**usrsctplib/netinet/sctp_sha1.**h:46:
> >> struct
> >> > > PK11Context *pk11_ctx;
> >> > > Didn't we go this route because depending on OpenSSL was bad idea for
> >> Chrome.
> >> > > On 2013/06/06 21:05:19, Ryan Sleevi wrote:
> >> > > > On 2013/06/06 15:14:40, juberti wrote:
> >> > > > > Ryan, will this work on Android? Or do we need to define an
> >> OpenSSL
> >> > variant
> >> > > as
> >> > > > > well?
> >> > > >
> >> > > > usrsctp already has an OpenSSL implementation, hidden behind some
> >>
> > #ifdefs
> >
> >> -
> >> > > >
> >> > >
> >> >
> >>
> >
> > https://code.google.com/p/**chromium/codesearch#chromium/**
> > src/third_party/usrsctp/**usrsctplib/netinet/sctp_os_**
> > userspace.h&sq=package:**chromium&q=SHA1_Final%**2525252520file:src/third_
> >
>
**party/usrsctp&type=cs&l=1010<https://code.google.com/p/chromium/codesearch#chromium/src/third_party/usrsctp/usrsctplib/netinet/sctp_os_userspace.h&sq=package:chromium&q=SHA1_Final%2525252520file:src/third_party/usrsctp&type=cs&l=1010>
> >
> >  > > >
> >> > > > With the right header flipping, you should be able to pull in
> >> OpenSSL's
> >> > > version
> >> > > > with minimal changes.
> >> >
> >> > Like I said, "with the right header flipping"
> >> >
> >> > I'm not trying to be obtuse here - the NSS code is not going to work on
> >> Android,
> >> > so you're going to need to do an OpenSSL implementation as well for
> >> Android,
> >> and
> >> > the OpenSSL implementation is trivial - as shown through the headers.
> >> >
> >> > With an NSS + OpenSSL implementation (eg: not shipping
> >> yet-another-SHA1),
> >>
> > then
> >
> >> > you'll have no problems rolling forward.
> >>
> >
> >  We've got that abstraction in webrtc - the MessageDigest class will thunk
> >> to
> >> OpenSSL or NSS depending on how it's compiled.
> >>
> >
> >  My preference here would be to allow the SHA impl to be injected at
> >> runtime -
> >>
> > in
> >
> >> essence, the SCTP SHA entry points would just be thunks to the supplied
> >>
> > function
> >
> >> pointers - and then we can inject MessageDigest from the sctpdataengine.cc
> >>
> > code.
> >
> > Yup, I have no complaints with that approach either.
> >
> > I just want to make sure we're not introducing yet-another-SHA1, and
> > instead
> > relying on the OpenSSL and NSS bits that we already have. The pointers I
> > gave
> > were for if you wanted to do it directly in this code. I have no personal
> > stake
> > in you thunking in WebRTC code here, other than it seems to just be adding
> > an
> > extra layer of indirection, but that's neither here nor there.
> >
> > Just no new SHA1 :)
> >
> >
>
https://codereview.chromium.**org/16196007/%3Chttps://codereview.chromium.org...>
> >

[Mallinath (Gone from Chromium), 2013-06-10 18:44:40]

On 2013/06/07 14:45:10, mallinath2 wrote:
> Went with #defines approach. Ready for review.
> 
> On 2013/06/06 23:42:12, juberti wrote:
> > Agree on the end goal.
> > 
> > Mallinath, alternately, we could have
> > SCTP_USE_NSS_SHA1/SCTP_USE_OPENSSL_SHA1 #defines, and each of those would
> > compile in the right thunks to NSS or OpenSSL SHA-1 code. If that's a lot
> > easier, we can just do that.
> > 
> > 
> > On Thu, Jun 6, 2013 at 4:34 PM, <mailto:rsleevi@chromium.org> wrote:
> > 
> > > On 2013/06/06 23:25:21, juberti wrote:
> > >
> > >> On 2013/06/06 21:29:47, Ryan Sleevi wrote:
> > >> > On 2013/06/06 21:27:08, mallinath2 wrote:
> > >> > >
> > >> >
> > >>
> > >
> > > https://codereview.chromium.**org/16196007/diff/120001/**
> > >
> >
>
third_party/usrsctp/overrides/**usrsctplib/netinet/sctp_sha1.h<https://codereview.chromium.org/16196007/diff/120001/third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h>
> > >
> > >> > > File third_party/usrsctp/overrides/**usrsctplib/netinet/sctp_sha1.h
> > >> (right):
> > >> > >
> > >> > >
> > >> >
> > >>
> > >
> > > https://codereview.chromium.**org/16196007/diff/120001/**
> > >
> >
>
third_party/usrsctp/overrides/**usrsctplib/netinet/sctp_sha1.**h#newcode46<https://codereview.chromium.org/16196007/diff/120001/third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h#newcode46>
> > >
> > >> > > third_party/usrsctp/overrides/**usrsctplib/netinet/sctp_sha1.**h:46:
> > >> struct
> > >> > > PK11Context *pk11_ctx;
> > >> > > Didn't we go this route because depending on OpenSSL was bad idea for
> > >> Chrome.
> > >> > > On 2013/06/06 21:05:19, Ryan Sleevi wrote:
> > >> > > > On 2013/06/06 15:14:40, juberti wrote:
> > >> > > > > Ryan, will this work on Android? Or do we need to define an
> > >> OpenSSL
> > >> > variant
> > >> > > as
> > >> > > > > well?
> > >> > > >
> > >> > > > usrsctp already has an OpenSSL implementation, hidden behind some
> > >>
> > > #ifdefs
> > >
> > >> -
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> > > https://code.google.com/p/**chromium/codesearch#chromium/**
> > > src/third_party/usrsctp/**usrsctplib/netinet/sctp_os_**
> > > userspace.h&sq=package:**chromium&q=SHA1_Final%**2525252520file:src/third_
> > >
> >
>
**party/usrsctp&type=cs&l=1010<https://code.google.com/p/chromium/codesearch#chromium/src/third_party/usrsctp/usrsctplib/netinet/sctp_os_userspace.h&sq=package:chromium&q=SHA1_Final%2525252520file:src/third_party/usrsctp&type=cs&l=1010>
> > >
> > >  > > >
> > >> > > > With the right header flipping, you should be able to pull in
> > >> OpenSSL's
> > >> > > version
> > >> > > > with minimal changes.
> > >> >
> > >> > Like I said, "with the right header flipping"
> > >> >
> > >> > I'm not trying to be obtuse here - the NSS code is not going to work on
> > >> Android,
> > >> > so you're going to need to do an OpenSSL implementation as well for
> > >> Android,
> > >> and
> > >> > the OpenSSL implementation is trivial - as shown through the headers.
> > >> >
> > >> > With an NSS + OpenSSL implementation (eg: not shipping
> > >> yet-another-SHA1),
> > >>
> > > then
> > >
> > >> > you'll have no problems rolling forward.
> > >>
> > >
> > >  We've got that abstraction in webrtc - the MessageDigest class will thunk
> > >> to
> > >> OpenSSL or NSS depending on how it's compiled.
> > >>
> > >
> > >  My preference here would be to allow the SHA impl to be injected at
> > >> runtime -
> > >>
> > > in
> > >
> > >> essence, the SCTP SHA entry points would just be thunks to the supplied
> > >>
> > > function
> > >
> > >> pointers - and then we can inject MessageDigest from the
sctpdataengine.cc
> > >>
> > > code.
> > >
> > > Yup, I have no complaints with that approach either.
> > >
> > > I just want to make sure we're not introducing yet-another-SHA1, and
> > > instead
> > > relying on the OpenSSL and NSS bits that we already have. The pointers I
> > > gave
> > > were for if you wanted to do it directly in this code. I have no personal
> > > stake
> > > in you thunking in WebRTC code here, other than it seems to just be adding
> > > an
> > > extra layer of indirection, but that's neither here nor there.
> > >
> > > Just no new SHA1 :)
> > >
> > >
> >
>
https://codereview.chromium.**org/16196007/%253Chttps://codereview.chromium.o...>
> > >

Ryan, is it possible to review this today?

[Ryan Sleevi, 2013-06-10 18:53:46]

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/OWNERS
File third_party/usrsctp/OWNERS (right):

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/OWNE...
third_party/usrsctp/OWNERS:1: ldixon@google.com
Per http://www.chromium.org/developers/owners-files

"be a Chromium project member with full svn commit access of at least 6 months
tenure"

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/READ...
File third_party/usrsctp/README.chromium (right):

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/READ...
third_party/usrsctp/README.chromium:11: The libjingle/webrtc uses usrsctp as the
reliability layer for data channels.
This is supposed to be a description of usrsctp more than how it's used.

"usrsctp provides a userland implementation of SCTP [RFC XXX]. It is adapted
from the FreeBSD kernel to be a cross-platform library that can be embedded
within applications.

It is used to provide a reliability layer for data channels in WebRTC, as
specified by [IETF-ID YYY]"

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/READ...
third_party/usrsctp/README.chromium:15: modifying method names from SHA1_XXX to
SCTP_SHA1_XXX.
Please prep a patch file for any local modifications you've made, so it can be
cleanly applied when rolling forward.

Additionally, your overrides contains more than just the files you mentioned,
and it's not immediately clear why.

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/over...
File third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.c (right):

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/over...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.c:46: 
Delete newline

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/over...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.c:59:
PK11_DestroyContext(ctx->pk11_ctx, 1);
s/1/PR_TRUE/

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/usrs...
File third_party/usrsctp/usrsctp.gyp (right):

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/usrs...
third_party/usrsctp/usrsctp.gyp:63: '$(SDKROOT)/usr/lib/libresolv.dylib',
Why is this necessary? You should only need a depdency on NSS types.

[Ryan Sleevi, 2013-06-10 18:55:24]

LGTM otherwise, mod the nits previously expressed.

[juberti, 2013-06-10 18:56:59]

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/over...
File third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.c (right):

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/over...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.c:2: * Copyright (c)
2001-2007, by Cisco Systems, Inc. All rights reserved.
Suggest making this sctp_nss_sha1.cc, or just inlining these functions in the
header

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/over...
File third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h (right):

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/over...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h:39: #ifndef
__SCTP_NSS_SHA1_h__
Is this right? Filename is still sctp_sha1.h. I was thinking all the various
types of SHA1 could be defined here - NSS, OpenSSL, built-in - and we would use
macros like SCTP_USE_NSS_SHA1/SCTP_USE_OPENSSL_SHA1/etc to pick which version to
use.

[Mallinath (Gone from Chromium), 2013-06-10 23:30:51]

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/OWNERS
File third_party/usrsctp/OWNERS (right):

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/OWNE...
third_party/usrsctp/OWNERS:1: ldixon@google.com
On 2013/06/10 18:53:47, Ryan Sleevi wrote:
> Per http://www.chromium.org/developers/owners-files
> 
> "be a Chromium project member with full svn commit access of at least 6 months
> tenure"

Done.

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/READ...
File third_party/usrsctp/README.chromium (right):

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/READ...
third_party/usrsctp/README.chromium:11: The libjingle/webrtc uses usrsctp as the
reliability layer for data channels.
On 2013/06/10 18:53:47, Ryan Sleevi wrote:
> This is supposed to be a description of usrsctp more than how it's used.
> 
> "usrsctp provides a userland implementation of SCTP [RFC XXX]. It is adapted
> from the FreeBSD kernel to be a cross-platform library that can be embedded
> within applications.
> 
> It is used to provide a reliability layer for data channels in WebRTC, as
> specified by [IETF-ID YYY]"

Done.

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/READ...
third_party/usrsctp/README.chromium:15: modifying method names from SHA1_XXX to
SCTP_SHA1_XXX.
On 2013/06/10 18:53:47, Ryan Sleevi wrote:
> Please prep a patch file for any local modifications you've made, so it can be
> cleanly applied when rolling forward.
> 
> Additionally, your overrides contains more than just the files you mentioned,
> and it's not immediately clear why.

Done.

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/over...
File third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.c (right):

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/over...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.c:2: * Copyright (c)
2001-2007, by Cisco Systems, Inc. All rights reserved.
On 2013/06/10 18:56:59, juberti wrote:
> Suggest making this sctp_nss_sha1.cc, or just inlining these functions in the
> header

Done.

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/over...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.c:46: 
On 2013/06/10 18:53:47, Ryan Sleevi wrote:
> Delete newline

Done.

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/over...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.c:59:
PK11_DestroyContext(ctx->pk11_ctx, 1);
On 2013/06/10 18:53:47, Ryan Sleevi wrote:
> s/1/PR_TRUE/

Done.

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/over...
File third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h (right):

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/over...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_sha1.h:39: #ifndef
__SCTP_NSS_SHA1_h__
This file is renamed to sctp_nss_sha1.c/h. But OPENSSL and NSS is decided in
sctp_os_userspace.h. Which is the normal flow for usrsctplib.

On 2013/06/10 18:56:59, juberti wrote:
> Is this right? Filename is still sctp_sha1.h. I was thinking all the various
> types of SHA1 could be defined here - NSS, OpenSSL, built-in - and we would
use
> macros like SCTP_USE_NSS_SHA1/SCTP_USE_OPENSSL_SHA1/etc to pick which version
to
> use.
>

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/usrs...
File third_party/usrsctp/usrsctp.gyp (right):

https://codereview.chromium.org/16196007/diff/148001/third_party/usrsctp/usrs...
third_party/usrsctp/usrsctp.gyp:63: '$(SDKROOT)/usr/lib/libresolv.dylib',
On 2013/06/10 18:53:47, Ryan Sleevi wrote:
> Why is this necessary? You should only need a depdency on NSS types.

Done.

[juberti, 2013-06-10 23:57:13]

https://codereview.chromium.org/16196007/diff/194001/third_party/usrsctp/over...
File third_party/usrsctp/overrides/usrsctplib/netinet/sctp_nss_sha1.h (right):

https://codereview.chromium.org/16196007/diff/194001/third_party/usrsctp/over...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_nss_sha1.h:2: * Copyright
(c) 2001-2007, by Cisco Systems, Inc. All rights reserved.
I was thinking we would need to keep the existing sctp_sha1.h name, so that we
could limit the amount of changes to calling code (which already includes
sctp_sha1.h). 

Inside sctp_sha1.h, we would have #ifdefs to decide which implementation to
actually use.

[Mallinath (Gone from Chromium), 2013-06-11 00:11:25]

https://codereview.chromium.org/16196007/diff/194001/third_party/usrsctp/over...
File third_party/usrsctp/overrides/usrsctplib/netinet/sctp_nss_sha1.h (right):

https://codereview.chromium.org/16196007/diff/194001/third_party/usrsctp/over...
third_party/usrsctp/overrides/usrsctplib/netinet/sctp_nss_sha1.h:2: * Copyright
(c) 2001-2007, by Cisco Systems, Inc. All rights reserved.
I see. Since we ruled out the existing sha1 implementation and
sctp_os_userspace.h already had place for openssl, so i removed sctp_sha1.h
implementation from overrides folder.

I know this is ugly, but sctp_sha1.h included from sctp_os_userspace.h and
sctp_auth.h both are already in overrides folder, so this didn't cause any
issues.

Let me try to do what you are suggesting here.

On 2013/06/10 23:57:14, juberti wrote:
> I was thinking we would need to keep the existing sctp_sha1.h name, so that we
> could limit the amount of changes to calling code (which already includes
> sctp_sha1.h). 
> 
> Inside sctp_sha1.h, we would have #ifdefs to decide which implementation to
> actually use.

[commit-bot: I haz the power, 2013-06-12 06:38:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mallinath@chromium.org/16196007/269001

[commit-bot: I haz the power, 2013-06-12 06:54:02]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-12 07:01:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mallinath@chromium.org/16196007/269001

[commit-bot: I haz the power, 2013-06-12 07:06:00]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-12 13:22:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mallinath@chromium.org/16196007/269001

[commit-bot: I haz the power, 2013-06-12 14:02:21]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-12 14:45:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mallinath@chromium.org/16196007/63010

[commit-bot: I haz the power, 2013-06-12 17:57:42]

Change committed as 205864