Change the size of the ICE ufrag to 4 bytes

Change the size of the ICE ufrag to 4 bytes.

This is the minumum allowed size, and will allow STUN pings to be smaller.
The unit tests on the the Gturn are also modified. A username with length of 16 bytes will be generated for Gturn only.

Committed: https://crrev.com/6d0d4bf31de1e36cab276ec3db994a952ba4b612
Cr-Commit-Position: refs/heads/master@{#12876}

[Zhi Huang, 2016-03-31 22:15:10]

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

[Taylor Brandstetter, 2016-03-31 22:19:30]

Description was changed from

==========
reset the size of ICE_UFRAG


remove the dead code

BUG=
==========

to

==========
Change the size of the ICE ufrag to 4 bytes.

This is the minumum allowed size, and will allow STUN pings to be smaller.
==========

[Taylor Brandstetter, 2016-03-31 22:20:24]

https://codereview.webrtc.org/1848083002/diff/1/webrtc/p2p/base/transport.cc
File webrtc/p2p/base/transport.cc (left):

https://codereview.webrtc.org/1848083002/diff/1/webrtc/p2p/base/transport.cc#...
webrtc/p2p/base/transport.cc:244: }
This is unrelated to changing the size of the UFRAG, so I'd put it in a separate
CL.

[Zhi Huang, 2016-03-31 23:11:03]

On 2016/03/31 22:20:24, Taylor Brandstetter wrote:
> https://codereview.webrtc.org/1848083002/diff/1/webrtc/p2p/base/transport.cc
> File webrtc/p2p/base/transport.cc (left):
> 
>
https://codereview.webrtc.org/1848083002/diff/1/webrtc/p2p/base/transport.cc#...
> webrtc/p2p/base/transport.cc:244: }
> This is unrelated to changing the size of the UFRAG, so I'd put it in a
separate
> CL.

Done.

I reverted the unrelated code change and amended the commit.

[Taylor Brandstetter, 2016-04-01 00:46:55]

lgtm

Question for Peter: Do you think there are any legacy (GTURN) servers that still
require a ufrag length of 16?

https://codereview.webrtc.org/1848083002/diff/1/webrtc/p2p/base/transport.cc
File webrtc/p2p/base/transport.cc (left):

https://codereview.webrtc.org/1848083002/diff/1/webrtc/p2p/base/transport.cc#...
webrtc/p2p/base/transport.cc:244: }
On 2016/03/31 22:20:24, Taylor Brandstetter wrote:
> This is unrelated to changing the size of the UFRAG, so I'd put it in a
separate
> CL.

Done.

[pthatcher1, 2016-05-05 18:07:19]

https://codereview.webrtc.org/1848083002/diff/20001/webrtc/p2p/base/relayserv...
File webrtc/p2p/base/relayserver.cc (right):

https://codereview.webrtc.org/1848083002/diff/20001/webrtc/p2p/base/relayserv...
webrtc/p2p/base/relayserver.cc:268: username = username.substr(0, colonpos);
I don't think this is correct.

The place where we put a binding into bindings_ gets a username from code kind
of like this:

  std::string username;
  const StunByteStringAttribute* username_attr =
msg->GetByteString(STUN_ATTR_USERNAME);
  username.append(username_attr->bytes(), username_attr->length());

So we should just make this code match and use the full username, not the
username up to a colon. 

The only thing the code did before that we might remove is set a maximum
username length to 16.  But this is legacy code and it's probably not worth
changing it for that.


In other words, just revert this file.  It doesn't need to be changed.

[pthatcher1, 2016-05-05 18:16:27]

On 2016/04/01 00:46:55, Taylor Brandstetter wrote:
> lgtm
> 
> Question for Peter: Do you think there are any legacy (GTURN) servers that
still
> require a ufrag length of 16?

Yes, Chromoting, I believe, is still using GTURN servers.  And they use this
constant here:

https://code.google.com/p/chromium/codesearch#chromium/src/remoting/protocol/...

We should probably warn them that we are changing this.  Luckily, the fix is
very easy.  They just need to change
"rtc::CreateRandomString(cricket::ICE_UFRAG_LENGTH)" to
"rtc::CreateRandomString(16)" and that's it.  In fact, we could make that CL for
them and mail it to them for review.


