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

Always serialize schemas when available #2406

Merged
merged 4 commits into from
Dec 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 14 additions & 3 deletions appsec/src/extension/commands/client_init.c
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,21 @@ static dd_result _pack_command(
mpack_start_map(w, 2);

dd_mpack_write_lstr(w, "enabled");
mpack_write_bool(w, get_global_DD_EXPERIMENTAL_API_SECURITY_ENABLED());

dd_mpack_write_lstr(w, "sample_rate");
mpack_write(w, get_global_DD_API_SECURITY_REQUEST_SAMPLE_RATE());
#define MIN_SE_SAMPLE_RATE 0.0001

double se_sample_rate = get_global_DD_API_SECURITY_REQUEST_SAMPLE_RATE();
if (se_sample_rate >= MIN_SE_SAMPLE_RATE) {
mpack_write_bool(w, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to add some comments explaining what this line is


dd_mpack_write_lstr(w, "sample_rate");
mpack_write(w, se_sample_rate);
} else {
mpack_write_bool(w, false);

dd_mpack_write_lstr(w, "sample_rate");
mpack_write(w, 0.0);
}

mpack_finish_map(w);

Expand Down
1 change: 0 additions & 1 deletion appsec/src/extension/configuration.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ extern bool runtime_config_first_init;
CONFIG(CUSTOM(STRING), DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING, "safe", .parser = dd_parse_automated_user_events_tracking) \
CONFIG(STRING, DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML, "") \
CONFIG(STRING, DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON, "") \
CONFIG(BOOL, DD_EXPERIMENTAL_API_SECURITY_ENABLED, "false") \
CONFIG(DOUBLE, DD_API_SECURITY_REQUEST_SAMPLE_RATE, "0.1")
// clang-format on

Expand Down
10 changes: 0 additions & 10 deletions appsec/src/helper/client.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -471,16 +471,6 @@ bool client::handle_command(network::request_shutdown::request &command)

response->triggers = std::move(res->events);
response->force_keep = res->force_keep;
for (const auto &[key, value] : res->schemas) {
std::string schema = value;
if (value.length() > max_plain_schema_allowed) {
auto encoded = compress(schema);
if (encoded) {
schema = base64_encode(encoded.value(), false);
}
}
response->meta.emplace(key, std::move(schema));
}

DD_STDLOG(DD_STDLOG_ATTACK_DETECTED);
} else {
Expand Down
1 change: 0 additions & 1 deletion appsec/src/helper/client.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ namespace dds {
class client {
public:
// Below this limit the encoding+compression might result on a longer string
static constexpr int max_plain_schema_allowed = 260;
client(std::shared_ptr<service_manager> service_manager,
network::base_broker::ptr &&broker)
: service_manager_(std::move(service_manager)),
Expand Down
5 changes: 1 addition & 4 deletions appsec/src/helper/engine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ std::optional<engine::result> engine::context::publish(parameter &&param)

std::vector<std::string> event_data;
std::unordered_set<std::string> event_actions;
std::map<std::string, std::string> schemas;

for (auto &sub : common_->subscribers) {
auto it = listeners_.find(sub);
Expand All @@ -89,7 +88,6 @@ std::optional<engine::result> engine::context::publish(parameter &&param)
std::make_move_iterator(event->data.begin()),
std::make_move_iterator(event->data.end()));
event_actions.merge(event->actions);
schemas.merge(event->schemas);
}
} catch (std::exception &e) {
SPDLOG_ERROR("subscriber failed: {}", e.what());
Expand All @@ -100,8 +98,7 @@ std::optional<engine::result> engine::context::publish(parameter &&param)
return std::nullopt;
}

dds::engine::result res{
action_type::record, {}, std::move(event_data), std::move(schemas)};
dds::engine::result res{action_type::record, {}, std::move(event_data)};
// Currently the only action the extension can perform is block
if (!event_actions.empty()) {
// The extension can only handle one action, so we pick the first one
Expand Down
1 change: 0 additions & 1 deletion appsec/src/helper/engine.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ class engine {
action_type type;
std::unordered_map<std::string, std::string> parameters;
std::vector<std::string> events;
std::map<std::string, std::string> schemas;
bool force_keep;
};

Expand Down
3 changes: 1 addition & 2 deletions appsec/src/helper/network/proto.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -280,10 +280,9 @@ struct request_shutdown {

std::map<std::string, std::string> meta;
std::map<std::string_view, double> metrics;
std::map<std::string_view, std::string> schemas;

MSGPACK_DEFINE(
verdict, parameters, triggers, force_keep, meta, metrics, schemas);
verdict, parameters, triggers, force_keep, meta, metrics);
};
};

Expand Down
1 change: 0 additions & 1 deletion appsec/src/helper/subscriber/base.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ class subscriber {
struct event {
std::vector<std::string> data;
std::unordered_set<std::string> actions;
std::map<std::string, std::string> schemas;
};

class listener {
Expand Down
30 changes: 23 additions & 7 deletions appsec/src/helper/subscriber/waf.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
//
// This product includes software developed at Datadog
// (https://www.datadoghq.com/). Copyright 2021 Datadog, Inc.
#include "../std_logging.hpp"
#include "ddwaf.h"
#include <atomic>
#include <chrono>
#include <cstdlib>
Expand All @@ -18,7 +16,11 @@
#include <string_view>

#include "../json_helper.hpp"
#include "../std_logging.hpp"
#include "../tags.hpp"
#include "base64.h"
#include "compression.hpp"
#include "ddwaf.h"
#include "waf.hpp"

namespace dds::waf {
Expand All @@ -39,11 +41,6 @@ dds::subscriber::event format_waf_result(ddwaf_result &res)
output.data.emplace_back(std::move(parameter_to_json(event)));
}

const parameter_view schemas{res.derivatives};
for (const auto &schema : schemas) {
output.schemas.emplace(
schema.key(), std::move(parameter_to_json(schema)));
}
} catch (const std::exception &e) {
SPDLOG_ERROR("failed to parse WAF output: {}", e.what());
}
Expand Down Expand Up @@ -209,6 +206,11 @@ std::optional<subscriber::event> instance::listener::call(
// NOLINTNEXTLINE
total_runtime_ += res.total_runtime / 1000.0;

const parameter_view schemas{res.derivatives};
for (const auto &schema : schemas) {
schemas_.emplace(schema.key(), std::move(parameter_to_json(schema)));
}

switch (code) {
case DDWAF_MATCH:
return format_waf_result(res);
Expand Down Expand Up @@ -236,6 +238,20 @@ void instance::listener::get_meta_and_metrics(
{
meta[std::string(tag::event_rules_version)] = ruleset_version_;
metrics[tag::waf_duration] = total_runtime_;

for (const auto &[key, value] : schemas_) {
std::string schema = value;
if (value.length() > max_plain_schema_allowed) {
auto encoded = compress(schema);
if (encoded) {
schema = base64_encode(encoded.value(), false);
}
}

if (schema.length() <= max_schema_size) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to add a debug line here saying schema key has been discarded because of size.

meta.emplace(key, std::move(schema));
}
}
}

instance::instance(parameter &rule, std::map<std::string, std::string> &meta,
Expand Down
4 changes: 3 additions & 1 deletion appsec/src/helper/subscriber/waf.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ void initialise_logging(spdlog::level::level_enum level);
class instance : public dds::subscriber {
public:
static constexpr int default_waf_timeout_us = 10000;

static constexpr int max_plain_schema_allowed = 260;
static constexpr int max_schema_size = 25000;
using ptr = std::shared_ptr<instance>;
class listener : public dds::subscriber::listener {
public:
Expand All @@ -46,6 +47,7 @@ class instance : public dds::subscriber {
std::chrono::microseconds waf_timeout_;
double total_runtime_{0.0};
std::string_view ruleset_version_;
std::map<std::string, std::string> schemas_;
};

// NOLINTNEXTLINE(google-runtime-references)
Expand Down
3 changes: 0 additions & 3 deletions appsec/tests/extension/api_security_env_variables.phpt
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
--TEST--
Set and test API security ini settings
--ENV--
DD_EXPERIMENTAL_API_SECURITY_ENABLED=1
DD_API_SECURITY_REQUEST_SAMPLE_RATE=0.8
--FILE--
<?php
var_dump(ini_get("datadog.experimental_api_security_enabled"));
var_dump(ini_get("datadog.api_security_request_sample_rate"));
?>
--EXPECTF--
string(1) "1"
string(3) "0.8"
Binary file modified appsec/tests/extension/rinit_rshutdown_basic.phpt
Binary file not shown.
3 changes: 1 addition & 2 deletions appsec/tests/helper/broker_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ TEST(BrokerTest, SendRequestShutdown)
packer.pack_array(1); // Array of messages
packer.pack_array(2); // First message
pack_str(packer, "request_shutdown"); // Type
packer.pack_array(7);
packer.pack_array(6);
pack_str(packer, "block");
packer.pack_map(2);
pack_str(packer, "type");
Expand All @@ -179,7 +179,6 @@ TEST(BrokerTest, SendRequestShutdown)
packer.pack_true(); // Force keep
packer.pack_map(0);
packer.pack_map(0);
packer.pack_map(0);
const auto &expected_data = ss.str();

network::header_t h;
Expand Down
2 changes: 1 addition & 1 deletion appsec/tests/helper/client_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2365,7 +2365,7 @@ TEST(ClientTest, SchemasOverTheLimitAreCompressed)
int body_length = 0;
int i = 0;
// Lets generate a body which goes over max_plain_schema_allowed limit
while (body_length < client::max_plain_schema_allowed) {
while (body_length < waf::instance::max_plain_schema_allowed) {
body.add(std::to_string(i), parameter::int64(i));
auto schema = parameter::array();
schema.add(parameter::int64(4));
Expand Down
5 changes: 3 additions & 2 deletions appsec/tests/helper/waf_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,8 @@ TEST(WafTest, SchemasAreAdded)
EXPECT_FALSE(doc.HasParseError());
EXPECT_TRUE(doc.IsObject());

EXPECT_FALSE(res->schemas.empty());
EXPECT_STREQ(res->schemas["_dd.appsec.s.arg2"].c_str(), "[8]");
ctx->get_meta_and_metrics(meta, metrics);
EXPECT_FALSE(meta.empty());
EXPECT_STREQ(meta["_dd.appsec.s.arg2"].c_str(), "[8]");
}
} // namespace dds
Loading