Merge pull request #52 from DataDog/validate-helper-version

Validate helper version

cmake/helper.cmake

@@ -1,6 +1,8 @@
 hunter_add_package(Boost COMPONENTS system)
 find_package(Boost CONFIG REQUIRED COMPONENTS system)
 
+configure_file(src/helper/version.hpp.in ${CMAKE_CURRENT_SOURCE_DIR}/src/helper/version.hpp)
+
 set(HELPER_SOURCE_DIR src/helper)
 set(HELPER_INCLUDE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/src/helper)
 

src/extension/commands/client_init.c

@@ -9,6 +9,7 @@
 #include "../ddappsec.h"
 #include "../logging.h"
 #include "../msgpack_helpers.h"
+#include "../version.h"
 #include "client_init.h"
 #include "mpack-common.h"
 #include "mpack-node.h"
@@ -58,22 +59,25 @@ static dd_result _pack_command(
     return dd_success;
 }
 
+static dd_result _check_helper_version(mpack_node_t root);
 static dd_result _process_response(
     mpack_node_t root, ATTR_UNUSED void *nullable ctx)
 {
+    // check verdict
     mpack_node_t verdict = mpack_node_array_at(root, 0);
     bool is_ok = dd_mpack_node_lstr_eq(verdict, "ok");
     if (is_ok) {
         mlog(dd_log_debug, "Response to client_init is ok");
-        return dd_success;
+
+        return _check_helper_version(root);
     }
 
     // not ok, in which case expect at least one error message
 
     const char *ver = mpack_node_str(verdict);
     size_t verlen = mpack_node_strlen(verdict);
 
-    mpack_node_t errors = mpack_node_array_at(root, 1);
+    mpack_node_t errors = mpack_node_array_at(root, 2);
     mpack_node_t first_error_node = mpack_node_array_at(errors, 0);
     const char *first_error = mpack_node_str(first_error_node);
     size_t first_error_len = mpack_node_strlen(first_error_node);
@@ -96,3 +100,27 @@ static dd_result _process_response(
 
     return dd_error;
 }
+
+static dd_result _check_helper_version(mpack_node_t root)
+{
+    mpack_node_t version_node = mpack_node_array_at(root, 1);
+    const char *version = mpack_node_str(version_node);
+    size_t version_len = mpack_node_strlen(version_node);
+    int version_len_int = version_len > INT_MAX ? INT_MAX : (int)version_len;
+    mlog(
+        dd_log_debug, "Helper reported version %.*s", version_len_int, version);
+
+    if (!version) {
+        mlog(dd_log_warning, "Malformed client_init response when "
+                             "reading helper version");
+        return dd_error;
+    }
+    if (!STR_CONS_EQ(version, version_len, PHP_DDAPPSEC_VERSION)) {
+        mlog(dd_log_warning,
+            "Mismatch of helper and extension version. "
+            "helper %.*s and extension %s",
+            version_len_int, version, PHP_DDAPPSEC_VERSION);
+        return dd_error;
+    }
+    return dd_success;
+}

src/helper/.gitignore

@@ -0,0 +1 @@
+/version.hpp

src/helper/network/proto.hpp

@@ -12,6 +12,7 @@
 #include <optional>
 #include <type_traits>
 #include <typeinfo>
+#include <version.hpp>
 
 using stream_packer = msgpack::packer<std::stringstream>;
 
@@ -97,9 +98,10 @@ struct client_init {
         static constexpr response_id id = response_id::client_init;
 
         std::string status;
+        std::string version{dds::php_ddappsec_version};
         std::vector<std::string> errors;
 
-        MSGPACK_DEFINE(status, errors);
+        MSGPACK_DEFINE(status, version, errors);
     };
 };
 

src/helper/version.hpp.in

@@ -0,0 +1,13 @@
+// Unless explicitly stated otherwise all files in this repository are
+// dual-licensed under the Apache-2.0 License or BSD-3-Clause License.
+//
+// This product includes software developed at Datadog
+// (https://www.datadoghq.com/). Copyright 2021 Datadog, Inc.
+
+#pragma once
+
+#include <string_view>
+
+namespace dds {
+constexpr inline std::string_view php_ddappsec_version{"${CMAKE_PROJECT_VERSION}"};
+} // namespace dds