> 
> https://codereview.webrtc.org/1848083002/diff/1/webrtc/p2p/base/transport.cc
> File webrtc/p2p/base/transport.cc (left):
> 
>
https://codereview.webrtc.org/1848083002/diff/1/webrtc/p2p/base/transport.cc#...
> webrtc/p2p/base/transport.cc:244: }
> On 2016/03/31 22:20:24, Taylor Brandstetter wrote:
> > This is unrelated to changing the size of the UFRAG, so I'd put it in a
> separate
> > CL.
> 
> Done.

[Zhi Huang, 2016-05-05 18:27:09]

https://codereview.webrtc.org/1848083002/diff/20001/webrtc/p2p/base/relayserv...
File webrtc/p2p/base/relayserver.cc (right):

https://codereview.webrtc.org/1848083002/diff/20001/webrtc/p2p/base/relayserv...
webrtc/p2p/base/relayserver.cc:268: username = username.substr(0, colonpos);
On 2016/05/05 18:07:19, pthatcher1 wrote:
> I don't think this is correct.
> 
> The place where we put a binding into bindings_ gets a username from code kind
> of like this:
> 
>   std::string username;
>   const StunByteStringAttribute* username_attr =
> msg->GetByteString(STUN_ATTR_USERNAME);
>   username.append(username_attr->bytes(), username_attr->length());
> 
> So we should just make this code match and use the full username, not the
> username up to a colon. 
> 
> The only thing the code did before that we might remove is set a maximum
> username length to 16.  But this is legacy code and it's probably not worth
> changing it for that.
> 
> 
> In other words, just revert this file.  It doesn't need to be changed.
> 
> 

The reason I made these changes is some unit tests will fail.

I looked into this and I found that username_attr contains two parts (two names
I think) which is separated by a colon and the length of user_name is 2 *
length(ufrag) + 1. When the ufrag length is 16, the length of username_attr is
33. After min(33, 16), the length will be 16, which works fine. However, when
the ufrag length is 4, the length(username_attr) = 9. After min(9, 16), the
length will be 9, which will be a problem.

[pthatcher1, 2016-05-05 20:24:32]

https://codereview.webrtc.org/1848083002/diff/20001/webrtc/p2p/base/relayserv...
File webrtc/p2p/base/relayserver.cc (right):

https://codereview.webrtc.org/1848083002/diff/20001/webrtc/p2p/base/relayserv...
webrtc/p2p/base/relayserver.cc:268: username = username.substr(0, colonpos);
On 2016/05/05 18:27:09, Zhi Huang wrote:
> On 2016/05/05 18:07:19, pthatcher1 wrote:
> > I don't think this is correct.
> > 
> > The place where we put a binding into bindings_ gets a username from code
kind
> > of like this:
> > 
> >   std::string username;
> >   const StunByteStringAttribute* username_attr =
> > msg->GetByteString(STUN_ATTR_USERNAME);
> >   username.append(username_attr->bytes(), username_attr->length());
> > 
> > So we should just make this code match and use the full username, not the
> > username up to a colon. 
> > 
> > The only thing the code did before that we might remove is set a maximum
> > username length to 16.  But this is legacy code and it's probably not worth
> > changing it for that.
> > 
> > 
> > In other words, just revert this file.  It doesn't need to be changed.
> > 
> > 
> 
> The reason I made these changes is some unit tests will fail.
> 
> I looked into this and I found that username_attr contains two parts (two
names
> I think) which is separated by a colon and the length of user_name is 2 *
> length(ufrag) + 1. When the ufrag length is 16, the length of username_attr is
> 33. After min(33, 16), the length will be 16, which works fine. However, when
> the ufrag length is 4, the length(username_attr) = 9. After min(9, 16), the
> length will be 9, which will be a problem.

I don't see why a length of 9 would cause this code to fail, but perhaps there
is something I missing.  In that case, we should fix the unittest to use 16
rather than 4.

Which unit test is it?

[Zhi Huang, 2016-05-05 23:10:59]

https://codereview.webrtc.org/1848083002/diff/20001/webrtc/p2p/base/relayserv...
File webrtc/p2p/base/relayserver.cc (right):

