[match][sigh] add option to automatically renew expired certificates …

…(defaults to enabled) (#21691)

* [match]: renew expired certificates

* chore: shell-escape filename when removing files from gi

* chore: don’t add all changes to git if files_to_delete is empty

* chore: better key_path assign redability

* chore: refactor renews an outdated certificate test suite

* chore: rename RENEWABLE_CERT_TYPES to RENEWABLE_CERT_TYPES_VIA_API

* chore: update match/lib/match/options.rb

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

* chore: renew expired certs by default

* chore: fix tests

---------

Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>

match/lib/match/options.rb

@@ -292,6 +292,11 @@ def self.available_options
                                      description: "Renew the provisioning profiles if the certificate count on the developer portal has changed. Works only for the 'development' provisioning profile type. Requires 'include_all_certificates' option to be 'true'",
                                      type: Boolean,
                                      default_value: false),
+        FastlaneCore::ConfigItem.new(key: :renew_expired_certs,
+                                     env_name: "MATCH_RENEW_EXPIRED_CERTS",
+                                     description: "Automatically renew expired certificates. Note: to renew `developer_id` and `developer_id_installer` certificates you must log in with the Account Holder account by using username and password; App Store Connect API key doesn't work in this case",
+                                     type: Boolean,
+                                     default_value: true),
         FastlaneCore::ConfigItem.new(key: :skip_confirmation,
                                      env_name: "MATCH_SKIP_CONFIRMATION",
                                      description: "Disables confirmation prompts during nuke, answering them with yes",

match/lib/match/runner.rb

@@ -19,6 +19,7 @@ module Match
   # rubocop:disable Metrics/ClassLength
   class Runner
     attr_accessor :files_to_commit
+    attr_accessor :files_to_delete
     attr_accessor :spaceship
 
     attr_accessor :storage
@@ -28,6 +29,7 @@ class Runner
     # rubocop:disable Metrics/PerceivedComplexity
     def run(params)
       self.files_to_commit = []
+      self.files_to_delete = []
 
       FileUtils.mkdir_p(params[:output_path]) if params[:output_path]
 
@@ -82,12 +84,12 @@ def run(params)
       end
 
       # Certificate
-      cert_id = fetch_certificate(params: params, working_directory: storage.working_directory)
+      cert_id = fetch_certificate(params: params, renew_expired_certs: params[:renew_expired_certs])
 
       # Mac Installer Distribution Certificate
       additional_cert_types = params[:additional_cert_types] || []
       cert_ids = additional_cert_types.map do |additional_cert_type|
-        fetch_certificate(params: params, working_directory: storage.working_directory, specific_cert_type: additional_cert_type)
+        fetch_certificate(params: params, renew_expired_certs: params[:renew_expired_certs], specific_cert_type: additional_cert_type)
       end
 
       profile_type = Sigh.profile_type_for_distribution_type(
@@ -112,9 +114,10 @@ def run(params)
         end
       end
 
-      if self.files_to_commit.count > 0 && !params[:readonly]
+      has_file_changes = self.files_to_commit.count > 0 || self.files_to_delete.count > 0
+      if has_file_changes && !params[:readonly]
         encryption.encrypt_files if encryption
-        storage.save_changes!(files_to_commit: self.files_to_commit)
+        storage.save_changes!(files_to_commit: self.files_to_commit, files_to_delete: self.files_to_delete)
       end
 
       # Print a summary table for each app_identifier
@@ -152,13 +155,48 @@ def update_optional_values_depending_on_storage_type(params)
       end
     end
 
-    def fetch_certificate(params: nil, working_directory: nil, specific_cert_type: nil)
+    RENEWABLE_CERT_TYPES_VIA_API = [:mac_installer_distribution, :development, :distribution, :enterprise]
+
+    def fetch_certificate(params: nil, renew_expired_certs: false, specific_cert_type: nil)
       cert_type = Match.cert_type_sym(specific_cert_type || params[:type])
 
       certs = Dir[File.join(prefixed_working_directory, "certs", cert_type.to_s, "*.cer")]
       keys = Dir[File.join(prefixed_working_directory, "certs", cert_type.to_s, "*.p12")]
 
-      if certs.count == 0 || keys.count == 0
+      storage_has_certs = certs.count != 0 && keys.count != 0
+
+      # Determine if cert is renewable.
+      # Can't renew developer_id certs with Connect API token. Account holder access is required.
+      is_authenticated_with_login = Spaceship::ConnectAPI.token.nil?
+      is_cert_renewable_via_api = RENEWABLE_CERT_TYPES_VIA_API.include?(cert_type)
+      is_cert_renewable = is_authenticated_with_login || is_cert_renewable_via_api
+
+      # Validate existing certificate first.
+      if renew_expired_certs && is_cert_renewable && storage_has_certs
+        cert_path = select_cert_or_key(paths: certs)
+
+        unless Utils.is_cert_valid?(cert_path)
+          UI.important("Removing invalid certificate '#{File.basename(cert_path)}'")
+
+          # Remove expired cert.
+          self.files_to_delete << cert_path
+          File.delete(cert_path)
+
+          # Key filename is the same as cert but with .p12 extension.
+          key_path = cert_path.gsub(/\.cer$/, ".p12")
+          # Remove expired key .p12 file.
+          if File.exist?(key_path)
+            self.files_to_delete << key_path
+            File.delete(key_path)
+          end
+
+          certs = []
+          keys = []
+          storage_has_certs = false
+        end
+      end
+
+      if !storage_has_certs
         UI.important("Couldn't find a valid code signing identity for #{cert_type}... creating one for you now")
         UI.crash!("No code signing identity found and cannot create a new one because you enabled `readonly`") if params[:readonly]
         cert_path = Generator.generate_certificate(params, cert_type, prefixed_working_directory, specific_cert_type: specific_cert_type)
@@ -318,6 +356,7 @@ def fetch_provisioning_profile(params: nil, profile_type:, certificate_id: nil,
 
       if params[:output_path]
         FileUtils.cp(stored_profile_path, params[:output_path])
+        installed_profile = FastlaneCore::ProvisioningProfile.install(profile, keychain_path)
       end
 
       Utils.fill_environment(Utils.environment_variable_name(app_identifier: app_identifier,

match/lib/match/storage/git_storage.rb

@@ -140,9 +140,10 @@ def human_readable_description
       end
 
       def delete_files(files_to_delete: [], custom_message: nil)
-        # No specific list given, e.g. this happens on `fastlane match nuke`
-        # We just want to run `git add -A` to commit everything
-        git_push(commands: ["git add -A"], commit_message: custom_message)
+        if files_to_delete.count > 0
+          commands = files_to_delete.map { |filename|  "git rm #{filename.shellescape}" }
+          git_push(commands: commands, commit_message: custom_message)
+        end
       end
 
       def upload_files(files_to_upload: [], custom_message: nil)

match/lib/match/storage/interface.rb

@@ -55,11 +55,14 @@ def save_changes!(files_to_commit: nil, files_to_delete: nil, custom_message: ni
         # Custom init to `[]` in case `nil` is passed
         files_to_commit ||= []
         files_to_delete ||= []
+        files_to_delete -= files_to_commit # Make sure we are not removing added files.
+
+        if files_to_commit.count == 0 && files_to_delete.count == 0
+          UI.user_error!("Neither `files_to_commit` nor `files_to_delete` were provided to the `save_changes!` method call")
+        end
 
         Dir.chdir(File.expand_path(self.working_directory)) do
           if files_to_commit.count > 0 # everything that isn't `match nuke`
-            UI.user_error!("You can't provide both `files_to_delete` and `files_to_commit` right now") if files_to_delete.count > 0
-
             if !File.exist?(MATCH_VERSION_FILE_NAME) || File.read(MATCH_VERSION_FILE_NAME) != Fastlane::VERSION.to_s
               files_to_commit << MATCH_VERSION_FILE_NAME
               File.write(MATCH_VERSION_FILE_NAME, Fastlane::VERSION) # stored unencrypted
@@ -74,13 +77,14 @@ def save_changes!(files_to_commit: nil, files_to_delete: nil, custom_message: ni
 
             self.upload_files(files_to_upload: files_to_commit, custom_message: custom_message)
             UI.message("Finished uploading files to #{self.human_readable_description}")
-          elsif files_to_delete.count > 0
+          end
+
+          if files_to_delete.count > 0
             self.delete_files(files_to_delete: files_to_delete, custom_message: custom_message)
             UI.message("Finished deleting files from #{self.human_readable_description}")
-          else
-            UI.user_error!("Neither `files_to_commit` nor `files_to_delete` were provided to the `save_changes!` method call")
           end
         end
+      ensure # Always clear working_directory after save
         self.clear_changes
       end
 

match/spec/fixtures/invalid/certs/distribution/F7P4EE896K.p12

@@ -0,0 +1,24 @@
+-----BEGIN RSA PRIVATE KEY-----
+MIIEogIBAAKCAQEAyTm/v40AZb6I1eXRPVaUF+q683gm+XTRaRC9fjqK3seL117n
+gLte6+YOihzt88v7uJvEP0NN5pFLU4x8v/s+S/VC9Rp2Qd7CZIU1P+LJVWbIjJ31
+HPW9vVPLILbFERgTE8IblCkUa52KLcegTkvpqE/uS+ERXCsQM8FpK2urMHvIisCa
+c2f7O+B/7my+DOaAQaAEqvQtaIxMIIEogIBAAKCAQEAyTm/v40AZb6I1eXRPVaUF+q683gm+XTRaRC9fjqK3seL117n
+gLte6+YOihzt88v7uJvEP0NN5pFLU4x8v/s+S/VC9Rp2Qd7CZIU1P+LJVWbIjJ31
+HPW9vVPLILbFERgTE8IblCkUa52KLcegTkvpqE/uS+ERXCsQM8FpK2urMHvIisCa
+c2f7O+B/7my+DOaAQaAEqvQtaIxMIIEogIBAAKCAQEAyTm/v40AZb6I1eXRPVaUF+q683gm+XTRaRC9fjqK3seL117n
+gLte6+YOihzt88v7uJvEP0NN5pFLU4x8v/s+S/VC9Rp2Qd7CZIU1P+LJVWbIjJ31
+HPW9vVPLILbFERgTE8IblCkUa52KLcegTkvpqE/uS+ERXCsQM8FpK2urMHvIisCa
+c2f7O+B/7my+DOaAQaAEqvQtaIxMIIEogIBAAKCAQEAyTm/v40AZb6I1eXRPVaUF+q683gm+XTRaRC9fjqK3seL117n
+gLte6+YOihzt88v7uJvEP0NN5pFLU4x8v/s+S/VC9Rp2Qd7CZIU1P+LJVWbIjJ31
+HPW9vVPLILbFERgTE8IblCkUa52KLcegTkvpqE/uS+ERXCsQM8FpK2urMHvIisCa
+c2f7O+B/7my+DOaAQaAEqvQtaIxMIIEogIBAAKCAQEAyTm/v40AZb6I1eXRPVaUF+q683gm+XTRaRC9fjqK3seL117n
+gLte6+YOihzt88v7uJvEP0NN5pFLU4x8v/s+S/VC9Rp2Qd7CZIU1P+LJVWbIjJ31
+HPW9vVPLILbFERgTE8IblCkUa52KLcegTkvpqE/uS+ERXCsQM8FpK2urMHvIisCa
+c2f7O+B/7my+DOaAQaAEqvQtaIxMIIEogIBAAKCAQEAyTm/v40AZb6I1eXRPVaUF+q683gm+XTRaRC9fjqK3seL117n
+gLte6+YOihzt88v7uJvEP0NN5pFLU4x8v/s+S/VC9Rp2Qd7CZIU1P+LJVWbIjJ31
+HPW9vVPLILbFERgTE8IblCkUa52KLcegTkvpqE/uS+ERXCsQM8FpK2urMHvIisCa
+c2f7O+B/7my+DOaAQaAEqvQtaIxMIIEogIBAAKCAQEAyTm/v40AZb6I1eXRPVaUF+q683gm+XTRaRC9fjqK3seL117n
+gLte6+YOihzt88v7uJvEP0NN5pFLU4x8v/s+S/VC9Rp2Qd7CZIU1P+LJVWbIjJ31
+HPW9vVPLILbFERgTE8IblCkUa52KLcegTkvpqE/uS+ERXCsQM8FpK2urMHvIisCa
+c2f7O+B/7my+DOaAQaAEqvQtaIx
+-----END RSA PRIVATE KEY-----

match/spec/runner_spec.rb

@@ -77,7 +77,8 @@
               File.join(repo_dir, "something.cer"),
               File.join(repo_dir, "something.p12"), # this is important, as a cert consists out of 2 files
               "./match/spec/fixtures/test.mobileprovision"
-            ]
+            ],
+            files_to_delete: []
           )
 
           spaceship = "spaceship"
@@ -178,13 +179,15 @@
                                                                              type: "appstore")]).to eql("fastlane certificate name")
         end
 
-        it "fails because of an outdated certificate", requires_security: true do
+        it "fails because of an outdated certificate in readonly mode", requires_security: true do
           git_url = "https://github.com/fastlane/fastlane/tree/master/certificates"
           values = {
             app_identifier: "tools.fastlane.app",
             type: "appstore",
             git_url: git_url,
-            username: "flapple@something.com"
+            username: "flapple@something.com",
+            renew_expired_certs: false,
+            readonly: true
           }
 
           config = FastlaneCore::Configuration.create(Match::Options.available_options, values)
@@ -194,36 +197,15 @@
 
           create_fake_cache
 
-          fake_storage = "fake_storage"
-          expect(Match::Storage::GitStorage).to receive(:configure).with({
-            git_url: git_url,
-            shallow_clone: false,
-            skip_docs: false,
-            git_branch: "master",
-            git_full_name: nil,
-            git_user_email: nil,
-            clone_branch_directly: false,
-            git_basic_authorization: nil,
-            git_bearer_authorization: nil,
-            git_private_key: nil,
-            type: config[:type],
-            platform: config[:platform]
-          }).and_return(fake_storage)
-
-          expect(fake_storage).to receive(:download).and_return(nil)
-          expect(fake_storage).to receive(:clear_changes).and_return(nil)
-          allow(fake_storage).to receive(:git_url).and_return(git_url)
-          allow(fake_storage).to receive(:working_directory).and_return(repo_dir)
-          allow(fake_storage).to receive(:prefixed_working_directory).and_return(repo_dir)
+          fake_storage = create_fake_storage(match_config: config, repo_dir: repo_dir)
 
           fake_encryption = "fake_encryption"
           expect(Match::Encryption::OpenSSL).to receive(:new).with(keychain_name: fake_storage.git_url, working_directory: fake_storage.working_directory).and_return(fake_encryption)
           expect(fake_encryption).to receive(:decrypt_files).and_return(nil)
 
           spaceship = "spaceship"
           allow(spaceship).to receive(:team_id).and_return("")
-          expect(Match::SpaceshipEnsure).to receive(:new).and_return(spaceship)
-          expect(spaceship).to receive(:bundle_identifier_exists).and_return(true)
+          expect(Match::SpaceshipEnsure).not_to receive(:new)
 
           expect(Match::Utils).to receive(:is_cert_valid?).and_return(false)
 
@@ -264,7 +246,7 @@
 
           # Utils
           # Ensure match validates stored certificate.
-          expect(Match::Utils).to receive(:is_cert_valid?).with(stored_valid_cert_path).and_return(true)
+          expect(Match::Utils).to receive(:is_cert_valid?).with(stored_valid_cert_path).and_return(true).twice
 
           # Certificates
           # Ensure a new certificate is not generated.
@@ -284,6 +266,96 @@
           # Rely on expectations defined above.
         end
 
+        it "renews an outdated certificate", requires_security: true do
+          # GIVEN
+
+          # Downloaded and decrypted storage location.
+          repo_dir = "./match/spec/fixtures/invalid"
+          #   Invalid cert and key
+          stored_invalid_cert_path = "#{repo_dir}/certs/distribution/F7P4EE896K.cer"
+          stored_invalid_key_path = "#{repo_dir}/certs/distribution/F7P4EE896K.p12"
+
+          #   Valid cert and key
+          new_stored_valid_cert_path = "./match/spec/fixtures/valid/certs/distribution/E7P4EE896K.cer"
+          new_stored_valid_key_path = "./match/spec/fixtures/valid/certs/distribution/E7P4EE896K.p12"
+
+          # match options
+          match_test_options = {
+            renew_expired_certs: true, # Current test suite.
+            skip_provisioning_profiles: true # We test certificate renewal, not profile.
+          }
+          match_config = create_match_config_with_git_storage(extra_values: match_test_options)
+
+          fake_cache = create_fake_cache
+
+          # EXPECTATIONS
+
+          # Storage
+          fake_storage = create_fake_storage(match_config: match_config, repo_dir: repo_dir)
+          begin # Ensure old certificates are removed from the storage and new are added.
+            expect(fake_storage).to receive(:save_changes!).with(
+              files_to_commit: [
+                new_stored_valid_cert_path,
+                new_stored_valid_key_path # this is important, as a cert consists out of 2 files
+              ],
+              files_to_delete: [
+                stored_invalid_cert_path,
+                stored_invalid_key_path
+              ]
+            )
+          end
+
+          # Encryption
+          fake_encryption = create_fake_encryption(storage: fake_storage)
+          # Ensure new files are encrypted.
+          expect(fake_encryption).to receive(:encrypt_files).and_return(nil)
+
+          # Certificate generator
+          # Ensure a new certificate is generated.
+          expect(Match::Generator).to receive(:generate_certificate).with(match_config, :distribution, fake_storage.working_directory, specific_cert_type: nil).and_return(new_stored_valid_cert_path)
+
+          # Spaceship ensure helper
+          spaceship_ensure = create_fake_spaceship_ensure
+          begin # Ensure match checks validity of the new certificate.
+            profile_type = Sigh.profile_type_for_distribution_type(
+              platform: match_config[:platform],
+              distribution_type: match_config[:type]
+            )
+
+            certificates_exists_params = {
+              username: match_config[:username],
+              certificate_ids: ['E7P4EE896K'],
+              cached_certificates: fake_cache.certificates,
+              platform: match_config[:platform],
+              profile_type: profile_type
+            }
+            expect(spaceship_ensure).to receive(:certificates_exists).with(certificates_exists_params).and_return(true)
+          end
+
+          # Utils
+          # Ensure match validates stored certificate and make it invalid for the current test suite.
+          expect(Match::Utils).to receive(:is_cert_valid?).with(stored_invalid_cert_path).and_return(false)
+
+          # File system
+          begin # Ensure old certificates are removed from the file system.
+            expect(File).to receive(:delete).with(stored_invalid_cert_path).and_return(nil)
+            expect(File).to receive(:delete).with(stored_invalid_key_path).and_return(nil)
+          end
+
+          # Extra
+          begin # Ensure profiles are not created, not installed, and not validated.
+            expect(Match::Generator).not_to receive(:generate_provisioning_profile)
+            expect(FastlaneCore::ProvisioningProfile).not_to receive(:install)
+            expect(spaceship_ensure).not_to receive(:profile_exists)
+          end
+
+          # WHEN
+          Match::Runner.new.run(match_config)
+
+          # THEN
+          # Rely on expectations defined above.
+        end
+
         it "skips provisioning profiles when skip_provisioning_profiles set to true", requires_security: true do
           git_url = "https://github.com/fastlane/fastlane/tree/master/certificates"
           values = {
@@ -330,7 +402,8 @@
             files_to_commit: [
               File.join(repo_dir, "something.cer"),
               File.join(repo_dir, "something.p12") # this is important, as a cert consists out of 2 files
-            ]
+            ],
+            files_to_delete: []
           )
 
           spaceship = "spaceship"

match/spec/spec_helper.rb

@@ -12,13 +12,13 @@ def before_each_match
 def create_fake_storage(match_config:, repo_dir:)
   fake_storage = "fake_storage"
   expect(Match::Storage::GitStorage).to receive(:configure).with({
-    git_url: match_config[:git_url],
-    shallow_clone: true,
-    skip_docs: false,
-    git_branch: "master",
+    git_url: match_config[:git_url] || default_git_url,
+    shallow_clone: match_config[:shallow_clone] || false,
+    skip_docs: match_config[:skip_docs] || false,
+    git_branch: match_config[:git_branch] || "master",
     git_full_name: nil,
     git_user_email: nil,
-    clone_branch_directly: false,
+    clone_branch_directly: match_config[:clone_branch_directly] || false,
     git_basic_authorization: nil,
     git_bearer_authorization: nil,
     git_private_key: nil,