From 0816aa3e92bb4892fe5bbeb8fd0d6291f7422490 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Fri, 9 Feb 2018 17:44:32 -0800 Subject: [PATCH 1/6] listener: Test for v4-mapped addressing consistency. Fail if remote and local addresses of a new server connection are of different address instance types. Signed-off-by: Jarno Rajahalme --- source/common/network/utility.cc | 19 ++++++++-- source/common/network/utility.h | 3 +- test/common/network/listener_impl_test.cc | 43 +++++++++++++++++++++++ test/test_common/network_utility.cc | 4 +-- test/test_common/network_utility.h | 3 +- 5 files changed, 65 insertions(+), 7 deletions(-) diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index bd97e09a413d4..87fe7d46d76e4 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -270,10 +270,23 @@ Address::InstanceConstSharedPtr Utility::getIpv4AnyAddress() { return any; } -Address::InstanceConstSharedPtr Utility::getIpv6AnyAddress() { +// There is no portable way to initialize sockaddr_in6 with a static initializer, do it with a +// helper function instead +static sockaddr_in6& v6any_() { + static sockaddr_in6 v6any = {}; + v6any.sin6_family = AF_INET6; + v6any.sin6_addr = in6addr_any; + + return v6any; +} + +Address::InstanceConstSharedPtr Utility::getIpv6AnyAddress(bool v6only) { // Initialized on first call in a thread-safe manner. - static Address::InstanceConstSharedPtr any(new Address::Ipv6Instance(static_cast(0))); - return any; + static sockaddr_in6 v6any = v6any_(); + static Address::InstanceConstSharedPtr any(new Address::Ipv6Instance(v6any, false)); + static Address::InstanceConstSharedPtr any_v6only(new Address::Ipv6Instance(v6any, true)); + + return v6only ? any_v6only : any; } Address::InstanceConstSharedPtr Utility::getAddressWithPort(const Address::Instance& address, diff --git a/source/common/network/utility.h b/source/common/network/utility.h index 8def4a45952ae..ea047e6f6b843 100644 --- a/source/common/network/utility.h +++ b/source/common/network/utility.h @@ -146,11 +146,12 @@ class Utility { static Address::InstanceConstSharedPtr getIpv4AnyAddress(); /** + * @param v6only a boolean to determine how to set the IPV6_V6ONLY socket option. * @return Address::InstanceConstSharedPtr an address that represents the IPv6 wildcard address * (i.e. "::"). Used during binding to indicate that incoming connections to any local * IPv6 address are to be accepted. */ - static Address::InstanceConstSharedPtr getIpv6AnyAddress(); + static Address::InstanceConstSharedPtr getIpv6AnyAddress(bool v6only = true); /** * @param address IP address instance. diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index d7c0dc3ff170c..c6778ba4d53c0 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -154,5 +154,48 @@ TEST_P(ListenerImplTest, WildcardListenerUseActualDst) { dispatcher.run(Event::Dispatcher::RunType::Block); } +TEST_P(ListenerImplTest, WildcardListenerIpv4Compat) { + Stats::IsolatedStoreImpl stats_store; + Event::DispatcherImpl dispatcher; + Network::TcpListenSocket socket(Network::Test::getAnyAddress(version_, true), true); + Network::MockListenerCallbacks listener_callbacks; + Network::MockConnectionHandler connection_handler; + + ASSERT_TRUE(socket.localAddress()->ip()->isAnyAddress()); + + // Do not redirect since use_original_dst is false. + Network::TestListenerImpl listener(dispatcher, socket, listener_callbacks, true, true); + + auto listener_address = Network::Utility::getAddressWithPort( + *Network::Test::getCanonicalLoopbackAddress(version_), socket.localAddress()->ip()->port()); + auto local_dst_address = Network::Utility::getAddressWithPort( + *Network::Utility::getCanonicalIpv4LoopbackAddress(), socket.localAddress()->ip()->port()); + Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( + local_dst_address, Network::Address::InstanceConstSharedPtr(), + Network::Test::createRawBufferSocket(), nullptr); + client_connection->connect(); + + EXPECT_CALL(listener, getLocalAddress(_)) + .WillOnce(Invoke( + [](int fd) -> Address::InstanceConstSharedPtr { return Address::addressFromFd(fd); })); + + EXPECT_CALL(listener_callbacks, onAccept_(_, _)) + .WillOnce(Invoke([&](Network::ConnectionSocketPtr& socket, bool) -> void { + Network::ConnectionPtr new_connection = dispatcher.createServerConnection( + std::move(socket), Network::Test::createRawBufferSocket()); + listener_callbacks.onNewConnection(std::move(new_connection)); + })); + EXPECT_CALL(listener_callbacks, onNewConnection_(_)) + .WillOnce(Invoke([&](Network::ConnectionPtr& conn) -> void { + EXPECT_EQ(conn->localAddress()->ip()->version(), conn->remoteAddress()->ip()->version()); + EXPECT_EQ(*conn->localAddress(), *local_dst_address); + client_connection->close(ConnectionCloseType::NoFlush); + conn->close(ConnectionCloseType::NoFlush); + dispatcher.exit(); + })); + + dispatcher.run(Event::Dispatcher::RunType::Block); +} + } // namespace Network } // namespace Envoy diff --git a/test/test_common/network_utility.cc b/test/test_common/network_utility.cc index 5c99cf00ceaeb..30838f78a459f 100644 --- a/test/test_common/network_utility.cc +++ b/test/test_common/network_utility.cc @@ -116,11 +116,11 @@ Address::InstanceConstSharedPtr getCanonicalLoopbackAddress(Address::IpVersion v return Network::Utility::getIpv6LoopbackAddress(); } -Address::InstanceConstSharedPtr getAnyAddress(const Address::IpVersion version) { +Address::InstanceConstSharedPtr getAnyAddress(const Address::IpVersion version, bool v4_compat) { if (version == Address::IpVersion::v4) { return Network::Utility::getIpv4AnyAddress(); } - return Network::Utility::getIpv6AnyAddress(); + return Network::Utility::getIpv6AnyAddress(!v4_compat); } bool supportsIpVersion(const Address::IpVersion version) { diff --git a/test/test_common/network_utility.h b/test/test_common/network_utility.h index d59355afc3374..e78ec89ada884 100644 --- a/test/test_common/network_utility.h +++ b/test/test_common/network_utility.h @@ -75,7 +75,8 @@ Address::InstanceConstSharedPtr getCanonicalLoopbackAddress(const Address::IpVer * @param version the IP version of the any address. * @returns the any address for the specified IP version. */ -Address::InstanceConstSharedPtr getAnyAddress(const Address::IpVersion version); +Address::InstanceConstSharedPtr getAnyAddress(const Address::IpVersion version, + bool v4_compat = false); /** * This function tries to create a socket of type IpVersion version and bind to it. If From 59b1077d435f559f7a30ea9d49af68f97d22ad80 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Fri, 9 Feb 2018 11:19:18 -0800 Subject: [PATCH 2/6] listener: Use v4-mapped addressing consistently. Currently the local address of a new connection is mapped to an IPv4 address instance if the listening socket is not in V6ONLY mode and the local address of the socket is an IPv4-mapped IPv6 address. This leads to connections having an (IPv4-mapped) IPv6 instance for the remote address and an IPv4 instance for the local address, which is confusing. Fix this by allowing the remote address to be v4-mapped if the local address is an IPv4 address. Or conversely, set the 'v6only' parameter to 'true' if the local address is an IPv6 address instance. Fixes: 3b3c0094d098 ("address_impl: Return an Ipv4Instance for v4-mapped v6 addresses. (#2309)") Signed-off-by: Jarno Rajahalme --- source/common/network/listener_impl.cc | 27 +++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index f25cea4045a8a..a37c17032583e 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -23,20 +23,25 @@ Address::InstanceConstSharedPtr ListenerImpl::getLocalAddress(int fd) { void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* remote_addr, int remote_addr_len, void* arg) { ListenerImpl* listener = static_cast(arg); - ConnectionSocketPtr socket(new AcceptedSocketImpl( - fd, - // Get the local address from the new socket if the listener is listening on IP ANY - // (e.g., 0.0.0.0 for IPv4) (local_address_ is nullptr in this case). - !listener->local_address_ ? listener->getLocalAddress(fd) : listener->local_address_, - // The accept() call that filled in remote_addr doesn't fill in more than the sa_family field - // for Unix domain sockets; apparently there isn't a mechanism in the kernel to get the - // sockaddr_un associated with the client socket when starting from the server socket. - // We work around this by using our own name for the socket in this case. + // Get the local address from the new socket if the listener is listening on IP ANY + // (e.g., 0.0.0.0 for IPv4) (local_address_ is nullptr in this case). + const Address::InstanceConstSharedPtr& local_address = + !listener->local_address_ ? listener->getLocalAddress(fd) : listener->local_address_; + // The accept() call that filled in remote_addr doesn't fill in more than the sa_family field + // for Unix domain sockets; apparently there isn't a mechanism in the kernel to get the + // sockaddr_un associated with the client socket when starting from the server socket. + // We work around this by using our own name for the socket in this case. + // Pass the 'v6only' parameter as true if the local_address is an IPv6 address. This has no effect + // if the socket is a v4 socket, but for v6 sockets this will create an IPv4 remote address if an + // IPv4 local_address was created from an IPv6 mapped IPv4 address. + const Address::InstanceConstSharedPtr& remote_address = (remote_addr->sa_family == AF_UNIX) ? Address::peerAddressFromFd(fd) : Address::addressFromSockAddr(*reinterpret_cast(remote_addr), - remote_addr_len))); - listener->cb_.onAccept(std::move(socket), listener->hand_off_restored_destination_connections_); + remote_addr_len, + local_address->ip()->version() == Address::IpVersion::v6); + listener->cb_.onAccept(std::make_unique(fd, local_address, remote_address), + listener->hand_off_restored_destination_connections_); } ListenerImpl::ListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, ListenerCallbacks& cb, From 3d8e28196b1cb7e67a7945a3a544d57010b5db14 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Tue, 13 Feb 2018 09:59:31 -0800 Subject: [PATCH 3/6] Review update. Revert the changes to source/common/network/utility.[h|cc], as those were only needed for testing. Signed-off-by: Jarno Rajahalme --- source/common/network/utility.cc | 19 +++---------------- source/common/network/utility.h | 3 +-- test/common/network/listener_impl_test.cc | 4 ++++ test/test_common/network_utility.cc | 17 ++++++++++++++++- test/test_common/network_utility.h | 3 +++ 5 files changed, 27 insertions(+), 19 deletions(-) diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 87fe7d46d76e4..bd97e09a413d4 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -270,23 +270,10 @@ Address::InstanceConstSharedPtr Utility::getIpv4AnyAddress() { return any; } -// There is no portable way to initialize sockaddr_in6 with a static initializer, do it with a -// helper function instead -static sockaddr_in6& v6any_() { - static sockaddr_in6 v6any = {}; - v6any.sin6_family = AF_INET6; - v6any.sin6_addr = in6addr_any; - - return v6any; -} - -Address::InstanceConstSharedPtr Utility::getIpv6AnyAddress(bool v6only) { +Address::InstanceConstSharedPtr Utility::getIpv6AnyAddress() { // Initialized on first call in a thread-safe manner. - static sockaddr_in6 v6any = v6any_(); - static Address::InstanceConstSharedPtr any(new Address::Ipv6Instance(v6any, false)); - static Address::InstanceConstSharedPtr any_v6only(new Address::Ipv6Instance(v6any, true)); - - return v6only ? any_v6only : any; + static Address::InstanceConstSharedPtr any(new Address::Ipv6Instance(static_cast(0))); + return any; } Address::InstanceConstSharedPtr Utility::getAddressWithPort(const Address::Instance& address, diff --git a/source/common/network/utility.h b/source/common/network/utility.h index ea047e6f6b843..8def4a45952ae 100644 --- a/source/common/network/utility.h +++ b/source/common/network/utility.h @@ -146,12 +146,11 @@ class Utility { static Address::InstanceConstSharedPtr getIpv4AnyAddress(); /** - * @param v6only a boolean to determine how to set the IPV6_V6ONLY socket option. * @return Address::InstanceConstSharedPtr an address that represents the IPv6 wildcard address * (i.e. "::"). Used during binding to indicate that incoming connections to any local * IPv6 address are to be accepted. */ - static Address::InstanceConstSharedPtr getIpv6AnyAddress(bool v6only = true); + static Address::InstanceConstSharedPtr getIpv6AnyAddress(); /** * @param address IP address instance. diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index c6778ba4d53c0..237359bcc5013 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -154,6 +154,10 @@ TEST_P(ListenerImplTest, WildcardListenerUseActualDst) { dispatcher.run(Event::Dispatcher::RunType::Block); } +// Test for the correct behavior when a listener is configured with an ANY address that allows +// receiving IPv4 connections on an IPv6 socket. In this case the address instances of both +// local and remove addresses of the connection should be IPv4 instances, as the connection really +// is an IPv4 connection. TEST_P(ListenerImplTest, WildcardListenerIpv4Compat) { Stats::IsolatedStoreImpl stats_store; Event::DispatcherImpl dispatcher; diff --git a/test/test_common/network_utility.cc b/test/test_common/network_utility.cc index 30838f78a459f..ec31e86eb06b2 100644 --- a/test/test_common/network_utility.cc +++ b/test/test_common/network_utility.cc @@ -116,11 +116,26 @@ Address::InstanceConstSharedPtr getCanonicalLoopbackAddress(Address::IpVersion v return Network::Utility::getIpv6LoopbackAddress(); } +// There is no portable way to initialize sockaddr_in6 with a static initializer, do it with a +// helper function instead +static sockaddr_in6& v6any_() { + static sockaddr_in6 v6any = {}; + v6any.sin6_family = AF_INET6; + v6any.sin6_addr = in6addr_any; + + return v6any; +} + Address::InstanceConstSharedPtr getAnyAddress(const Address::IpVersion version, bool v4_compat) { if (version == Address::IpVersion::v4) { return Network::Utility::getIpv4AnyAddress(); } - return Network::Utility::getIpv6AnyAddress(!v4_compat); + if (v4_compat) { + static sockaddr_in6 v6any = v6any_(); + static Address::InstanceConstSharedPtr any(new Address::Ipv6Instance(v6any, false)); + return any; + } + return Network::Utility::getIpv6AnyAddress(); } bool supportsIpVersion(const Address::IpVersion version) { diff --git a/test/test_common/network_utility.h b/test/test_common/network_utility.h index e78ec89ada884..ee6c8a8039cba 100644 --- a/test/test_common/network_utility.h +++ b/test/test_common/network_utility.h @@ -73,6 +73,9 @@ Address::InstanceConstSharedPtr getCanonicalLoopbackAddress(const Address::IpVer /** * Returns the any address for the specified IP version. * @param version the IP version of the any address. + * @param v4_compat determines whether a v4-mapped addresses bound to a socket listening on the + * returned ANY address are to be treated as IPv4 or IPv6 addresses. Defaults to 'false', + * has no effect with IPv4 ANY address. * @returns the any address for the specified IP version. */ Address::InstanceConstSharedPtr getAnyAddress(const Address::IpVersion version, From c23d30597c80ccfa522f79ab6414c1f0ff6205c1 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Tue, 13 Feb 2018 15:04:06 -0800 Subject: [PATCH 4/6] Review fixes. Signed-off-by: Jarno Rajahalme --- test/common/network/listener_impl_test.cc | 2 +- test/test_common/network_utility.cc | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/test/common/network/listener_impl_test.cc b/test/common/network/listener_impl_test.cc index 237359bcc5013..2b01c6186abfa 100644 --- a/test/common/network/listener_impl_test.cc +++ b/test/common/network/listener_impl_test.cc @@ -156,7 +156,7 @@ TEST_P(ListenerImplTest, WildcardListenerUseActualDst) { // Test for the correct behavior when a listener is configured with an ANY address that allows // receiving IPv4 connections on an IPv6 socket. In this case the address instances of both -// local and remove addresses of the connection should be IPv4 instances, as the connection really +// local and remote addresses of the connection should be IPv4 instances, as the connection really // is an IPv4 connection. TEST_P(ListenerImplTest, WildcardListenerIpv4Compat) { Stats::IsolatedStoreImpl stats_store; diff --git a/test/test_common/network_utility.cc b/test/test_common/network_utility.cc index ec31e86eb06b2..40400b2099ccf 100644 --- a/test/test_common/network_utility.cc +++ b/test/test_common/network_utility.cc @@ -118,8 +118,8 @@ Address::InstanceConstSharedPtr getCanonicalLoopbackAddress(Address::IpVersion v // There is no portable way to initialize sockaddr_in6 with a static initializer, do it with a // helper function instead -static sockaddr_in6& v6any_() { - static sockaddr_in6 v6any = {}; +static sockaddr_in6 v6any_() { + sockaddr_in6 v6any = {}; v6any.sin6_family = AF_INET6; v6any.sin6_addr = in6addr_any; @@ -131,8 +131,7 @@ Address::InstanceConstSharedPtr getAnyAddress(const Address::IpVersion version, return Network::Utility::getIpv4AnyAddress(); } if (v4_compat) { - static sockaddr_in6 v6any = v6any_(); - static Address::InstanceConstSharedPtr any(new Address::Ipv6Instance(v6any, false)); + static Address::InstanceConstSharedPtr any(new Address::Ipv6Instance(v6any_(), false)); return any; } return Network::Utility::getIpv6AnyAddress(); From 659a3aa3fa6a43db5ce4126d22c53312dde08707 Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Thu, 15 Feb 2018 10:36:03 -0800 Subject: [PATCH 5/6] Review fixes. Signed-off-by: Jarno Rajahalme --- source/common/network/listener_impl.cc | 2 +- test/test_common/network_utility.cc | 14 +++++++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index a37c17032583e..eb41b640f8053 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -26,7 +26,7 @@ void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* // Get the local address from the new socket if the listener is listening on IP ANY // (e.g., 0.0.0.0 for IPv4) (local_address_ is nullptr in this case). const Address::InstanceConstSharedPtr& local_address = - !listener->local_address_ ? listener->getLocalAddress(fd) : listener->local_address_; + listener->local_address_ ? listener->local_address_ : listener->getLocalAddress(fd); // The accept() call that filled in remote_addr doesn't fill in more than the sa_family field // for Unix domain sockets; apparently there isn't a mechanism in the kernel to get the // sockaddr_un associated with the client socket when starting from the server socket. diff --git a/test/test_common/network_utility.cc b/test/test_common/network_utility.cc index 40400b2099ccf..2dbb4bef91e35 100644 --- a/test/test_common/network_utility.cc +++ b/test/test_common/network_utility.cc @@ -116,22 +116,30 @@ Address::InstanceConstSharedPtr getCanonicalLoopbackAddress(Address::IpVersion v return Network::Utility::getIpv6LoopbackAddress(); } +namespace { // There is no portable way to initialize sockaddr_in6 with a static initializer, do it with a -// helper function instead -static sockaddr_in6 v6any_() { +// helper function instead. +sockaddr_in6 sockaddrIn6Any() { sockaddr_in6 v6any = {}; v6any.sin6_family = AF_INET6; v6any.sin6_addr = in6addr_any; return v6any; } +} // namespace Address::InstanceConstSharedPtr getAnyAddress(const Address::IpVersion version, bool v4_compat) { if (version == Address::IpVersion::v4) { return Network::Utility::getIpv4AnyAddress(); } if (v4_compat) { - static Address::InstanceConstSharedPtr any(new Address::Ipv6Instance(v6any_(), false)); + // This will return an IPv6 ANY address ("[::]:0") like the getIpv6AnyAddress() below, but + // with the internal 'v6only' member set to false. This will allow a socket created from this + // address to accept IPv4 connections. IPv4 connections received on IPv6 sockets will have + // Ipv4-mapped IPv6 addresses, which we will then internally interpret as IPv4 addresses so + // that, for example, access logging will show IPv4 address format for IPv4 connections even + // if they were received on an IPv6 socket. + static Address::InstanceConstSharedPtr any(new Address::Ipv6Instance(sockaddrIn6Any(), false)); return any; } return Network::Utility::getIpv6AnyAddress(); From 0acec797ca6801a56e2b53c9123133783e2670db Mon Sep 17 00:00:00 2001 From: Jarno Rajahalme Date: Thu, 15 Feb 2018 11:37:14 -0800 Subject: [PATCH 6/6] Fix format. Signed-off-by: Jarno Rajahalme --- source/common/network/listener_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index eb41b640f8053..da70a74fffce5 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -26,7 +26,7 @@ void ListenerImpl::listenCallback(evconnlistener*, evutil_socket_t fd, sockaddr* // Get the local address from the new socket if the listener is listening on IP ANY // (e.g., 0.0.0.0 for IPv4) (local_address_ is nullptr in this case). const Address::InstanceConstSharedPtr& local_address = - listener->local_address_ ? listener->local_address_ : listener->getLocalAddress(fd); + listener->local_address_ ? listener->local_address_ : listener->getLocalAddress(fd); // The accept() call that filled in remote_addr doesn't fill in more than the sa_family field // for Unix domain sockets; apparently there isn't a mechanism in the kernel to get the // sockaddr_un associated with the client socket when starting from the server socket.