https://codereview.webrtc.org/1848083002/diff/20001/webrtc/p2p/base/relayserv...
webrtc/p2p/base/relayserver.cc:268: username = username.substr(0, colonpos);
On 2016/05/05 20:24:31, pthatcher1 wrote:
> On 2016/05/05 18:27:09, Zhi Huang wrote:
> > On 2016/05/05 18:07:19, pthatcher1 wrote:
> > > I don't think this is correct.
> > > 
> > > The place where we put a binding into bindings_ gets a username from code
> kind
> > > of like this:
> > > 
> > >   std::string username;
> > >   const StunByteStringAttribute* username_attr =
> > > msg->GetByteString(STUN_ATTR_USERNAME);
> > >   username.append(username_attr->bytes(), username_attr->length());
> > > 
> > > So we should just make this code match and use the full username, not the
> > > username up to a colon. 
> > > 
> > > The only thing the code did before that we might remove is set a maximum
> > > username length to 16.  But this is legacy code and it's probably not
worth
> > > changing it for that.
> > > 
> > > 
> > > In other words, just revert this file.  It doesn't need to be changed.
> > > 
> > > 
> > 
> > The reason I made these changes is some unit tests will fail.
> > 
> > I looked into this and I found that username_attr contains two parts (two
> names
> > I think) which is separated by a colon and the length of user_name is 2 *
> > length(ufrag) + 1. When the ufrag length is 16, the length of username_attr
is
> > 33. After min(33, 16), the length will be 16, which works fine. However,
when
> > the ufrag length is 4, the length(username_attr) = 9. After min(9, 16), the
> > length will be 9, which will be a problem.
> 
> I don't see why a length of 9 would cause this code to fail, but perhaps there
> is something I missing.  In that case, we should fix the unittest to use 16
> rather than 4.
> 
> Which unit test is it?

I changed code like this:
 const uint32_t USERNAME_LENGTH = 16;
 ...
 uint32_t length = 
  std::min(static_cast<uint32_t>(username_attr->length()),       
  USERNAME_LENGTH);
  std::string username(username_attr->bytes(), length);
  // TODO: Check the HMAC.
  // The binding should already be present.
  BindingMap::iterator biter = bindings_.find(username);
  if (biter == bindings_.end()) {
    LOG(LS_WARNING) << "Dropping packet: no binding with username";
    return;
  }

These tests will fail.
 out/Debug/rtc_unittests: PortTest.TestLocalToTcpGturn
 out/Debug/rtc_unittests: PortTest.TestLocalToGturn
 out/Debug/rtc_unittests: PortTest.TestPRNatToTcpGturn
 out/Debug/rtc_unittests: PortTest.TestLocalToSslTcpGturn
 out/Debug/rtc_unittests: PortTest.TestPRNatToGturn
 out/Debug/rtc_unittests: PortTest.TestSymNatToGturn
 out/Debug/rtc_unittests: PortTest.TestSymNatToTcpGturn
 out/Debug/rtc_unittests: PortTest.TestARNatToGturn
 out/Debug/rtc_unittests: PortTest.TestARNATNatToTcpGturn
 out/Debug/rtc_unittests: PortTest.TestConeNatToGturn
 out/Debug/rtc_unittests: PortTest.TestConeNatToTcpGturn

When I change USERNAME_LENGTH = 4, these tests will fail.
out/Debug/rtc_unittests: RelayServerTest.TestRemoteBind
 out/Debug/rtc_unittests: RelayServerTest.TestSendRequestBadUsername
 out/Debug/rtc_unittests: RelayServerTest.TestSendRequestNoDestinationAddress
 out/Debug/rtc_unittests: RelayServerTest.TestSendRequestNoData
 out/Debug/rtc_unittests: RelayServerTest.TestSendRaw

Since the USERNAME_LENGTH is only used here, I think its purpose is to extract
the substring from username_attribute. So using the colon to extract the
username might be more flexible.

[pthatcher1, 2016-05-09 23:59:25]

Please try this code.

https://codereview.webrtc.org/1848083002/diff/20001/webrtc/p2p/base/relayserv...
File webrtc/p2p/base/relayserver.cc (right):

https://codereview.webrtc.org/1848083002/diff/20001/webrtc/p2p/base/relayserv...
webrtc/p2p/base/relayserver.cc:268: username = username.substr(0, colonpos);
uint32_t length = static_cast<uint32_t>(username_attr->length()));
std::string username(username_attr->bytes(), length);
// TODO: Check the HMAC.
// The binding should already be present.
BindingMap::iterator biter = bindings_.find(username);
if (biter == bindings_.end()) {
  LOG(LS_WARNING) << "Dropping packet: no binding with username";
  return;
}

