From c4f0bd938bc1d62a67c04969f1e84e5d7524b414 Mon Sep 17 00:00:00 2001 From: EC2 Default User Date: Sat, 6 Apr 2024 00:12:12 +0000 Subject: [PATCH] fix: improvements to credentials-fetcher codestyle --- CMakeLists.txt | 2 +- api/src/gmsa_service.cpp | 11 ++++--- api/tests/gmsa_test_client.cpp | 8 ++--- auth/kerberos/src/krb.cpp | 54 ++++++++++++++++++++++++++-------- common/daemon.h | 5 ++-- suppressions.txt | 9 ------ 6 files changed, 54 insertions(+), 35 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index bbd3f875..316e6701 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,5 +1,5 @@ cmake_minimum_required(VERSION 3.10) -project(credentials-fetcher VERSION 1.2.0) +project(credentials-fetcher VERSION 1.3.6) include(GNUInstallDirs) diff --git a/api/src/gmsa_service.cpp b/api/src/gmsa_service.cpp index 4f123cbb..5cb2ee6d 100644 --- a/api/src/gmsa_service.cpp +++ b/api/src/gmsa_service.cpp @@ -155,8 +155,8 @@ class CredentialsFetcherImpl final * @param unix_socket_dir: path to unix domain socket * @param cf_logger : log to systemd */ - void RunServer( std::string& unix_socket_dir, std::string& krb_files_dir, - creds_fetcher::CF_logger& cf_logger, std::string& aws_sm_secret_name ) + void RunServer( const std::string& unix_socket_dir, const std::string& krb_files_dir, + creds_fetcher::CF_logger& cf_logger, const std::string& aws_sm_secret_name ) { std::string unix_socket_address = std::string( "unix:" ) + unix_socket_dir + "/" + std::string( UNIX_SOCKET_NAME ); @@ -846,7 +846,6 @@ class CredentialsFetcherImpl final std::string region = renew_krb_arn_request_.region(); std::string username = ""; std::string password = ""; - std::string domain = ""; std::string err_msg; if ( !accessId.empty() && !secretKey.empty() && !sessionToken.empty() && @@ -913,7 +912,7 @@ class CredentialsFetcherImpl final username = std::get<0>( userCreds ); password = std::get<1>( userCreds ); - domain = std::get<2>( userCreds ); + std::string domain = std::get<2>( userCreds ); if ( isValidDomain( domain ) && !contains_invalid_characters_in_ad_account_name( @@ -2531,8 +2530,8 @@ bool check_file_size_s3(std::string s3_arn, std::string region, return false; } long objLen = outcome.GetResult().GetContentLength(); - //value should be less than 2000 bytes - if(objLen > 2000) + //value should be less than 4000 bytes + if(objLen > 4000) { return false; } diff --git a/api/tests/gmsa_test_client.cpp b/api/tests/gmsa_test_client.cpp index e825761e..47d1d7c5 100644 --- a/api/tests/gmsa_test_client.cpp +++ b/api/tests/gmsa_test_client.cpp @@ -37,7 +37,7 @@ class CredentialsFetcherClient { public: - CredentialsFetcherClient( std::shared_ptr channel ) + explicit CredentialsFetcherClient( std::shared_ptr channel ) : _stub{ credentialsfetcher::CredentialsFetcherService::NewStub( channel ) } { } @@ -48,7 +48,6 @@ class CredentialsFetcherClient */ std::string HealthCheckMethod( std::string service_name ) { - std::string result; // Prepare request credentialsfetcher::HealthCheckRequest request; request.set_service( service_name ); @@ -675,7 +674,6 @@ int main( int argc, char** argv ) std::string username; std::string password; std::string domain; - std::string is_renewal; std::string accessId; std::string secretkey; std::string sessionToken; @@ -719,7 +717,6 @@ int main( int argc, char** argv ) std::list credspec_contents_arns_domainless = {"arn:aws:s3:::gmsacredspec/gmsa-cred-spec.json"}; - std::list lease_ids = {"12345", "34567", "45678"}; // create and delete krb tickets if ( argc == 1 ) @@ -749,6 +746,9 @@ int main( int argc, char** argv ) std::cout << "client tests successful" << std::endl; return EXIT_SUCCESS; } + //These methods are added only to test a specific flow, not unit testable + retrieve_credspec_from_s3_test(); + retrieve_credspec_from_secrets_manager_test(); } else if ( arg == "--check" ) { diff --git a/auth/kerberos/src/krb.cpp b/auth/kerberos/src/krb.cpp index 06f0f643..944343c1 100644 --- a/auth/kerberos/src/krb.cpp +++ b/auth/kerberos/src/krb.cpp @@ -6,6 +6,7 @@ #include #include #include +#include // renew the ticket 1 hrs before the expiration #define RENEW_TICKET_HOURS 1 @@ -14,6 +15,9 @@ // https://learn.microsoft.com/en-us/troubleshoot/windows-server/identity/naming-conventions-for-computer-domain-site-ou #define HOST_NAME_LENGTH_LIMIT 15 #define ENV_CF_GMSA_OU "CF_GMSA_OU" +static const std::vector invalid_characters = { + '&', '|', ';', ':', '$', '*', '?', '<', '>', '!',' ', '\\', '.',']', '[', '+', '\'', '`', + '~', '}', '{', '"', ')', '('}; static const std::string install_path_for_decode_exe = "/usr/sbin/credentials_fetcher_utf16_private.exe"; @@ -82,10 +86,11 @@ static std::pair get_machine_principal( std::string domain_nam { std::pair result; - std::pair hostname_result = exec_shell_cmd( "hostname -s | tr -d '\n'" ); - if ( hostname_result.first != 0 ) + char hostname[HOST_NAME_MAX]; + int status = gethostname(hostname, HOST_NAME_MAX); + if ( status ) { - result.first = hostname_result.first; + result.first = status; return result; } @@ -113,7 +118,8 @@ static std::pair get_machine_principal( std::string domain_nam return result; } - std::string host_name = hostname_result.second; + std::string s = std::string(hostname); + std::string host_name = s.substr(0, s.find('.')); // truncate the hostname to the host name size limit defined by microsoft if(host_name.length() > HOST_NAME_LENGTH_LIMIT){ @@ -122,8 +128,8 @@ static std::pair get_machine_principal( std::string domain_nam __func__, __LINE__ ); host_name = host_name.substr(0,HOST_NAME_LENGTH_LIMIT); std::cout << getCurrentTime() << '\t' << "INFO: hostname exceeds 15 characters this can " - "cause problems in getting kerberos tickets, please reduce hostname length" << - std::endl; + "cause problems in getting kerberos tickets, " + "please reduce hostname length" << std::endl; } /** @@ -319,8 +325,8 @@ int get_domainless_user_krb_ticket( std::string domain_name, std::string usernam kinit_argv[1] = (char *)username.c_str(); kinit_argv[2] = (char *)password.c_str(); ret = my_kinit_main(2, kinit_argv); - username = "xxxx"; - password = "xxxx"; + clearString(username); + clearString(password); //TODO: nit - return pair later return ret; @@ -868,7 +874,7 @@ bool is_ticket_ready_for_renewal( creds_fetcher::krb_ticket_info* krb_ticket_inf char renewal_date[80]; char renewal_time[80]; - sscanf( renewal_date_time.c_str(), "%s %s", renewal_date, renewal_time ); + sscanf( renewal_date_time.c_str(), "%79s %79s", renewal_date, renewal_time ); renew_until = std::string( renewal_date ) + " " + std::string( renewal_time ); // trim extra spaces @@ -999,7 +1005,7 @@ std::string renew_gmsa_ticket( creds_fetcher::krb_ticket_info* krb_ticket, std:: krb_cc_name, cf_logger ); if ( gmsa_ticket_result.first != 0 ) { - if ( num_retries == 0 ) + if ( i == 0 ) { cf_logger.logger( LOG_WARNING, "WARNING: Cannot get gMSA krb ticket " @@ -1118,7 +1124,7 @@ std::vector delete_krb_tickets( std::string krb_files_dir, std::str std::string retrieve_secret_from_ecs_config(std::string ecs_variable_name) { - const char* ecs_config_file_name = "/etc/ecs/ecs.config"; // TBD:: Add commandline if needed + const char* ecs_config_file_name = "/etc/ecs/ecs.config"; std::ifstream config_file( ecs_config_file_name ); std::string line; @@ -1127,9 +1133,16 @@ std::string retrieve_secret_from_ecs_config(std::string ecs_variable_name) while ( std::getline( config_file, line ) ) { results = split_string(line, '='); + + if(results.empty() || results.size() != 2) + { + std::cout << getCurrentTime() << '\t' << "invalid configuration format" << std::endl; + return ""; + } + std::string key = results[0]; std::string value = results[1]; - if ( ecs_variable_name.compare( key ) == 0 ) + if ( !contains_invalid_characters_in_credentials(value) && ecs_variable_name.compare(key)==0 ) { value.erase( std::remove( value.begin(), value.end(), '"' ), value.end() ); @@ -1139,9 +1152,14 @@ std::string retrieve_secret_from_ecs_config(std::string ecs_variable_name) std::endl; return ""; } - return value; } + else{ + std::cout << getCurrentTime() << '\t' << "invalid configuration provided, either " + "key/value is not in the correct format" << + std::endl; + return ""; + } } return ""; } @@ -1201,3 +1219,13 @@ std::string getCurrentTime() std::string curr_time = std::string(buf); return curr_time; } + +/** clear string **/ +void clearString(std::string& str) { + if (!str.empty()) { + // Use OPENSSL_cleanse to securely clear the memory + OPENSSL_cleanse(&str[0], str.size()); + } + // Clear the string content + str.clear(); +} \ No newline at end of file diff --git a/common/daemon.h b/common/daemon.h index ec5fdaf9..4de02d16 100644 --- a/common/daemon.h +++ b/common/daemon.h @@ -70,8 +70,6 @@ namespace creds_fetcher */ class CF_logger { - /* TBD:: Fill this later */ - public: int log_level = LOG_NOTICE; @@ -223,6 +221,8 @@ int ProcessCredSpecFile(std::string krb_files_dir, std::string credspec_filepath std::string generate_lease_id(); +void clearString(std::string& str); + #if AMAZON_LINUX_DISTRO std::string retrieve_credspec_from_s3(std::string s3_arn, std::string region, Aws::Auth::AWSCredentials credentials, bool test); bool check_file_size_s3(std::string s3_arn, std::string region, @@ -250,4 +250,5 @@ int write_meta_data_json( creds_fetcher::krb_ticket_info* krb_ticket_info, int write_meta_data_json( std::list krb_ticket_info_list, std::string lease_id, std::string krb_files_dir ); + #endif // _daemon_h_ diff --git a/suppressions.txt b/suppressions.txt index 3e60eb76..44757446 100644 --- a/suppressions.txt +++ b/suppressions.txt @@ -7,15 +7,6 @@ missingInclude:* unmatchedSuppression:* ConfigurationNotChecked:* cstyleCast:daemon/src/daemon.cpp -unusedFunction:* -unreadVariable:* -unusedVariable:* -uselessAssignmentArg:* -knownConditionTrueFalse:* passedByValue:* -toomanyconfigs:* -constParameter:* -noExplicitConstructor:* -invalidscanf:auth/kerberos/src/krb.cpp useStlAlgorithm:auth/kerberos/src/krb.cpp useInitializationList:api/src/gmsa_service.cpp \ No newline at end of file