From b2e48d73b0044ec317fbcb90eb1ced2c4058265d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Magnus=20Gro=C3=9F?= Date: Wed, 8 Jul 2026 11:37:42 +0200 Subject: [PATCH 1/2] OAuth: Call redirect with httplib MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A missing trailing slash caused opencmw::URI to return no path. The best solution would be to add a fallback "/" root path to opencmw::URI, in case no path is present. However, this would trigger the assert in URI::returnOpt that checks if the returned string is part of the original provided string. Therefore we opt for this less invasive change. Signed-off-by: Magnus Groß --- .github/workflows/build_cmake.yml | 6 +++--- src/client/test/setup-keycloak.sh | 2 +- src/services/test/OAuthClient_tests.cpp | 11 +---------- 3 files changed, 5 insertions(+), 14 deletions(-) diff --git a/.github/workflows/build_cmake.yml b/.github/workflows/build_cmake.yml index 8859dc9f..bac2542e 100644 --- a/.github/workflows/build_cmake.yml +++ b/.github/workflows/build_cmake.yml @@ -96,7 +96,7 @@ jobs: shell: bash env: KEYCLOAK_URL: http://keycloak:8080 - KEYCLOAK_REDIRECT_URI: http://localhost:8091 + KEYCLOAK_REDIRECT_URI: http://localhost:8091/ KEYCLOAK_ADMIN_PASSWORD: admin run: | if ! command -v jq >/dev/null 2>&1; then @@ -111,7 +111,7 @@ jobs: shell: bash env: KEYCLOAK_URL: http://keycloak:8080 - KEYCLOAK_REDIRECT_URI: http://localhost:8091 + KEYCLOAK_REDIRECT_URI: http://localhost:8091/ # Execute tests defined by the CMake configuration. The coverage target runs the autodiscovered catch2 tests using # ctest and records the coverage using gcov # See https://cmake.org/cmake/help/latest/manual/ctest.1.html for more detail @@ -123,7 +123,7 @@ jobs: shell: bash env: KEYCLOAK_URL: http://keycloak:8080 - KEYCLOAK_REDIRECT_URI: http://localhost:8091 + KEYCLOAK_REDIRECT_URI: http://localhost:8091/ # Execute tests defined by the CMake configuration. The coverage target runs the autodiscovered catch2 tests using # ctest and records the coverage using gcov # See https://cmake.org/cmake/help/latest/manual/ctest.1.html for more detail diff --git a/src/client/test/setup-keycloak.sh b/src/client/test/setup-keycloak.sh index 877dabc1..f111b01e 100755 --- a/src/client/test/setup-keycloak.sh +++ b/src/client/test/setup-keycloak.sh @@ -7,7 +7,7 @@ # For more information see: https://www.keycloak.org/getting-started/getting-started-docker URL="${KEYCLOAK_URL:-http://localhost:8090}" -REDIRECT_URI="${KEYCLOAK_REDIRECT_URI:-http://localhost:8091}" +REDIRECT_URI="${KEYCLOAK_REDIRECT_URI:-http://localhost:8091/}" ADMIN_PASSWORD="${KEYCLOAK_ADMIN_PASSWORD:-admin}" while ! curl -s "$URL" >/dev/null; do diff --git a/src/services/test/OAuthClient_tests.cpp b/src/services/test/OAuthClient_tests.cpp index 569825de..18b80fd2 100644 --- a/src/services/test/OAuthClient_tests.cpp +++ b/src/services/test/OAuthClient_tests.cpp @@ -72,17 +72,8 @@ bool loginAtUri(const opencmw::URI &uri) { auto redirectClient = opencmw::OAuthClient::getClient(redirecUri); relativeRef = redirecUri.relativeRef(); REQUIRE(relativeRef); -#if 0 - // TODO: Why does this crash? - // In theory we should be able to use httplib to call the redirect URI res = redirectClient->Get(*relativeRef); REQUIRE(res); -#else - // call the final redirect URI with curl as a workaround - const std::string curl = "curl '" + it->second + "'"; - // flawfinder: ignore - res->status = std::system(curl.c_str()) + 200; -#endif REQUIRE(res->status == 200); return true; @@ -100,7 +91,7 @@ TEST_CASE("Worker test", "[OAuth]") { }; const std::string kcBase = envOr("KEYCLOAK_URL", "http://localhost:8090"); - const std::string redirectBase = envOr("KEYCLOAK_REDIRECT_URI", "http://localhost:8091"); + const std::string redirectBase = envOr("KEYCLOAK_REDIRECT_URI", "http://localhost:8091/"); opencmw::OAuthWorker oauthWorker{ opencmw::URI(redirectBase), From 73a842cc4d275454ae5013f91cc49cd9eda0401e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Magnus=20Gro=C3=9F?= Date: Wed, 8 Jul 2026 11:37:42 +0200 Subject: [PATCH 2/2] Add RBAC message discarding filters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Magnus Groß --- src/client/include/Client.hpp | 14 +- src/majordomo/include/majordomo/Broker.hpp | 9 ++ src/majordomo/include/majordomo/Worker.hpp | 11 ++ src/majordomo/test/rbac_tests.cpp | 141 ++++++++++++++++++ src/services/include/services/KeyStore.hpp | 8 + src/services/include/services/OAuthClient.hpp | 3 + src/services/test/OAuthClient_tests.cpp | 4 + 7 files changed, 187 insertions(+), 3 deletions(-) diff --git a/src/client/include/Client.hpp b/src/client/include/Client.hpp index 87e383aa..69b829eb 100644 --- a/src/client/include/Client.hpp +++ b/src/client/include/Client.hpp @@ -244,7 +244,7 @@ class SubscriptionClient : public MDClientBase { } void set(const URI & /*uri*/, std::string_view, const std::span & /*request*/) override { - throw std::logic_error("get not implemented"); + throw std::logic_error("set not implemented"); } void subscribe(const URI &uri, std::string_view /*reqId*/) override { @@ -399,11 +399,11 @@ class MDClientCtx : public ClientBase { _requests.erase(0); // todo: lookup correct subscription } } - sendCmd(cmd.topic, cmd.command, req_id, cmd.data); + sendCmd(cmd.topic, cmd.command, req_id, cmd.data, cmd.rbac); } private: - void sendCmd(const URI &uri, mdp::Command commandType, std::size_t req_id, IoBuffer data = {}) const { + void sendCmd(const URI &uri, mdp::Command commandType, std::size_t req_id, IoBuffer data = {}, IoBuffer rbac = {}) const { const bool isSet = commandType == mdp::Command::Set; zmq::MessageFrame cmdType{ std::string{ static_cast(commandType) } }; cmdType.send(_control_socket_send, ZMQ_SNDMORE).assertSuccess(); @@ -411,6 +411,8 @@ class MDClientCtx : public ClientBase { reqId.send(_control_socket_send, ZMQ_SNDMORE).assertSuccess(); zmq::MessageFrame endpoint{ std::string(uri.str()) }; endpoint.send(_control_socket_send, isSet ? ZMQ_SNDMORE : 0).assertSuccess(); + zmq::MessageFrame rbacframe{ std::move(rbac) }; + rbacframe.send(_control_socket_send, 0).assertSuccess(); if (isSet) { zmq::MessageFrame dataframe{ std::move(data) }; dataframe.send(_control_socket_send, 0).assertSuccess(); @@ -421,6 +423,7 @@ class MDClientCtx : public ClientBase { zmq::MessageFrame cmd; zmq::MessageFrame reqId; zmq::MessageFrame endpoint; + zmq::MessageFrame rbac; while (cmd.receive(_control_socket_recv, ZMQ_DONTWAIT).isValid()) { if (!reqId.receive(_control_socket_recv, ZMQ_DONTWAIT).isValid()) { throw std::logic_error("invalid request received: failure receiving message"); @@ -428,6 +431,11 @@ class MDClientCtx : public ClientBase { if (!endpoint.receive(_control_socket_recv, ZMQ_DONTWAIT).isValid()) { throw std::logic_error("invalid request received: invalid message contents"); } + if (!rbac.receive(_control_socket_recv, ZMQ_DONTWAIT).isValid()) { + throw std::logic_error("invalid request received: invalid rbac data"); + } + auto rbacdata = as_bytes(std::span(rbac.data().data(), rbac.data().size())); + IoBuffer rbacData{ reinterpret_cast(rbacdata.data()), rbacdata.size() }; URI uri{ std::string(endpoint.data()) }; auto &client = getClient(uri); if (cmd.data().size() != 1) { diff --git a/src/majordomo/include/majordomo/Broker.hpp b/src/majordomo/include/majordomo/Broker.hpp index c0c9e73c..6a8d8f13 100644 --- a/src/majordomo/include/majordomo/Broker.hpp +++ b/src/majordomo/include/majordomo/Broker.hpp @@ -280,6 +280,7 @@ class Broker { bool _connectedToDns = false; const IoBuffer _rbac = IoBuffer("RBAC=ADMIN,abcdef12345"); + std::function _rbacHandler; std::atomic _shutdownRequested = false; @@ -422,6 +423,10 @@ class Broker { _subscriptionMatcher.addFilter(key); } + void setRbacHandler(decltype(_rbacHandler) handler) { + _rbacHandler = std::move(handler); + } + /** * Bind broker to endpoint, can call this multiple times. We use a single * socket for both clients and workers. @@ -855,6 +860,10 @@ class Broker { auto clientMessage = std::move(client.requests.front()); client.requests.pop_front(); + if (_rbacHandler) { + _rbacHandler(clientMessage); + } + if (auto service = bestMatchingService(clientMessage.serviceName)) { if (service->internalHandler) { auto reply = service->internalHandler(std::move(clientMessage)); diff --git a/src/majordomo/include/majordomo/Worker.hpp b/src/majordomo/include/majordomo/Worker.hpp index 8a7e757c..650a2379 100644 --- a/src/majordomo/include/majordomo/Worker.hpp +++ b/src/majordomo/include/majordomo/Worker.hpp @@ -98,6 +98,7 @@ class BasicWorker { std::shared_mutex _notificationHandlersLock; const std::string _notifyAddress; const IoBuffer _defaultRbacToken = IoBuffer("RBAC=NONE,"); + std::function _rbacPredicate; public: static constexpr std::string_view name = serviceName.data(); @@ -184,6 +185,10 @@ class BasicWorker { disconnect(); } + void setRbacPredicate(decltype(_rbacPredicate) rbacPredicate) { + _rbacPredicate = std::move(rbacPredicate); + } + protected: void setHandler(std::function handler) { _handler = std::move(handler); @@ -352,6 +357,12 @@ class BasicWorker { const auto clientRole = parse_rbac::role(request.rbac.asString()); const auto permission = _permissionsByRole.at(clientRole, _defaultPermission); + if (_rbacPredicate && !_rbacPredicate(request.rbac)) { + auto errorReply = replyFromRequest(request); + errorReply.error = "request rejected by RBAC handler"; + return errorReply; + } + if (request.command == mdp::Command::Get && !(permission == Permission::RW || permission == Permission::RO)) { auto errorReply = replyFromRequest(request); errorReply.error = std::format("GET access denied to role '{}'", clientRole); diff --git a/src/majordomo/test/rbac_tests.cpp b/src/majordomo/test/rbac_tests.cpp index bb9de1dd..f3162b80 100644 --- a/src/majordomo/test/rbac_tests.cpp +++ b/src/majordomo/test/rbac_tests.cpp @@ -1,3 +1,5 @@ +#include +#include #include #include @@ -30,3 +32,142 @@ TEST_CASE("RBAC parser tests", "[rbac][parsing]") { STATIC_REQUIRE2(parse_rbac::roleAndHash("RBAC=ADMIN,hash") == std::pair("ADMIN"sv, "hash"sv)); STATIC_REQUIRE2(parse_rbac::hash("RBAC=ADMIN,hash,invalidHash") == "hash,invalidHash"); // TODO in java, this throws, should we throw/return "", too? } + +namespace opencmw { + +struct RbacContext { + int dummy; +}; + +struct RbacInput { + int dummy; +}; + +struct RbacOutput { + int i; +}; + +} // namespace opencmw + +ENABLE_REFLECTION_FOR(opencmw::RbacContext, dummy) +ENABLE_REFLECTION_FOR(opencmw::RbacInput, dummy) +ENABLE_REFLECTION_FOR(opencmw::RbacOutput, i) + +namespace { +using RbacWorkerType = majordomo::Worker<"/rbac", opencmw::RbacContext, opencmw::RbacInput, opencmw::RbacOutput, majordomo::description<"Rbac test">>; +}; + +class RbacWorker : public RbacWorkerType { +public: + template + explicit RbacWorker(const BrokerType &broker) : RbacWorkerType(broker, {}) { init(); }; + +private: + void init() { + RbacWorkerType::setCallback([this](const majordomo::RequestContext &rawCtx, const opencmw::RbacContext &context, const opencmw::RbacInput &in, opencmw::RbacContext &replyContext, opencmw::RbacOutput &out) { + out.i = 42; + }); + }; +}; + +TEST_CASE("RBAC basic predicate tests", "[rbac][predicate]") { + opencmw::majordomo::Broker<> broker{ "/Broker", {} }; + RbacWorker worker{ broker }; + + worker.setRbacPredicate([](auto &) { return false; }); + + REQUIRE(broker.bind(opencmw::URI<>("mds://127.0.0.1:12345"))); + REQUIRE(broker.bind(opencmw::URI<>("mdp://127.0.0.1:12346"))); + + RunInThread brokerThread(broker); + RunInThread workerThread(worker); + + REQUIRE(waitUntilWorkerServiceAvailable(broker.context, worker)); + + std::vector> clients; + clients.emplace_back(std::make_unique(broker.context, 20ms, "")); + opencmw::client::ClientContext client{ std::move(clients) }; + + opencmw::IoBuffer inBuf; + opencmw::RbacInput in; + opencmw::serialise(inBuf, in); + + std::condition_variable cv; + std::mutex m; + bool done = false; + + client.set(opencmw::URI("mdp://127.0.0.1:12346/rbac"), [&](const mdp::Message &reply) { + REQUIRE(reply.error.size()); + + worker.setRbacPredicate([](auto& rb){ + return true;}); + + opencmw::IoBuffer iBuf; + opencmw::serialise(iBuf, in); + client.set(opencmw::URI("mdp://127.0.0.1:12346/rbac"), [&](const mdp::Message &reply) { + auto outBuf = reply.data; + REQUIRE(reply.error.empty()); + REQUIRE(outBuf.size()); + opencmw::RbacOutput out; + opencmw::deserialise(outBuf, out); + REQUIRE(out.i == 42); + + std::lock_guard lk(m); + done = true; + cv.notify_one(); + }, std::move(iBuf)); }, std::move(inBuf)); + + std::unique_lock lk(m); + cv.wait(lk, [&] { return done; }); +} + +TEST_CASE("RBAC predicate tests", "[rbac][predicate]") { + opencmw::majordomo::Broker<> broker{ "/Broker", {} }; + RbacWorker worker{ broker }; + + broker.setRbacHandler([](auto &msg) { msg.rbac.put("RBAC=USER,hash"); }); + worker.setRbacPredicate([](auto &buf) { return buf.asString().contains("ADMIN"); }); + + REQUIRE(broker.bind(opencmw::URI<>("mds://127.0.0.1:12345"))); + REQUIRE(broker.bind(opencmw::URI<>("mdp://127.0.0.1:12346"))); + + RunInThread brokerThread(broker); + RunInThread workerThread(worker); + + REQUIRE(waitUntilWorkerServiceAvailable(broker.context, worker)); + + std::vector> clients; + clients.emplace_back(std::make_unique(broker.context, 20ms, "")); + opencmw::client::ClientContext client{ std::move(clients) }; + + opencmw::IoBuffer inBuf; + opencmw::RbacInput in; + opencmw::serialise(inBuf, in); + + std::condition_variable cv; + std::mutex m; + bool done = false; + + client.set(opencmw::URI("mdp://127.0.0.1:12346/rbac"), [&](const mdp::Message &reply) { + REQUIRE(reply.error.size()); + + broker.setRbacHandler([](auto& msg){msg.rbac.put("RBAC=ADMIN,hash");}); + + opencmw::IoBuffer iBuf; + opencmw::serialise(iBuf, in); + client.set(opencmw::URI("mdp://127.0.0.1:12346/rbac"), [&](const mdp::Message &reply) { + auto outBuf = reply.data; + REQUIRE_MESSAGE(reply.error.empty(), reply.error); + REQUIRE(outBuf.size()); + opencmw::RbacOutput out; + opencmw::deserialise(outBuf, out); + REQUIRE(out.i == 42); + + std::lock_guard lk(m); + done = true; + cv.notify_one(); + }, std::move(iBuf)); }, std::move(inBuf)); + + std::unique_lock lk(m); + cv.wait(lk, [&] { return done; }); +} diff --git a/src/services/include/services/KeyStore.hpp b/src/services/include/services/KeyStore.hpp index 6957dbe2..7e1ca9f7 100644 --- a/src/services/include/services/KeyStore.hpp +++ b/src/services/include/services/KeyStore.hpp @@ -56,6 +56,14 @@ class KeystoreWorker : public KeystoreWorkerType { _keys[keyHash(roleKey.key)] = roleKey; } + void addRoles(const std::string &publicKey, const std::string &roles) { + const auto hash = keyHash(publicKey); + if (_keys.contains(hash)) { + auto &key = _keys.at(hash); + key.role = roles; + } + } + private: void init() { KeystoreWorkerType::setCallback([this](const majordomo::RequestContext &rawCtx, const KeystoreContext &context, const KeystoreInput &in, KeystoreContext &replyContext, KeystoreOutput &out) { diff --git a/src/services/include/services/OAuthClient.hpp b/src/services/include/services/OAuthClient.hpp index d37ebaa9..aa1136d8 100644 --- a/src/services/include/services/OAuthClient.hpp +++ b/src/services/include/services/OAuthClient.hpp @@ -324,6 +324,9 @@ class OAuthWorker : public OAuthWorkerType { // get RBAC roles out.roles = _client.getAssignedRoles(out.accessToken); + if (!in.publicKey.empty()) { + _keystore.addRoles(in.publicKey, out.roles); + } } }); _keystoreThread = std::thread([this] { _keystore.run(); }); diff --git a/src/services/test/OAuthClient_tests.cpp b/src/services/test/OAuthClient_tests.cpp index 18b80fd2..de13a172 100644 --- a/src/services/test/OAuthClient_tests.cpp +++ b/src/services/test/OAuthClient_tests.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include @@ -128,6 +129,7 @@ TEST_CASE("Worker test", "[OAuth]") { client.set(opencmw::URI("mdp://127.0.0.1:12346/oauth"), [&](const mdp::Message &reply) { opencmw::OAuthOutput out; auto outBuf = reply.data; + REQUIRE_MESSAGE(reply.error.empty(), reply.error); opencmw::deserialise(outBuf, out); REQUIRE(out.authorizationUri.size()); @@ -140,6 +142,7 @@ TEST_CASE("Worker test", "[OAuth]") { client.set(opencmw::URI("mdp://127.0.0.1:12346/oauth"), [&](const mdp::Message& rep) { opencmw::OAuthOutput tokenOut; auto tokenOutBuf = rep.data; + REQUIRE_MESSAGE(rep.error.empty(), rep.error); opencmw::deserialise(tokenOutBuf, tokenOut); // we must have got an access token now @@ -154,6 +157,7 @@ TEST_CASE("Worker test", "[OAuth]") { client.set(opencmw::URI("mdp://127.0.0.1:12346/keystore"), [&](const mdp::Message &keyResp) { opencmw::KeystoreOutput keyOut; auto keyOutBuf = keyResp.data; + REQUIRE_MESSAGE(keyResp.error.empty(), keyResp.error); opencmw::deserialise(keyOutBuf, keyOut); // the hashes should be the same