On 2016/05/05 23:10:59, Zhi Huang wrote:
> On 2016/05/05 20:24:31, pthatcher1 wrote:
> > On 2016/05/05 18:27:09, Zhi Huang wrote:
> > > On 2016/05/05 18:07:19, pthatcher1 wrote:
> > > > I don't think this is correct.
> > > > 
> > > > The place where we put a binding into bindings_ gets a username from
code
> > kind
> > > > of like this:
> > > > 
> > > >   std::string username;
> > > >   const StunByteStringAttribute* username_attr =
> > > > msg->GetByteString(STUN_ATTR_USERNAME);
> > > >   username.append(username_attr->bytes(), username_attr->length());
> > > > 
> > > > So we should just make this code match and use the full username, not
the
> > > > username up to a colon. 
> > > > 
> > > > The only thing the code did before that we might remove is set a maximum
> > > > username length to 16.  But this is legacy code and it's probably not
> worth
> > > > changing it for that.
> > > > 
> > > > 
> > > > In other words, just revert this file.  It doesn't need to be changed.
> > > > 
> > > > 
> > > 
> > > The reason I made these changes is some unit tests will fail.
> > > 
> > > I looked into this and I found that username_attr contains two parts (two
> > names
> > > I think) which is separated by a colon and the length of user_name is 2 *
> > > length(ufrag) + 1. When the ufrag length is 16, the length of
username_attr
> is
> > > 33. After min(33, 16), the length will be 16, which works fine. However,
> when
> > > the ufrag length is 4, the length(username_attr) = 9. After min(9, 16),
the
> > > length will be 9, which will be a problem.
> > 
> > I don't see why a length of 9 would cause this code to fail, but perhaps
there
> > is something I missing.  In that case, we should fix the unittest to use 16
> > rather than 4.
> > 
> > Which unit test is it?
> 
> I changed code like this:
>  const uint32_t USERNAME_LENGTH = 16;
>  ...
>  uint32_t length = 
>   std::min(static_cast<uint32_t>(username_attr->length()),       
>   USERNAME_LENGTH);
>   std::string username(username_attr->bytes(), length);
>   // TODO: Check the HMAC.
>   // The binding should already be present.
>   BindingMap::iterator biter = bindings_.find(username);
>   if (biter == bindings_.end()) {
>     LOG(LS_WARNING) << "Dropping packet: no binding with username";
>     return;
>   }
> 
> These tests will fail.
>  out/Debug/rtc_unittests: PortTest.TestLocalToTcpGturn
>  out/Debug/rtc_unittests: PortTest.TestLocalToGturn
>  out/Debug/rtc_unittests: PortTest.TestPRNatToTcpGturn
>  out/Debug/rtc_unittests: PortTest.TestLocalToSslTcpGturn
>  out/Debug/rtc_unittests: PortTest.TestPRNatToGturn
>  out/Debug/rtc_unittests: PortTest.TestSymNatToGturn
>  out/Debug/rtc_unittests: PortTest.TestSymNatToTcpGturn
>  out/Debug/rtc_unittests: PortTest.TestARNatToGturn
>  out/Debug/rtc_unittests: PortTest.TestARNATNatToTcpGturn
>  out/Debug/rtc_unittests: PortTest.TestConeNatToGturn
>  out/Debug/rtc_unittests: PortTest.TestConeNatToTcpGturn
> 
> When I change USERNAME_LENGTH = 4, these tests will fail.
> out/Debug/rtc_unittests: RelayServerTest.TestRemoteBind
>  out/Debug/rtc_unittests: RelayServerTest.TestSendRequestBadUsername
>  out/Debug/rtc_unittests: RelayServerTest.TestSendRequestNoDestinationAddress
>  out/Debug/rtc_unittests: RelayServerTest.TestSendRequestNoData
>  out/Debug/rtc_unittests: RelayServerTest.TestSendRaw
> 
> Since the USERNAME_LENGTH is only used here, I think its purpose is to extract
> the substring from username_attribute. So using the colon to extract the
> username might be more flexible.
>

[zhihuang1, 2016-05-10 20:43:18]

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

[pthatcher1, 2016-05-10 21:04:57]

lgtm!

[zhihuang1, 2016-05-10 21:08:54]

The CQ bit was checked by zhihuang@chromium.org

[zhihuang1, 2016-05-10 21:08:55]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1848083002/#ps60001 (title:
"Revert unnecessary change. Minor.")

