From aa06a78ffee805ac1892b866f6d74d04d68090eb Mon Sep 17 00:00:00 2001 From: Bartosz Wieczorek Date: Wed, 11 Oct 2023 20:55:29 +0200 Subject: [PATCH] Fix user messages --- CMakeLists.txt | 5 +---- PAM/ssh/include/rublon/configuration.hpp | 18 +++++++---------- PAM/ssh/include/rublon/core_handler.hpp | 20 ++++++------------- PAM/ssh/include/rublon/method/OTP.hpp | 2 +- PAM/ssh/include/rublon/method/SMS.hpp | 2 +- .../rublon/method/passcode_based_auth.hpp | 5 ----- PAM/ssh/include/rublon/utils.hpp | 11 +++++++--- PAM/ssh/lib/pam.cpp | 5 +++-- 8 files changed, 27 insertions(+), 41 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 38648ae..e71b759 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -14,10 +14,7 @@ set(CMAKE_CXX_EXTENSIONS NO) add_compile_options(-Wall -Wextra -Wpedantic -Wno-format-security) -#add_compile_options(-fsanitize=address -fsanitize=undefined -fno-sanitize-recover=all -fsanitize=float-divide-by-zero -fsanitize=float-cast-overflow -fno-sanitize=null -fno-sanitize=alignment -fno-omit-frame-pointer) -#add_link_options(-fsanitize=address -fsanitize=undefined -fno-sanitize-recover=all -fsanitize=float-divide-by-zero -fsanitize=float-cast-overflow -fno-sanitize=null -fno-sanitize=alignment -fno-omit-frame-pointer) - -option(ENABLE_TESTS "Enable tests" OFF) +option(ENABLE_TESTS "Enable tests" ON) if (${ENABLE_TESTS}) enable_testing() diff --git a/PAM/ssh/include/rublon/configuration.hpp b/PAM/ssh/include/rublon/configuration.hpp index 2769bac..569086e 100644 --- a/PAM/ssh/include/rublon/configuration.hpp +++ b/PAM/ssh/include/rublon/configuration.hpp @@ -104,24 +104,20 @@ struct Entry { bool read(Configuration::Parameters * params, std::optional userInput) const{ constexpr const auto emptyString = ""; const auto logStored = [&](const auto & source) -> tl::expected< Source, ConfigurationError > { - if(source == Source::DefaultValue) { - rublon::log(LogLevel::Debug, "Parameter %s was set to {%s}, using default value", this->name, this->defaultValue); - } else { - rublon::log(LogLevel::Debug, "Parameter %s was set to {%s}", this->name, userInput.value().data()); - } + rublon::log(LogLevel::Debug, + "Configuration parameter '%s' was set to '%s'%s", + this->name, + this->defaultValue, + source == Source::DefaultValue ? " (default)" : ""); return source; }; const auto logError = [&](const auto & error) -> tl::expected< Source, ConfigurationError > { - rublon::log(LogLevel::Error, "Parameter %s is unavailable, aborting", this->name); + rublon::log(LogLevel::Error, "Configuration parameter '%s' is has no default value and is not provided in user configuraion, aborting", this->name); return tl::unexpected{error}; }; - return _read(this, params, userInput.value_or(emptyString)) - .and_then(logStored) - .or_else(logError) - .map([](const auto &) { return true; }) - .map_error([](const auto &) { return false; }).value(); + return _read(this, params, userInput.value_or(emptyString)).and_then(logStored).or_else(logError).has_value(); } }; diff --git a/PAM/ssh/include/rublon/core_handler.hpp b/PAM/ssh/include/rublon/core_handler.hpp index 6c67c71..a42bc44 100644 --- a/PAM/ssh/include/rublon/core_handler.hpp +++ b/PAM/ssh/include/rublon/core_handler.hpp @@ -19,7 +19,6 @@ class CoreHandler : public CoreHandlerInterface< CoreHandler< HttpHandler > > { std::string secretKey; std::string url; bool bypass; - mutable RapidJSONPMRAlloc alloc{std::pmr::get_default_resource()}; void signRequest(Request & request) const { request.headers["X-Rublon-Signature"] = std::pmr::string{signData(request.body, secretKey).data(), request.headers.get_allocator()}; @@ -49,9 +48,8 @@ class CoreHandler : public CoreHandlerInterface< CoreHandler< HttpHandler > > { return response; } - - tl::expected validateResponse(RapidJSONPMRAlloc &alloc, const Response & response) const { - log(LogLevel::Debug, "TRACE %s::%s:%d", "CoreHandler", "validateResponse", __LINE__); + + tl::expected< Document, Error > validateResponse(RapidJSONPMRAlloc & alloc, const Response & response) const { Document resp{&alloc}; resp.Parse(response.body.c_str()); @@ -70,19 +68,15 @@ class CoreHandler : public CoreHandlerInterface< CoreHandler< HttpHandler > > { } tl::unexpected< Error > handleCoreException(std::string_view exceptionString) const { - log(LogLevel::Debug, "TRACE %s::%s:%d", "CoreHandler", "handleCoreException", __LINE__); if(exceptionString == "UserBypassedException" or exceptionString == "UserNotFoundException") { - log(LogLevel::Debug, "TRACE %s::%s:%d", "CoreHandler", "handleCoreException", __LINE__); return tl::unexpected{Error{PamBaypass{}}}; } else { - log(LogLevel::Debug, "TRACE %s::%s:%d", "CoreHandler", "handleCoreException", __LINE__); return tl::unexpected{ Error{CoreHandlerError{CoreHandlerError::CoreException, std::string{exceptionString.data(), exceptionString.size()}}}}; } } tl::unexpected< Error > handleHttpError() const { - log(LogLevel::Debug, "TRACE %s::%s:%d", "CoreHandler", "handleHttpError", __LINE__); if(bypass) { log(LogLevel::Warning, "User login bypass"); return tl::unexpected{Error{PamBaypass{}}}; @@ -93,7 +87,6 @@ class CoreHandler : public CoreHandlerInterface< CoreHandler< HttpHandler > > { } tl::expected< Document, Error > handleError(const Error & error) const { - log(LogLevel::Debug, "TRACE %s::%s:%d", "CoreHandler", "handleError", __LINE__); if(error.is< HttpError >() and error.hasClass(HttpError::Error)) { return handleHttpError(); } @@ -110,14 +103,13 @@ class CoreHandler : public CoreHandlerInterface< CoreHandler< HttpHandler > > { body.Accept(writer); to = jsonStr.GetString(); } - - tl::expected< Document, Error > request(RapidJSONPMRAlloc &mr, std::string_view path, const Document & body) const { - log(LogLevel::Debug, "TRACE %s::%s:%d", "CoreHandler", "validateResponse", __LINE__); + + tl::expected< Document, Error > request(RapidJSONPMRAlloc & mr, std::string_view path, const Document & body) const { memory::StrictMonotonic_4k_HeapResource memoryResource; - const auto validateSignature = [this](const auto & arg) { return this->validateSignature(arg); }; + const auto validateSignature = [&](const auto & arg) { return this->validateSignature(arg); }; const auto validateResponse = [&](const auto & arg) { return this->validateResponse(mr, arg); }; - const auto handleError = [this](const auto & error) { return this->handleError(error); }; + const auto handleError = [&](const auto & error) { return this->handleError(error); }; const auto pmrs = [&](auto txt) { return std::pmr::string{txt, &memoryResource}; }; Request request{&memoryResource}; diff --git a/PAM/ssh/include/rublon/method/OTP.hpp b/PAM/ssh/include/rublon/method/OTP.hpp index f916fd1..5e8322d 100644 --- a/PAM/ssh/include/rublon/method/OTP.hpp +++ b/PAM/ssh/include/rublon/method/OTP.hpp @@ -13,7 +13,7 @@ namespace rublon::method { class OTP : public PasscodeBasedAuth { public: OTP(std::string systemToken, std::string tid) - : PasscodeBasedAuth(std::move(systemToken), std::move(tid), "OTP", "Mobile TOTP from Rublon Authenticator:") {} + : PasscodeBasedAuth(std::move(systemToken), std::move(tid), "OTP", "Enter code from Rublon Authenticator: ") {} }; } // namespace rublon::method diff --git a/PAM/ssh/include/rublon/method/SMS.hpp b/PAM/ssh/include/rublon/method/SMS.hpp index 9233765..501dcd3 100644 --- a/PAM/ssh/include/rublon/method/SMS.hpp +++ b/PAM/ssh/include/rublon/method/SMS.hpp @@ -12,7 +12,7 @@ namespace rublon::method { class SMS : public PasscodeBasedAuth { public: - SMS(std::string systemToken, std::string tid) : PasscodeBasedAuth(std::move(systemToken), std::move(tid), "SMS", "SMS passcode:") {} + SMS(std::string systemToken, std::string tid) : PasscodeBasedAuth(std::move(systemToken), std::move(tid), "SMS", "Enter SMS passcode: ") {} }; } // namespace rublon::method diff --git a/PAM/ssh/include/rublon/method/passcode_based_auth.hpp b/PAM/ssh/include/rublon/method/passcode_based_auth.hpp index e4b58c1..12ee6e5 100644 --- a/PAM/ssh/include/rublon/method/passcode_based_auth.hpp +++ b/PAM/ssh/include/rublon/method/passcode_based_auth.hpp @@ -27,12 +27,10 @@ class PasscodeBasedAuth : public AuthenticationStep< PasscodeBasedAuth > { template < typename PamInfo_t = LinuxPam > tl::expected< std::reference_wrapper< Document >, Error > readPasscode(Document & body, const PamInfo_t & pam) const { - log(LogLevel::Debug, "TRACE %s::%s:%d", "PasscodeBasedAuth", "readPasscode", __LINE__); auto & alloc = body.GetAllocator(); auto vericode = pam.scan([](const char * userInput) { return std::string{userInput}; }, userMessage); if(isProperLength(vericode) and hasDigitsOnly(vericode)) { - log(LogLevel::Debug, "TRACE %s::%s:%d", "PasscodeBasedAuth", "readPasscode", __LINE__); body.AddMember("vericode", Value{vericode.c_str(), alloc}, alloc); return body; } @@ -42,14 +40,12 @@ class PasscodeBasedAuth : public AuthenticationStep< PasscodeBasedAuth > { template < typename PamInfo_t = LinuxPam > tl::expected< std::reference_wrapper< Document >, Error > askForPasscodeAgain(Document & body, const PamInfo_t & pam) const { - log(LogLevel::Debug, "TRACE %s::%s:%d", "PasscodeBasedAuth", "askForPasscodeAgain", __LINE__); pam.print("passcode has wrong number of digits or illegal characters, please correct"); return readPasscode(body, pam); } template < typename PamInfo_t = LinuxPam > tl::expected< AuthenticationStatus, Error > checkAuthenticationStatus(const Document & coreResponse, const PamInfo_t & pam) const { - log(LogLevel::Debug, "TRACE %s::%s:%d", "PasscodeBasedAuth", "checkAuthenticationStatus", __LINE__); RapidJSONPMRStackAlloc< 1024 > alloc; auto error = JSONPointer{"/result/error", &alloc}.Get(coreResponse); @@ -70,7 +66,6 @@ class PasscodeBasedAuth : public AuthenticationStep< PasscodeBasedAuth > { template < typename Hander_t, typename PamInfo_t = LinuxPam > tl::expected< AuthenticationStatus, Error > handle(const CoreHandlerInterface< Hander_t > & coreHandler, const PamInfo_t & pam) const { - log(LogLevel::Debug, "TRACE %s::%s:%d", "PasscodeBasedAuth", "handle", __LINE__); RapidJSONPMRStackAlloc< 2048 > alloc{}; Document body{rapidjson::kObjectType, &alloc}; diff --git a/PAM/ssh/include/rublon/utils.hpp b/PAM/ssh/include/rublon/utils.hpp index 9d5c9ae..02e760e 100644 --- a/PAM/ssh/include/rublon/utils.hpp +++ b/PAM/ssh/include/rublon/utils.hpp @@ -106,10 +106,15 @@ constexpr LogLevel g_level = LogLevel::Debug; constexpr bool syncLogFile = true; namespace details { + + constexpr const char* logPath(){ + constexpr auto path = "/var/log/rublon-ssh.log"; + return path; + } + static void doLog(LogLevel level, const char * line) noexcept { - constexpr auto file_name = "/var/log/rublon-ssh.log"; - - auto fp = std::unique_ptr< FILE, int (*)(FILE *) >(fopen(file_name, "a"), fclose); + + static auto fp = std::unique_ptr< FILE, int (*)(FILE *) >(fopen(logPath(), "a"), fclose); if(fp) { fprintf(fp.get(), "%s [%s] %s\n", dateStr().data(), LogLevelNames[( int ) level], line); diff --git a/PAM/ssh/lib/pam.cpp b/PAM/ssh/lib/pam.cpp index 419fa5a..05416ab 100644 --- a/PAM/ssh/lib/pam.cpp +++ b/PAM/ssh/lib/pam.cpp @@ -36,8 +36,9 @@ pam_sm_authenticate(pam_handle_t * pamh, [[maybe_unused]] int flags, [[maybe_unu auto rublonConfig = ConfigurationFactory{}.systemConfig(); if(not rublonConfig.has_value()) { - pam.print("Rublon configuration not exists or is invalid (check logs)"); - pam.print("Please create /etc/rublon.config file"); + pam.print("\n"); + pam.print("Rublon configuration does not exists or is invalid"); + pam.print("\tcheck '%s' for more details\n", details::logPath()); return PAM_SUCCESS; }