Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add mode to PipeInstance #8423

Merged
merged 27 commits into from
Dec 17, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
147ccbf
syscall: add chmod #5808
athampy Sep 28, 2019
e8245db
api: add mode to Pipe #5808
athampy Sep 28, 2019
0715211
address: add mode to PipeInstance #5808
athampy Sep 28, 2019
ec7ea45
test: add tests for setting mode for PipeInstance #5808
athampy Sep 28, 2019
3920808
fix format
athampy Sep 30, 2019
5c30cf2
proto sync
athampy Sep 30, 2019
318defb
tools: add chmod to dictionary
athampy Sep 30, 2019
72dbba4
add leading zero for mode_t values
athampy Oct 2, 2019
970c88c
api: remove not-implemented-hide
athampy Oct 4, 2019
23d9f96
api: update validate rules for mode
athampy Oct 4, 2019
8c08b47
merge master
athampy Nov 16, 2019
4a2c94d
add docs; use '&&' instead of 'and' for MSVC compatibility
athampy Nov 16, 2019
fc7182a
fix format
athampy Nov 18, 2019
ba59ed5
network: throw exception if mode is set for abstract sockets;
athampy Nov 19, 2019
2f1862c
fix format
athampy Nov 25, 2019
f306f62
Merge branch 'master' into gh-5808
athampy Nov 25, 2019
1ad0a2c
add test for abstract PipeInstance with mode
athampy Nov 26, 2019
aa6f82b
remove unnecessary headers
athampy Nov 26, 2019
6a735c3
move platform specific includes to platform.h
athampy Nov 27, 2019
fd10e2e
docs: add release notes for Pipe::mode
athampy Dec 11, 2019
5330a69
Merge branch 'master' into gh-5808
athampy Dec 11, 2019
342c394
test: add test for Pipe chmod fail; create mock for chmod syscall
athampy Dec 11, 2019
8a32d78
docs: revert
athampy Dec 13, 2019
9585817
Merge branch 'master' into gh-5808
athampy Dec 13, 2019
ff16f6d
Kick CI
athampy Dec 13, 2019
0b72b99
fix coverage
athampy Dec 15, 2019
818e6b1
fix asan off by one
athampy Dec 16, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions api/envoy/api/v2/core/address.proto
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ message Pipe {
// Paths starting with '@' will result in an error in environments other than
// Linux.
string path = 1 [(validate.rules).string = {min_bytes: 1}];

// The mode for the Pipe. Not applicable for abstract sockets.
athampy marked this conversation as resolved.
Show resolved Hide resolved
uint32 mode = 2 [(validate.rules).uint32 = {lte: 511}];
athampy marked this conversation as resolved.
Show resolved Hide resolved
}

// [#next-free-field: 7]
Expand Down
3 changes: 3 additions & 0 deletions api/envoy/api/v3alpha/core/address.proto
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ message Pipe {
// Paths starting with '@' will result in an error in environments other than
// Linux.
string path = 1 [(validate.rules).string = {min_bytes: 1}];

// The mode for the Pipe. Not applicable for abstract sockets.
uint32 mode = 2 [(validate.rules).uint32 = {lte: 511}];
}

