Make PolicyBuilder a value class.

This makes the class more ergonomic because
* You don't have to heap allocate the builder.
* You can create a policy builder "template" and re-use it across sandboxes to avoid repetitive work.

PiperOrigin-RevId: 273555679
Change-Id: I4084ee9c74f95ebfde873eb0dc021b3b3cdc5ea2
This commit is contained in:
Kristj?n J?nsson 2019-10-08 10:45:07 -07:00 committed by Copybara-Service
parent 9931593fdc
commit 78824353d1
3 changed files with 32 additions and 22 deletions

View File

@ -49,8 +49,7 @@ namespace {
PolicyBuilder& PolicyBuilder::AllowSyscall(unsigned int num) { PolicyBuilder& PolicyBuilder::AllowSyscall(unsigned int num) {
if (handled_syscalls_.insert(num).second) { if (handled_syscalls_.insert(num).second) {
output_->user_policy_.insert(output_->user_policy_.end(), user_policy_.insert(user_policy_.end(), {SYSCALL(num, ALLOW)});
{SYSCALL(num, ALLOW)});
} }
return *this; return *this;
} }
@ -72,8 +71,7 @@ PolicyBuilder& PolicyBuilder::AllowSyscalls(SyscallInitializer nums) {
PolicyBuilder& PolicyBuilder::BlockSyscallWithErrno(unsigned int num, PolicyBuilder& PolicyBuilder::BlockSyscallWithErrno(unsigned int num,
int error) { int error) {
if (handled_syscalls_.insert(num).second) { if (handled_syscalls_.insert(num).second) {
output_->user_policy_.insert(output_->user_policy_.end(), user_policy_.insert(user_policy_.end(), {SYSCALL(num, ERRNO(error))});
{SYSCALL(num, ERRNO(error))});
} }
return *this; return *this;
} }
@ -588,8 +586,8 @@ PolicyBuilder& PolicyBuilder::AddPolicyOnSyscalls(
return out; return out;
}); });
// Pre-/Postcondition: Syscall number loaded into A register // Pre-/Postcondition: Syscall number loaded into A register
output_->user_policy_.insert(output_->user_policy_.end(), user_policy_.insert(user_policy_.end(), resolved_policy.begin(),
resolved_policy.begin(), resolved_policy.end()); resolved_policy.end());
return *this; return *this;
} }
@ -630,7 +628,7 @@ PolicyBuilder& PolicyBuilder::AddPolicyOnMmap(BpfFunc f) {
} }
PolicyBuilder& PolicyBuilder::DangerDefaultAllowAll() { PolicyBuilder& PolicyBuilder::DangerDefaultAllowAll() {
output_->user_policy_.push_back(ALLOW); user_policy_.push_back(ALLOW);
return *this; return *this;
} }
@ -665,11 +663,13 @@ std::vector<sock_filter> PolicyBuilder::ResolveBpfFunc(BpfFunc f) {
} }
sapi::StatusOr<std::unique_ptr<Policy>> PolicyBuilder::TryBuild() { sapi::StatusOr<std::unique_ptr<Policy>> PolicyBuilder::TryBuild() {
auto output = absl::WrapUnique(new Policy());
if (!last_status_.ok()) { if (!last_status_.ok()) {
return last_status_; return last_status_;
} }
if (!output_) { if (already_built_) {
return sapi::FailedPreconditionError("Can only build policy once."); return sapi::FailedPreconditionError("Can only build policy once.");
} }
@ -678,7 +678,7 @@ sapi::StatusOr<std::unique_ptr<Policy>> PolicyBuilder::TryBuild() {
return sapi::FailedPreconditionError( return sapi::FailedPreconditionError(
"Cannot set hostname without network namespaces."); "Cannot set hostname without network namespaces.");
} }
output_->SetNamespace(absl::make_unique<Namespace>( output->SetNamespace(absl::make_unique<Namespace>(
allow_unrestricted_networking_, std::move(mounts_), hostname_)); allow_unrestricted_networking_, std::move(mounts_), hostname_));
} else { } else {
// Not explicitly disabling them here as this is a technical limitation in // Not explicitly disabling them here as this is a technical limitation in
@ -687,16 +687,18 @@ sapi::StatusOr<std::unique_ptr<Policy>> PolicyBuilder::TryBuild() {
<< " crash"; << " crash";
} }
output_->collect_stacktrace_on_signal_ = collect_stacktrace_on_signal_; output->collect_stacktrace_on_signal_ = collect_stacktrace_on_signal_;
output_->collect_stacktrace_on_violation_ = collect_stacktrace_on_violation_; output->collect_stacktrace_on_violation_ = collect_stacktrace_on_violation_;
output_->collect_stacktrace_on_timeout_ = collect_stacktrace_on_timeout_; output->collect_stacktrace_on_timeout_ = collect_stacktrace_on_timeout_;
output_->collect_stacktrace_on_kill_ = collect_stacktrace_on_kill_; output->collect_stacktrace_on_kill_ = collect_stacktrace_on_kill_;
output->user_policy_ = std::move(user_policy_);
auto pb_description = absl::make_unique<PolicyBuilderDescription>(); auto pb_description = absl::make_unique<PolicyBuilderDescription>();
StoreDescription(pb_description.get()); StoreDescription(pb_description.get());
output_->policy_builder_description_ = std::move(pb_description); output->policy_builder_description_ = std::move(pb_description);
return std::move(output_); already_built_ = true;
return std::move(output);
} }
PolicyBuilder& PolicyBuilder::AddFile(absl::string_view path, bool is_ro) { PolicyBuilder& PolicyBuilder::AddFile(absl::string_view path, bool is_ro) {

View File

@ -94,11 +94,6 @@ class PolicyBuilder final {
using BpfFunc = const std::function<std::vector<sock_filter>(bpf_labels&)>&; using BpfFunc = const std::function<std::vector<sock_filter>(bpf_labels&)>&;
using SyscallInitializer = std::initializer_list<unsigned int>; using SyscallInitializer = std::initializer_list<unsigned int>;
PolicyBuilder() : output_{new Policy()} {}
PolicyBuilder(const PolicyBuilder&) = delete;
PolicyBuilder& operator=(const PolicyBuilder&) = delete;
// Appends code to allow a specific syscall // Appends code to allow a specific syscall
PolicyBuilder& AllowSyscall(unsigned int num); PolicyBuilder& AllowSyscall(unsigned int num);
@ -534,11 +529,12 @@ class PolicyBuilder final {
bool collect_stacktrace_on_kill_ = false; bool collect_stacktrace_on_kill_ = false;
// Seccomp fields // Seccomp fields
std::unique_ptr<Policy> output_; std::vector<sock_filter> user_policy_;
std::set<unsigned int> handled_syscalls_; std::set<unsigned int> handled_syscalls_;
// Error handling // Error handling
sapi::Status last_status_; sapi::Status last_status_;
bool already_built_ = false;
// This function returns a PolicyBuilder so that we can use it in the status // This function returns a PolicyBuilder so that we can use it in the status
// macros // macros
PolicyBuilder& SetError(const sapi::Status& status); PolicyBuilder& SetError(const sapi::Status& status);

View File

@ -55,7 +55,7 @@ class PolicyBuilderPeer {
public: public:
explicit PolicyBuilderPeer(PolicyBuilder* builder) : builder_{builder} {} explicit PolicyBuilderPeer(PolicyBuilder* builder) : builder_{builder} {}
int policy_size() const { return builder_->output_->user_policy_.size(); } int policy_size() const { return builder_->user_policy_.size(); }
static sapi::StatusOr<std::string> ValidateAbsolutePath( static sapi::StatusOr<std::string> ValidateAbsolutePath(
absl::string_view path) { absl::string_view path) {
@ -198,6 +198,18 @@ TEST_F(PolicyBuilderTest, TestCanOnlyBuildOnce) {
ASSERT_DEATH(b.BuildOrDie(), "Can only build policy once"); ASSERT_DEATH(b.BuildOrDie(), "Can only build policy once");
} }
TEST_F(PolicyBuilderTest, TestIsCopyable) {
PolicyBuilder builder;
builder.DangerDefaultAllowAll();
PolicyBuilder copy = builder;
ASSERT_EQ(PolicyBuilderPeer(&copy).policy_size(), 1);
// Building both does not crash.
builder.BuildOrDie();
copy.BuildOrDie();
}
TEST_F(PolicyBuilderTest, TestEcho) { TEST_F(PolicyBuilderTest, TestEcho) {
ASSERT_THAT(Run({"/bin/echo", "HELLO"}), StrEq("HELLO\n")); ASSERT_THAT(Run({"/bin/echo", "HELLO"}), StrEq("HELLO\n"));
} }