From b31ade36ffb6d70c8f85966dc6b7f33c9ed97866 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Tue, 13 Aug 2024 12:58:52 -0700 Subject: [PATCH] stun/turn: suppress icecandidateerror for incompatible address family Suppresses the ice candidate error callback when the STUN/TURN server address family is not compatible with the local candidate address family. This is similar to not pairing between candidates that have different incompatible address families as described in https://datatracker.ietf.org/doc/html/rfc5245#section-5.7.1 The spec actually says to emit the 701 error if *no* host candidate is able to reach the server: https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnectioniceerrorevent-errorcode Also use the same (spec) error code for STUN and TURN, see https://github.com/webrtc/samples/issues/1215 (error 600 for TURN) https://github.com/webrtc/samples/issues/1227 (error 701 with AF mismatch) Drive-by: misc logging fixes BUG=webrtc:359404135 Change-Id: I99574b7b2b79986a52ab38a7fa58ea1bebab954c Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/358961 Commit-Queue: Philipp Hancke Reviewed-by: Jonas Oreland Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#42830} --- api/transport/stun.h | 9 ++++-- p2p/base/stun_port.cc | 30 +++++++++++--------- p2p/base/stun_port_unittest.cc | 51 ++++++++++++++++++++++++---------- p2p/base/turn_port.cc | 50 ++++++++++++++++++++------------- p2p/base/turn_port_unittest.cc | 39 ++++++++++++++++++++------ 5 files changed, 122 insertions(+), 57 deletions(-) diff --git a/api/transport/stun.h b/api/transport/stun.h index ef5b08ecc6..8300f75505 100644 --- a/api/transport/stun.h +++ b/api/transport/stun.h @@ -95,13 +95,17 @@ enum StunAddressFamily { // These are the types of STUN error codes defined in RFC 5389. enum StunErrorCode { + // Not an actual error from RFC 5389 and not emitted via icecandidateerror. + STUN_ERROR_NOT_AN_ERROR = 0, STUN_ERROR_TRY_ALTERNATE = 300, STUN_ERROR_BAD_REQUEST = 400, STUN_ERROR_UNAUTHORIZED = 401, STUN_ERROR_UNKNOWN_ATTRIBUTE = 420, STUN_ERROR_STALE_NONCE = 438, STUN_ERROR_SERVER_ERROR = 500, - STUN_ERROR_GLOBAL_FAILURE = 600 + STUN_ERROR_GLOBAL_FAILURE = 600, + // https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnectioniceerrorevent-errorcode + STUN_ERROR_SERVER_NOT_REACHABLE = 701, }; // Strings for the error codes above. @@ -687,7 +691,8 @@ enum TurnErrorType { STUN_ERROR_UNSUPPORTED_PROTOCOL = 442 }; -extern const int SERVER_NOT_REACHABLE_ERROR; +[[deprecated("Use STUN_ERROR_SERVER_NOT_REACHABLE")]] extern const int + SERVER_NOT_REACHABLE_ERROR; extern const char STUN_ERROR_REASON_FORBIDDEN[]; extern const char STUN_ERROR_REASON_ALLOCATION_MISMATCH[]; diff --git a/p2p/base/stun_port.cc b/p2p/base/stun_port.cc index fd1eb72a92..bdd8830a29 100644 --- a/p2p/base/stun_port.cc +++ b/p2p/base/stun_port.cc @@ -102,7 +102,7 @@ class StunBindingRequest : public StunRequest { << port_->GetLocalAddress().ToSensitiveString() << " (" << port_->Network()->name() << ")"; port_->OnStunBindingOrResolveRequestFailed( - server_addr_, SERVER_NOT_REACHABLE_ERROR, + server_addr_, STUN_ERROR_SERVER_NOT_REACHABLE, "STUN binding request timed out."); } @@ -442,7 +442,7 @@ void UDPPort::OnResolveResult(const rtc::SocketAddress& input, int error) { RTC_LOG(LS_WARNING) << ToString() << ": StunPort: stun host lookup received error " << error; - OnStunBindingOrResolveRequestFailed(input, SERVER_NOT_REACHABLE_ERROR, + OnStunBindingOrResolveRequestFailed(input, STUN_ERROR_SERVER_NOT_REACHABLE, "STUN host lookup received error."); return; } @@ -466,11 +466,13 @@ void UDPPort::SendStunBindingRequest(const rtc::SocketAddress& stun_addr) { new StunBindingRequest(this, stun_addr, rtc::TimeMillis())); } else { // Since we can't send stun messages to the server, we should mark this - // port ready. - const char* reason = "STUN server address is incompatible."; - RTC_LOG(LS_WARNING) << reason; - OnStunBindingOrResolveRequestFailed(stun_addr, SERVER_NOT_REACHABLE_ERROR, - reason); + // port ready. This is not an error but similar to ignoring + // a mismatch of th address family when pairing candidates. + RTC_LOG(LS_WARNING) << ToString() + << ": STUN server address is incompatible."; + OnStunBindingOrResolveRequestFailed( + stun_addr, STUN_ERROR_NOT_AN_ERROR, + "STUN server address is incompatible."); } } } @@ -534,12 +536,14 @@ void UDPPort::OnStunBindingOrResolveRequestFailed( const rtc::SocketAddress& stun_server_addr, int error_code, absl::string_view reason) { - rtc::StringBuilder url; - url << "stun:" << stun_server_addr.ToString(); - SignalCandidateError( - this, IceCandidateErrorEvent(GetLocalAddress().HostAsSensitiveURIString(), - GetLocalAddress().port(), url.str(), - error_code, reason)); + if (error_code != STUN_ERROR_NOT_AN_ERROR) { + rtc::StringBuilder url; + url << "stun:" << stun_server_addr.ToString(); + SignalCandidateError( + this, IceCandidateErrorEvent( + GetLocalAddress().HostAsSensitiveURIString(), + GetLocalAddress().port(), url.str(), error_code, reason)); + } if (bind_request_failed_servers_.find(stun_server_addr) != bind_request_failed_servers_.end()) { return; diff --git a/p2p/base/stun_port_unittest.cc b/p2p/base/stun_port_unittest.cc index 842f3e49c6..08c224ad86 100644 --- a/p2p/base/stun_port_unittest.cc +++ b/p2p/base/stun_port_unittest.cc @@ -44,6 +44,7 @@ static const SocketAddress kStunAddr2("127.0.0.1", 4000); static const SocketAddress kStunAddr3("127.0.0.1", 3000); static const SocketAddress kIPv6StunAddr1("::1", 5000); static const SocketAddress kBadAddr("0.0.0.1", 5000); +static const SocketAddress kIPv6BadAddr("::ffff:0:1", 5000); static const SocketAddress kValidHostnameAddr("valid-hostname", 5000); static const SocketAddress kBadHostnameAddr("not-a-real-hostname", 5000); // STUN timeout (with all retries) is cricket::STUN_TOTAL_TIMEOUT. @@ -300,13 +301,24 @@ TEST_F(StunPortTest, TestPrepareAddressFail) { EXPECT_TRUE(error()); EXPECT_EQ(0U, port()->Candidates().size()); EXPECT_EQ_SIMULATED_WAIT(error_event_.error_code, - cricket::SERVER_NOT_REACHABLE_ERROR, kTimeoutMs, + cricket::STUN_ERROR_SERVER_NOT_REACHABLE, kTimeoutMs, fake_clock); - ASSERT_NE(error_event_.error_text.find('.'), std::string::npos); - ASSERT_NE(error_event_.address.find(kLocalAddr.HostAsSensitiveURIString()), + EXPECT_NE(error_event_.error_text.find('.'), std::string::npos); + EXPECT_NE(error_event_.address.find(kLocalAddr.HostAsSensitiveURIString()), std::string::npos); std::string server_url = "stun:" + kBadAddr.ToString(); - ASSERT_EQ(error_event_.url, server_url); + EXPECT_EQ(error_event_.url, server_url); +} + +// Test that we fail without emitting an error if we try to get an address from +// a STUN server with a different address family. IPv4 local, IPv6 STUN. +TEST_F(StunPortTest, TestServerAddressFamilyMismatch) { + CreateStunPort(kIPv6StunAddr1); + PrepareAddress(); + EXPECT_TRUE_SIMULATED_WAIT(done(), kTimeoutMs, fake_clock); + EXPECT_TRUE(error()); + EXPECT_EQ(0U, port()->Candidates().size()); + EXPECT_EQ(0, error_event_.error_code); } class StunPortWithMockDnsResolverTest : public StunPortTest { @@ -383,8 +395,8 @@ TEST_F(StunPortTestWithRealClock, TestPrepareAddressHostnameFail) { EXPECT_TRUE_WAIT(done(), kTimeoutMs); EXPECT_TRUE(error()); EXPECT_EQ(0U, port()->Candidates().size()); - EXPECT_EQ_WAIT(error_event_.error_code, cricket::SERVER_NOT_REACHABLE_ERROR, - kTimeoutMs); + EXPECT_EQ_WAIT(error_event_.error_code, + cricket::STUN_ERROR_SERVER_NOT_REACHABLE, kTimeoutMs); } // This test verifies keepalive response messages don't result in @@ -658,20 +670,31 @@ TEST_F(StunIPv6PortTest, TestPrepareAddress) { // Test that we fail properly if we can't get an address. TEST_F(StunIPv6PortTest, TestPrepareAddressFail) { - CreateStunPort(kBadAddr); + CreateStunPort(kIPv6BadAddr); PrepareAddress(); EXPECT_TRUE_SIMULATED_WAIT(done(), kTimeoutMs, fake_clock); EXPECT_TRUE(error()); EXPECT_EQ(0U, port()->Candidates().size()); EXPECT_EQ_SIMULATED_WAIT(error_event_.error_code, - cricket::SERVER_NOT_REACHABLE_ERROR, kTimeoutMs, + cricket::STUN_ERROR_SERVER_NOT_REACHABLE, kTimeoutMs, fake_clock); - ASSERT_NE(error_event_.error_text.find('.'), std::string::npos); - ASSERT_NE( + EXPECT_NE(error_event_.error_text.find('.'), std::string::npos); + EXPECT_NE( error_event_.address.find(kIPv6LocalAddr.HostAsSensitiveURIString()), std::string::npos); - std::string server_url = "stun:" + kBadAddr.ToString(); - ASSERT_EQ(error_event_.url, server_url); + std::string server_url = "stun:" + kIPv6BadAddr.ToString(); + EXPECT_EQ(error_event_.url, server_url); +} + +// Test that we fail without emitting an error if we try to get an address from +// a STUN server with a different address family. IPv6 local, IPv4 STUN. +TEST_F(StunIPv6PortTest, TestServerAddressFamilyMismatch) { + CreateStunPort(kStunAddr1); + PrepareAddress(); + EXPECT_TRUE_SIMULATED_WAIT(done(), kTimeoutMs, fake_clock); + EXPECT_TRUE(error()); + EXPECT_EQ(0U, port()->Candidates().size()); + EXPECT_EQ(0, error_event_.error_code); } // Test that we handle hostname lookup failures properly with a real clock. @@ -681,8 +704,8 @@ TEST_F(StunIPv6PortTestWithRealClock, TestPrepareAddressHostnameFail) { EXPECT_TRUE_WAIT(done(), kTimeoutMs); EXPECT_TRUE(error()); EXPECT_EQ(0U, port()->Candidates().size()); - EXPECT_EQ_WAIT(error_event_.error_code, cricket::SERVER_NOT_REACHABLE_ERROR, - kTimeoutMs); + EXPECT_EQ_WAIT(error_event_.error_code, + cricket::STUN_ERROR_SERVER_NOT_REACHABLE, kTimeoutMs); } class StunIPv6PortTestWithMockDnsResolver : public StunIPv6PortTest { diff --git a/p2p/base/turn_port.cc b/p2p/base/turn_port.cc index 1745019ca8..4dba2a5708 100644 --- a/p2p/base/turn_port.cc +++ b/p2p/base/turn_port.cc @@ -391,7 +391,8 @@ std::vector TurnPort::GetTlsEllipticCurves() const { void TurnPort::PrepareAddress() { if (credentials_.username.empty() || credentials_.password.empty()) { - RTC_LOG(LS_ERROR) << "Allocation can't be started without setting the" + RTC_LOG(LS_ERROR) << ToString() + << ": Allocation can't be started without setting the" " TURN server credentials for the user."; OnAllocateError(STUN_ERROR_UNAUTHORIZED, "Missing TURN server credentials."); @@ -406,7 +407,8 @@ void TurnPort::PrepareAddress() { if (!AllowedTurnPort(server_address_.address.port(), &field_trials())) { // This can only happen after a 300 ALTERNATE SERVER, since the port can't // be created with a disallowed port number. - RTC_LOG(LS_ERROR) << "Attempt to start allocation with disallowed port# " + RTC_LOG(LS_ERROR) << ToString() + << ": Attempt to start allocation with disallowed port# " << server_address_.address.port(); OnAllocateError(STUN_ERROR_SERVER_ERROR, "Attempt to start allocation to a disallowed port"); @@ -417,11 +419,12 @@ void TurnPort::PrepareAddress() { } else { // If protocol family of server address doesn't match with local, return. if (!IsCompatibleAddress(server_address_.address)) { - RTC_LOG(LS_ERROR) << "IP address family does not match. server: " + RTC_LOG(LS_ERROR) << ToString() + << ": IP address family does not match. Server: " << server_address_.address.family() << " local: " << Network()->GetBestIP().family(); - OnAllocateError(STUN_ERROR_GLOBAL_FAILURE, - "IP address family does not match."); + OnAllocateError(STUN_ERROR_NOT_AN_ERROR, + "TURN server address is incompatible."); return; } @@ -433,8 +436,9 @@ void TurnPort::PrepareAddress() { << ProtoToString(server_address_.proto) << " @ " << server_address_.address.ToSensitiveNameAndAddressString(); if (!CreateTurnClientSocket()) { - RTC_LOG(LS_ERROR) << "Failed to create TURN client socket"; - OnAllocateError(SERVER_NOT_REACHABLE_ERROR, + RTC_LOG(LS_ERROR) << ToString() + << ": Failed to create TURN client socket"; + OnAllocateError(STUN_ERROR_SERVER_NOT_REACHABLE, "Failed to create TURN client socket."); return; } @@ -540,26 +544,26 @@ void TurnPort::OnSocketConnect(rtc::AsyncPacketSocket* socket) { return socket_address.ipaddr() == addr; })) { if (socket->GetLocalAddress().IsLoopbackIP()) { - RTC_LOG(LS_WARNING) << "Socket is bound to the address:" + RTC_LOG(LS_WARNING) << ToString() << ": Socket is bound to the address:" << socket_address.ToSensitiveNameAndAddressString() << ", rather than an address associated with network:" << Network()->ToString() << ". Still allowing it since it's localhost."; } else if (IPIsAny(Network()->GetBestIP())) { RTC_LOG(LS_WARNING) - << "Socket is bound to the address:" + << ToString() << ": Socket is bound to the address:" << socket_address.ToSensitiveNameAndAddressString() << ", rather than an address associated with network:" << Network()->ToString() << ". Still allowing it since it's the 'any' address" ", possibly caused by multiple_routes being disabled."; } else { - RTC_LOG(LS_WARNING) << "Socket is bound to the address:" + RTC_LOG(LS_WARNING) << ToString() << ": Socket is bound to the address:" << socket_address.ToSensitiveNameAndAddressString() << ", rather than an address associated with network:" << Network()->ToString() << ". Discarding TURN port."; OnAllocateError( - STUN_ERROR_GLOBAL_FAILURE, + STUN_ERROR_SERVER_NOT_REACHABLE, "Address not associated with the desired network interface."); return; } @@ -835,7 +839,8 @@ bool TurnPort::SetAlternateServer(const rtc::SocketAddress& address) { // If protocol family of server address doesn't match with local, return. if (!IsCompatibleAddress(address)) { - RTC_LOG(LS_WARNING) << "Server IP address family does not match with " + RTC_LOG(LS_WARNING) << ToString() + << ": Server IP address family does not match with " "local host address family type"; return false; } @@ -876,7 +881,7 @@ void TurnPort::ResolveTurnAddress(const rtc::SocketAddress& address) { if (result.GetError() != 0 && (server_address_.proto == PROTO_TCP || server_address_.proto == PROTO_TLS)) { if (!CreateTurnClientSocket()) { - OnAllocateError(SERVER_NOT_REACHABLE_ERROR, + OnAllocateError(STUN_ERROR_SERVER_NOT_REACHABLE, "TURN host lookup received error."); } return; @@ -891,7 +896,7 @@ void TurnPort::ResolveTurnAddress(const rtc::SocketAddress& address) { RTC_LOG(LS_WARNING) << ToString() << ": TURN host lookup received error " << result.GetError(); error_ = result.GetError(); - OnAllocateError(SERVER_NOT_REACHABLE_ERROR, + OnAllocateError(STUN_ERROR_SERVER_NOT_REACHABLE, "TURN host lookup received error."); return; } @@ -955,8 +960,11 @@ void TurnPort::OnAllocateError(int error_code, absl::string_view reason) { address.clear(); port = 0; } - SignalCandidateError(this, IceCandidateErrorEvent(address, port, server_url_, - error_code, reason)); + if (error_code != STUN_ERROR_NOT_AN_ERROR) { + SignalCandidateError( + this, + IceCandidateErrorEvent(address, port, server_url_, error_code, reason)); + } } void TurnPort::OnRefreshError() { @@ -990,7 +998,7 @@ void TurnPort::Release() { void TurnPort::Close() { if (!ready()) { OnAllocateError( - SERVER_NOT_REACHABLE_ERROR, + STUN_ERROR_SERVER_NOT_REACHABLE, GetProtocol() != PROTO_UDP ? "Failed to establish connection" : ""); } request_manager_.Clear(); @@ -1037,7 +1045,7 @@ void TurnPort::TryAlternateServer() { } void TurnPort::OnAllocateRequestTimeout() { - OnAllocateError(SERVER_NOT_REACHABLE_ERROR, + OnAllocateError(STUN_ERROR_SERVER_NOT_REACHABLE, "TURN allocate request timed out."); } @@ -1212,7 +1220,8 @@ bool TurnPort::UpdateNonce(StunMessage* response) { const StunByteStringAttribute* realm_attr = response->GetByteString(STUN_ATTR_REALM); if (!realm_attr) { - RTC_LOG(LS_ERROR) << "Missing STUN_ATTR_REALM attribute in " + RTC_LOG(LS_ERROR) << ToString() + << ": Missing STUN_ATTR_REALM attribute in " "stale nonce error response."; return false; } @@ -1221,7 +1230,8 @@ bool TurnPort::UpdateNonce(StunMessage* response) { const StunByteStringAttribute* nonce_attr = response->GetByteString(STUN_ATTR_NONCE); if (!nonce_attr) { - RTC_LOG(LS_ERROR) << "Missing STUN_ATTR_NONCE attribute in " + RTC_LOG(LS_ERROR) << ToString() + << ": Missing STUN_ATTR_NONCE attribute in " "stale nonce error response."; return false; } diff --git a/p2p/base/turn_port_unittest.cc b/p2p/base/turn_port_unittest.cc index 0726128fce..b0f3124912 100644 --- a/p2p/base/turn_port_unittest.cc +++ b/p2p/base/turn_port_unittest.cc @@ -970,6 +970,27 @@ TEST_F(TurnPortTest, TestTurnBadCredentials) { EXPECT_EQ(error_event_.error_text, "Unauthorized"); } +// Test that we fail without emitting an error if we try to get an address from +// a TURN server with a different address family. IPv4 local, IPv6 TURN. +TEST_F(TurnPortTest, TestServerAddressFamilyMismatch) { + CreateTurnPort(kTurnUsername, kTurnPassword, kTurnUdpIPv6ProtoAddr); + turn_port_->PrepareAddress(); + EXPECT_TRUE_SIMULATED_WAIT(turn_error_, kSimulatedRtt * 3, fake_clock_); + ASSERT_EQ(0U, turn_port_->Candidates().size()); + EXPECT_EQ(0, error_event_.error_code); +} + +// Test that we fail without emitting an error if we try to get an address from +// a TURN server with a different address family. IPv6 local, IPv4 TURN. +TEST_F(TurnPortTest, TestServerAddressFamilyMismatch6) { + CreateTurnPort(kLocalIPv6Addr, kTurnUsername, kTurnPassword, + kTurnUdpProtoAddr); + turn_port_->PrepareAddress(); + EXPECT_TRUE_SIMULATED_WAIT(turn_error_, kSimulatedRtt * 3, fake_clock_); + ASSERT_EQ(0U, turn_port_->Candidates().size()); + EXPECT_EQ(0, error_event_.error_code); +} + // Testing a normal UDP allocation using TCP connection. TEST_F(TurnPortTest, TestTurnTcpAllocate) { turn_server_.AddInternalSocket(kTurnTcpIntAddr, PROTO_TCP); @@ -1019,15 +1040,16 @@ TEST_F(TurnPortTest, // Shouldn't take more than 1 RTT to realize the bound address isn't the one // expected. EXPECT_TRUE_SIMULATED_WAIT(turn_error_, kSimulatedRtt, fake_clock_); - EXPECT_EQ_SIMULATED_WAIT(error_event_.error_code, STUN_ERROR_GLOBAL_FAILURE, - kSimulatedRtt, fake_clock_); - ASSERT_NE(error_event_.error_text.find('.'), std::string::npos); - ASSERT_NE(error_event_.address.find(kLocalAddr2.HostAsSensitiveURIString()), + EXPECT_EQ_SIMULATED_WAIT(error_event_.error_code, + STUN_ERROR_SERVER_NOT_REACHABLE, kSimulatedRtt, + fake_clock_); + EXPECT_NE(error_event_.error_text.find('.'), std::string::npos); + EXPECT_NE(error_event_.address.find(kLocalAddr2.HostAsSensitiveURIString()), std::string::npos); - ASSERT_NE(error_event_.port, 0); + EXPECT_NE(error_event_.port, 0); std::string server_url = "turn:" + kTurnTcpIntAddr.ToString() + "?transport=tcp"; - ASSERT_EQ(error_event_.url, server_url); + EXPECT_EQ(error_event_.url, server_url); } // A caveat for the above logic: if the socket ends up bound to one of the IPs @@ -1095,8 +1117,9 @@ TEST_F(TurnPortTest, TestTurnTcpOnAddressResolveFailure) { // will proceed in creating a TCP socket which will fail as there is no // server on the above domain and error will be set to SOCKET_ERROR. EXPECT_EQ(SOCKET_ERROR, turn_port_->error()); - EXPECT_EQ_SIMULATED_WAIT(error_event_.error_code, SERVER_NOT_REACHABLE_ERROR, - kSimulatedRtt, fake_clock_); + EXPECT_EQ_SIMULATED_WAIT(error_event_.error_code, + STUN_ERROR_SERVER_NOT_REACHABLE, kSimulatedRtt, + fake_clock_); std::string server_url = "turn:" + kTurnInvalidAddr.ToString() + "?transport=tcp"; ASSERT_EQ(error_event_.url, server_url);