From 2fd67099526c1a1ea170ddba68a23aa591c9e770 Mon Sep 17 00:00:00 2001 From: Michael Friedrich Date: Mon, 18 Jun 2018 11:05:56 +0200 Subject: [PATCH] Remove ApiUser password_hash functionality This affects and fixes - Windows reload - Config validation - RHEL 7.5 OpenSSL memory corruption - Hash algorithm, requested changes refs #6378 refs #6279 refs #6278 --- doc/09-object-types.md | 1 - doc/11-cli-commands.md | 1 - doc/12-icinga2-api.md | 19 ------- doc/16-upgrading-icinga-2.md | 5 ++ lib/base/tlsutility.cpp | 30 ---------- lib/base/tlsutility.hpp | 2 - lib/cli/CMakeLists.txt | 1 - lib/cli/apiusercommand.cpp | 104 ----------------------------------- lib/cli/apiusercommand.hpp | 47 ---------------- lib/remote/apiuser.cpp | 38 +------------ lib/remote/apiuser.hpp | 4 -- lib/remote/apiuser.ti | 2 +- test/CMakeLists.txt | 2 - test/remote-user.cpp | 50 ----------------- 14 files changed, 8 insertions(+), 298 deletions(-) delete mode 100644 lib/cli/apiusercommand.cpp delete mode 100644 lib/cli/apiusercommand.hpp delete mode 100644 test/remote-user.cpp diff --git a/doc/09-object-types.md b/doc/09-object-types.md index 1c6436739..2d81d3ee3 100644 --- a/doc/09-object-types.md +++ b/doc/09-object-types.md @@ -112,7 +112,6 @@ Configuration Attributes: Name | Type | Description --------------------------|-----------------------|---------------------------------- password | String | **Optional.** Password string. Note: This attribute is hidden in API responses. - password\_hash | String | **Optional.** A hashed password string in the form of /etc/shadow. Note: This attribute is hidden in API responses. client\_cn | String | **Optional.** Client Common Name (CN). permissions | Array | **Required.** Array of permissions. Either as string or dictionary with the keys `permission` and `filter`. The latter must be specified as function. diff --git a/doc/11-cli-commands.md b/doc/11-cli-commands.md index d288c64f5..1b5011683 100644 --- a/doc/11-cli-commands.md +++ b/doc/11-cli-commands.md @@ -20,7 +20,6 @@ Usage: Supported commands: * api setup (setup for API) - * api user (API user creation helper) * ca list (lists all certificate signing requests) * ca sign (signs an outstanding certificate request) * console (Icinga console) diff --git a/doc/12-icinga2-api.md b/doc/12-icinga2-api.md index c029aee0e..722b88a2b 100644 --- a/doc/12-icinga2-api.md +++ b/doc/12-icinga2-api.md @@ -21,25 +21,6 @@ If you prefer to set up the API manually, you will have to perform the following The next chapter provides a quick overview of how you can use the API. -### Creating ApiUsers - -The CLI command `icinga2 api user` allows you to create an ApiUser object with a hashed password string, ready to be -added to your configuration. Example: - -``` -$ icinga2 api user --user icingaweb2 --password icinga -object ApiUser "icingaweb2" { - password_hash ="$5$d5f1a17ea308acb6$9e9fd5d24a9373a16e8811765cc5a5939687faf9ef8ed496db6e7f1d0ae9b2a9" - // client_cn = "" - - permissions = [ "*" ] -} -``` - -Optionally a salt can be provided with `--salt`, otherwise a random value will be used. When ApiUsers are stored this -way, even somebody able to read the configuration files won't be able to authenticate using this information. There is -no way to recover your password should you forget it, you'd need to create it anew. - ## Introduction The Icinga 2 API allows you to manage configuration objects diff --git a/doc/16-upgrading-icinga-2.md b/doc/16-upgrading-icinga-2.md index 2b8d486bf..cf509cf33 100644 --- a/doc/16-upgrading-icinga-2.md +++ b/doc/16-upgrading-icinga-2.md @@ -18,6 +18,11 @@ The CORS attributes `access_control_allow_credentials`, `access_control_allow_he The `node setup` parameter `--master_host` was deprecated and replaced with `--parent_host`. This parameter is now optional to allow connection-less client setups similar to the `node wizard` CLI command. The `parent_zone` parameter has been added to modify the parent zone name e.g. for client-to-satellite setups. +The `api user` command which was released in v2.8.2 turned out to cause huge problems with +configuration validation, windows restarts and OpenSSL versions. It is therefore removed in 2.9, +the `password_hash` attribute for the ApiUser object stays intact but has no effect. This is to ensure +that clients don't break on upgrade. We will revise this feature in future development iterations. + ## Upgrading to v2.8.2 With version 2.8.2 the location of settings formerly found in `/etc/icinga2/init.conf` has changed. They are now diff --git a/lib/base/tlsutility.cpp b/lib/base/tlsutility.cpp index 9b3c33fb2..c3fe07bd9 100644 --- a/lib/base/tlsutility.cpp +++ b/lib/base/tlsutility.cpp @@ -784,34 +784,4 @@ std::string to_string(const errinfo_openssl_error& e) return "[errinfo_openssl_error]" + tmp.str() + "\n"; } -bool ComparePassword(const String& hash, const String& password, const String& salt) -{ - String otherHash = PBKDF2_SHA256(password, salt, 1000); - VERIFY(otherHash.GetLength() == 64 && hash.GetLength() == 64); - - const char *p1 = otherHash.CStr(); - const char *p2 = hash.CStr(); - - /* By Novelocrat, https://stackoverflow.com/a/25374036 */ - volatile char c = 0; - - for (size_t i = 0; i < 64; ++i) - c |= p1[i] ^ p2[i]; - - return (c == 0); -} - -/* Returns a String in the format $algorithm$salt$hash or returns an empty string in case of an error */ -String CreateHashedPasswordString(const String& password, const String& salt, int algorithm) -{ - // We currently only support SHA256 - if (algorithm != 5) - return String(); - - if (salt.FindFirstOf('$') != String::NPos) - return String(); - - return String("$5$" + salt + "$" + PBKDF2_SHA256(password, salt, 1000)); -} - } diff --git a/lib/base/tlsutility.hpp b/lib/base/tlsutility.hpp index 31bb4e466..38df7e587 100644 --- a/lib/base/tlsutility.hpp +++ b/lib/base/tlsutility.hpp @@ -56,8 +56,6 @@ String SHA1(const String& s, bool binary = false); String SHA256(const String& s); String RandomString(int length); bool VerifyCertificate(const std::shared_ptr& caCertificate, const std::shared_ptr& certificate); -bool ComparePassword(const String& hash, const String& password, const String& Salt); -String CreateHashedPasswordString(const String& password, const String& salt, int algorithm = 5); class openssl_error : virtual public std::exception, virtual public boost::exception { }; diff --git a/lib/cli/CMakeLists.txt b/lib/cli/CMakeLists.txt index d92c22c45..258597dec 100644 --- a/lib/cli/CMakeLists.txt +++ b/lib/cli/CMakeLists.txt @@ -18,7 +18,6 @@ set(cli_SOURCES i2-cli.hpp apisetupcommand.cpp apisetupcommand.hpp - apiusercommand.cpp apiusercommand.hpp apisetuputility.cpp apisetuputility.hpp calistcommand.cpp calistcommand.hpp casigncommand.cpp casigncommand.hpp diff --git a/lib/cli/apiusercommand.cpp b/lib/cli/apiusercommand.cpp deleted file mode 100644 index 5bd77ab89..000000000 --- a/lib/cli/apiusercommand.cpp +++ /dev/null @@ -1,104 +0,0 @@ -/****************************************************************************** - * Icinga 2 * - * Copyright (C) 2012-2017 Icinga Development Team (https://www.icinga.com/) * - * * - * This program is free software; you can redistribute it and/or * - * modify it under the terms of the GNU General Public License * - * as published by the Free Software Foundation; either version 2 * - * of the License, or (at your option) any later version. * - * * - * This program is distributed in the hope that it will be useful, * - * but WITHOUT ANY WARRANTY; without even the implied warranty of * - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * - * GNU General Public License for more details. * - * * - * You should have received a copy of the GNU General Public License * - * along with this program; if not, write to the Free Software Foundation * - * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA. * - ******************************************************************************/ - -#include "cli/apiusercommand.hpp" -#include "base/logger.hpp" -#include "base/tlsutility.hpp" -#include "base/configwriter.hpp" -#include "remote/apiuser.hpp" -#include - -using namespace icinga; -namespace po = boost::program_options; - -REGISTER_CLICOMMAND("api/user", ApiUserCommand); - -String ApiUserCommand::GetDescription(void) const -{ - return "Create a hashed user and password string for the Icinga 2 API"; -} - -String ApiUserCommand::GetShortDescription(void) const -{ - return "API user creation helper"; -} - -void ApiUserCommand::InitParameters(boost::program_options::options_description& visibleDesc, - boost::program_options::options_description& hiddenDesc) const -{ - visibleDesc.add_options() - ("user", po::value(), "API username") - ("password", po::value(), "Password in clear text") - ("salt", po::value(), "Optional salt (default: 8 random chars)") - ("oneline", "Print only the password hash"); -} - -/** - * The entry point for the "api user" CLI command. - * - * @returns An exit status. - */ -int ApiUserCommand::Run(const boost::program_options::variables_map& vm, const std::vector& ap) const -{ - String passwd, salt; - if (!vm.count("user") && !vm.count("oneline")) { - Log(LogCritical, "cli", "Username (--user) must be specified."); - return 1; - } - - if (!vm.count("password")) { - Log(LogCritical, "cli", "Password (--password) must be specified."); - return 1; - } - - passwd = vm["password"].as(); - salt = vm.count("salt") ? String(vm["salt"].as()) : RandomString(8); - - if (salt.FindFirstOf('$') != String::NPos) { - Log(LogCritical, "cli", "Salt (--salt) may not contain '$'"); - return 1; - } - - String hashedPassword = CreateHashedPasswordString(passwd, salt, 5); - if (hashedPassword == String()) { - Log(LogCritical, "cli") << "Failed to hash password \"" << passwd << "\" with salt \"" << salt << "\""; - return 1; - } - - if (vm.count("oneline")) - std::cout << hashedPassword << std::endl; - else { - std::cout << "object ApiUser "; - - ConfigWriter::EmitString(std::cout, vm["user"].as()); - - std::cout << "{\n" - << " password_hash = "; - - ConfigWriter::EmitString(std::cout, hashedPassword); - - std::cout << "\n" - << " // client_cn = \"\"\n" - << "\n" - << " permissions = [ \"*\" ]\n" - << "}\n"; - } - - return 0; -} diff --git a/lib/cli/apiusercommand.hpp b/lib/cli/apiusercommand.hpp deleted file mode 100644 index 4a4bfb280..000000000 --- a/lib/cli/apiusercommand.hpp +++ /dev/null @@ -1,47 +0,0 @@ -/****************************************************************************** - * Icinga 2 * - * Copyright (C) 2012-2017 Icinga Development Team (https://www.icinga.com/) * - * * - * This program is free software; you can redistribute it and/or * - * modify it under the terms of the GNU General Public License * - * as published by the Free Software Foundation; either version 2 * - * of the License, or (at your option) any later version. * - * * - * This program is distributed in the hope that it will be useful, * - * but WITHOUT ANY WARRANTY; without even the implied warranty of * - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * - * GNU General Public License for more details. * - * * - * You should have received a copy of the GNU General Public License * - * along with this program; if not, write to the Free Software Foundation * - * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA. * - ******************************************************************************/ - -#ifndef APIUSERCOMMAND_H -#define APIUSERCOMMAND_H - -#include "cli/clicommand.hpp" - -namespace icinga -{ - -/** - * The "api user" command. - * - * @ingroup cli - */ -class ApiUserCommand : public CLICommand -{ -public: - DECLARE_PTR_TYPEDEFS(ApiUserCommand); - - virtual String GetDescription(void) const override; - virtual String GetShortDescription(void) const override; - virtual void InitParameters(boost::program_options::options_description& visibleDesc, - boost::program_options::options_description& hiddenDesc) const override; - virtual int Run(const boost::program_options::variables_map& vm, const std::vector& ap) const override; -}; - -} - -#endif /* APIUSERCOMMAND_H */ diff --git a/lib/remote/apiuser.cpp b/lib/remote/apiuser.cpp index 02a6efc07..d7b40d13e 100644 --- a/lib/remote/apiuser.cpp +++ b/lib/remote/apiuser.cpp @@ -27,18 +27,6 @@ using namespace icinga; REGISTER_TYPE(ApiUser); -void ApiUser::OnConfigLoaded(void) -{ - ObjectImpl::OnConfigLoaded(); - - if (GetPasswordHash().IsEmpty()) { - String hashedPassword = CreateHashedPasswordString(GetPassword(), RandomString(8), 5); - VERIFY(hashedPassword != String()); - SetPasswordHash(hashedPassword); - SetPassword("********"); - } -} - ApiUser::Ptr ApiUser::GetByClientCN(const String& cn) { for (const ApiUser::Ptr& user : ConfigType::GetObjectsByType()) { @@ -75,31 +63,9 @@ ApiUser::Ptr ApiUser::GetByAuthHeader(const String& auth_header) */ if (!user || password.IsEmpty()) return nullptr; - else if (user && user->GetPassword() != password) { - Dictionary::Ptr passwordDict = user->GetPasswordDict(); - if (!passwordDict || !ComparePassword(passwordDict->Get("password"), password, passwordDict->Get("salt"))) - return nullptr; - } + else if (user && user->GetPassword() != password) + return nullptr; return user; } -Dictionary::Ptr ApiUser::GetPasswordDict(void) const -{ - String password = GetPasswordHash(); - if (password.IsEmpty() || password[0] != '$') - return nullptr; - - String::SizeType saltBegin = password.FindFirstOf('$', 1); - String::SizeType passwordBegin = password.FindFirstOf('$', saltBegin+1); - - if (saltBegin == String::NPos || saltBegin == 1 || passwordBegin == String::NPos) - return nullptr; - - Dictionary::Ptr passwordDict = new Dictionary(); - passwordDict->Set("algorithm", password.SubStr(1, saltBegin - 1)); - passwordDict->Set("salt", password.SubStr(saltBegin + 1, passwordBegin - saltBegin - 1)); - passwordDict->Set("password", password.SubStr(passwordBegin + 1)); - - return passwordDict; -} diff --git a/lib/remote/apiuser.hpp b/lib/remote/apiuser.hpp index 44cb84df7..15b1c41e3 100644 --- a/lib/remote/apiuser.hpp +++ b/lib/remote/apiuser.hpp @@ -35,12 +35,8 @@ public: DECLARE_OBJECT(ApiUser); DECLARE_OBJECTNAME(ApiUser); - virtual void OnConfigLoaded(void) override; - static ApiUser::Ptr GetByClientCN(const String& cn); static ApiUser::Ptr GetByAuthHeader(const String& auth_header); - - Dictionary::Ptr GetPasswordDict(void) const; }; } diff --git a/lib/remote/apiuser.ti b/lib/remote/apiuser.ti index a767f833a..36ec98704 100644 --- a/lib/remote/apiuser.ti +++ b/lib/remote/apiuser.ti @@ -29,7 +29,7 @@ class ApiUser : ConfigObject { /* No show config */ [no_user_view, no_user_modify] String password; - [config, no_user_view] String password_hash; + [deprecated, config, no_user_view] String password_hash; [config] String client_cn (ClientCN); [config] array(Value) permissions; }; diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 29a16646f..789550052 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -43,7 +43,6 @@ set(base_test_SOURCES icinga-notification.cpp icinga-perfdata.cpp remote-url.cpp - remote-user.cpp ${base_OBJS} $ $ @@ -142,7 +141,6 @@ add_boost_test(base remote_url/get_and_set remote_url/format remote_url/illegal_legal_strings - api_user/password ) if(ICINGA2_WITH_LIVESTATUS) diff --git a/test/remote-user.cpp b/test/remote-user.cpp deleted file mode 100644 index f43816a19..000000000 --- a/test/remote-user.cpp +++ /dev/null @@ -1,50 +0,0 @@ -/****************************************************************************** - * Icinga 2 * - * Copyright (C) 2012-2017 Icinga Development Team (https://www.icinga.com/) * - * * - * This program is free software; you can redistribute it and/or * - * modify it under the terms of the GNU General Public License * - * as published by the Free Software Foundation; either version 2 * - * of the License, or (at your option) any later version. * - * * - * This program is distributed in the hope that it will be useful, * - * but WITHOUT ANY WARRANTY; without even the implied warranty of * - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * - * GNU General Public License for more details. * - * * - * You should have received a copy of the GNU General Public License * - * along with this program; if not, write to the Free Software Foundation * - * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA. * - ******************************************************************************/ - -#include "remote/apiuser.hpp" -#include "base/tlsutility.hpp" -#include - -#include - -using namespace icinga; - -BOOST_AUTO_TEST_SUITE(api_user) - -BOOST_AUTO_TEST_CASE(password) -{ - ApiUser::Ptr user = new ApiUser(); - String passwd = RandomString(16); - String salt = RandomString(8); - user->SetPasswordHash(CreateHashedPasswordString(passwd, salt)); - user->OnConfigLoaded(); - user->OnAllConfigLoaded(); - user->Start(); - - BOOST_CHECK(user->GetPasswordHash() != passwd); - - Dictionary::Ptr passwdd = user->GetPasswordDict(); - - BOOST_CHECK(passwdd); - BOOST_CHECK(passwdd->Get("salt") == salt); - BOOST_CHECK(ComparePassword(passwdd->Get("password"), passwd, salt)); - BOOST_CHECK(!ComparePassword(passwdd->Get("password"), "wrong password uwu!", salt)); -} - -BOOST_AUTO_TEST_SUITE_END() -- 2.40.0