Merge pull request #120 from aws/cf_improvements

fix: improvements to credentials-fetcher codestyle
aws · Apr 9, 2024 · 5580dd1 · 5580dd1
2 parents 715fd78 + c4f0bd9
commit 5580dd1
Show file tree

Hide file tree

Showing 6 changed files with 54 additions and 35 deletions.
diff --git 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
@@ -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
@@ -37,7 +37,7 @@
 class CredentialsFetcherClient
 {
   public:
-    CredentialsFetcherClient( std::shared_ptr<grpc::Channel> channel )
+    explicit CredentialsFetcherClient( std::shared_ptr<grpc::Channel> 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<std::string>  credspec_contents_arns_domainless = {"arn:aws:s3:::gmsacredspec/gmsa-cred-spec.json"};
-    std::list<std::string> 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
@@ -6,6 +6,7 @@
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <regex>
+#include <openssl/crypto.h>
 
 // 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<char> 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<int, std::string> get_machine_principal( std::string domain_nam
 {
     std::pair<int, std::string> result;
 
-    std::pair<int, std::string> 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<int, std::string> 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<int, std::string> 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<std::string> 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();
+}
diff --git 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<creds_fetcher::krb_ticket_info*> krb_ticket_info_list,
                           std::string lease_id, std::string krb_files_dir );
 
+
 #endif // _daemon_h_
diff --git 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