From ccabd787d0e0897b5f2fe873bd01fc3903f6bd48 Mon Sep 17 00:00:00 2001 From: raakella1 <114193113+raakella1@users.noreply.github.com> Date: Fri, 24 May 2024 10:54:33 -0700 Subject: [PATCH] Remove LRU cashe for invalid tokens (#235) * Add issued_at leeway and not_before leeway to jwt verifier. Remove LRU cache for tokens and always verify them using jwt library * add unit tests * download token 5 minutes before expiry * use LRU cache for valid tokens * request new token during the half life of the existing token --------- Co-authored-by: Ravi Nagarjun Akella --- conanfile.py | 2 +- include/sisl/auth_manager/auth_manager.hpp | 16 +----- include/sisl/auth_manager/trf_client.hpp | 6 +- src/auth_manager/auth_manager.cpp | 35 +++++------ src/auth_manager/security_config.fbs | 8 ++- src/auth_manager/tests/AuthTest.cpp | 67 +++++++++++++++++++--- src/auth_manager/trf_client.cpp | 9 ++- src/grpc/tests/unit/auth_test.cpp | 42 ++++++-------- 8 files changed, 109 insertions(+), 76 deletions(-) diff --git a/conanfile.py b/conanfile.py index 2f32bc5f..8cd7c3f4 100644 --- a/conanfile.py +++ b/conanfile.py @@ -8,7 +8,7 @@ class SISLConan(ConanFile): name = "sisl" - version = "8.6.8" + version = "8.7.0" homepage = "https://github.com/eBay/sisl" description = "Library for fast data structures, utilities" topics = ("ebay", "components", "core", "efficiency") diff --git a/include/sisl/auth_manager/auth_manager.hpp b/include/sisl/auth_manager/auth_manager.hpp index 01885809..69c26b16 100644 --- a/include/sisl/auth_manager/auth_manager.hpp +++ b/include/sisl/auth_manager/auth_manager.hpp @@ -32,21 +32,7 @@ class LRUCache; * they were extracted from decoded token. */ struct CachedToken { - AuthVerifyStatus response_status; - std::string msg; - bool valid; std::chrono::system_clock::time_point expires_at; - - inline void set_invalid(AuthVerifyStatus code, const std::string& reason) { - valid = false; - response_status = code; - msg = reason; - } - - inline void set_valid() { - valid = true; - response_status = AuthVerifyStatus::OK; - } }; class AuthManager { @@ -68,4 +54,4 @@ class AuthManager { // key_id -> signing public key mutable LRUCache< std::string, std::string > m_cached_keys; }; -} // namespace sisl +} // namespace sisl \ No newline at end of file diff --git a/include/sisl/auth_manager/trf_client.hpp b/include/sisl/auth_manager/trf_client.hpp index 9632b7e7..b0fbeae8 100644 --- a/include/sisl/auth_manager/trf_client.hpp +++ b/include/sisl/auth_manager/trf_client.hpp @@ -26,10 +26,8 @@ class TrfClient { private: void validate_grant_path() const; bool grant_path_exists() const { return std::filesystem::exists(SECURITY_DYNAMIC_CONFIG(trf_client->grant_path)); } - bool access_token_expired() const { - return (std::chrono::system_clock::now() > - m_expiry + std::chrono::seconds(SECURITY_DYNAMIC_CONFIG(auth_manager->leeway))); - } + // If leeway is set, this will force us to download token ahead of its expiry + bool access_token_expired() const { return (std::chrono::system_clock::now() > m_expiry); } static bool get_file_contents(const std::string& file_name, std::string& contents); private: diff --git a/src/auth_manager/auth_manager.cpp b/src/auth_manager/auth_manager.cpp index 6e083cd4..795ad6b1 100644 --- a/src/auth_manager/auth_manager.cpp +++ b/src/auth_manager/auth_manager.cpp @@ -40,15 +40,12 @@ AuthVerifyStatus AuthManager::verify(const std::string& token, std::string& msg) // if we have it in cache, just use it to make the decision auto const token_hash = md5_sum(token); if (auto const ct = m_cached_tokens.get(token_hash); ct) { - if (ct->valid) { - auto now = std::chrono::system_clock::now(); - if (now > ct->expires_at + std::chrono::seconds(SECURITY_DYNAMIC_CONFIG(auth_manager->leeway))) { - m_cached_tokens.put(token_hash, - CachedToken{AuthVerifyStatus::UNAUTH, "token expired", false, ct->expires_at}); - } + auto now = std::chrono::system_clock::now(); + if (now > ct->expires_at + std::chrono::seconds(SECURITY_DYNAMIC_CONFIG(auth_manager->expiry_leeway_secs))) { + msg = "token expired"; + return AuthVerifyStatus::UNAUTH; } - msg = ct->msg; - return ct->response_status; + return AuthVerifyStatus::OK; } // not found in cache @@ -63,31 +60,23 @@ AuthVerifyStatus AuthManager::verify(const std::string& token, std::string& msg) verify_decoded(decoded); app_name = get_app(decoded); cached_token.expires_at = decoded.get_expires_at(); - cached_token.set_valid(); - } catch (const incomplete_verification_error& e) { + } catch (const std::exception& e) { // verification incomplete, the token validity is not determined, shouldn't // cache msg = e.what(); return AuthVerifyStatus::UNAUTH; - } catch (const std::exception& e) { - cached_token.set_invalid(AuthVerifyStatus::UNAUTH, e.what()); - m_cached_tokens.put(token_hash, cached_token); - msg = cached_token.msg; - return cached_token.response_status; } // check client application - if (SECURITY_DYNAMIC_CONFIG(auth_manager->auth_allowed_apps) != "all") { if (SECURITY_DYNAMIC_CONFIG(auth_manager->auth_allowed_apps).find(app_name) == std::string::npos) { - cached_token.set_invalid(AuthVerifyStatus::FORBIDDEN, - fmt::format("application '{}' is not allowed to perform the request", app_name)); + msg = fmt::format("application '{}' is not allowed to perform the request", app_name); + return AuthVerifyStatus::FORBIDDEN; } } m_cached_tokens.put(token_hash, cached_token); - msg = cached_token.msg; - return cached_token.response_status; + return AuthVerifyStatus::OK; } void AuthManager::verify_decoded(const jwt::decoded_jwt& decoded) const { @@ -125,7 +114,9 @@ void AuthManager::verify_decoded(const jwt::decoded_jwt& decoded) const { const auto verifier{jwt::verify() .with_issuer(SECURITY_DYNAMIC_CONFIG(auth_manager->issuer)) .allow_algorithm(jwt::algorithm::rs256(signing_key)) - .expires_at_leeway(SECURITY_DYNAMIC_CONFIG(auth_manager->leeway))}; + .expires_at_leeway(SECURITY_DYNAMIC_CONFIG(auth_manager->expiry_leeway_secs)) + .issued_at_leeway(SECURITY_DYNAMIC_CONFIG(auth_manager->iat_leeway_secs)) + .not_before_leeway(SECURITY_DYNAMIC_CONFIG(auth_manager->nbf_leeway_secs))}; // if verification fails, an instance of std::system_error subclass is thrown. verifier.verify(decoded); @@ -166,4 +157,4 @@ std::string AuthManager::get_app(const jwt::decoded_jwt& decoded) const { const auto end{client_id.find_first_of(",", start)}; return client_id.substr(start, end - start); } -} // namespace sisl +} // namespace sisl \ No newline at end of file diff --git a/src/auth_manager/security_config.fbs b/src/auth_manager/security_config.fbs index 20cbec5a..33b04983 100644 --- a/src/auth_manager/security_config.fbs +++ b/src/auth_manager/security_config.fbs @@ -29,7 +29,13 @@ table AuthManager { tf_token_url: string; // leeway to the token expiration - leeway: uint32 = 0; + expiry_leeway_secs: uint32 = 0; + + // leeway to the token issued_at + iat_leeway_secs: uint32 = 5; + + // leeway to the token not_before + nbf_leeway_secs: uint32 = 5; // ssl verification for the signing key download url verify: bool = true; diff --git a/src/auth_manager/tests/AuthTest.cpp b/src/auth_manager/tests/AuthTest.cpp index 79ba44ac..30941b1e 100644 --- a/src/auth_manager/tests/AuthTest.cpp +++ b/src/auth_manager/tests/AuthTest.cpp @@ -33,10 +33,9 @@ class MockAuthManager : public AuthManager { public: using AuthManager::AuthManager; MOCK_METHOD(std::string, download_key, (const std::string&), (const)); - AuthVerifyStatus verify(const std::string& token) { - std::string msg; - return AuthManager::verify(token, msg); - } + std::string msg; + AuthVerifyStatus verify(const std::string& token) { return verify(token, msg); } + AuthVerifyStatus verify(const std::string& token, std::string& msg) { return AuthManager::verify(token, msg); } }; class AuthTest : public ::testing::Test { @@ -57,7 +56,6 @@ class AuthTest : public ::testing::Test { SECURITY_SETTINGS_FACTORY().modifiable_settings([](auto& s) { s.auth_manager->auth_allowed_apps = "app1, testapp, app2"; s.auth_manager->tf_token_url = "http://127.0.0.1"; - s.auth_manager->leeway = 0; s.auth_manager->issuer = "trustfabric"; }); SECURITY_SETTINGS_FACTORY().save(); @@ -140,6 +138,48 @@ TEST_F(AuthTest, reject_unauthorized_app) { EXPECT_EQ(mock_auth_mgr->verify(token.sign_rs256()), AuthVerifyStatus::FORBIDDEN); } +TEST_F(AuthTest, leeway_test) { + auto test_token = TestToken(); + auto& trf_token = test_token.get_token(); + + // default leeway is 0 seconds for exp + trf_token.set_expires_at(std::chrono::system_clock::now() + std::chrono::seconds(1)); + // default leeway is 5 seconds for iat and nbf + trf_token.set_issued_at(std::chrono::system_clock::now() + std::chrono::seconds(4)); + trf_token.set_not_before(std::chrono::system_clock::now() + std::chrono::seconds(4)); + auto raw_token = test_token.sign_rs256(); + + EXPECT_CALL(*mock_auth_mgr, download_key(_)).Times(1).WillOnce(Return(rsa_pub_key)); + EXPECT_EQ(mock_auth_mgr->verify(raw_token), AuthVerifyStatus::OK); + + std::string unauth_msg; + // token expired + trf_token.set_expires_at(std::chrono::system_clock::now() - std::chrono::seconds(1)); + raw_token = test_token.sign_rs256(); + EXPECT_CALL(*mock_auth_mgr, download_key(_)).Times(0); + EXPECT_EQ(mock_auth_mgr->verify(raw_token, unauth_msg), AuthVerifyStatus::UNAUTH); + EXPECT_EQ(unauth_msg, "token verification failed: token expired"); + + unauth_msg.clear(); + // iat expired + trf_token.set_expires_at(std::chrono::system_clock::now() + std::chrono::seconds(1)); + trf_token.set_issued_at(std::chrono::system_clock::now() + std::chrono::seconds(6)); + trf_token.set_key_id("new_key_id"); + raw_token = test_token.sign_rs256(); + EXPECT_CALL(*mock_auth_mgr, download_key(_)).Times(1).WillOnce(Return(rsa_pub_key)); + EXPECT_EQ(mock_auth_mgr->verify(raw_token, unauth_msg), AuthVerifyStatus::UNAUTH); + EXPECT_EQ(unauth_msg, "token verification failed: token expired"); + + unauth_msg.clear(); + // nbf expired + trf_token.set_issued_at(std::chrono::system_clock::now() - std::chrono::seconds(1)); + trf_token.set_not_before(std::chrono::system_clock::now() + std::chrono::seconds(6)); + raw_token = test_token.sign_rs256(); + EXPECT_CALL(*mock_auth_mgr, download_key(_)).Times(0); + EXPECT_EQ(mock_auth_mgr->verify(raw_token, unauth_msg), AuthVerifyStatus::UNAUTH); + EXPECT_EQ(unauth_msg, "token verification failed: token expired"); +} + // Testing trf client class MockTrfClient : public TrfClient { public: @@ -169,7 +209,7 @@ static void load_trf_settings() { s.trf_client->grant_path = grant_path; s.trf_client->server = "127.0.0.1:12346/token"; s.auth_manager->verify = false; - s.auth_manager->leeway = 30; + s.auth_manager->expiry_leeway_secs = 30; }); SECURITY_SETTINGS_FACTORY().save(); } @@ -200,7 +240,8 @@ TEST_F(AuthTest, trf_allow_valid_token) { const auto raw_token{TestToken().sign_rs256()}; // mock_trf_client is expected to be called twice // 1. First time when access_token is empty - // 2. When token is set to be expired + // 2. When expiry - leeway is less than current time + // 3. When access_token is expired EXPECT_CALL(mock_trf_client, request_with_grant_token()).Times(2); ON_CALL(mock_trf_client, request_with_grant_token()) .WillByDefault( @@ -212,6 +253,8 @@ TEST_F(AuthTest, trf_allow_valid_token) { // use the acces_token saved from the previous call EXPECT_CALL(*mock_auth_mgr, download_key(_)).Times(0); EXPECT_EQ(mock_auth_mgr->verify(mock_trf_client.get_token()), AuthVerifyStatus::OK); + mock_trf_client.set_expiry(std::chrono::system_clock::now() + std::chrono::seconds(25)); + EXPECT_EQ(mock_auth_mgr->verify(mock_trf_client.get_token()), AuthVerifyStatus::OK); // set token to be expired invoking request_with_grant_token mock_trf_client.set_expiry(std::chrono::system_clock::now() - std::chrono::seconds(100)); @@ -222,9 +265,11 @@ TEST_F(AuthTest, trf_allow_valid_token) { static const std::string trf_token_server_ip{"127.0.0.1"}; static const uint32_t trf_token_server_port{12346}; static std::string token_response; +static uint32_t token_expiry{4000}; static void set_token_response(const std::string& raw_token) { token_response = "{\"access_token\":\"" + raw_token + - "\",\"token_type\":\"Bearer\",\"expires_in\":2000,\"refresh_token\":\"dummy_refresh_token\"}\n"; + "\",\"token_type\":\"Bearer\",\"expires_in\":" + std::to_string(token_expiry) + + ",\"refresh_token\":\"dummy_refresh_token\"}\n"; } class TokenApiImpl : public TokenApi { @@ -294,6 +339,9 @@ TEST_F(TrfClientTest, request_with_grant_token) { mock_trf_client.__request_with_grant_token(); })); mock_trf_client.get_token(); + auto time_to_expiry = std::chrono::duration_cast< std::chrono::seconds >(mock_trf_client.get_expiry() - + std::chrono::system_clock::now()); + EXPECT_LT(time_to_expiry, std::chrono::seconds{token_expiry} / 2); EXPECT_EQ(raw_token, mock_trf_client.get_access_token()); EXPECT_EQ("Bearer", mock_trf_client.get_token_type()); } @@ -309,6 +357,9 @@ TEST(TrfClientParseTest, parse_token) { EXPECT_EQ(raw_token, mock_trf_client.get_access_token()); EXPECT_EQ("Bearer", mock_trf_client.get_token_type()); EXPECT_TRUE(mock_trf_client.get_expiry() > std::chrono::system_clock::now()); + auto time_to_expiry = std::chrono::duration_cast< std::chrono::seconds >(mock_trf_client.get_expiry() - + std::chrono::system_clock::now()); + EXPECT_LT(time_to_expiry, std::chrono::seconds{token_expiry} / 2); remove_grant_path(); } } // namespace sisl::testing diff --git a/src/auth_manager/trf_client.cpp b/src/auth_manager/trf_client.cpp index 92e5f062..dfd4bb4d 100644 --- a/src/auth_manager/trf_client.cpp +++ b/src/auth_manager/trf_client.cpp @@ -34,6 +34,11 @@ bool TrfClient::get_file_contents(const std::string& file_path, std::string& con return false; } +static std::chrono::seconds get_expiry(const std::chrono::seconds& token_expiry) { + // refresh after half the expiry time + return token_expiry / 2; +} + void TrfClient::request_with_grant_token() { std::string grant_token; if (!get_file_contents(SECURITY_DYNAMIC_CONFIG(trf_client->grant_path), grant_token)) { @@ -72,7 +77,7 @@ void TrfClient::request_with_grant_token() { try { const nlohmann::json resp_json = nlohmann::json::parse(resp.text); - m_expiry = std::chrono::system_clock::now() + std::chrono::seconds(resp_json["expires_in"]); + m_expiry = std::chrono::system_clock::now() + get_expiry(std::chrono::seconds(resp_json["expires_in"])); m_access_token = resp_json["access_token"]; m_token_type = resp_json["token_type"]; } catch ([[maybe_unused]] const nlohmann::detail::exception& e) { @@ -91,7 +96,7 @@ void TrfClient::parse_response(const std::string& resp) { if (m_token_type = get_quoted_string(resp, token2); m_access_token.empty()) { return; } auto expiry_str = get_string(resp, token3); if (expiry_str.empty()) { return; } - m_expiry = std::chrono::system_clock::now() + std::chrono::seconds(std::stol(expiry_str)); + m_expiry = std::chrono::system_clock::now() + get_expiry(std::chrono::seconds(std::stol(expiry_str))); } catch (const std::exception& e) { LOGERROR("failed to parse response: {}, what: {}", resp, e.what()); } } diff --git a/src/grpc/tests/unit/auth_test.cpp b/src/grpc/tests/unit/auth_test.cpp index 82960526..4f9d08e6 100644 --- a/src/grpc/tests/unit/auth_test.cpp +++ b/src/grpc/tests/unit/auth_test.cpp @@ -245,7 +245,7 @@ static void load_auth_settings() { SECURITY_SETTINGS_FACTORY().modifiable_settings([](auto& s) { s.auth_manager->auth_allowed_apps = "app1, testapp, app2"; s.auth_manager->tf_token_url = "http://127.0.0.1"; - s.auth_manager->leeway = 0; + s.auth_manager->expiry_leeway_secs = 0; s.auth_manager->issuer = "trustfabric"; s.trf_client->grant_path = grant_path; s.trf_client->server = fmt::format("{}:{}/token", trf_token_server_ip, trf_token_server_port); @@ -401,12 +401,12 @@ TEST(GenericServiceDeathTest, basic_test) { auto g_grpc_server = GrpcServer::make("0.0.0.0:56789", nullptr, 1, "", ""); // register rpc before generic service is registered #ifndef NDEBUG - ASSERT_DEATH(g_grpc_server->register_generic_rpc( - "method1", [](boost::intrusive_ptr< GenericRpcData >&) { return true; }), - "Assertion .* failed"); + ASSERT_DEATH( + g_grpc_server->register_generic_rpc("method1", [](boost::intrusive_ptr< GenericRpcData >&) { return true; }), + "Assertion .* failed"); #else - EXPECT_FALSE(g_grpc_server->register_generic_rpc( - "method1", [](boost::intrusive_ptr< GenericRpcData >&) { return true; })); + EXPECT_FALSE( + g_grpc_server->register_generic_rpc("method1", [](boost::intrusive_ptr< GenericRpcData >&) { return true; })); #endif ASSERT_TRUE(g_grpc_server->register_async_generic_service()); @@ -414,21 +414,21 @@ TEST(GenericServiceDeathTest, basic_test) { EXPECT_FALSE(g_grpc_server->register_async_generic_service()); // register rpc before server is run #ifndef NDEBUG - ASSERT_DEATH(g_grpc_server->register_generic_rpc( - "method1", [](boost::intrusive_ptr< GenericRpcData >&) { return true; }), - "Assertion .* failed"); + ASSERT_DEATH( + g_grpc_server->register_generic_rpc("method1", [](boost::intrusive_ptr< GenericRpcData >&) { return true; }), + "Assertion .* failed"); #else - EXPECT_FALSE(g_grpc_server->register_generic_rpc( - "method1", [](boost::intrusive_ptr< GenericRpcData >&) { return true; })); + EXPECT_FALSE( + g_grpc_server->register_generic_rpc("method1", [](boost::intrusive_ptr< GenericRpcData >&) { return true; })); #endif g_grpc_server->run(); - EXPECT_TRUE(g_grpc_server->register_generic_rpc( - "method1", [](boost::intrusive_ptr< GenericRpcData >&) { return true; })); - EXPECT_TRUE(g_grpc_server->register_generic_rpc( - "method2", [](boost::intrusive_ptr< GenericRpcData >&) { return true; })); + EXPECT_TRUE( + g_grpc_server->register_generic_rpc("method1", [](boost::intrusive_ptr< GenericRpcData >&) { return true; })); + EXPECT_TRUE( + g_grpc_server->register_generic_rpc("method2", [](boost::intrusive_ptr< GenericRpcData >&) { return true; })); // re-register method 1 - EXPECT_FALSE(g_grpc_server->register_generic_rpc( - "method1", [](boost::intrusive_ptr< GenericRpcData >&) { return true; })); + EXPECT_FALSE( + g_grpc_server->register_generic_rpc("method1", [](boost::intrusive_ptr< GenericRpcData >&) { return true; })); auto client = std::make_unique< GrpcAsyncClient >("0.0.0.0:56789", "", ""); client->init(); @@ -437,15 +437,11 @@ TEST(GenericServiceDeathTest, basic_test) { ::grpc::ByteBuffer cli_buf; generic_stub->call_unary( cli_buf, "method1", - [method = "method1"](::grpc::ByteBuffer&, ::grpc::Status& status) { - validate_generic_reply(method, status); - }, + [method = "method1"](::grpc::ByteBuffer&, ::grpc::Status& status) { validate_generic_reply(method, status); }, 1); generic_stub->call_unary( cli_buf, "method2", - [method = "method2"](::grpc::ByteBuffer&, ::grpc::Status& status) { - validate_generic_reply(method, status); - }, + [method = "method2"](::grpc::ByteBuffer&, ::grpc::Status& status) { validate_generic_reply(method, status); }, 1); generic_stub->call_unary( cli_buf, "method_unknown",