Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 11 months ago by honghaiz3 Modified:
4 years, 10 months ago Reviewers:
Taylor Brandstetter, pthatcher1, andresp CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionReset TURN port NONCE when a new socket is created.
For example, when the TURN port has an ALLOCATE_MISMATCH error.
BUG=webrtc:5432
Committed: https://crrev.com/c463e20069c670594117a44755805ac0989cc137
Cr-Commit-Position: refs/heads/master@{#11453}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : #
Total comments: 9
Patch Set 3 : Addressed comments #
Total comments: 12
Patch Set 4 : #Patch Set 5 : merge with head #
Created: 4 years, 10 months ago
Messages
Total messages: 24 (11 generated)
honghaiz@webrtc.org changed reviewers: + deadbeef@webrtc.org, pthatcher@webrtc.org
Patchset #1 (id:1) has been deleted
Description was changed from ========== Reset TURN port NONCE when a new socket is created. For example, when the TURN port has an ALLOCATE_MISMATCH error. BUG=webrtc:5432 ========== to ========== Reset TURN port NONCE when a new socket is created. For example, when the TURN port has an ALLOCATE_MISMATCH error. BUG=webrtc:5432 ==========
honghaiz@webrtc.org changed reviewers: + andresp@webrtc.org
https://codereview.webrtc.org/1595613004/diff/20001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/1595613004/diff/20001/webrtc/p2p/base/turnport.... webrtc/p2p/base/turnport.cc:768: ResetNonce(); It looks like we don't have a unit test for this. Should we? https://codereview.webrtc.org/1595613004/diff/20001/webrtc/p2p/base/turnport_... File webrtc/p2p/base/turnport_unittest.cc (right): https://codereview.webrtc.org/1595613004/diff/20001/webrtc/p2p/base/turnport_... webrtc/p2p/base/turnport_unittest.cc:608: // Tests that TURN port NONCE will be reset even when receiving an even when => when https://codereview.webrtc.org/1595613004/diff/20001/webrtc/p2p/base/turnport_... webrtc/p2p/base/turnport_unittest.cc:630: turn_port_->set_hash(old_hash); Instead of altering the nonce+hash of the client, can we instead alter the nonce of the server?
Patchset #2 (id:40001) has been deleted
PTAL. Thanks! https://codereview.webrtc.org/1595613004/diff/20001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/1595613004/diff/20001/webrtc/p2p/base/turnport.... webrtc/p2p/base/turnport.cc:768: ResetNonce(); On 2016/01/19 01:48:38, pthatcher1 wrote: > It looks like we don't have a unit test for this. Should we? I removed this change because The alternate server response is the first-step processing a turnserver will do when handling a stun request. So it should be always empty, unless the server has included a NONCE in the response. In that case, it is not clear whether it is the best practice to reset the NONCE (because no server is doing that now). Allocate-mismatch is different. A server first checks whether the nonce is set. If not, it returns an error response with NONCE. When the client sends a request with the NONCE, the server will check for allocate-mismatch. https://codereview.webrtc.org/1595613004/diff/20001/webrtc/p2p/base/turnport_... File webrtc/p2p/base/turnport_unittest.cc (right): https://codereview.webrtc.org/1595613004/diff/20001/webrtc/p2p/base/turnport_... webrtc/p2p/base/turnport_unittest.cc:608: // Tests that TURN port NONCE will be reset even when receiving an On 2016/01/19 01:48:38, pthatcher1 wrote: > even when => when Done. https://codereview.webrtc.org/1595613004/diff/20001/webrtc/p2p/base/turnport_... webrtc/p2p/base/turnport_unittest.cc:630: turn_port_->set_hash(old_hash); On 2016/01/19 01:48:38, pthatcher1 wrote: > Instead of altering the nonce+hash of the client, can we instead alter the nonce > of the server? Done. We cannot directly change the NONCE but the timestamp for generating the NONCE.
"ResetNonce" in itself looks good; I just have some ideas on how the unit test could be made more robust. https://codereview.webrtc.org/1595613004/diff/60001/webrtc/p2p/base/turnport_... File webrtc/p2p/base/turnport_unittest.cc (right): https://codereview.webrtc.org/1595613004/diff/60001/webrtc/p2p/base/turnport_... webrtc/p2p/base/turnport_unittest.cc:610: TEST_F(TurnPortTest, TestTurnAllocateNonceRestAfterAllocateMismatch) { "NonceReset" instead of "NonceRest" https://codereview.webrtc.org/1595613004/diff/60001/webrtc/p2p/base/turnport_... webrtc/p2p/base/turnport_unittest.cc:618: turn_port_->OnSocketClose(turn_port_->socket(), 0); I don't think it's ideal to call "OnSocketClose" in a unit test, since OnSocketClose is effectively a private method (I don't see why it isn't one). Eventually it could go away, or its implementation could change, breaking this test. A more foolproof way to test this would be: 1. Create TurnPort and wait for it to be ready. 2. Set the drop probability of the virtual socket server to 100%. 3. Destroy the TurnPort (the allocation release message will be dropped). 4. Set the drop probability back to 0%. 5. Create a new TurnPort, ensuring it gets the same UDP port number, proceed with test. https://codereview.webrtc.org/1595613004/diff/60001/webrtc/p2p/base/turnport_... webrtc/p2p/base/turnport_unittest.cc:626: // NONCE generated by the server will be different. I would also add a comment saying: "It's expected that turn_port_ will set next_nonce, but then get an allocate mismatch error and set an ever newer nonce." Just so the unit test expectation is clearer to someone like me. https://codereview.webrtc.org/1595613004/diff/60001/webrtc/p2p/base/turnserver.h File webrtc/p2p/base/turnserver.h (right): https://codereview.webrtc.org/1595613004/diff/60001/webrtc/p2p/base/turnserve... webrtc/p2p/base/turnserver.h:207: } Instead of this, would it be reasonable to add a "mismatch_error_on_duplicate_nonce_" mode to the TurnServer, to simulate the google3 TURN server? If the flag is set, you could enumerate the current allocations, looking for one where "last_nonce" matches. This would possibly be valuable for other tests in the future, at it matches what we've done for other things like "enable_otu_nonce_" and "reject_private_addresses_".
Thanks! PTAL. https://codereview.webrtc.org/1595613004/diff/60001/webrtc/p2p/base/turnport_... File webrtc/p2p/base/turnport_unittest.cc (right): https://codereview.webrtc.org/1595613004/diff/60001/webrtc/p2p/base/turnport_... webrtc/p2p/base/turnport_unittest.cc:610: TEST_F(TurnPortTest, TestTurnAllocateNonceRestAfterAllocateMismatch) { On 2016/01/26 02:02:06, Taylor Brandstetter wrote: > "NonceReset" instead of "NonceRest" Done. https://codereview.webrtc.org/1595613004/diff/60001/webrtc/p2p/base/turnport_... webrtc/p2p/base/turnport_unittest.cc:618: turn_port_->OnSocketClose(turn_port_->socket(), 0); On 2016/01/26 02:02:06, Taylor Brandstetter wrote: > I don't think it's ideal to call "OnSocketClose" in a unit test, since > OnSocketClose is effectively a private method (I don't see why it isn't one). > Eventually it could go away, or its implementation could change, breaking this > test. A more foolproof way to test this would be: > > 1. Create TurnPort and wait for it to be ready. > 2. Set the drop probability of the virtual socket server to 100%. > 3. Destroy the TurnPort (the allocation release message will be dropped). > 4. Set the drop probability back to 0%. > 5. Create a new TurnPort, ensuring it gets the same UDP port number, proceed > with test. Done. https://codereview.webrtc.org/1595613004/diff/60001/webrtc/p2p/base/turnport_... webrtc/p2p/base/turnport_unittest.cc:626: // NONCE generated by the server will be different. On 2016/01/26 02:02:06, Taylor Brandstetter wrote: > I would also add a comment saying: "It's expected that turn_port_ will set > next_nonce, but then get an allocate mismatch error and set an ever newer > nonce." > > Just so the unit test expectation is clearer to someone like me. Done. Text is slightly different though. https://codereview.webrtc.org/1595613004/diff/60001/webrtc/p2p/base/turnserver.h File webrtc/p2p/base/turnserver.h (right): https://codereview.webrtc.org/1595613004/diff/60001/webrtc/p2p/base/turnserve... webrtc/p2p/base/turnserver.h:207: } On 2016/01/26 02:02:06, Taylor Brandstetter wrote: > Instead of this, would it be reasonable to add a > "mismatch_error_on_duplicate_nonce_" mode to the TurnServer, to simulate the > google3 TURN server? If the flag is set, you could enumerate the current > allocations, looking for one where "last_nonce" matches. > > This would possibly be valuable for other tests in the future, at it matches > what we've done for other things like "enable_otu_nonce_" and > "reject_private_addresses_". Looking through all allocations to find the duplicate nonce could be quite expensive. Another way would be to remember the allocate when the first request with empty nonce comes in, but that will change the behavior of turnserver substantially. Will keep this as we discussed.
lgtm https://codereview.webrtc.org/1595613004/diff/60001/webrtc/p2p/base/turnserver.h File webrtc/p2p/base/turnserver.h (right): https://codereview.webrtc.org/1595613004/diff/60001/webrtc/p2p/base/turnserve... webrtc/p2p/base/turnserver.h:207: } On 2016/01/27 01:45:23, honghaiz3 wrote: > On 2016/01/26 02:02:06, Taylor Brandstetter wrote: > > Instead of this, would it be reasonable to add a > > "mismatch_error_on_duplicate_nonce_" mode to the TurnServer, to simulate the > > google3 TURN server? If the flag is set, you could enumerate the current > > allocations, looking for one where "last_nonce" matches. > > > > This would possibly be valuable for other tests in the future, at it matches > > what we've done for other things like "enable_otu_nonce_" and > > "reject_private_addresses_". > > Looking through all allocations to find the duplicate nonce could be quite > expensive. Another way would be to remember the allocate when the first request > with empty nonce comes in, but that will change the behavior of turnserver > substantially. Will keep this as we discussed. Now that we discussed this and I understand the problem better, I agree it would require a major redesign of the TurnServer class. So what you have here is good.
Mostly nits, but I would like to know why the unit test requires to allocations and why one won't do. https://codereview.webrtc.org/1595613004/diff/80001/webrtc/p2p/base/turnport_... File webrtc/p2p/base/turnport_unittest.cc (right): https://codereview.webrtc.org/1595613004/diff/80001/webrtc/p2p/base/turnport_... webrtc/p2p/base/turnport_unittest.cc:608: // Tests that TURN port NONCE will be reset when receiving an ALLOCATE MISMATCH NONCE => nonce, here and below https://codereview.webrtc.org/1595613004/diff/80001/webrtc/p2p/base/turnport_... webrtc/p2p/base/turnport_unittest.cc:621: ss_->set_drop_probability(0.0); What's the point of doing a normal allocation first if we just destroy the turn_port_ anyway? Wouldn't it be enough to just do one round of allocation, but with the turn_server_.server()->SetTimestampForNextNonce(now); trick from below? https://codereview.webrtc.org/1595613004/diff/80001/webrtc/p2p/base/turnport_... webrtc/p2p/base/turnport_unittest.cc:632: uint32_t now = rtc::Time() - 1; Well it's not really "now", is it? Isn't it "before"? https://codereview.webrtc.org/1595613004/diff/80001/webrtc/p2p/base/turnport_... webrtc/p2p/base/turnport_unittest.cc:634: std::string next_nonce = turn_server_.server()->GenerateNonce(now); Wouldn't a better name be first_nonce? https://codereview.webrtc.org/1595613004/diff/80001/webrtc/p2p/base/turnserve... File webrtc/p2p/base/turnserver.cc (right): https://codereview.webrtc.org/1595613004/diff/80001/webrtc/p2p/base/turnserve... webrtc/p2p/base/turnserver.cc:468: ts_for_next_nonce_ = 0; I think this would be slightly more readable as: uint32_t timestamp = rtc::Time(); if (ts_for_next_nonce_) { timestamp = ts_for_next_nonce_; ts_for_next_nonce_ = 0; } https://codereview.webrtc.org/1595613004/diff/80001/webrtc/p2p/base/turnserver.h File webrtc/p2p/base/turnserver.h (right): https://codereview.webrtc.org/1595613004/diff/80001/webrtc/p2p/base/turnserve... webrtc/p2p/base/turnserver.h:203: std::string GenerateNonce(uint32_t now) const; Instead of making GenerateNonce visible, why not have SetTimestampForNextNonce simply return GenerateNonce(timestamp)? That would make the test a little more simple as well.
Thanks! PTAL. https://codereview.webrtc.org/1595613004/diff/80001/webrtc/p2p/base/turnport_... File webrtc/p2p/base/turnport_unittest.cc (right): https://codereview.webrtc.org/1595613004/diff/80001/webrtc/p2p/base/turnport_... webrtc/p2p/base/turnport_unittest.cc:608: // Tests that TURN port NONCE will be reset when receiving an ALLOCATE MISMATCH On 2016/01/29 17:37:46, pthatcher1 wrote: > NONCE => nonce, here and below Done. https://codereview.webrtc.org/1595613004/diff/80001/webrtc/p2p/base/turnport_... webrtc/p2p/base/turnport_unittest.cc:621: ss_->set_drop_probability(0.0); On 2016/01/29 17:37:46, pthatcher1 wrote: > What's the point of doing a normal allocation first if we just destroy the > turn_port_ anyway? Wouldn't it be enough to just do one round of allocation, > but with the turn_server_.server()->SetTimestampForNextNonce(now); trick from > below? The first allocation is needed to generate the mismatch error. The first one get an allocation. When the second allocation is made, it gets the nonce error. When the third allocation is made, it gets a mismatch error. The fourth allocation will be on a different port, but gets a nonce error. The fifth finally gets the allocation. https://codereview.webrtc.org/1595613004/diff/80001/webrtc/p2p/base/turnport_... webrtc/p2p/base/turnport_unittest.cc:632: uint32_t now = rtc::Time() - 1; On 2016/01/29 17:37:46, pthatcher1 wrote: > Well it's not really "now", is it? Isn't it "before"? Done. https://codereview.webrtc.org/1595613004/diff/80001/webrtc/p2p/base/turnport_... webrtc/p2p/base/turnport_unittest.cc:634: std::string next_nonce = turn_server_.server()->GenerateNonce(now); On 2016/01/29 17:37:46, pthatcher1 wrote: > Wouldn't a better name be first_nonce? Done. https://codereview.webrtc.org/1595613004/diff/80001/webrtc/p2p/base/turnserve... File webrtc/p2p/base/turnserver.cc (right): https://codereview.webrtc.org/1595613004/diff/80001/webrtc/p2p/base/turnserve... webrtc/p2p/base/turnserver.cc:468: ts_for_next_nonce_ = 0; On 2016/01/29 17:37:46, pthatcher1 wrote: > I think this would be slightly more readable as: > > uint32_t timestamp = rtc::Time(); > if (ts_for_next_nonce_) { > timestamp = ts_for_next_nonce_; > ts_for_next_nonce_ = 0; > } > Done. https://codereview.webrtc.org/1595613004/diff/80001/webrtc/p2p/base/turnserver.h File webrtc/p2p/base/turnserver.h (right): https://codereview.webrtc.org/1595613004/diff/80001/webrtc/p2p/base/turnserve... webrtc/p2p/base/turnserver.h:203: std::string GenerateNonce(uint32_t now) const; On 2016/01/29 17:37:46, pthatcher1 wrote: > Instead of making GenerateNonce visible, why not have SetTimestampForNextNonce > simply return GenerateNonce(timestamp)? That would make the test a little more > simple as well. Done.
lgtm
The CQ bit was checked by honghaiz@webrtc.org
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/1595613004/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1595613004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1595613004/100001
The CQ bit was checked by honghaiz@webrtc.org
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/1595613004/#ps120001 (title: "merge with head")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1595613004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1595613004/120001
Message was sent while issue was closed.
Description was changed from ========== Reset TURN port NONCE when a new socket is created. For example, when the TURN port has an ALLOCATE_MISMATCH error. BUG=webrtc:5432 ========== to ========== Reset TURN port NONCE when a new socket is created. For example, when the TURN port has an ALLOCATE_MISMATCH error. BUG=webrtc:5432 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Reset TURN port NONCE when a new socket is created. For example, when the TURN port has an ALLOCATE_MISMATCH error. BUG=webrtc:5432 ========== to ========== Reset TURN port NONCE when a new socket is created. For example, when the TURN port has an ALLOCATE_MISMATCH error. BUG=webrtc:5432 Committed: https://crrev.com/c463e20069c670594117a44755805ac0989cc137 Cr-Commit-Position: refs/heads/master@{#11453} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c463e20069c670594117a44755805ac0989cc137 Cr-Commit-Position: refs/heads/master@{#11453} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
