[match][sigh] add option to automatically renew expired certificates (defaults to enabled) #21691

nekrich · 2023-12-04T14:00:46Z

Checklist

I've run bundle exec rspec from the root directory to see all new and existing tests pass
I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
I see several green ci/circleci builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)
I've read the Contribution Guidelines
I've updated the documentation if necessary.
I've added or updated relevant unit tests.

Motivation and Context

Motivation: Automation 😅, and we had one of our certs expired.

Current flow on expired certificate:

Nuke or remove certs and provisioning profiles manually from the storage. Which is scary.
Remove all provisioning profiles from the portal.

New flow: do nothing, match/sync_code_signing will do all the things for you 🎉.

Description

Check the certificate expiration date for renewable certificates. Remove expired certs from storage.
This behavior is optional, and there is a new parameter renew_expired_certs to turn it on.

Extra profile validation

Provisioning profiles are not invalidated automatically on the dev portal when the certificate expires. They become Invalid only when opened directly in the portal 🤷. We need to do an extra check on the expiration date to ensure the profile is valid.

Git commit of removed files

Commit deletion of only listed deleted files. Encryption modifies all files in git.

Profile installation

I moved the profile installation logic a little bit lower. Now, only valid profiles are installed 🧹.

The working_directory parameter was not used in the fetch_certificate, so I removed it.

Testing Steps

Tests provided: run match with an expired certificate and active provisioning profiles.

match/lib/match/storage/interface.rb

match/lib/match/storage/git_storage.rb

lacostej

that's a nice feature. Seems to fix a few corner cases issues as well.

match/lib/match/runner.rb

lacostej · 2023-12-07T19:07:00Z

match/lib/match/storage/interface.rb

          end
        end
+      ensure # Always clear working_directory after save


lacostej · 2023-12-07T20:01:42Z

spaceship/lib/spaceship/portal/provisioning_profile.rb

@@ -484,7 +484,14 @@ def certificate_valid?
      # @return (Bool) Is the current provisioning profile valid?
      #                To also verify the certificate call certificate_valid?
      def valid?
-        return status == 'Active'
+        # Provisioning profiles are not invalidated automatically on the dev portal when the certificate expires.


Does this mean that this PR also fixes the fact that we were not detecting expired certificates properly before?

Yes 😅. I don't know why Apple does not revoke profiles or make them invalid.
But even in this case, we cannot sign an app since the certificate has expired.

The date is Dec 4, and the expiration date is Dec 1.
The only Expired cert is that I opened it to see why it's not expired, and it expired immediately after opening it.

They didn't bother setting up like a cron job that goes and expire everything like once a day 😂

lacostej · 2023-12-07T20:17:51Z

match/lib/match/runner.rb