tests/bench_helper/Gemfile

@@ -1,3 +1,4 @@
 source 'http://rubygems.org'
 
 gem 'msgpack'
+gem 'irb'

tests/bench_helper/Gemfile.lock

@@ -1,12 +1,18 @@
 GEM
   remote: http://rubygems.org/
   specs:
+    io-console (0.5.9)
+    irb (1.4.1)
+      reline (>= 0.3.0)
     msgpack (1.4.2)
+    reline (0.3.0)
+      io-console (~> 0.5)
 
 PLATFORMS
   ruby
 
 DEPENDENCIES
+  irb
   msgpack
 
 BUNDLED WITH

tests/bench_helper/bench_helper.cc

@@ -115,6 +115,23 @@ static const std::string default_payload_sh{"req_shutdown.msgpack"}; // NOLINT
 static constexpr std::uint64_t waf_timeout_ms = 10;
 static constexpr int max_simultaneous_connects = 5;
 
+namespace mpack {
+std::string read_string(mpack_reader_t *r, mpack_tag_t tag)
+{
+    if (tag.type != mpack_type_str) {
+        r->error = mpack_error_data;
+        return {};
+    }
+
+    std::string res_str(static_cast<size_t>(tag.v.l), '\0');
+    mpack_read_utf8(r, &res_str[0], tag.v.l);
+    if (mpack_reader_error(r) != mpack_ok) {
+        return {};
+    }
+    return res_str;
+}
+} // namespace mpack
+
 // avoid blowing off the backlog of the socket, esp at startup
 class ConnectionLimiter {
 public:
@@ -358,7 +375,12 @@ void Benchmark::client_notify(std::chrono::microseconds req_duration)
 void Client::run()
 {
     SPDLOG_DEBUG("C#{} Doing client_init", id_);
-    do_client_init();
+    try {
+        do_client_init();
+    } catch (const std::exception &e) {
+        SPDLOG_ERROR("C#{} Error during client_init: {}", id_, e.what());
+        throw;
+    }
     SPDLOG_DEBUG("C#{} client_init done", id_);
     while (requests_left_-- > 0) {
         auto start{std::chrono::steady_clock::now()};
@@ -429,16 +451,32 @@ void Client::do_client_init()
     // read
     {
         SPDLOG_DEBUG("C#{} Reading client_init message response", id_);
-        // we read the error array that should be after the status
-        auto log_error = [this](mpack_reader_t *r, const std::string &status,
-                             uint32_t num_elements) {
-            if (status == "ok") {
+        // we read the error array and version that should be after the status
+        auto handle_resp = [this](mpack_reader_t *r, const std::string &status,
+                               uint32_t num_elements) {
+            mpack_tag_t tag;
+
+            if (num_elements < 3) {
+                throw std::runtime_error{
+                    "Expected response to have at least 3 elements; has " +
+                    std::to_string(num_elements)};
+            }
+
+            tag = mpack_read_tag(r);
+            std::string version_str{mpack::read_string(r, tag)};
+            if (mpack_reader_error(r) != mpack_ok) {
                 return;
             }
-            if (num_elements < 2) {
+            spdlog::default_logger_raw()->log(
+                spdlog::source_loc{__FILE__, __LINE__, "do_client_init"},
+                spdlog::level::debug, "C#{} Reported helper version is {}", id_,
+                version_str);
+
+            // if ok, we should have no error messages
+            if (status == "ok") {
                 return;
             }
-            mpack_tag_t tag = mpack_read_tag(r);
+            tag = mpack_read_tag(r);
             if (tag.type != mpack_type_array) {
                 r->error = mpack_error_data;
                 return;
@@ -447,23 +485,18 @@ void Client::do_client_init()
                 return;
             }
             tag = mpack_read_tag(r);
-            if (tag.type != mpack_type_str) {
-                r->error = mpack_error_data;
-                return;
-            }
-            std::string res_str(static_cast<size_t>(tag.v.l), '\0');
-            mpack_read_utf8(r, &res_str[0], tag.v.l);
+            std::string err_str{mpack::read_string(r, tag)};
             if (mpack_reader_error(r) != mpack_ok) {
                 return;
             }
             spdlog::default_logger_raw()->log(
                 spdlog::source_loc{__FILE__, __LINE__, "do_client_init"},
                 spdlog::level::err, "C#{} Error message for client_init is: {}",
-                id_, res_str);
+                id_, err_str);
         };
 
-        std::string resp{read_helper_response(std::move(log_error))};
-        SPDLOG_DEBUG("C#{} client_init response:", id_, resp);
+        std::string resp{read_helper_response(std::move(handle_resp))};
+        SPDLOG_DEBUG("C#{} client_init response: {}", id_, resp);
         if (resp != "ok") {
             throw std::runtime_error{"client_init response was not ok"};
         }
@@ -572,11 +605,6 @@ std::string Client::read_helper_response(Function &&f)
             throw std::runtime_error{"expected array at root"};
         }
         num_elements = root.v.n;
-        if (num_elements != 2) {
-            throw std::runtime_error{
-                "expected root array to have 2 elements; has " +
-                std::to_string(num_elements)};
-        }
     }
 
     {

tests/extension/client_init_fail.phpt

@@ -6,7 +6,7 @@ use function datadog\appsec\testing\{rinit,backoff_status,is_without_holes};
 
 include __DIR__ . '/inc/mock_helper.php';
 
-$helper = Helper::createRun([['not-ok', ['such and such error occurred']]]);
+$helper = Helper::createRun([['not-ok', phpversion('ddappsec'), ['such and such error occurred']]]);
 
 var_dump(rinit());
 

tests/extension/client_init_wrong_version.phpt

@@ -0,0 +1,28 @@
+--TEST--
+client_init returns a mismatched version
+--FILE--
+<?php
+use function datadog\appsec\testing\{rinit,backoff_status,is_without_holes};
+
+include __DIR__ . '/inc/mock_helper.php';
+
+$helper = Helper::createRun([['ok', '0.0.0']]);
+
+var_dump(rinit());
+
+var_dump(backoff_status());
+
+?>
+--EXPECTF--
+Warning: datadog\appsec\testing\rinit(): [ddappsec] Mismatch of helper and extension version. helper 0.0.0 and extension %s in %s on line %s
+
+Warning: datadog\appsec\testing\rinit(): [ddappsec] Processing for command client_init failed: dd_error in %s on line %d
+
+Warning: datadog\appsec\testing\rinit(): [ddappsec] Initial exchange with helper failed; abandoning the connection in %s on line %d
+bool(true)
+array(2) {
+  ["failed_count"]=>
+  int(1)
+  ["next_retry"]=>
+  float(%f)
+}

tests/extension/ddtrace_basic.phpt

@@ -20,7 +20,7 @@ use const datadog\appsec\testing\log_level\DEBUG;
 
 include __DIR__ . '/inc/mock_helper.php';
 
-$helper = Helper::createRun([['ok']], ['continuous' => true]);
+$helper = Helper::createInitedRun([['ok']], ['continuous' => true]);
 
 mlog(DEBUG, "Call rinit");
 echo "rinit\n";

tests/extension/helper_launch_ok.phpt

@@ -43,7 +43,7 @@ $helper_args = implode(' ', array_merge(
         '-d', 'display_errors=1',
         '-d', 'error_reporting=2147483647',
         __DIR__ . "/inc/helper_invocation.php",
-        '--'
+        phpversion('ddappsec')
     )));
 set_helper_extra_args($helper_args);
 

tests/extension/inc/helper_invocation.php

@@ -77,7 +77,10 @@
 fwrite(STDERR, "read remaining data");
 
 fflush(STDERR);
+$version = $argv[1];
 $data = "dds\0" .
-	"\x04\x00\x00\x00" . // 3 in little-endian
-	"\x91\xa2ok"; // ["ok"] in msgpack
+	chr(4 + 1 + strlen($version)) . "\x00\x00\x00" . // length in little-endian
+	"\x92\xa2ok" . // ["ok", <>] in msgpack
+	chr(0xA0 + strlen($version)) . $version; // "$version" in msgpack
+
 fwrite($cstream, $data);

tests/extension/inc/mock_helper.php

@@ -45,6 +45,11 @@ static function createRun($responses, $opts = array()) {
         return $helper;
     }
 
+    static function createInitedRun($responses, $opts = array()) {
+        $responses = array_merge([['ok', phpversion('ddappsec')]], $responses);
+        return self::createRun($responses, $opts);
+    }
+
     function run($responses, $continuous = false) {
         $esc_resp = "";
         foreach ($responses as $response) {

tests/extension/rinit_body_multipart.phpt

@@ -24,7 +24,7 @@ use function datadog\appsec\testing\rinit;
 
 include __DIR__ . '/inc/mock_helper.php';
 
-$helper = Helper::createRun([['ok'], ['ok']]);
+$helper = Helper::createInitedRun([['ok']]);
 
 var_dump(rinit());
 

tests/extension/rinit_body_urlencoded.phpt

@@ -8,7 +8,7 @@ use function datadog\appsec\testing\{rinit,rshutdown};
 
 include __DIR__ . '/inc/mock_helper.php';
 
-$helper = Helper::createRun([['ok'], ['ok']]);
+$helper = Helper::createInitedRun([['ok']]);
 
 var_dump($_POST);
 var_dump(rinit());

tests/extension/rinit_body_xml.phpt

@@ -10,7 +10,7 @@ use function datadog\appsec\testing\rinit;
 
 include __DIR__ . '/inc/mock_helper.php';
 
-$helper = Helper::createRun([['ok'], ['ok']]);
+$helper = Helper::createInitedRun([['ok']]);
 
 var_dump(rinit());
 

tests/extension/rinit_fail_malformed_resp.phpt

@@ -7,7 +7,7 @@ use function datadog\appsec\testing\{rinit,rshutdown,backoff_status,is_connected
 include __DIR__ . '/inc/mock_helper.php';
 
 // respond correctly to client_init and request_shutdown, but not request_init
-$helper = Helper::createRun([['ok'], [['foo' => 'ok']], ['ok']]);
+$helper = Helper::createInitedRun([[['foo' => 'ok']], ['ok']]);
 
 echo "rinit:\n";
 var_dump(rinit());

tests/extension/rinit_fail_network.phpt

@@ -6,7 +6,7 @@ use function datadog\appsec\testing\{rinit,rshutdown,backoff_status,is_connected
 
 include __DIR__ . '/inc/mock_helper.php';
 
-$helper = Helper::createRun([['ok']]); // respond to client_init, but not request_init
+$helper = Helper::createInitedRun([]); // respond to client_init, but not request_init
 
 echo "rinit:\n";
 var_dump(rinit());

tests/extension/rinit_record_span_tags.phpt

@@ -28,8 +28,7 @@ print_r(root_span_get_meta());
 
 include __DIR__ . '/inc/mock_helper.php';
 
-$helper = Helper::createRun([
-    ['ok'],
+$helper = Helper::createInitedRun([
     ['record', ['{"found":"attack"}','{"another":"attack"}']],
     ['record', ['{"yet another":"attack"}']],
 ], ['continuous' => true]);

tests/extension/rinit_record_span_tags_fail.phpt

@@ -12,7 +12,7 @@ use function datadog\appsec\testing\{rinit,ddtrace_rshutdown,root_span_get_meta}
 
 include __DIR__ . '/inc/mock_helper.php';
 
-$helper = Helper::createRun([['ok'], ['record', '[{"found":"attack"}]']], ['continuous' => true]);
+$helper = Helper::createInitedRun([['record', '[{"found":"attack"}]']], ['continuous' => true]);
 
 echo "root_span_get_meta (should fail: no root span):\n";
 var_dump(root_span_get_meta());