From db4def9f59e8f58e49185975f13f683650e98cd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Mon, 18 Mar 2019 16:53:59 +0100 Subject: [PATCH] Update parsing of stun and turn urls for RFC 7064-7065 Main change is deleting support for @userinfo in turn urls. This was specified in early internet drafts, but never made it into RFC 7065. Bug: webrtc:6663, webrtc:10422 Change-Id: Idd315a9e6001326f3104be62be3bd0991adc7db4 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/128423 Commit-Queue: Niels Moller Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/master@{#27171} --- pc/ice_server_parsing.cc | 71 +++++++++--------------- pc/ice_server_parsing_unittest.cc | 13 +---- pc/peer_connection_factory_unittest.cc | 44 +++++++++------ pc/peer_connection_interface_unittest.cc | 9 ++- 4 files changed, 61 insertions(+), 76 deletions(-) diff --git a/pc/ice_server_parsing.cc b/pc/ice_server_parsing.cc index 533d59787d..3d05d66b13 100644 --- a/pc/ice_server_parsing.cc +++ b/pc/ice_server_parsing.cc @@ -20,13 +20,9 @@ #include "rtc_base/ip_address.h" #include "rtc_base/logging.h" #include "rtc_base/socket_address.h" -#include "rtc_base/string_encode.h" namespace webrtc { -// The min number of tokens must present in Turn host uri. -// e.g. user@turn.example.org -static const size_t kTurnHostTokensNum = 2; // Number of tokens must be preset when TURN uri has transport param. static const size_t kTurnTransportTokensNum = 2; // The default stun port. @@ -50,15 +46,12 @@ static_assert(INVALID == arraysize(kValidIceServiceTypes), "kValidIceServiceTypes must have as many strings as ServiceType " "has values."); -// |in_str| should be of format -// stunURI = scheme ":" stun-host [ ":" stun-port ] -// scheme = "stun" / "stuns" -// stun-host = IP-literal / IPv4address / reg-name -// stun-port = *DIGIT -// -// draft-petithuguenin-behave-turn-uris-01 -// turnURI = scheme ":" turn-host [ ":" turn-port ] -// turn-host = username@IP-literal / IPv4address / reg-name +// |in_str| should follow of RFC 7064/7065 syntax, but with an optional +// "?transport=" already stripped. I.e., +// stunURI = scheme ":" host [ ":" port ] +// scheme = "stun" / "stuns" / "turn" / "turns" +// host = IP-literal / IPv4address / reg-name +// port = *DIGIT static bool GetServiceTypeAndHostnameFromUri(const std::string& in_str, ServiceType* service_type, std::string* hostname) { @@ -139,20 +132,21 @@ static RTCErrorType ParseIceServerUrl( const std::string& url, cricket::ServerAddresses* stun_servers, std::vector* turn_servers) { - // draft-nandakumar-rtcweb-stun-uri-01 - // stunURI = scheme ":" stun-host [ ":" stun-port ] + // RFC 7064 + // stunURI = scheme ":" host [ ":" port ] // scheme = "stun" / "stuns" - // stun-host = IP-literal / IPv4address / reg-name - // stun-port = *DIGIT - // draft-petithuguenin-behave-turn-uris-01 - // turnURI = scheme ":" turn-host [ ":" turn-port ] + // RFC 7065 + // turnURI = scheme ":" host [ ":" port ] // [ "?transport=" transport ] // scheme = "turn" / "turns" // transport = "udp" / "tcp" / transport-ext // transport-ext = 1*unreserved - // turn-host = IP-literal / IPv4address / reg-name - // turn-port = *DIGIT + + // RFC 3986 + // host = IP-literal / IPv4address / reg-name + // port = *DIGIT + RTC_DCHECK(stun_servers != nullptr); RTC_DCHECK(turn_servers != nullptr); std::vector tokens; @@ -191,32 +185,18 @@ static RTCErrorType ParseIceServerUrl( // GetServiceTypeAndHostnameFromUri should never give an empty hoststring RTC_DCHECK(!hoststring.empty()); - // Let's break hostname. - tokens.clear(); - rtc::tokenize_with_empty_tokens(hoststring, '@', &tokens); - - std::string username(server.username); - if (tokens.size() > kTurnHostTokensNum) { - RTC_LOG(LS_WARNING) << "Invalid user@hostname format: " << hoststring; - return RTCErrorType::SYNTAX_ERROR; - } - if (tokens.size() == kTurnHostTokensNum) { - if (tokens[0].empty() || tokens[1].empty()) { - RTC_LOG(LS_WARNING) << "Invalid user@hostname format: " << hoststring; - return RTCErrorType::SYNTAX_ERROR; - } - username.assign(rtc::s_url_decode(tokens[0])); - hoststring = tokens[1]; - } else { - hoststring = tokens[0]; - } - int port = kDefaultStunPort; if (service_type == TURNS) { port = kDefaultStunTlsPort; turn_transport_type = cricket::PROTO_TLS; } + if (hoststring.find('@') != std::string::npos) { + RTC_LOG(WARNING) << "Invalid url: " << uri_without_transport; + RTC_LOG(WARNING) + << "Note that user-info@ in turn:-urls is long-deprecated."; + return RTCErrorType::SYNTAX_ERROR; + } std::string address; if (!ParseHostnameAndPortFromString(hoststring, &address, &port)) { RTC_LOG(WARNING) << "Invalid hostname format: " << uri_without_transport; @@ -235,10 +215,10 @@ static RTCErrorType ParseIceServerUrl( break; case TURN: case TURNS: { - if (username.empty() || server.password.empty()) { + if (server.username.empty() || server.password.empty()) { // The WebRTC spec requires throwing an InvalidAccessError when username // or credential are ommitted; this is the native equivalent. - RTC_LOG(LS_ERROR) << "TURN URL without username, or password empty"; + RTC_LOG(LS_ERROR) << "TURN server with empty username or password"; return RTCErrorType::INVALID_PARAMETER; } // If the hostname field is not empty, then the server address must be @@ -259,8 +239,9 @@ static RTCErrorType ParseIceServerUrl( } socket_address.SetResolvedIP(ip); } - cricket::RelayServerConfig config = cricket::RelayServerConfig( - socket_address, username, server.password, turn_transport_type); + cricket::RelayServerConfig config = + cricket::RelayServerConfig(socket_address, server.username, + server.password, turn_transport_type); if (server.tls_cert_policy == PeerConnectionInterface::kTlsCertPolicyInsecureNoCheck) { config.tls_cert_policy = diff --git a/pc/ice_server_parsing_unittest.cc b/pc/ice_server_parsing_unittest.cc index 0ec59caa23..9f1b81718e 100644 --- a/pc/ice_server_parsing_unittest.cc +++ b/pc/ice_server_parsing_unittest.cc @@ -200,16 +200,9 @@ TEST_F(IceServerParsingTest, ParseTransport) { EXPECT_FALSE(ParseTurnUrl("?")); } -// Test parsing ICE username contained in URL. -TEST_F(IceServerParsingTest, ParseUsername) { - EXPECT_TRUE(ParseTurnUrl("turn:user@hostname")); - EXPECT_EQ(1U, turn_servers_.size()); - EXPECT_EQ("user", turn_servers_[0].credentials.username); - - EXPECT_FALSE(ParseTurnUrl("turn:@hostname")); - EXPECT_FALSE(ParseTurnUrl("turn:username@")); - EXPECT_FALSE(ParseTurnUrl("turn:@")); - EXPECT_FALSE(ParseTurnUrl("turn:user@name@hostname")); +// Reject pre-RFC 7065 syntax with ICE username contained in URL. +TEST_F(IceServerParsingTest, ParseRejectsUsername) { + EXPECT_FALSE(ParseTurnUrl("turn:user@hostname")); } // Test that username and password from IceServer is copied into the resulting diff --git a/pc/peer_connection_factory_unittest.cc b/pc/peer_connection_factory_unittest.cc index 1c3eff7a73..41a540839c 100644 --- a/pc/peer_connection_factory_unittest.cc +++ b/pc/peer_connection_factory_unittest.cc @@ -57,14 +57,14 @@ using webrtc::VideoTrackInterface; namespace { static const char kStunIceServer[] = "stun:stun.l.google.com:19302"; -static const char kTurnIceServer[] = "turn:test%40hello.com@test.com:1234"; +static const char kTurnIceServer[] = "turn:test.com:1234"; static const char kTurnIceServerWithTransport[] = - "turn:test@hello.com?transport=tcp"; -static const char kSecureTurnIceServer[] = "turns:test@hello.com?transport=tcp"; + "turn:hello.com?transport=tcp"; +static const char kSecureTurnIceServer[] = "turns:hello.com?transport=tcp"; static const char kSecureTurnIceServerWithoutTransportParam[] = - "turns:test_no_transport@hello.com:443"; + "turns:hello.com:443"; static const char kSecureTurnIceServerWithoutTransportAndPortParam[] = - "turns:test_no_transport@hello.com"; + "turns:hello.com"; static const char kTurnIceServerWithNoUsernameInUri[] = "turn:test.com:1234"; static const char kTurnPassword[] = "turnpassword"; static const int kDefaultStunPort = 3478; @@ -75,8 +75,7 @@ static const char kStunIceServerWithIPv4AddressWithoutPort[] = "stun:1.2.3.4"; static const char kStunIceServerWithIPv6Address[] = "stun:[2401:fa00:4::]:1234"; static const char kStunIceServerWithIPv6AddressWithoutPort[] = "stun:[2401:fa00:4::]"; -static const char kTurnIceServerWithIPv6Address[] = - "turn:test@[2401:fa00:4::]:1234"; +static const char kTurnIceServerWithIPv6Address[] = "turn:[2401:fa00:4::]:1234"; class NullPeerConnectionObserver : public PeerConnectionObserver { public: @@ -272,9 +271,11 @@ TEST_F(PeerConnectionFactoryTest, CreatePCUsingIceServers) { ice_server.uri = kStunIceServer; config.servers.push_back(ice_server); ice_server.uri = kTurnIceServer; + ice_server.username = kTurnUsername; ice_server.password = kTurnPassword; config.servers.push_back(ice_server); ice_server.uri = kTurnIceServerWithTransport; + ice_server.username = kTurnUsername; ice_server.password = kTurnPassword; config.servers.push_back(ice_server); std::unique_ptr cert_generator( @@ -288,10 +289,10 @@ TEST_F(PeerConnectionFactoryTest, CreatePCUsingIceServers) { stun_servers.insert(stun1); VerifyStunServers(stun_servers); std::vector turn_servers; - cricket::RelayServerConfig turn1("test.com", 1234, "test@hello.com", + cricket::RelayServerConfig turn1("test.com", 1234, kTurnUsername, kTurnPassword, cricket::PROTO_UDP); turn_servers.push_back(turn1); - cricket::RelayServerConfig turn2("hello.com", kDefaultStunPort, "test", + cricket::RelayServerConfig turn2("hello.com", kDefaultStunPort, kTurnUsername, kTurnPassword, cricket::PROTO_TCP); turn_servers.push_back(turn2); VerifyTurnServers(turn_servers); @@ -305,6 +306,7 @@ TEST_F(PeerConnectionFactoryTest, CreatePCUsingIceServersUrls) { ice_server.urls.push_back(kStunIceServer); ice_server.urls.push_back(kTurnIceServer); ice_server.urls.push_back(kTurnIceServerWithTransport); + ice_server.username = kTurnUsername; ice_server.password = kTurnPassword; config.servers.push_back(ice_server); std::unique_ptr cert_generator( @@ -318,10 +320,10 @@ TEST_F(PeerConnectionFactoryTest, CreatePCUsingIceServersUrls) { stun_servers.insert(stun1); VerifyStunServers(stun_servers); std::vector turn_servers; - cricket::RelayServerConfig turn1("test.com", 1234, "test@hello.com", + cricket::RelayServerConfig turn1("test.com", 1234, kTurnUsername, kTurnPassword, cricket::PROTO_UDP); turn_servers.push_back(turn1); - cricket::RelayServerConfig turn2("hello.com", kDefaultStunPort, "test", + cricket::RelayServerConfig turn2("hello.com", kDefaultStunPort, kTurnUsername, kTurnPassword, cricket::PROTO_TCP); turn_servers.push_back(turn2); VerifyTurnServers(turn_servers); @@ -355,6 +357,7 @@ TEST_F(PeerConnectionFactoryTest, CreatePCUsingTurnUrlWithTransportParam) { PeerConnectionInterface::RTCConfiguration config; webrtc::PeerConnectionInterface::IceServer ice_server; ice_server.uri = kTurnIceServerWithTransport; + ice_server.username = kTurnUsername; ice_server.password = kTurnPassword; config.servers.push_back(ice_server); std::unique_ptr cert_generator( @@ -364,7 +367,7 @@ TEST_F(PeerConnectionFactoryTest, CreatePCUsingTurnUrlWithTransportParam) { std::move(cert_generator), &observer_)); ASSERT_TRUE(pc.get() != NULL); std::vector turn_servers; - cricket::RelayServerConfig turn("hello.com", kDefaultStunPort, "test", + cricket::RelayServerConfig turn("hello.com", kDefaultStunPort, kTurnUsername, kTurnPassword, cricket::PROTO_TCP); turn_servers.push_back(turn); VerifyTurnServers(turn_servers); @@ -374,12 +377,15 @@ TEST_F(PeerConnectionFactoryTest, CreatePCUsingSecureTurnUrl) { PeerConnectionInterface::RTCConfiguration config; webrtc::PeerConnectionInterface::IceServer ice_server; ice_server.uri = kSecureTurnIceServer; + ice_server.username = kTurnUsername; ice_server.password = kTurnPassword; config.servers.push_back(ice_server); ice_server.uri = kSecureTurnIceServerWithoutTransportParam; + ice_server.username = kTurnUsername; ice_server.password = kTurnPassword; config.servers.push_back(ice_server); ice_server.uri = kSecureTurnIceServerWithoutTransportAndPortParam; + ice_server.username = kTurnUsername; ice_server.password = kTurnPassword; config.servers.push_back(ice_server); std::unique_ptr cert_generator( @@ -389,15 +395,16 @@ TEST_F(PeerConnectionFactoryTest, CreatePCUsingSecureTurnUrl) { std::move(cert_generator), &observer_)); ASSERT_TRUE(pc.get() != NULL); std::vector turn_servers; - cricket::RelayServerConfig turn1("hello.com", kDefaultStunTlsPort, "test", - kTurnPassword, cricket::PROTO_TLS); + cricket::RelayServerConfig turn1("hello.com", kDefaultStunTlsPort, + kTurnUsername, kTurnPassword, + cricket::PROTO_TLS); turn_servers.push_back(turn1); // TURNS with transport param should be default to tcp. - cricket::RelayServerConfig turn2("hello.com", 443, "test_no_transport", + cricket::RelayServerConfig turn2("hello.com", 443, kTurnUsername, kTurnPassword, cricket::PROTO_TLS); turn_servers.push_back(turn2); cricket::RelayServerConfig turn3("hello.com", kDefaultStunTlsPort, - "test_no_transport", kTurnPassword, + kTurnUsername, kTurnPassword, cricket::PROTO_TLS); turn_servers.push_back(turn3); VerifyTurnServers(turn_servers); @@ -415,6 +422,7 @@ TEST_F(PeerConnectionFactoryTest, CreatePCUsingIPLiteralAddress) { ice_server.uri = kStunIceServerWithIPv6AddressWithoutPort; config.servers.push_back(ice_server); ice_server.uri = kTurnIceServerWithIPv6Address; + ice_server.username = kTurnUsername; ice_server.password = kTurnPassword; config.servers.push_back(ice_server); std::unique_ptr cert_generator( @@ -435,8 +443,8 @@ TEST_F(PeerConnectionFactoryTest, CreatePCUsingIPLiteralAddress) { VerifyStunServers(stun_servers); std::vector turn_servers; - cricket::RelayServerConfig turn1("2401:fa00:4::", 1234, "test", kTurnPassword, - cricket::PROTO_UDP); + cricket::RelayServerConfig turn1("2401:fa00:4::", 1234, kTurnUsername, + kTurnPassword, cricket::PROTO_UDP); turn_servers.push_back(turn1); VerifyTurnServers(turn_servers); } diff --git a/pc/peer_connection_interface_unittest.cc b/pc/peer_connection_interface_unittest.cc index 06d077a079..7cec6ed9dc 100644 --- a/pc/peer_connection_interface_unittest.cc +++ b/pc/peer_connection_interface_unittest.cc @@ -104,7 +104,7 @@ static const char kStunAddressOnly[] = "stun:address"; static const char kStunInvalidPort[] = "stun:address:-1"; static const char kStunAddressPortAndMore1[] = "stun:address:port:more"; static const char kStunAddressPortAndMore2[] = "stun:address:port more"; -static const char kTurnIceServerUri[] = "turn:user@turn.example.org"; +static const char kTurnIceServerUri[] = "turn:turn.example.org"; static const char kTurnUsername[] = "user"; static const char kTurnPassword[] = "password"; static const char kTurnHostname[] = "turn.example.org"; @@ -729,10 +729,12 @@ class PeerConnectionInterfaceBaseTest : public testing::Test { } void CreatePeerConnectionWithIceServer(const std::string& uri, + const std::string& username, const std::string& password) { PeerConnectionInterface::RTCConfiguration config; PeerConnectionInterface::IceServer server; server.uri = uri; + server.username = username; server.password = password; config.servers.push_back(server); CreatePeerConnection(config); @@ -785,7 +787,7 @@ class PeerConnectionInterfaceBaseTest : public testing::Test { } void CreatePeerConnectionWithDifferentConfigurations() { - CreatePeerConnectionWithIceServer(kStunAddressOnly, ""); + CreatePeerConnectionWithIceServer(kStunAddressOnly, "", ""); EXPECT_EQ(1u, port_allocator_->stun_servers().size()); EXPECT_EQ(0u, port_allocator_->turn_servers().size()); EXPECT_EQ("address", port_allocator_->stun_servers().begin()->hostname()); @@ -796,7 +798,8 @@ class PeerConnectionInterfaceBaseTest : public testing::Test { CreatePeerConnectionExpectFail(kStunAddressPortAndMore1); CreatePeerConnectionExpectFail(kStunAddressPortAndMore2); - CreatePeerConnectionWithIceServer(kTurnIceServerUri, kTurnPassword); + CreatePeerConnectionWithIceServer(kTurnIceServerUri, kTurnUsername, + kTurnPassword); EXPECT_EQ(0u, port_allocator_->stun_servers().size()); EXPECT_EQ(1u, port_allocator_->turn_servers().size()); EXPECT_EQ(kTurnUsername,