@@ -277,6 +311,10 @@ def fetch_provisioning_profile(params: nil, certificate_id: nil, app_identifier:
        return nil
      end

+      if Helper.mac?


does the fact that we move this after the previous block means that we stop installing expired profiles into the keychain?

Yes, I see no point in installing an expired profile, generating a new one, and installing a new one after.

jfy, I did the same in my other PR: https://github.com/fastlane/fastlane/pull/21694/files#diff-8cd09d7ab3f4398e3160f603839ac2891e3cdf60e7f09930a1288ce33ab8fc1fL279

match/lib/match/options.rb

match/spec/runner_spec.rb

match/lib/match/runner.rb

match/lib/match/storage/interface.rb

match/lib/match/storage/git_storage.rb

lacostej · 2023-12-12T19:48:04Z

match/lib/match/runner.rb

@@ -133,13 +136,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 = [:mac_installer_distribution, :development, :distribution, :enterprise]


maybe we should rename this to RENEWABLE_CERT_TYPES_VIA_API?

Yeah, it's for sure will improve readability. Done in eb0db7b.

nekrich · 2023-12-14T14:17:38Z

Rebased, fixed conflicts and updated the test to include new parameters for the certificates_exists stub.

rogerluan

This is a great DX improvement over what we have currently 🎉 thanks for the hard work on this PR @nekrich ! 🙌

Just left some suggestions and comments, nothing blocking 😊

match/lib/match/options.rb

match/spec/spec_helper.rb

spaceship/lib/spaceship/connect_api/models/profile.rb

rogerluan · 2024-01-14T14:57:59Z

spaceship/lib/spaceship/portal/provisioning_profile.rb

@@ -484,7 +484,14 @@ def certificate_valid?
      # @return (Bool) Is the current provisioning profile valid?
      #                To also verify the certificate call certificate_valid?
      def valid?
-        return status == 'Active'
+        # Provisioning profiles are not invalidated automatically on the dev portal when the certificate expires.


They didn't bother setting up like a cron job that goes and expire everything like once a day 😂

rogerluan · 2024-01-14T15:00:21Z

It seems this PR developed conflicts @nekrich, if you could resolve those and my comments above, this PR should be good to go then 💪

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

rogerluan · 2024-01-15T12:28:17Z

Thanks for addressing those changes @nekrich 🙏 I see CI is failing now, could you take a look? 🙇

nekrich · 2024-01-15T12:29:43Z

Yeah, I'm looking into it. It looks like default renew certs affect all the tests.

rogerluan

LGTM 💪 Thanks @nekrich ! 🚀

@lacostej any final thoughts on the recent changes before we merge this in?

lacostej · 2024-01-15T15:56:09Z

LGTM

…21691 and revert the default behavior while we address issues with it (#21812) * [match] remove the renew_expired_certs option and make behaviour default * chore: don’t auto renew cert * chore: remove auto renewal test

nekrich force-pushed the feat/match-renew-expired-cert branch from 0388ecb to e4fc47e Compare December 4, 2023 14:33

nekrich commented Dec 4, 2023

View reviewed changes

match/lib/match/storage/interface.rb Show resolved Hide resolved

nekrich commented Dec 4, 2023

View reviewed changes

match/lib/match/storage/git_storage.rb Outdated Show resolved Hide resolved

nekrich changed the title ~~[match]: renew expired certificates~~ [match] renew expired certificates Dec 4, 2023

nekrich changed the title ~~[match] renew expired certificates~~ [match] add option to automatically renew expired certificates Dec 4, 2023

lacostej reviewed Dec 7, 2023

View reviewed changes

nekrich requested a review from lacostej December 8, 2023 13:39

lacostej reviewed Dec 12, 2023

View reviewed changes

lacostej requested a review from rogerluan December 12, 2023 19:48

nekrich mentioned this pull request Dec 12, 2023

[match] add caching layer to significantly improve performance by up to 100x #21694

Merged

6 tasks

nekrich requested a review from lacostej December 13, 2023 09:58

nekrich force-pushed the feat/match-renew-expired-cert branch from 5758fa2 to 3b859ad Compare December 14, 2023 14:14

nekrich force-pushed the feat/match-renew-expired-cert branch from 3b859ad to 3d8a2d8 Compare December 14, 2023 14:21

lacostej approved these changes Dec 19, 2023

View reviewed changes

rogerluan added tool: match tool: sigh tool: spaceship labels Dec 27, 2023

nekrich mentioned this pull request Jan 3, 2024

[match] fix cache issues in read-only mode #21767

Merged

6 tasks

rogerluan added the status: merge conflicts label Jan 10, 2024

rogerluan approved these changes Jan 14, 2024

View reviewed changes

nekrich added 6 commits January 15, 2024 13:06

[match]: renew expired certificates

6dd6d5f

chore: shell-escape filename when removing files from gi

d18077e

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

8f2dbfe

chore: better key_path assign redability

9976260

chore: refactor renews an outdated certificate test suite

1b5f229

chore: rename RENEWABLE_CERT_TYPES to RENEWABLE_CERT_TYPES_VIA_API

37bc483

nekrich force-pushed the feat/match-renew-expired-cert branch from 3d8a2d8 to 37bc483 Compare January 15, 2024 10:16

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

e7d3822

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

chore: renew expired certs by default

f474c76

rogerluan removed the status: merge conflicts label Jan 15, 2024

chore: fix tests

7f22554

rogerluan approved these changes Jan 15, 2024

View reviewed changes

rogerluan changed the title ~~[match] add option to automatically renew expired certificates~~ [match][sigh] add option to automatically renew expired certificates (defaults to enabled) Jan 15, 2024

rogerluan merged commit d4c0ca4 into fastlane:master Jan 15, 2024
3 checks passed

This was referenced Jan 16, 2024

[match] select cert with the most furthest expiration date in the future #21809

Draft

[match][hotfix] remove the renew_expired_certs option introduced in #21691 and revert the default behavior while we address issues with it #21812

Merged

lacostej mentioned this pull request Feb 2, 2024

[match] doesn't renew expired certificate automatically #21848

Open

4 tasks

joshdholtz mentioned this pull request Apr 4, 2024

Version bump to 2.220.0 #21943

Merged

jboulter11 mentioned this pull request Apr 10, 2024

Provisioning profile expiration comparison compares Time and DateTime #21966

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[match][sigh] add option to automatically renew expired certificates (defaults to enabled) #21691

[match][sigh] add option to automatically renew expired certificates (defaults to enabled) #21691

nekrich commented Dec 4, 2023

lacostej left a comment

lacostej Dec 7, 2023

lacostej Dec 7, 2023

nekrich Dec 8, 2023

rogerluan Jan 14, 2024

lacostej Dec 7, 2023

nekrich Dec 8, 2023

nekrich Dec 8, 2023

lacostej Dec 12, 2023

nekrich Dec 13, 2023

nekrich commented Dec 14, 2023 •

edited

rogerluan left a comment

rogerluan Jan 14, 2024

rogerluan commented Jan 14, 2024

rogerluan commented Jan 15, 2024

nekrich commented Jan 15, 2024

rogerluan left a comment

lacostej commented Jan 15, 2024

[match][sigh] add option to automatically renew expired certificates (defaults to enabled) #21691

[match][sigh] add option to automatically renew expired certificates (defaults to enabled) #21691

Conversation

nekrich commented Dec 4, 2023

Checklist

Motivation and Context

Description

Extra profile validation

Git commit of removed files

Profile installation

Testing Steps

lacostej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nekrich commented Dec 14, 2023 • edited

rogerluan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogerluan commented Jan 14, 2024

rogerluan commented Jan 15, 2024

nekrich commented Jan 15, 2024

rogerluan left a comment

Choose a reason for hiding this comment

lacostej commented Jan 15, 2024

nekrich commented Dec 14, 2023 •

edited