// [#next-free-field: 7]
Expand Down
1 change: 1 addition & 0 deletions docs/root/intro/version_history.rst
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ Version history
* access log: added FILTER_STATE :ref:`access log formatters <config_access_log_format>` and gRPC access logger.
* access log: added a :ref:`typed JSON logging mode <config_access_log_format_dictionaries>` to output access logs in JSON format with non-string values
* api: remove all support for v1
* api: added ability to specify `mode` for :ref:`Pipe <envoy_api_field_core.Pipe.mode>`.
* buffer: remove old implementation
* build: official released binary is now built against libc++.
* cluster: added :ref: `aggregate cluster <arch_overview_aggregate_cluster>` that allows load balancing between clusters.
Expand Down
5 changes: 5 additions & 0 deletions include/envoy/api/os_sys_calls.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ class OsSysCalls {
*/
virtual SysCallIntResult bind(int sockfd, const sockaddr* addr, socklen_t addrlen) PURE;

/**
* @see chmod (man 2 chmod)
*/
virtual SysCallIntResult chmod(const std::string& path, mode_t mode) PURE;

/**
* @see ioctl (man 2 ioctl)
*/
Expand Down
2 changes: 2 additions & 0 deletions include/envoy/common/platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ typedef unsigned int sa_family_t;
#include <sys/socket.h>
#include <sys/uio.h> // for iovec
#include <sys/un.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>

#if defined(__linux__)
Expand Down
6 changes: 6 additions & 0 deletions source/common/api/os_sys_calls_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <unistd.h>

#include <cerrno>
#include <string>

namespace Envoy {
namespace Api {
Expand All @@ -14,6 +15,11 @@ SysCallIntResult OsSysCallsImpl::bind(int sockfd, const sockaddr* addr, socklen_
return {rc, errno};
}

SysCallIntResult OsSysCallsImpl::chmod(const std::string& path, mode_t mode) {
const int rc = ::chmod(path.c_str(), mode);
return {rc, errno};
}

SysCallIntResult OsSysCallsImpl::ioctl(int sockfd, unsigned long int request, void* argp) {
const int rc = ::ioctl(sockfd, request, argp);
return {rc, errno};
Expand Down
3 changes: 3 additions & 0 deletions source/common/api/os_sys_calls_impl.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#pragma once

#include <string>

#include "envoy/api/os_sys_calls.h"

#include "common/singleton/threadsafe_singleton.h"
Expand All @@ -11,6 +13,7 @@ class OsSysCallsImpl : public OsSysCalls {
public:
// Api::OsSysCalls
SysCallIntResult bind(int sockfd, const sockaddr* addr, socklen_t addrlen) override;
SysCallIntResult chmod(const std::string& path, mode_t mode) override;
SysCallIntResult ioctl(int sockfd, unsigned long int request, void* argp) override;
SysCallSizeResult writev(int fd, const iovec* iovec, int num_iovec) override;
SysCallSizeResult readv(int fd, const iovec* iovec, int num_iovec) override;
Expand Down
21 changes: 18 additions & 3 deletions source/common/network/address_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ IoHandlePtr Ipv6Instance::socket(SocketType type) const {
return io_handle;
}

PipeInstance::PipeInstance(const sockaddr_un* address, socklen_t ss_len)
PipeInstance::PipeInstance(const sockaddr_un* address, socklen_t ss_len, mode_t mode)
: InstanceBase(Type::Pipe) {
if (address->sun_path[0] == '\0') {
#if !defined(__linux__)
Expand All @@ -345,15 +345,19 @@ PipeInstance::PipeInstance(const sockaddr_un* address, socklen_t ss_len)
}
address_ = *address;
if (abstract_namespace_) {
if (mode != 0) {
throw EnvoyException("Cannot set mode for Abstract AF_UNIX sockets");
lizan marked this conversation as resolved.
Show resolved Hide resolved
mattklein123 marked this conversation as resolved.
Show resolved Hide resolved
}
// Replace all null characters with '@' in friendly_name_.
friendly_name_ =
friendlyNameFromAbstractPath(absl::string_view(address_.sun_path, address_length_));
} else {
friendly_name_ = address->sun_path;
}
this->mode = mode;
lizan marked this conversation as resolved.
Show resolved Hide resolved
}

PipeInstance::PipeInstance(const std::string& pipe_path) : InstanceBase(Type::Pipe) {
PipeInstance::PipeInstance(const std::string& pipe_path, mode_t mode) : InstanceBase(Type::Pipe) {
if (pipe_path.size() >= sizeof(address_.sun_path)) {
throw EnvoyException(
fmt::format("Path \"{}\" exceeds maximum UNIX domain socket path size of {}.", pipe_path,
Expand All @@ -370,6 +374,9 @@ PipeInstance::PipeInstance(const std::string& pipe_path) : InstanceBase(Type::Pi
#if !defined(__linux__)
throw EnvoyException("Abstract AF_UNIX sockets are only supported on linux.");
#endif
if (mode != 0) {
throw EnvoyException("Cannot set mode for Abstract AF_UNIX sockets");
}
abstract_namespace_ = true;
address_length_ = pipe_path.size();
memcpy(&address_.sun_path[0], pipe_path.data(), pipe_path.size());
Expand All @@ -385,6 +392,7 @@ PipeInstance::PipeInstance(const std::string& pipe_path) : InstanceBase(Type::Pi
StringUtil::strlcpy(&address_.sun_path[0], pipe_path.c_str(), sizeof(address_.sun_path));
friendly_name_ = address_.sun_path;
}
this->mode = mode;
}

bool PipeInstance::operator==(const Instance& rhs) const { return asString() == rhs.asString(); }
Expand All @@ -397,7 +405,14 @@ Api::SysCallIntResult PipeInstance::bind(int fd) const {
unlink(address_.sun_path);
}
auto& os_syscalls = Api::OsSysCallsSingleton::get();
return os_syscalls.bind(fd, sockAddr(), sockAddrLen());
auto bind_result = os_syscalls.bind(fd, sockAddr(), sockAddrLen());
if (mode != 0 && !abstract_namespace_ && bind_result.rc_ == 0) {
auto set_permissions = os_syscalls.chmod(address_.sun_path, mode);
lizan marked this conversation as resolved.
Show resolved Hide resolved
if (set_permissions.rc_ != 0) {
throw EnvoyException(fmt::format("Failed to create socket with mode {}", mode));
mattklein123 marked this conversation as resolved.
Show resolved Hide resolved
}
}
return bind_result;
}

Api::SysCallIntResult PipeInstance::connect(int fd) const {
Expand Down
5 changes: 3 additions & 2 deletions source/common/network/address_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -228,12 +228,12 @@ class PipeInstance : public InstanceBase {
/**
* Construct from an existing unix address.
*/
explicit PipeInstance(const sockaddr_un* address, socklen_t ss_len);
explicit PipeInstance(const sockaddr_un* address, socklen_t ss_len, mode_t mode = 0);

/**
* Construct from a string pipe path.
*/
explicit PipeInstance(const std::string& pipe_path);
explicit PipeInstance(const std::string& pipe_path, mode_t mode = 0);

// Network::Address::Instance
bool operator==(const Instance& rhs) const override;
Expand All @@ -256,6 +256,7 @@ class PipeInstance : public InstanceBase {
// For abstract namespaces.
bool abstract_namespace_{false};
uint32_t address_length_{0};
mode_t mode{0};
};

} // namespace Address
Expand Down
3 changes: 2 additions & 1 deletion source/common/network/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,8 @@ Utility::protobufAddressToAddress(const envoy::api::v2::core::Address& proto_add
proto_address.socket_address().port_value(),
!proto_address.socket_address().ipv4_compat());
case envoy::api::v2::core::Address::kPipe:
return std::make_shared<Address::PipeInstance>(proto_address.pipe().path());
return std::make_shared<Address::PipeInstance>(proto_address.pipe().path(),
proto_address.pipe().mode());
default:
NOT_REACHED_GCOVR_EXCL_LINE;
}
Expand Down
2 changes: 2 additions & 0 deletions test/common/network/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,10 @@ envoy_cc_test(
deps = [
"//source/common/network:address_lib",
"//source/common/network:utility_lib",
"//test/mocks/api:api_mocks",
"//test/test_common:environment_lib",
"//test/test_common:network_utility_lib",
"//test/test_common:threadsafe_singleton_injector_lib",
"//test/test_common:utility_lib",
],
)
Expand Down
65 changes: 64 additions & 1 deletion test/common/network/address_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,24 @@
#include "envoy/common/exception.h"
#include "envoy/common/platform.h"

#include "common/api/os_sys_calls_impl.h"
#include "common/common/fmt.h"
#include "common/common/utility.h"
#include "common/network/address_impl.h"
#include "common/network/utility.h"

#include "test/mocks/api/mocks.h"
#include "test/test_common/environment.h"
#include "test/test_common/network_utility.h"
#include "test/test_common/threadsafe_singleton_injector.h"
#include "test/test_common/utility.h"

#include "gtest/gtest.h"

using testing::_;
using testing::NiceMock;
using testing::Return;

namespace Envoy {
namespace Network {
namespace Address {
Expand Down Expand Up @@ -312,6 +319,63 @@ TEST(PipeInstanceTest, Basic) {
EXPECT_EQ(nullptr, address.ip());
}

TEST(PipeInstanceTest, BasicPermission) {
std::string path = TestEnvironment::unixDomainSocketPath("foo.sock");

const mode_t mode = 0777;
PipeInstance address(path, mode);

IoHandlePtr io_handle = address.socket(SocketType::Stream);
ASSERT_GE(io_handle->fd(), 0) << address.asString();

Api::SysCallIntResult result = address.bind(io_handle->fd());
ASSERT_EQ(result.rc_, 0) << address.asString() << "\nerror: " << strerror(result.errno_)
<< "\terrno: " << result.errno_;

Api::OsSysCalls& os_sys_calls = Api::OsSysCallsSingleton::get();
struct stat stat_buf;
result = os_sys_calls.stat(path.c_str(), &stat_buf);
EXPECT_EQ(result.rc_, 0);
// Get file permissions bits
ASSERT_EQ(stat_buf.st_mode & 07777, mode)
<< path << std::oct << "\t" << (stat_buf.st_mode & 07777) << std::dec << "\t"
<< (stat_buf.st_mode) << strerror(result.errno_);
}

TEST(PipeInstanceTest, PermissionFail) {
NiceMock<Api::MockOsSysCalls> os_sys_calls;
TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls(&os_sys_calls);
std::string path = TestEnvironment::unixDomainSocketPath("foo.sock");

const mode_t mode = 0777;
PipeInstance address(path, mode);

IoHandlePtr io_handle = address.socket(SocketType::Stream);
ASSERT_GE(io_handle->fd(), 0) << address.asString();
EXPECT_CALL(os_sys_calls, bind(_, _, _)).WillOnce(Return(Api::SysCallIntResult{0, 0}));
EXPECT_CALL(os_sys_calls, chmod(_, _)).WillOnce(Return(Api::SysCallIntResult{-1, 0}));
EXPECT_THROW_WITH_REGEX(address.bind(io_handle->fd()), EnvoyException,
"Failed to create socket with mode");
}

TEST(PipeInstanceTest, AbstractNamespacePermission) {
#if defined(__linux__)
std::string path = "@/foo";
const mode_t mode = 0777;
EXPECT_THROW_WITH_REGEX(PipeInstance address(path, mode), EnvoyException,
"Cannot set mode for Abstract AF_UNIX sockets");

sockaddr_un sun;
sun.sun_family = AF_UNIX;
StringUtil::strlcpy(&sun.sun_path[1], path.data(), path.size());
sun.sun_path[0] = '\0';
socklen_t ss_len = offsetof(struct sockaddr_un, sun_path) + 1 + strlen(sun.sun_path);

EXPECT_THROW_WITH_REGEX(PipeInstance address(&sun, ss_len, mode), EnvoyException,
"Cannot set mode for Abstract AF_UNIX sockets");
#endif
}

TEST(PipeInstanceTest, AbstractNamespace) {
#if defined(__linux__)
PipeInstance address("@/foo");
Expand Down Expand Up @@ -499,7 +563,6 @@ struct TestCase test_cases[] = {
INSTANTIATE_TEST_SUITE_P(AddressCrossProduct, MixedAddressTest,
::testing::Combine(::testing::ValuesIn(test_cases),
::testing::ValuesIn(test_cases)));

} // namespace Address
} // namespace Network
} // namespace Envoy
1 change: 1 addition & 0 deletions test/mocks/api/mocks.h
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ class MockOsSysCalls : public OsSysCallsImpl {
MOCK_METHOD6(mmap, SysCallPtrResult(void* addr, size_t length, int prot, int flags, int fd,
off_t offset));
MOCK_METHOD2(stat, SysCallIntResult(const char* name, struct stat* stat));
MOCK_METHOD2(chmod, SysCallIntResult(const std::string& name, mode_t mode));
MOCK_METHOD5(setsockopt_,
int(int sockfd, int level, int optname, const void* optval, socklen_t optlen));
MOCK_METHOD5(getsockopt_,
Expand Down
1 change: 1 addition & 0 deletions tools/spelling_dictionary.txt
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ CDS
CEL
CHACHA
CHLO
CHMOD
CHLOS
CIDR
CLA
Expand Down