From ae010bfa73aa1a164abb0b767e7b9c6bcdee75eb Mon Sep 17 00:00:00 2001 From: judyjoseph <53951155+judyjoseph@users.noreply.github.com> Date: Tue, 5 Sep 2023 19:18:39 -0700 Subject: [PATCH 1/2] Support type7 encoded CAK key for macsec in config_db (#2892) * Add decode type 7 alogorithm and use it to decode the encoded key from config_db * Remove the Error log added earlier for debugging * Add check for 66 bytes or 130 bytes encoded string based on cipher suite --- cfgmgr/macsecmgr.cpp | 70 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 69 insertions(+), 1 deletion(-) diff --git a/cfgmgr/macsecmgr.cpp b/cfgmgr/macsecmgr.cpp index 0edb86a5afd..42e06731cc2 100644 --- a/cfgmgr/macsecmgr.cpp +++ b/cfgmgr/macsecmgr.cpp @@ -34,6 +34,20 @@ constexpr std::uint64_t RETRY_TIME = 30; /* retry interval, in millisecond */ constexpr std::uint64_t RETRY_INTERVAL = 100; +/* + * The input cipher_str is the encoded string which can be either of length 66 bytes or 130 bytes. + * + * 66 bytes of length, for 128-byte cipher suite + * - first 2 bytes of the string will be the index from the magic salt string. + * - remaining 64 bytes will be encoded string from the 32-byte plain text CAK input string. + * + * 130 bytes of length, for 256-byte cipher suite + * - first 2 bytes of the string will be the index from the magic salt string. + * - remaining 128 bytes will be encoded string from the 32 byte plain text CAK input string. +*/ +constexpr std::size_t AES_LEN_128_BYTE = 66; +constexpr std::size_t AES_LEN_256_BYTE = 130; + static void lexical_convert(const std::string &policy_str, MACsecMgr::MACsecProfile::Policy & policy) { SWSS_LOG_ENTER(); @@ -78,6 +92,60 @@ static void lexical_convert(const std::string &cipher_str, MACsecMgr::MACsecProf } } + + +/* Decodes a Type 7 encoded input. + * + * The Type 7 encoding consists of two decimal digits(encoding the salt), followed a series of hexadecimal characters, + * two for every byte in the encoded password. An example encoding(of "password") is 044B0A151C36435C0D. + * This has a salt/offset of 4 (04 in the example), and encodes password via 4B0A151C36435C0D. + * + * The algorithm is a straightforward XOR Cipher that relies on the following ascii-encoded 53-byte constant: + * "dsfd;kfoA,.iyewrkldJKDHSUBsgvca69834ncxv9873254k;fg87" + * + * Decode() + * Get the salt index from the first 2 chars + * For each byte in the provided text after the encoded salt: + * j = (salt index + 1) % 53 + * XOR the i'th byte of the password with the j'th byte of the magic constant. + * append to the decoded string. + */ +static std::string decodeKey(const std::string &cipher_str, const MACsecMgr::MACsecProfile::CipherSuite & cipher_suite) +{ + int salts[] = { 0x64, 0x73, 0x66, 0x64, 0x3B, 0x6B, 0x66, 0x6F, 0x41, 0x2C, 0x2E, 0x69, 0x79, 0x65, 0x77, 0x72, 0x6B, 0x6C, 0x64, 0x4A, 0x4B, 0x44, 0x48, 0x53, 0x55, 0x42, 0x73, 0x67, 0x76, 0x63, 0x61, 0x36, 0x39, 0x38, 0x33, 0x34, 0x6E, 0x63, 0x78, 0x76, 0x39, 0x38, 0x37, 0x33, 0x32, 0x35, 0x34, 0x6B, 0x3B, 0x66, 0x67, 0x38, 0x37 }; + + std::string decodedPassword = std::string(""); + std::string cipher_hex_str = std::string(""); + unsigned int hex_int, saltIdx; + + if ((cipher_suite == MACsecMgr::MACsecProfile::CipherSuite::GCM_AES_128) || + (cipher_suite == MACsecMgr::MACsecProfile::CipherSuite::GCM_AES_XPN_128)) + { + if (cipher_str.length() != AES_LEN_128_BYTE) + throw std::invalid_argument("Invalid length for cipher_string : " + cipher_str); + } + else if ((cipher_suite == MACsecMgr::MACsecProfile::CipherSuite::GCM_AES_256) || + (cipher_suite == MACsecMgr::MACsecProfile::CipherSuite::GCM_AES_XPN_256)) + { + if (cipher_str.length() != AES_LEN_256_BYTE) + throw std::invalid_argument("Invalid length for cipher_string : " + cipher_str); + } + + // Get the salt index from the cipher_str + saltIdx = (unsigned int) stoi(cipher_str.substr(0,2)); + + // Convert the hex string (eg: "aabbcc") to hex integers (eg: 0xaa, 0xbb, 0xcc) taking a substring of 2 chars at a time + // and do xor with the magic salt string + for (size_t i = 2; i < cipher_str.length(); i += 2) { + std::stringstream ss; + ss << std::hex << cipher_str.substr(i,2); + ss >> hex_int; + decodedPassword += (char)(hex_int ^ salts[saltIdx++ % (sizeof(salts)/sizeof(salts[0]))]); + } + + return decodedPassword; +} + template static bool get_value( const MACsecMgr::TaskArgs & ta, @@ -699,7 +767,7 @@ bool MACsecMgr::configureMACsec( port_name, network_id, "mka_cak", - profile.primary_cak); + decodeKey(profile.primary_cak, profile.cipher_suite)); wpa_cli_exec_and_check( session.sock, From 13ef25bf14d0b75d7ef5adec43493f206f701ff4 Mon Sep 17 00:00:00 2001 From: Lawrence Lee Date: Wed, 13 Sep 2023 14:04:29 -0700 Subject: [PATCH 2/2] [teamd]: Clean teamd process if LAG creation fails (#2888) * [teamd]: Clean teamd process if LAG creation fails --- .gitignore | 1 + cfgmgr/teammgr.cpp | 41 ++++++++++++- tests/mock_tests/Makefile.am | 26 +++++++- tests/mock_tests/teammgrd/teammgr_ut.cpp | 78 ++++++++++++++++++++++++ 4 files changed, 141 insertions(+), 5 deletions(-) create mode 100644 tests/mock_tests/teammgrd/teammgr_ut.cpp diff --git a/.gitignore b/.gitignore index a0c8c5ac821..001db00e4bc 100644 --- a/.gitignore +++ b/.gitignore @@ -77,6 +77,7 @@ tests/tests tests/mock_tests/tests_response_publisher tests/mock_tests/tests_fpmsyncd tests/mock_tests/tests_intfmgrd +tests/mock_tests/tests_teammgrd tests/mock_tests/tests_portsyncd diff --git a/cfgmgr/teammgr.cpp b/cfgmgr/teammgr.cpp index 40eca9d9211..36c9d134e14 100644 --- a/cfgmgr/teammgr.cpp +++ b/cfgmgr/teammgr.cpp @@ -307,6 +307,8 @@ void TeamMgr::doLagTask(Consumer &consumer) { if (addLag(alias, min_links, fallback, fast_rate) == task_need_retry) { + // If LAG creation fails, we need to clean up any potentially orphaned teamd processes + removeLag(alias); it++; continue; } @@ -627,7 +629,7 @@ task_process_status TeamMgr::addLag(const string &alias, int min_links, bool fal SWSS_LOG_INFO("Port channel %s teamd configuration: %s", alias.c_str(), conf.str().c_str()); - string warmstart_flag = WarmStart::isWarmStart() ? " -w -o " : " -r "; + string warmstart_flag = WarmStart::isWarmStart() ? " -w -o" : " -r"; cmd << TEAMD_CMD << warmstart_flag @@ -654,9 +656,42 @@ bool TeamMgr::removeLag(const string &alias) stringstream cmd; string res; + pid_t pid; - cmd << TEAMD_CMD << " -k -t " << shellquote(alias); - EXEC_WITH_ERROR_THROW(cmd.str(), res); + try + { + std::stringstream cmd; + cmd << "cat " << shellquote("/var/run/teamd/" + alias + ".pid"); + EXEC_WITH_ERROR_THROW(cmd.str(), res); + } + catch (const std::exception &e) + { + SWSS_LOG_NOTICE("Failed to remove non-existent port channel %s pid...", alias.c_str()); + return false; + } + + try + { + pid = static_cast(std::stoul(res, nullptr, 10)); + SWSS_LOG_INFO("Read port channel %s pid %d", alias.c_str(), pid); + } + catch (const std::exception &e) + { + SWSS_LOG_ERROR("Failed to read port channel %s pid: %s", alias.c_str(), e.what()); + return false; + } + + try + { + std::stringstream cmd; + cmd << "kill -TERM " << pid; + EXEC_WITH_ERROR_THROW(cmd.str(), res); + } + catch (const std::exception &e) + { + SWSS_LOG_ERROR("Failed to send SIGTERM to port channel %s pid %d: %s", alias.c_str(), pid, e.what()); + return false; + } SWSS_LOG_NOTICE("Stop port channel %s", alias.c_str()); diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am index 83510452a89..2156a5dd1cf 100644 --- a/tests/mock_tests/Makefile.am +++ b/tests/mock_tests/Makefile.am @@ -5,9 +5,9 @@ DASH_PROTO_DIR = $(top_srcdir)/orchagent/dash/proto CFLAGS_SAI = -I /usr/include/sai -TESTS = tests tests_intfmgrd tests_portsyncd tests_fpmsyncd tests_response_publisher +TESTS = tests tests_intfmgrd tests_teammgrd tests_portsyncd tests_fpmsyncd tests_response_publisher -noinst_PROGRAMS = tests tests_intfmgrd tests_portsyncd tests_fpmsyncd tests_response_publisher +noinst_PROGRAMS = tests tests_intfmgrd tests_teammgrd tests_portsyncd tests_fpmsyncd tests_response_publisher LDADD_SAI = -lsaimeta -lsaimetadata -lsaivs -lsairedis @@ -190,6 +190,28 @@ tests_intfmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTE tests_intfmgrd_LDADD = $(LDADD_GTEST) $(LDADD_SAI) -lnl-genl-3 -lhiredis -lhiredis \ -lswsscommon -lswsscommon -lgtest -lgtest_main -lzmq -lnl-3 -lnl-route-3 -lpthread -lgmock -lgmock_main +## teammgrd unit tests + +tests_teammgrd_SOURCES = teammgrd/teammgr_ut.cpp \ + $(top_srcdir)/cfgmgr/teammgr.cpp \ + $(top_srcdir)/lib/subintf.cpp \ + $(top_srcdir)/lib/recorder.cpp \ + $(top_srcdir)/orchagent/orch.cpp \ + $(top_srcdir)/orchagent/request_parser.cpp \ + mock_orchagent_main.cpp \ + mock_dbconnector.cpp \ + mock_table.cpp \ + mock_hiredis.cpp \ + fake_response_publisher.cpp \ + mock_redisreply.cpp \ + common/mock_shell_command.cpp + +tests_teammgrd_INCLUDES = $(tests_INCLUDES) -I$(top_srcdir)/cfgmgr -I$(top_srcdir)/lib +tests_teammgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) +tests_teammgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) $(tests_teammgrd_INCLUDES) +tests_teammgrd_LDADD = $(LDADD_GTEST) $(LDADD_SAI) -lnl-genl-3 -lhiredis -lhiredis \ + -lswsscommon -lswsscommon -lgtest -lgtest_main -lzmq -lnl-3 -lnl-route-3 -lpthread -lgmock -lgmock_main + ## fpmsyncd unit tests tests_fpmsyncd_SOURCES = fpmsyncd/test_fpmlink.cpp \ diff --git a/tests/mock_tests/teammgrd/teammgr_ut.cpp b/tests/mock_tests/teammgrd/teammgr_ut.cpp new file mode 100644 index 00000000000..32f064f5526 --- /dev/null +++ b/tests/mock_tests/teammgrd/teammgr_ut.cpp @@ -0,0 +1,78 @@ +#include "gtest/gtest.h" +#include "../mock_table.h" +#include "teammgr.h" + +extern int (*callback)(const std::string &cmd, std::string &stdout); +extern std::vector mockCallArgs; + +int cb(const std::string &cmd, std::string &stdout) +{ + mockCallArgs.push_back(cmd); + if (cmd.find("/usr/bin/teamd -r -t PortChannel1") != std::string::npos) + { + return 1; + } + else if (cmd.find("cat \"/var/run/teamd/PortChannel1.pid\"") != std::string::npos) + { + stdout = "1234"; + return 0; + } + return 0; +} + +namespace teammgr_ut +{ + struct TeamMgrTest : public ::testing::Test + { + std::shared_ptr m_config_db; + std::shared_ptr m_app_db; + std::shared_ptr m_state_db; + std::vector cfg_lag_tables; + + virtual void SetUp() override + { + testing_db::reset(); + m_config_db = std::make_shared("CONFIG_DB", 0); + m_app_db = std::make_shared("APPL_DB", 0); + m_state_db = std::make_shared("STATE_DB", 0); + + swss::Table metadata_table = swss::Table(m_config_db.get(), CFG_DEVICE_METADATA_TABLE_NAME); + std::vector vec; + vec.emplace_back("mac", "01:23:45:67:89:ab"); + metadata_table.set("localhost", vec); + + TableConnector conf_lag_table(m_config_db.get(), CFG_LAG_TABLE_NAME); + TableConnector conf_lag_member_table(m_config_db.get(), CFG_LAG_MEMBER_TABLE_NAME); + TableConnector state_port_table(m_state_db.get(), STATE_PORT_TABLE_NAME); + + std::vector tables = { + conf_lag_table, + conf_lag_member_table, + state_port_table + }; + + cfg_lag_tables = tables; + mockCallArgs.clear(); + callback = cb; + } + }; + + TEST_F(TeamMgrTest, testProcessKilledAfterAddLagFailure) + { + swss::TeamMgr teammgr(m_config_db.get(), m_app_db.get(), m_state_db.get(), cfg_lag_tables); + swss::Table cfg_lag_table = swss::Table(m_config_db.get(), CFG_LAG_TABLE_NAME); + cfg_lag_table.set("PortChannel1", { { "admin_status", "up" }, + { "mtu", "9100" }, + { "lacp_key", "auto" }, + { "min_links", "2" } }); + teammgr.addExistingData(&cfg_lag_table); + teammgr.doTask(); + int kill_cmd_called = 0; + for (auto cmd : mockCallArgs){ + if (cmd.find("kill -TERM 1234") != std::string::npos){ + kill_cmd_called++; + } + } + ASSERT_EQ(kill_cmd_called, 1); + } +} \ No newline at end of file