diff --git a/CHANGELOG.md b/CHANGELOG.md index 60f5d99a3..838017ee3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,25 @@ documentation before upgrading to a new release. Released closed milestones can be found on [GitHub](https://github.com/Icinga/icinga2/milestones?state=closed). +## 2.16.2 (2026-06-29) + +This release fixes some critical security vulnerabilities in Icinga 2. Users are advised to upgrade immediately, as two +of them allow an unauthenticated attacker to take over or crash the Icinga 2 process over the network. The other +security fixes only affect authenticated API users. + +In addition, a new permission named `filter-expression` is introduced, which allows specifying if individual API users +are allowed to use DSL filter expressions in API queries. This allows further restricting some API users that don't need +this capability, for example, those only submitting individual check results. Due to the incompatibility of this change, +enforcement of this permission is opt-in until v2.17; see the +[upgrading docs](https://icinga.com/docs/icinga-2/latest/doc/16-upgrading-icinga-2/#upgrading-to-2-16-2) for details. + +* Verify that certificate update requests come from an authorized endpoint ([GHSA-vj39-ww8j-vvx5](https://github.com/Icinga/icinga2/security/advisories/GHSA-vj39-ww8j-vvx5)) +* Fix stack overflow due to deeply nested data structures ([GHSA-wh38-wg57-5w7g](https://github.com/Icinga/icinga2/security/advisories/GHSA-wh38-wg57-5w7g)) +* Prevent arbitrary config injection on object creation via the API ([GHSA-jgqj-x5j9-vgcm](https://github.com/Icinga/icinga2/security/advisories/GHSA-jgqj-x5j9-vgcm)) +* Fix that `/v1/config/files` could send uninitialized memory in case of file I/O errors (#10871) +* Add `filter-expression` permission to make it possible to prevent API users from using DSL filter expressions +* Windows: Update bundled OpenSSL to v3.5.7 (#10893) + ## 2.16.1 (2026-05-21) This is a small release mostly to fix the issues some users were encountering in connection with perfdata writers, diff --git a/CMakeLists.txt b/CMakeLists.txt index 96cfedab4..f0f872a14 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -390,6 +390,7 @@ check_function_exists(backtrace_symbols HAVE_BACKTRACE_SYMBOLS) check_function_exists(pipe2 HAVE_PIPE2) check_function_exists(nice HAVE_NICE) check_function_exists(malloc_info HAVE_MALLOC_INFO) +check_function_exists(pthread_create HAVE_PTHREAD_CREATE) check_function_exists(pthread_set_name_np HAVE_PTHREAD_SET_NAME_NP) check_function_exists(pthread_setname_np HAVE_PTHREAD_SETNAME_NP) check_library_exists(dl dladdr "dlfcn.h" HAVE_DLADDR) diff --git a/ICINGA2_VERSION b/ICINGA2_VERSION index 0fa1c46b1..8cbec1792 100644 --- a/ICINGA2_VERSION +++ b/ICINGA2_VERSION @@ -1,2 +1,2 @@ -Version: 2.16.1 +Version: 2.16.2 Revision: 1 diff --git a/config.h.cmake b/config.h.cmake index 2bfadb4a0..b3b36207a 100644 --- a/config.h.cmake +++ b/config.h.cmake @@ -9,6 +9,7 @@ #cmakedefine HAVE_CXXABI_H #cmakedefine HAVE_NICE #cmakedefine HAVE_MALLOC_INFO +#cmakedefine HAVE_PTHREAD_CREATE #cmakedefine HAVE_PTHREAD_SET_NAME_NP #cmakedefine HAVE_PTHREAD_SETNAME_NP #cmakedefine HAVE_EDITLINE diff --git a/doc/02-installation.md b/doc/02-installation.md index d369f5111..dbfc38a2b 100644 --- a/doc/02-installation.md +++ b/doc/02-installation.md @@ -363,6 +363,15 @@ Run the following command to: icinga2 api setup ``` +For new installations, it is recommended to set the following additional attribute inside the `ApiListener` object +definition in `/etc/icinga2/features-enabled/api.conf`. This will already enforce stricter permissions as they will +become the default with v2.17 (see the [upgrading documentation](16-upgrading-icinga-2.md#upgrading-to-2-16-2) for the +version that introduced that setting for more details): + +``` +enforce_filter_expression_permission = true +``` + Restart Icinga 2 for these changes to take effect. ```bash diff --git a/doc/09-object-types.md b/doc/09-object-types.md index 9833d159d..f558ef0a7 100644 --- a/doc/09-object-types.md +++ b/doc/09-object-types.md @@ -1088,28 +1088,29 @@ object ApiListener "api" { Configuration Attributes: - Name | Type | Description - --------------------------------------|-----------------------|---------------------------------- - cert\_path | String | **Deprecated.** Path to the public key. - key\_path | String | **Deprecated.** Path to the private key. - ca\_path | String | **Deprecated.** Path to the CA certificate file. - ticket\_salt | String | **Optional.** Private key for [CSR auto-signing](06-distributed-monitoring.md#distributed-monitoring-setup-csr-auto-signing). **Required** for a signing master instance. - crl\_path | String | **Optional.** Path to the CRL file. - bind\_host | String | **Optional.** The IP address the api listener should be bound to. If not specified, the ApiListener is bound to `::` and listens for both IPv4 and IPv6 connections or to `0.0.0.0` if IPv6 is not supported by the operating system. - bind\_port | Number | **Optional.** The port the api listener should be bound to. Defaults to `5665`. - accept\_config | Boolean | **Optional.** Accept zone configuration. Defaults to `false`. - accept\_commands | Boolean | **Optional.** Accept remote commands. Defaults to `false`. - max\_anonymous\_clients | Number | **Optional.** Limit the number of anonymous client connections (not configured endpoints and signing requests). - cipher\_list | String | **Optional.** Cipher list that is allowed. For a list of available ciphers run `openssl ciphers`. Defaults to `ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384:DHE-RSA-CHACHA20-POLY1305:DHE-RSA-AES128-GCM-SHA256`. - tls\_protocolmin | String | **Optional.** Minimum TLS protocol version. Since v2.11, only `TLSv1.2` is supported. Defaults to `TLSv1.2`. - tls\_handshake\_timeout | Number | **Deprecated.** TLS Handshake timeout. Defaults to `10s`. - connect\_timeout | Number | **Optional.** Timeout for establishing new connections. Affects both incoming and outgoing connections. Within this time, the TCP and TLS handshakes must complete and either a HTTP request or an Icinga cluster connection must be initiated. Defaults to `15s`. - access\_control\_allow\_origin | Array | **Optional.** Specifies an array of origin URLs that may access the API. [(MDN docs)](https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS#Access-Control-Allow-Origin) - access\_control\_allow\_credentials | Boolean | **Deprecated.** Indicates whether or not the actual request can be made using credentials. Defaults to `true`. [(MDN docs)](https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS#Access-Control-Allow-Credentials) - access\_control\_allow\_headers | String | **Deprecated.** Used in response to a preflight request to indicate which HTTP headers can be used when making the actual request. Defaults to `Authorization`. [(MDN docs)](https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS#Access-Control-Allow-Headers) - access\_control\_allow\_methods | String | **Deprecated.** Used in response to a preflight request to indicate which HTTP methods can be used when making the actual request. Defaults to `GET, POST, PUT, DELETE`. [(MDN docs)](https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS#Access-Control-Allow-Methods) - environment | String | **Optional.** Used as suffix in TLS SNI extension name; default from constant `ApiEnvironment`, which is empty. - http\_response\_headers | Dictionary | **Optional.** Additional headers to add to HTTP responses, for example `{"Strict-Transport-Security" = "max-age=31536000"}`. Defaults to none. + Name | Type | Description + ----------------------------------------|------------|---------------------------------- + cert\_path | String | **Deprecated.** Path to the public key. + key\_path | String | **Deprecated.** Path to the private key. + ca\_path | String | **Deprecated.** Path to the CA certificate file. + ticket\_salt | String | **Optional.** Private key for [CSR auto-signing](06-distributed-monitoring.md#distributed-monitoring-setup-csr-auto-signing). **Required** for a signing master instance. + crl\_path | String | **Optional.** Path to the CRL file. + bind\_host | String | **Optional.** The IP address the api listener should be bound to. If not specified, the ApiListener is bound to `::` and listens for both IPv4 and IPv6 connections or to `0.0.0.0` if IPv6 is not supported by the operating system. + bind\_port | Number | **Optional.** The port the api listener should be bound to. Defaults to `5665`. + accept\_config | Boolean | **Optional.** Accept zone configuration. Defaults to `false`. + accept\_commands | Boolean | **Optional.** Accept remote commands. Defaults to `false`. + max\_anonymous\_clients | Number | **Optional.** Limit the number of anonymous client connections (not configured endpoints and signing requests). + cipher\_list | String | **Optional.** Cipher list that is allowed. For a list of available ciphers run `openssl ciphers`. Defaults to `ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384:DHE-RSA-CHACHA20-POLY1305:DHE-RSA-AES128-GCM-SHA256`. + tls\_protocolmin | String | **Optional.** Minimum TLS protocol version. Since v2.11, only `TLSv1.2` is supported. Defaults to `TLSv1.2`. + tls\_handshake\_timeout | Number | **Deprecated.** TLS Handshake timeout. Defaults to `10s`. + connect\_timeout | Number | **Optional.** Timeout for establishing new connections. Affects both incoming and outgoing connections. Within this time, the TCP and TLS handshakes must complete and either a HTTP request or an Icinga cluster connection must be initiated. Defaults to `15s`. + access\_control\_allow\_origin | Array | **Optional.** Specifies an array of origin URLs that may access the API. [(MDN docs)](https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS#Access-Control-Allow-Origin) + access\_control\_allow\_credentials | Boolean | **Deprecated.** Indicates whether or not the actual request can be made using credentials. Defaults to `true`. [(MDN docs)](https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS#Access-Control-Allow-Credentials) + access\_control\_allow\_headers | String | **Deprecated.** Used in response to a preflight request to indicate which HTTP headers can be used when making the actual request. Defaults to `Authorization`. [(MDN docs)](https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS#Access-Control-Allow-Headers) + access\_control\_allow\_methods | String | **Deprecated.** Used in response to a preflight request to indicate which HTTP methods can be used when making the actual request. Defaults to `GET, POST, PUT, DELETE`. [(MDN docs)](https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS#Access-Control-Allow-Methods) + environment | String | **Optional.** Used as suffix in TLS SNI extension name; default from constant `ApiEnvironment`, which is empty. + http\_response\_headers | Dictionary | **Optional.** Additional headers to add to HTTP responses, for example `{"Strict-Transport-Security" = "max-age=31536000"}`. Defaults to none. + enforce\_filter\_expression\_permission | Boolean | **Optional.** Enforce the `filter-expression` permission. Defaults to `false` until v2.17 for compatibility. The attributes `access_control_allow_credentials`, `access_control_allow_headers` and `access_control_allow_methods` are controlled by Icinga 2 and are not changeable by config any more. diff --git a/doc/12-icinga2-api.md b/doc/12-icinga2-api.md index 532da3b03..a42425376 100644 --- a/doc/12-icinga2-api.md +++ b/doc/12-icinga2-api.md @@ -299,6 +299,18 @@ Available permissions for specific URL endpoints: types | /v1/types | Yes | 1 variables | /v1/variables | Yes | 1 +Available permissions that are not bound to specific URL endpoints: + + Permissions | Description + ------------------------------|------------ + filter-expression | Allows the user to provide their own [advanced filter expressions](12-icinga2-api.md#icinga2-api-advanced-filters). + +!!! warning + + The `filter-expression` permission was introduced in v2.16.2 and is only enforced if the + [`enforce_filter_expression_permission` attribute of `ApiListener`](09-object-types.md#objecttype-apilistener) + is set to `true`. For compatibility reasons, this will not be enforced by default until v2.17. + The required actions or types can be replaced by using a wildcard match ("\*"). @@ -435,6 +447,16 @@ The syntax for these filters is the same like for [apply rule expressions](03-mo The `filter` parameter can only be specified once, complex filters must be defined once in the provided string value. +!!! warning + + In order to use these advanced filters, the `ApiUser` must be granted the `filter-expression` permission, + which should only be done for trusted users. The evaluation happens in the main Icinga 2 worker process and may be + abused for denial-of-service attacks, potentially crashing the Icinga 2 daemon. + + Note that before v2.17, this permission is not enforced by default but only if the + [`enforce_filter_expression_permission` attribute of `ApiListener`](09-object-types.md#objecttype-apilistener) + is set accordingly. + > **Note** > > Filters used as URL parameter must be URL-encoded. The following examples diff --git a/doc/16-upgrading-icinga-2.md b/doc/16-upgrading-icinga-2.md index dbd9567ef..c94fe4b3a 100644 --- a/doc/16-upgrading-icinga-2.md +++ b/doc/16-upgrading-icinga-2.md @@ -8,6 +8,23 @@ Specific version upgrades are described below. Please note that version updates are incremental. An upgrade from v2.6 to v2.8 requires to follow the instructions for v2.7 too. +## Upgrading to v2.16.2, v2.15.4, or v2.14.9 + +### New `filter-expression` permission + +When using the Icinga 2 REST API, filter expressions are a powerful tool. However, that power can also be abused for +denial-of-service attacks by authenticated users. Given that these filter expressions are Icinga 2 DSL expressions and +their evaluation happens in the main Icinga 2 worker process, this may also crash the Icinga 2 daemon. + +In order to allow mitigating such problems, a new permission named `filter-expression` was added. **Before v2.17, this +permission will not be enforced by default** due to the impact on existing installations. Nonetheless, we recommend +reviewing your `ApiUser` configuration, explicitly allowing the new permission where necessary and enabling the +enforcement of the permission using the [`enforce_filter_expression_permission` attribute of +`ApiListener`](09-object-types.md#objecttype-apilistener). If an `ApiUser` makes use of the permission while the +permission is not enforced yet, it will be logged. Thus, it is also possible to upgrade, observe the logs, grant the +permission as needed or adapting API clients to avoid using filters if possible, and enable the enforcement at a later +time. + ## Upgrading to v2.16 ### Migrating from ElasticsearchWriter to OTLPMetricsWriter diff --git a/lib/base/configobject.cpp b/lib/base/configobject.cpp index b43b07800..45531e01c 100644 --- a/lib/base/configobject.cpp +++ b/lib/base/configobject.cpp @@ -98,6 +98,13 @@ void ConfigObject::ModifyAttribute(const String& attr, const Value& value, bool std::vector tokens = attr.Split("."); + // This is reachable from ModifyObjectHandler. This check prevents API clients from creating deeply nested data + // structures that could overflow the stack later on. + if (tokens.size() > VarDepthLimit) { + BOOST_THROW_EXCEPTION(std::invalid_argument("Attribute '" + attr + "' exceeds maximum nesting level of " + + std::to_string(VarDepthLimit) + ".")); + } + String fieldName = tokens[0]; int fid = type->GetFieldId(fieldName); @@ -503,7 +510,7 @@ void ConfigObject::DumpObjects(const String& filename, int attributeTypes) void ConfigObject::RestoreObject(const String& message, int attributeTypes) { - Dictionary::Ptr persistentObject = JsonDecode(message); + Dictionary::Ptr persistentObject = JsonDecode(message, std::numeric_limits::max()); String type = persistentObject->Get("type"); String name = persistentObject->Get("name"); diff --git a/lib/base/configobject.hpp b/lib/base/configobject.hpp index 7ee8c5b68..431727813 100644 --- a/lib/base/configobject.hpp +++ b/lib/base/configobject.hpp @@ -26,6 +26,8 @@ class ConfigObject : public ObjectImpl public: DECLARE_OBJECT(ConfigObject); + static constexpr size_t VarDepthLimit = 16; + static boost::signals2::signal OnStateChanged; bool IsActive() const; diff --git a/lib/base/configwriter.cpp b/lib/base/configwriter.cpp index b9d912615..541bd8b5a 100644 --- a/lib/base/configwriter.cpp +++ b/lib/base/configwriter.cpp @@ -64,7 +64,8 @@ void ConfigWriter::EmitScope(std::ostream& fp, int indentLevel, const Dictionary for (const Value& import : imports) { fp << "\n"; EmitIndent(fp, indentLevel); - fp << "import \"" << import << "\""; + fp << "import "; + EmitString(fp, import.Get()); } fp << "\n"; @@ -79,6 +80,13 @@ void ConfigWriter::EmitScope(std::ostream& fp, int indentLevel, const Dictionary if (splitDot) { std::vector tokens = kv.first.Split("."); + // This is reachable from CreateObjectHandler. This check prevents API clients from creating deeply + // nested data structures that could overflow the stack later on. + if (tokens.size() > ConfigObject::VarDepthLimit) { + BOOST_THROW_EXCEPTION(std::invalid_argument("Attribute '" + kv.first + + "' exceeds maximum nesting level of " + std::to_string(ConfigObject::VarDepthLimit) + ".")); + } + EmitIdentifier(fp, tokens[0], true); for (std::vector::size_type i = 1; i < tokens.size(); i++) { @@ -175,11 +183,6 @@ void ConfigWriter::EmitConfigItem(std::ostream& fp, const String& type, const St EmitScope(fp, 1, attrs, imports, true); } -void ConfigWriter::EmitComment(std::ostream& fp, const String& text) -{ - fp << "/* " << text << " */\n"; -} - void ConfigWriter::EmitFunctionCall(std::ostream& fp, const String& name, const Array::Ptr& arguments) { EmitIdentifier(fp, name, false); diff --git a/lib/base/configwriter.hpp b/lib/base/configwriter.hpp index 9f222ab5d..6b005c2ce 100644 --- a/lib/base/configwriter.hpp +++ b/lib/base/configwriter.hpp @@ -54,7 +54,6 @@ public: static void EmitConfigItem(std::ostream& fp, const String& type, const String& name, bool isTemplate, bool ignoreOnError, const Array::Ptr& imports, const Dictionary::Ptr& attrs); - static void EmitComment(std::ostream& fp, const String& text); static void EmitFunctionCall(std::ostream& fp, const String& name, const Array::Ptr& arguments); static const std::vector& GetKeywords(); diff --git a/lib/base/io-engine.hpp b/lib/base/io-engine.hpp index cc2eb72de..9c35e84d6 100644 --- a/lib/base/io-engine.hpp +++ b/lib/base/io-engine.hpp @@ -18,7 +18,7 @@ #include #include #include -#include +#include #include #include #include @@ -153,7 +153,7 @@ public: #if BOOST_VERSION >= 108700 boost::asio::spawn(h, - std::allocator_arg, boost::context::fixedsize_stack(GetCoroutineStackSize()), + std::allocator_arg, boost::context::protected_fixedsize_stack(GetCoroutineStackSize()), std::move(wrapper), boost::asio::detached ); diff --git a/lib/base/json.cpp b/lib/base/json.cpp index 5ab256200..122d25b4e 100644 --- a/lib/base/json.cpp +++ b/lib/base/json.cpp @@ -324,6 +324,8 @@ void JsonEncoder::Flusher::FlushIfSafe(boost::asio::yield_context* yc) const class JsonSax : public nlohmann::json_sax { public: + explicit JsonSax(std::size_t depthLimit) : m_DepthLimit(depthLimit) {} + bool null() override; bool boolean(bool val) override; bool number_integer(number_integer_t val) override; @@ -341,11 +343,22 @@ public: Value GetResult(); private: + struct Node + { + Dictionary::Ptr m_Dictionary{}; + Array::Ptr m_Array{}; + String m_CurrentKey{}; + + explicit Node(Dictionary::Ptr dict) : m_Dictionary(std::move(dict)) {} + explicit Node(Array::Ptr array) : m_Array(std::move(array)) {} + }; + Value m_Root; - std::stack> m_CurrentSubtree; - String m_CurrentKey; + std::vector m_Stack; + std::size_t m_DepthLimit; void FillCurrentTarget(Value value); + void CheckDepthLimit() const; }; String icinga::JsonEncode(const Value& value, bool prettify) @@ -369,11 +382,23 @@ void icinga::JsonEncode(const Value& value, std::ostream& os, bool prettify) encoder.Encode(value); } -Value icinga::JsonDecode(const String& data) +/** + * Parses a JSON string into the Icinga Value type. + * + * A depth limit can be provided. Both JSON arrays (mapped to icinga::Array) and JSON objects + * (mapped to icinga::Dictionary) count towards that limit. A limit of 1 means, that the outer object can be an array + * or object, but it can only contain scalar values. + * + * @param data The JSON to be parsed (throws if the JSON is invalid). + * @param depthLimit The maximum depth of the returned data structure, + * defaults to 24 (throws if the JSON is nested too deep). + * @return The parsed value. + */ +Value icinga::JsonDecode(const String& data, size_t depthLimit) { String sanitized (Utility::ValidateUTF8(data)); - JsonSax stateMachine; + JsonSax stateMachine{depthLimit}; nlohmann::json::sax_parse(sanitized.Begin(), sanitized.End(), &stateMachine); @@ -440,10 +465,9 @@ inline bool JsonSax::start_object(std::size_t) { auto object (new Dictionary()); - FillCurrentTarget(object); - - m_CurrentSubtree.push({object, nullptr}); + CheckDepthLimit(); + m_Stack.push_back(Node{object}); return true; } @@ -451,7 +475,7 @@ bool JsonSax::start_object(std::size_t) inline bool JsonSax::key(JsonSax::string_t& val) { - m_CurrentKey = String(std::move(val)); + m_Stack.back().m_CurrentKey = String(std::move(val)); return true; } @@ -459,8 +483,7 @@ bool JsonSax::key(JsonSax::string_t& val) inline bool JsonSax::end_object() { - m_CurrentSubtree.pop(); - m_CurrentKey = String(); + m_Stack.pop_back(); return true; } @@ -469,10 +492,9 @@ inline bool JsonSax::start_array(std::size_t) { auto array (new Array()); - FillCurrentTarget(array); - - m_CurrentSubtree.push({nullptr, array}); + CheckDepthLimit(); + m_Stack.push_back(Node{array}); return true; } @@ -480,7 +502,7 @@ bool JsonSax::start_array(std::size_t) inline bool JsonSax::end_array() { - m_CurrentSubtree.pop(); + m_Stack.pop_back(); return true; } @@ -500,15 +522,34 @@ Value JsonSax::GetResult() inline void JsonSax::FillCurrentTarget(Value value) { - if (m_CurrentSubtree.empty()) { + if (m_Stack.empty()) { m_Root = value; } else { - auto& node (m_CurrentSubtree.top()); + auto& node (m_Stack.back()); - if (node.first) { - node.first->Set(m_CurrentKey, value); + if (node.m_Dictionary) { + node.m_Dictionary->Set(node.m_CurrentKey, value); } else { - node.second->Add(value); + node.m_Array->Add(value); } } } + +void JsonSax::CheckDepthLimit() const +{ + if (m_Stack.size() >= m_DepthLimit) { + std::ostringstream buf; + buf << "JSON decoding recursion limit reached (path: root"; + for (const auto& node : m_Stack) { + buf << "["; + if (node.m_Dictionary) { + buf << JsonEncode(node.m_CurrentKey); + } else { + buf << node.m_Array->GetLength() - 1; + } + buf << "]"; + } + buf << ")"; + BOOST_THROW_EXCEPTION(std::runtime_error(buf.str())); + } +} diff --git a/lib/base/json.hpp b/lib/base/json.hpp index c83d839d5..02b05e649 100644 --- a/lib/base/json.hpp +++ b/lib/base/json.hpp @@ -118,9 +118,11 @@ private: } m_Flusher; }; +static constexpr size_t JsonDecodeDefaultDepthLimit = 24; + String JsonEncode(const Value& value, bool prettify = false); void JsonEncode(const Value& value, std::ostream& os, bool prettify = false); -Value JsonDecode(const String& data); +Value JsonDecode(const String& data, size_t depthLimit = JsonDecodeDefaultDepthLimit); } diff --git a/lib/cli/objectlistutility.cpp b/lib/cli/objectlistutility.cpp index abbf414c0..c8c66d18c 100644 --- a/lib/cli/objectlistutility.cpp +++ b/lib/cli/objectlistutility.cpp @@ -14,7 +14,7 @@ using namespace icinga; bool ObjectListUtility::PrintObject(std::ostream& fp, bool& first, const String& message, std::map& type_count, const String& name_filter, const String& type_filter) { - Dictionary::Ptr object = JsonDecode(message); + Dictionary::Ptr object = JsonDecode(message, std::numeric_limits::max()); Dictionary::Ptr properties = object->Get("properties"); diff --git a/lib/remote/actionshandler.cpp b/lib/remote/actionshandler.cpp index 76874cee4..40d6607f4 100644 --- a/lib/remote/actionshandler.cpp +++ b/lib/remote/actionshandler.cpp @@ -54,6 +54,9 @@ bool ActionsHandler::HandleRequest( try { objs = FilterUtility::GetFilterTargets(qd, params, user); + } catch (const MissingPermissionError& ex) { + HttpUtility::SendJsonError(response, params, 403, ex.what()); + return true; } catch (const std::exception& ex) { HttpUtility::SendJsonError(response, params, 404, "No objects found.", diff --git a/lib/remote/apilistener.cpp b/lib/remote/apilistener.cpp index aa13eaf56..2a358ee03 100644 --- a/lib/remote/apilistener.cpp +++ b/lib/remote/apilistener.cpp @@ -234,6 +234,18 @@ void ApiListener::OnAllConfigLoaded() if (!m_LocalEndpoint) BOOST_THROW_EXCEPTION(ScriptError("Endpoint object for '" + GetIdentity() + "' is missing.", GetDebugInfo())); + + if (!GetEnforceFilterExpressionPermission()) { + Log(LogWarning, "ApiListener") << "Security notice:\n" + " Currently, all ApiUsers are allowed to use Icinga 2 DSL filter expressions\n" + " in API queries because enforce_filter_expression_permission is set to false.\n" + " This can pose a security risk as filters are evaluated within the Icinga 2\n" + " process and their complexity can be used for denial of service attacks. The\n" + " new '" << ApiUser::FilterExpressionPerm << "' permission can be used to allow this for individual\n" + " ApiUsers, which should only be granted to trusted users. It is recommended\n" + " to set enforce_filter_expression_permission to true to enforce the\n" + " permission. This will become the default in v2.17."; + } } /** diff --git a/lib/remote/apilistener.ti b/lib/remote/apilistener.ti index b3cd883cd..afa13f5ce 100644 --- a/lib/remote/apilistener.ti +++ b/lib/remote/apilistener.ti @@ -63,6 +63,10 @@ class ApiListener : ConfigObject [no_user_modify] String identity; [state, no_user_modify] Dictionary::Ptr last_failed_zones_stage_validation; + + [config, no_user_modify] bool enforce_filter_expression_permission { + default {{{ return false; }}} + }; }; } diff --git a/lib/remote/apiuser.hpp b/lib/remote/apiuser.hpp index 0e6336ed4..b044e4abc 100644 --- a/lib/remote/apiuser.hpp +++ b/lib/remote/apiuser.hpp @@ -21,6 +21,8 @@ public: static ApiUser::Ptr GetByClientCN(const String& cn); static ApiUser::Ptr GetByAuthHeader(const String& auth_header); + + static inline const String FilterExpressionPerm = "filter-expression"; }; } diff --git a/lib/remote/deleteobjecthandler.cpp b/lib/remote/deleteobjecthandler.cpp index a918d4a30..beecc2001 100644 --- a/lib/remote/deleteobjecthandler.cpp +++ b/lib/remote/deleteobjecthandler.cpp @@ -58,6 +58,9 @@ bool DeleteObjectHandler::HandleRequest( try { objs = FilterUtility::GetFilterTargets(qd, params, user); + } catch (const MissingPermissionError& ex) { + HttpUtility::SendJsonError(response, params, 403, ex.what()); + return true; } catch (const std::exception& ex) { HttpUtility::SendJsonError(response, params, 404, "No objects found.", diff --git a/lib/remote/eventshandler.cpp b/lib/remote/eventshandler.cpp index e1c354b96..545c3ba75 100644 --- a/lib/remote/eventshandler.cpp +++ b/lib/remote/eventshandler.cpp @@ -92,7 +92,18 @@ bool EventsHandler::HandleRequest( } } - EventsSubscriber subscriber (std::move(eventTypes), HttpUtility::GetLastParameter(params, "filter"), l_ApiQuery); + String filter = ""; + if (params && params->Contains("filter")) { + if (!FilterUtility::HasPermission(request.User(), ApiUser::FilterExpressionPerm, nullptr)) { + HttpUtility::SendJsonError(response, params, 403, + "Missing permission: " + ApiUser::FilterExpressionPerm); + return true; + } + + filter = HttpUtility::GetLastParameter(params, "filter"); + } + + EventsSubscriber subscriber (std::move(eventTypes), std::move(filter), l_ApiQuery); response.result(http::status::ok); response.set(http::field::content_type, "application/json"); diff --git a/lib/remote/filterutility.cpp b/lib/remote/filterutility.cpp index ac04b2395..ba342d257 100644 --- a/lib/remote/filterutility.cpp +++ b/lib/remote/filterutility.cpp @@ -2,6 +2,7 @@ // SPDX-License-Identifier: GPL-2.0-or-later #include "remote/filterutility.hpp" +#include "remote/apilistener.hpp" #include "remote/httputility.hpp" #include "config/applyrule.hpp" #include "config/configcompiler.hpp" @@ -300,6 +301,22 @@ bool FilterUtility::HasPermission(const ApiUser::Ptr& user, const String& permis } } + // Requiring the "filter-expression" permission to use any filter expression is an incompatible change. Therefore, + // there is a config option to configure whether that permission check is enforced or not. If it is not enforced, + // a message is logged, allowing the admin to determine which ApiUsers require this permission. This allows granting + // the permission and/or adapting clients as needed until this message does not show up anymore and then switching + // to enforcing the permission check. + if (!foundPermission && permission == ApiUser::FilterExpressionPerm) { + ApiListener::Ptr listener = ApiListener::GetInstance(); + if (listener && !listener->GetEnforceFilterExpressionPermission()) { + Log(LogWarning, "FilterUtility") << "ApiUser '" << user->GetName() + << "' was allowed to use a filter expression despite missing the '" << ApiUser::FilterExpressionPerm + << "' permission due to ApiListener.enforce_filter_expression_permission = false."; + + return true; + } + } + if (!foundPermission) { Log(LogWarning, "FilterUtility") << "Missing permission: " << requiredPermission; @@ -311,7 +328,7 @@ bool FilterUtility::HasPermission(const ApiUser::Ptr& user, const String& permis void FilterUtility::CheckPermission(const ApiUser::Ptr& user, const String& permission, std::unique_ptr* permissionFilter) { if (!HasPermission(user, permission, permissionFilter)) { - BOOST_THROW_EXCEPTION(ScriptError("Missing permission: " + permission.ToLower())); + BOOST_THROW_EXCEPTION(MissingPermissionError("Missing permission: " + permission.ToLower())); } } @@ -386,6 +403,7 @@ std::vector FilterUtility::GetFilterTargets(const QueryDescription& qd, c frame.PermChecker = permissionChecker; if (query->Contains("filter")) { + CheckPermission(user, ApiUser::FilterExpressionPerm, nullptr); String filter = HttpUtility::GetLastParameter(query, "filter"); std::unique_ptr ufilter = ConfigCompiler::CompileText("", filter); Dictionary::Ptr filter_vars = query->Get("filter_vars"); diff --git a/lib/remote/filterutility.hpp b/lib/remote/filterutility.hpp index 0f3a1b257..d952df6ee 100644 --- a/lib/remote/filterutility.hpp +++ b/lib/remote/filterutility.hpp @@ -62,6 +62,19 @@ public: const Object::Ptr& target, const String& variableName = String()); }; +/** + * Exception to report a missing permission to an API user. + * + * IMPORTANT: The what() message is reported back to the user and MUST NOT contain sensitive information like names of + * objects they are not allowed to access. When using the exception, also pay attention that throwing the exception + * does not introduce a sidechannel. For example, it should not be returned if a user-specified object exists but the + * user is not allowed to access it, otherwise they would learn that the object exists. + */ +class MissingPermissionError : public ScriptError +{ + using ScriptError::ScriptError; +}; + } #endif /* FILTERUTILITY_H */ diff --git a/lib/remote/jsonrpc.cpp b/lib/remote/jsonrpc.cpp index f1d4f8421..1626f89f8 100644 --- a/lib/remote/jsonrpc.cpp +++ b/lib/remote/jsonrpc.cpp @@ -147,7 +147,9 @@ String JsonRpc::ReadMessage(const Shared::Ptr& stream, boost::asi */ Dictionary::Ptr JsonRpc::DecodeMessage(const String& message) { - Value value = JsonDecode(message); + // Use something a bit higher than the default limit to accommodate for data that was accepted by Icinga 2 elsewhere + // and gained some additional nesting levels when being wrapped in a JSON-RPC message. + Value value = JsonDecode(message, JsonDecodeDefaultDepthLimit + 8); if (!value.IsObjectType()) { BOOST_THROW_EXCEPTION(std::invalid_argument("JSON-RPC" diff --git a/lib/remote/jsonrpcconnection-pki.cpp b/lib/remote/jsonrpcconnection-pki.cpp index 102781f4a..43de54294 100644 --- a/lib/remote/jsonrpcconnection-pki.cpp +++ b/lib/remote/jsonrpcconnection-pki.cpp @@ -344,7 +344,7 @@ void JsonRpcConnection::SendCertificateRequest(const JsonRpcConnection::Ptr& acl Value UpdateCertificateHandler(const MessageOrigin::Ptr& origin, const Dictionary::Ptr& params) { - if (origin->FromZone && !Zone::GetLocalZone()->IsChildOf(origin->FromZone)) { + if (!origin->FromClient->GetEndpoint() || (origin->FromZone && !Zone::GetLocalZone()->IsChildOf(origin->FromZone))) { Log(LogWarning, "ClusterEvents") << "Discarding 'update certificate' message from '" << origin->FromClient->GetIdentity() << "': Invalid endpoint origin (client not allowed)."; diff --git a/lib/remote/jsonrpcconnection.cpp b/lib/remote/jsonrpcconnection.cpp index 582c5f526..3b01f83ca 100644 --- a/lib/remote/jsonrpcconnection.cpp +++ b/lib/remote/jsonrpcconnection.cpp @@ -97,7 +97,25 @@ void JsonRpcConnection::HandleIncomingMessages(boost::asio::yield_context yc) // Cache the elapsed time to acquire a CPU semaphore used to detect extremely heavy workloads. cpuBoundDuration = ch::steady_clock::now() - start; - Dictionary::Ptr message = JsonRpc::DecodeMessage(jsonString); + Dictionary::Ptr message; + try { + message = JsonRpc::DecodeMessage(jsonString); + } catch (const std::exception& ex) { + if (m_Authenticated) { + Log (LogWarning, "JsonRpcConnection") + << "Ignoring JSON-RPC message for identity '" << m_Identity + << "' that could not be parsed: " << DiagnosticInformation(ex); + + // If only the JSON message is broken but the netstring format is intact, we can continue with the + // next message as we know the message boundaries. This is done as a defensive approach so that if + // we missed a case that triggers the JSON decoding depth limit, this doesn't break the connection + // and worst case takes down the whole cluster communication with it. + continue; + } + + // Unauthenticated clients proceed to the outer error handling that terminated the connection. + throw; + } if (String method = message->Get("method"); !method.IsEmpty()) { rpcMethod = std::move(method); } diff --git a/lib/remote/modifyobjecthandler.cpp b/lib/remote/modifyobjecthandler.cpp index 5d97919dd..3c0ae41ca 100644 --- a/lib/remote/modifyobjecthandler.cpp +++ b/lib/remote/modifyobjecthandler.cpp @@ -56,6 +56,9 @@ bool ModifyObjectHandler::HandleRequest( try { objs = FilterUtility::GetFilterTargets(qd, params, user); + } catch (const MissingPermissionError& ex) { + HttpUtility::SendJsonError(response, params, 403, ex.what()); + return true; } catch (const std::exception& ex) { HttpUtility::SendJsonError(response, params, 404, "No objects found.", diff --git a/lib/remote/objectqueryhandler.cpp b/lib/remote/objectqueryhandler.cpp index 2c81c6a73..ef73515c4 100644 --- a/lib/remote/objectqueryhandler.cpp +++ b/lib/remote/objectqueryhandler.cpp @@ -178,6 +178,9 @@ bool ObjectQueryHandler::HandleRequest( try { objs = FilterUtility::GetFilterTargets(qd, params, user); + } catch (const MissingPermissionError& ex) { + HttpUtility::SendJsonError(response, params, 403, ex.what()); + return true; } catch (const std::exception& ex) { HttpUtility::SendJsonError(response, params, 404, "No objects found.", diff --git a/lib/remote/statushandler.cpp b/lib/remote/statushandler.cpp index 1b0aa1717..4202b6a87 100644 --- a/lib/remote/statushandler.cpp +++ b/lib/remote/statushandler.cpp @@ -102,6 +102,9 @@ bool StatusHandler::HandleRequest( try { objs = FilterUtility::GetFilterTargets(qd, params, user); + } catch (const MissingPermissionError& ex) { + HttpUtility::SendJsonError(response, params, 403, ex.what()); + return true; } catch (const std::exception& ex) { HttpUtility::SendJsonError(response, params, 404, "No objects found.", diff --git a/lib/remote/templatequeryhandler.cpp b/lib/remote/templatequeryhandler.cpp index 1aaba7906..0684df681 100644 --- a/lib/remote/templatequeryhandler.cpp +++ b/lib/remote/templatequeryhandler.cpp @@ -119,6 +119,9 @@ bool TemplateQueryHandler::HandleRequest( try { objs = FilterUtility::GetFilterTargets(qd, params, user, "tmpl"); + } catch (const MissingPermissionError& ex) { + HttpUtility::SendJsonError(response, params, 403, ex.what()); + return true; } catch (const std::exception& ex) { HttpUtility::SendJsonError(response, params, 404, "No templates found.", diff --git a/lib/remote/typequeryhandler.cpp b/lib/remote/typequeryhandler.cpp index 203dfbf41..9460070a2 100644 --- a/lib/remote/typequeryhandler.cpp +++ b/lib/remote/typequeryhandler.cpp @@ -81,6 +81,9 @@ bool TypeQueryHandler::HandleRequest( try { objs = FilterUtility::GetFilterTargets(qd, params, user); + } catch (const MissingPermissionError& ex) { + HttpUtility::SendJsonError(response, params, 403, ex.what()); + return true; } catch (const std::exception& ex) { HttpUtility::SendJsonError(response, params, 404, "No objects found.", diff --git a/lib/remote/variablequeryhandler.cpp b/lib/remote/variablequeryhandler.cpp index 505983c45..c5634de3b 100644 --- a/lib/remote/variablequeryhandler.cpp +++ b/lib/remote/variablequeryhandler.cpp @@ -91,6 +91,9 @@ bool VariableQueryHandler::HandleRequest( try { objs = FilterUtility::GetFilterTargets(qd, params, user, "variable"); + } catch (const MissingPermissionError& ex) { + HttpUtility::SendJsonError(response, params, 403, ex.what()); + return true; } catch (const std::exception& ex) { HttpUtility::SendJsonError(response, params, 404, "No variables found.", diff --git a/test/base-json.cpp b/test/base-json.cpp index 0f7145fb8..165c6110c 100644 --- a/test/base-json.cpp +++ b/test/base-json.cpp @@ -1,13 +1,16 @@ // SPDX-FileCopyrightText: 2012 Icinga GmbH // SPDX-License-Identifier: GPL-2.0-or-later +#include "base/convert.hpp" #include "base/dictionary.hpp" #include "base/function.hpp" #include "base/namespace.hpp" #include "base/array.hpp" #include "base/generator.hpp" +#include "base/io-engine.hpp" #include "base/objectlock.hpp" #include "base/json.hpp" +#include "test/utils.hpp" #include #include #include @@ -151,4 +154,179 @@ BOOST_AUTO_TEST_CASE(invalid1) BOOST_CHECK_THROW(JsonDecode("{\"test\": \"test\""), std::exception); } +static std::string MakeNestedJsonArray(size_t depth) +{ + return std::string(depth, '[') + std::string(depth, ']'); +} + +static std::string MakeNestedJsonObject(size_t depth) +{ + std::ostringstream buf; + for (size_t i = 0; i < depth; ++i) { + buf << "{\"" << i << "\":"; + } + buf << "null"; + for (size_t i = 0; i < depth; ++i) { + buf << "}"; + } + return buf.str(); +} + +BOOST_AUTO_TEST_CASE(decode_depth_limit) +{ + auto ExpectLimitExceeded = [](std::string_view input, size_t limit, std::string_view path) { + try { + JsonDecode(std::string(input), limit); + + boost::test_tools::assertion_result result{false}; + result.message() << "Decoding '" << input << "' with limit " << limit << " did not throw an exception"; + return result; + } catch (const std::exception& ex) { + std::ostringstream expected; + expected << "JSON decoding recursion limit reached (path: " << path << ")"; + + boost::test_tools::assertion_result result{ex.what() == expected.str()}; + result.message() << "Decoding '" << input << "' with limit " << limit << ":\n" + << " got exception: " << ex.what() << "\n" + << " expected exception: " << expected.str() << "\n"; + return result; + } + }; + + // Scalars parse even with depth limit 0. + BOOST_CHECK_EQUAL(JsonDecode("42", 0), Value(42)); + BOOST_CHECK_EQUAL(JsonDecode("true", 0), Value(true)); + BOOST_CHECK_EQUAL(JsonDecode("\"test\"", 0), Value("test")); + + // Arrays and objects require at least depth limit 1. + BOOST_CHECK(ExpectLimitExceeded("[]", 0, "root")); + BOOST_CHECK(ExpectLimitExceeded("{}", 0, "root")); + BOOST_CHECK(ExpectLimitExceeded("[42]", 0, "root")); + BOOST_CHECK(ExpectLimitExceeded("{\"foo\": 23}", 0, "root")); + + // Array in array and object in object require at least depth limit 2. + BOOST_CHECK(ExpectLimitExceeded("[[]]", 1, "root[0]")); + BOOST_CHECK_NO_THROW(JsonDecode("[[]]", 2)); + BOOST_CHECK(ExpectLimitExceeded(R"({"a":{}})", 1, R"(root["a"])")); + BOOST_CHECK_NO_THROW(JsonDecode(R"({"a":{}})", 2)); + + // Mixed nesting of arrays and objects is both counted towards the same limit. + BOOST_CHECK(ExpectLimitExceeded("[{}]", 1, "root[0]")); + BOOST_CHECK_NO_THROW(JsonDecode("[{}]", 2)); + BOOST_CHECK(ExpectLimitExceeded(R"({"a":[]})", 1, R"(root["a"])")); + BOOST_CHECK_NO_THROW(JsonDecode(R"({"a":[]})", 2)); + + // Siblings are not added up for the depth check. + BOOST_CHECK_NO_THROW(JsonDecode("[[], [], [], {}, [], [], {}, {}, [], {}, {}, {}]", 2)); + BOOST_CHECK_NO_THROW(JsonDecode(R"({"a": [], "b": {}, "c": {}, "d": [], "e": [], "f": {}})", 2)); + + // Some deeper nested array. + std::string arrayWithNesting42 = MakeNestedJsonArray(42); + std::ostringstream arrayPathWithNesting41; + arrayPathWithNesting41 << "root"; + for (size_t i = 0; i < 41; ++i) { + arrayPathWithNesting41 << "[0]"; + } + BOOST_CHECK(ExpectLimitExceeded(arrayWithNesting42, 41, arrayPathWithNesting41.str())); + BOOST_CHECK_NO_THROW(JsonDecode(arrayWithNesting42, 42)); + + // Some deeper nested object. + std::string objectWithNesting42 = MakeNestedJsonObject(42); + std::ostringstream objectPathWithNesting41; + objectPathWithNesting41 << "root"; + for (size_t i = 0; i < 41; ++i) { + objectPathWithNesting41 << "[\"" << i << "\"]"; + } + BOOST_CHECK(ExpectLimitExceeded(objectWithNesting42, 41, objectPathWithNesting41.str())); + BOOST_CHECK_NO_THROW(JsonDecode(objectWithNesting42, 42)); + + // And some deeper nested mixed containers. + std::string deeperMixedNesting = R"({ + "1st-level": [ + true, + "2nd-level", + { + "dummy": 42, + "3rd-level": { + "4th-level": [ + "5th-level", + { + "6th-level": { + "7th-level": { + "8th-level": [ + "9th-level", + [ + "10th-level" + ] + ] + } + } + } + ] + } + } + ] + })"; + auto deeperPath = R"(root["1st-level"][2]["3rd-level"]["4th-level"][1]["6th-level"]["7th-level"]["8th-level"][1])"; + BOOST_CHECK(ExpectLimitExceeded(deeperMixedNesting, 9, deeperPath)); + BOOST_CHECK_NO_THROW(JsonDecode(deeperMixedNesting, 10)); +} + +/* This test case decodes JSON nested much deeper (see safetyFactor) than the default depth limit and performs some + * operations on the resulting values. This is done with its limited stack size on order to verify that the default + * limit is low enough to be safe. Note that this isn't an exact science unfortunately: other recursive operations may + * have larger stack frames and these operations aren't the only thing on a stack (could be done inside an HTTP or + * JSON-RPC connection for example). Therefor, aim for a sufficiently large safety factor. The test function may be + * executed twice, once in a coroutine (which may not have a guard page and reliably detect an overflow depending on + * the Boost version), once in a pthread thread (which may not be available everywhere). + */ +static void TestJsonStackSize() +{ + constexpr size_t safetyFactor = 10; + constexpr size_t depth = JsonDecodeDefaultDepthLimit * safetyFactor; + + Value val; + + BOOST_REQUIRE_NO_THROW(val = JsonDecode(MakeNestedJsonArray(depth), depth)); + BOOST_REQUIRE_NO_THROW(val.Clone()); + BOOST_REQUIRE_NO_THROW(Convert::ToString(val)); + BOOST_REQUIRE_NO_THROW(JsonDecode(JsonEncode(val), depth)); + + BOOST_REQUIRE_NO_THROW(val = JsonDecode(MakeNestedJsonObject(depth), depth)); + BOOST_REQUIRE_NO_THROW(val.Clone()); + BOOST_REQUIRE_NO_THROW(Convert::ToString(val)); + BOOST_REQUIRE_NO_THROW(JsonDecode(JsonEncode(val), depth)); +} + +BOOST_AUTO_TEST_CASE(stack_size_coroutine) +{ + auto future = SpawnSynchronizedCoroutine([](boost::asio::yield_context) { + TestJsonStackSize(); + }); + future.get(); +} + +#ifdef HAVE_PTHREAD_CREATE +BOOST_AUTO_TEST_CASE(stack_size_pthread) +{ + auto pagesize = sysconf(_SC_PAGESIZE); + BOOST_REQUIRE_GT(pagesize, 0); + + pthread_attr_t attr; + BOOST_REQUIRE_EQUAL(0, pthread_attr_init(&attr)); + BOOST_REQUIRE_EQUAL(0, pthread_attr_setstacksize(&attr, IoEngine::GetCoroutineStackSize())); + BOOST_REQUIRE_EQUAL(0, pthread_attr_setguardsize(&attr, pagesize)); + + pthread_t thread; + BOOST_REQUIRE_EQUAL(0, pthread_create(&thread, &attr, [](void*) -> void* { + TestJsonStackSize(); + return nullptr; + }, nullptr)); + + BOOST_REQUIRE_EQUAL(0, pthread_join(thread, nullptr)); + BOOST_REQUIRE_EQUAL(0, pthread_attr_destroy(&attr)); + +} +#endif /* HAVE_PTHREAD_CREATE */ + BOOST_AUTO_TEST_SUITE_END() diff --git a/test/remote-filterutility.cpp b/test/remote-filterutility.cpp index aa5441d95..be0634344 100644 --- a/test/remote-filterutility.cpp +++ b/test/remote-filterutility.cpp @@ -15,6 +15,60 @@ BOOST_AUTO_TEST_SUITE(remote_filterutility, *boost::unit_test::label("config")) // clang-format on +BOOST_FIXTURE_TEST_CASE(filter_expression_permission, IcingaApplicationFixture) +{ + auto createObjects = []() { + String config = R"CONFIG({ +object CheckCommand "dummy" { + command = "/bin/echo" +} + +object ApiUser "withFilterPermission" { + permissions = [ "objects/query/*", "filter-expression" ] +} + +object ApiUser "withoutFilterPermission" { + permissions = [ "objects/query/*" ] +} + +object Host "host1" { + address = "host1" + check_command = "dummy" +} +})CONFIG"; + std::unique_ptr expr = ConfigCompiler::CompileText("", config); + expr->Evaluate(*ScriptFrame::GetCurrentFrame()); + }; + + ConfigItem::RunWithActivationContext(new Function("CreateTestObjects", createObjects)); + + auto userWithPerm = ApiUser::GetByName("withFilterPermission"); + auto userWithoutPerm = ApiUser::GetByName("withoutFilterPermission"); + + QueryDescription qd; + qd.Types.insert("Host"); + qd.Permission = "objects/query/Host"; + + Dictionary::Ptr queryParams = new Dictionary(); + queryParams->Set("type", "Host"); + + // This is a filter that uses a get_object call on an object the permissionFilterUser + // has access to. A second user is tested that has access to everything, to make sure + // the filter evaluates properly in the first place. + queryParams->Set("filter", "true"); + + std::vector objs; + BOOST_REQUIRE_NO_THROW(objs = FilterUtility::GetFilterTargets(qd, queryParams, userWithPerm)); + BOOST_CHECK_EQUAL(objs.size(), 1); + + BOOST_CHECK_EXCEPTION(FilterUtility::GetFilterTargets(qd, queryParams, userWithoutPerm), std::exception, + [](const std::exception& ex) { + boost::test_tools::assertion_result result{std::string_view(ex.what()) == "Missing permission: filter-expression"}; + result.message() << "got exception: " << ex.what(); + return result; + }); +} + BOOST_FIXTURE_TEST_CASE(safe_function_permissions, IcingaApplicationFixture) { auto createObjects = []() { @@ -29,6 +83,7 @@ object ApiUser "allPermissionsUser" { object ApiUser "permissionFilterUser" { permissions = [ + "filter-expression", { permission = "objects/query/Host" filter = {{ host.name == {{{host1}}} }} @@ -161,6 +216,7 @@ object ApiUser "allPermissionsUser" { object ApiUser "permissionFilterUser" { permissions = [ + "filter-expression", "objects/query/Host", { permission = "variables" @@ -170,7 +226,10 @@ object ApiUser "permissionFilterUser" { } object ApiUser "noVariablePermUser" { - permissions = [ "objects/query/Host" ] + permissions = [ + "filter-expression", + "objects/query/Host", + ] } object Host "host1" { diff --git a/test/remote-httpmessage.cpp b/test/remote-httpmessage.cpp index 23e3e0823..311a45287 100644 --- a/test/remote-httpmessage.cpp +++ b/test/remote-httpmessage.cpp @@ -8,29 +8,13 @@ #include "remote/httputility.hpp" #include "test/base-tlsstream-fixture.hpp" #include "test/test-ctest.hpp" +#include "test/utils.hpp" #include #include using namespace icinga; using namespace boost::beast; -static std::future SpawnSynchronizedCoroutine(std::function fn) -{ - auto promise = std::make_unique>(); - auto future = promise->get_future(); - auto& io = IoEngine::Get().GetIoContext(); - IoEngine::SpawnCoroutine(io, [promise = std::move(promise), fn = std::move(fn)](boost::asio::yield_context yc) { - try { - fn(std::move(yc)); - } catch (const std::exception&) { - promise->set_exception(std::current_exception()); - return; - } - promise->set_value(); - }); - return future; -} - BOOST_FIXTURE_TEST_SUITE(remote_httpmessage, TlsStreamFixture, *RequiresCertificate(TlsStreamFixture::RequiredCerts) *boost::unit_test::label("network") diff --git a/test/utils.cpp b/test/utils.cpp index a0aba80d0..d2ac20ad3 100644 --- a/test/utils.cpp +++ b/test/utils.cpp @@ -2,9 +2,12 @@ // SPDX-License-Identifier: GPL-2.0-or-later #include "utils.hpp" +#include "base/io-engine.hpp" #include +#include #include #include +#include #include tm make_tm(std::string s) @@ -66,3 +69,22 @@ GlobalTimezoneFixture::~GlobalTimezoneFixture() #endif tzset(); } + +std::future SpawnSynchronizedCoroutine(std::function fn) +{ + using namespace icinga; + + auto promise = std::make_unique>(); + auto future = promise->get_future(); + auto& io = IoEngine::Get().GetIoContext(); + IoEngine::SpawnCoroutine(io, [promise = std::move(promise), fn = std::move(fn)](boost::asio::yield_context yc) { + try { + fn(std::move(yc)); + } catch (const std::exception&) { + promise->set_exception(std::current_exception()); + return; + } + promise->set_value(); + }); + return future; +} diff --git a/test/utils.hpp b/test/utils.hpp index 67d2575a2..6de3a3ad7 100644 --- a/test/utils.hpp +++ b/test/utils.hpp @@ -4,7 +4,10 @@ #pragma once #include +#include +#include #include +#include tm make_tm(std::string s); @@ -24,3 +27,5 @@ struct GlobalTimezoneFixture char *tz; }; + +std::future SpawnSynchronizedCoroutine(std::function fn);