[commit-bot: I haz the power, 2016-05-10 21:09:01]

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

[zhihuang1, 2016-05-10 21:09:03]

The CQ bit was unchecked by zhihuang@chromium.org

[zhihuang1, 2016-05-10 21:11:24]

Description was changed from

==========
Change the size of the ICE ufrag to 4 bytes.

This is the minumum allowed size, and will allow STUN pings to be smaller.
==========

to

==========
Change the size of the ICE ufrag to 4 bytes.

This is the minumum allowed size, and will allow STUN pings to be smaller. The
unit tests on the the Gturn are also modified. A username with length of 16
bytes will be generated for Gturn only.
==========

[zhihuang1, 2016-05-10 21:11:36]

Description was changed from

==========
Change the size of the ICE ufrag to 4 bytes.

This is the minumum allowed size, and will allow STUN pings to be smaller. The
unit tests on the the Gturn are also modified. A username with length of 16
bytes will be generated for Gturn only.
==========

to

==========
Change the size of the ICE ufrag to 4 bytes.

This is the minumum allowed size, and will allow STUN pings to be smaller. 
The unit tests on the the Gturn are also modified. A username with length of 16
bytes will be generated for Gturn only.
==========

[zhihuang1, 2016-05-10 21:12:02]

The CQ bit was checked by zhihuang@chromium.org

[commit-bot: I haz the power, 2016-05-10 21:12:11]

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

[commit-bot: I haz the power, 2016-05-10 21:19:11]

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

[commit-bot: I haz the power, 2016-05-10 21:19:11]

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

[Zhi Huang, 2016-05-16 22:04:02]

The CQ bit was checked by zhihuang@webrtc.org

[commit-bot: I haz the power, 2016-05-16 22:04:12]

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

[commit-bot: I haz the power, 2016-05-17 00:04:48]

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

[commit-bot: I haz the power, 2016-05-17 00:04:49]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[zhihuang1, 2016-05-23 16:56:22]

The CQ bit was checked by zhihuang@chromium.org

[zhihuang1, 2016-05-23 16:56:23]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1848083002/#ps80001 (title:
"Modified the constants in unit tests based on new ICE_UFRAG_LENGTH.")

[commit-bot: I haz the power, 2016-05-23 16:56:26]

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

[commit-bot: I haz the power, 2016-05-23 18:57:00]

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

[commit-bot: I haz the power, 2016-05-23 18:57:01]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[Zhi Huang, 2016-05-24 17:00:17]

The CQ bit was checked by zhihuang@webrtc.org

[commit-bot: I haz the power, 2016-05-24 17:00:30]

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

[commit-bot: I haz the power, 2016-05-24 17:13:38]

Description was changed from

==========
Change the size of the ICE ufrag to 4 bytes.

This is the minumum allowed size, and will allow STUN pings to be smaller. 
The unit tests on the the Gturn are also modified. A username with length of 16
bytes will be generated for Gturn only.
==========

to

==========
Change the size of the ICE ufrag to 4 bytes.

This is the minumum allowed size, and will allow STUN pings to be smaller. 
The unit tests on the the Gturn are also modified. A username with length of 16
bytes will be generated for Gturn only.
==========

[commit-bot: I haz the power, 2016-05-24 17:13:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-24 17:13:49]

Description was changed from

==========
Change the size of the ICE ufrag to 4 bytes.

This is the minumum allowed size, and will allow STUN pings to be smaller. 
The unit tests on the the Gturn are also modified. A username with length of 16
bytes will be generated for Gturn only.
==========

to

==========
Change the size of the ICE ufrag to 4 bytes.

This is the minumum allowed size, and will allow STUN pings to be smaller.
The unit tests on the the Gturn are also modified. A username with length of 16
bytes will be generated for Gturn only.

Committed: https://crrev.com/6d0d4bf31de1e36cab276ec3db994a952ba4b612
Cr-Commit-Position: refs/heads/master@{#12876}
==========

[commit-bot: I haz the power, 2016-05-24 17:13:50]

Patchset 5 (id:??) landed as
https://crrev.com/6d0d4bf31de1e36cab276ec3db994a952ba4b612
Cr-Commit-Position: refs/heads/master@{#12876}

[Zhi Huang, 2016-05-24 17:30:22]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.webrtc.org/2006903004/ by zhihuang@webrtc.org.

The reason for reverting is: Break the build on Win32..