fastlane icon indicating copy to clipboard operation
fastlane copied to clipboard

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

Open nekrich opened this issue 1 year ago • 13 comments

Checklist

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

Motivation and Context

In most cases, there is only one cert/key pair in the match storage. But when dealing with developer_id certificates, there could be two or even three. The reason for this is that developer ID certs have a 5-year lifespan. And it looks like an app signed with developer_id cert stops working when the developer id cert used to sign it is expired. Every couple of years, we issue a new developer id cert and use it to sign new app versions. In this case, we are sure that the latest release will work for a couple more years, and users will have plenty of time to update the app.

Since the developer id cert is not expired we use manual import. After that, we have two cert/key pairs in the match storage.

The problem I want to address is that the match uses a cert with the biggest id (filename). And this is wrong. We need a match to select a cert with the furthest expiration date in the future.

Description

  1. select_cert_or_key is now select_cert. The method returns the cert with the furthest expiration date.
  2. New runner_unit_spec not to mix unit tests with looks like integration tests in the runner_spec.
  3. There could be several cert/key pairs in the storage. Refactored cert deletion logic to address this case. There is no value in expired certs at all.

Cert renewal updates

  1. cert_paths contain only valid certificates. It looks better and requires fewer checks in the code afterward.
  2. There is no need for the renew_expired_certs option 😅.

Other improvements

  1. There is no need in keys variable. We need only certs with corresponding p12 keys.
  2. Updates some gsubs to use regex, indicating the correct file extension and the end of a string.

PS. Looks like failing tests on Ubuntu is not related to my changes.

Testing Steps

Run the match with storage having 2 or more certs of the same type. Run the match with storage having 2 or more certs that are expired.

nekrich avatar Jan 16 '24 08:01 nekrich

@rogerluan, I would appreciate feedback on this. At some point, it slightly changes the renewal certs behavior introduced in #21691.

nekrich avatar Jan 16 '24 12:01 nekrich

Well if that removes the just freshly added renew_expired_certs option, we need to either remove the previous PR before making a new release, or delay the release until we land this one, right?

lacostej avatar Jan 16 '24 15:01 lacostej

I'm good with both options )

nekrich avatar Jan 16 '24 15:01 nekrich

I can create a small PR that removes the option, but the logic stays the same. Give me a couple of mins

nekrich avatar Jan 16 '24 15:01 nekrich

And new PR: #21812

nekrich avatar Jan 16 '24 15:01 nekrich

Hi, @rogerluan and @lacostej. We are experiencing some issues with encryption. I don't know at this moment if it's this PR, or https://github.com/fastlane/fastlane/pull/21691. It's a double encryption or no encryption at all, idk. I think it's better to revert to #21691, and I'll be able to return back to this PR in 2 weeks. I'll try to make it faster, but no promises. Sorry for the bother 😥.

nekrich avatar Feb 08 '24 00:02 nekrich

@nekrich what encryption issues are you running into? Could it be related to the encryption changes I made since the last release?

lacostej avatar Feb 09 '24 20:02 lacostej

@nekrich so to summarize.

  • you are having an encryption issure
  • #21812 disables the newly introduced feature, but keep most of the code in.
  • you are assuming that this one needs more work

and you would like #21812 to be merged while you fix this one.

The plan sounds OK, but I would really like to know more about the encryption failures you are having, and make sure they are not related to other changes.

lacostej avatar Feb 09 '24 20:02 lacostej

@lacostej I'll be able to look only after Feb 20. I'll definitely describe what's wrong.

nekrich avatar Feb 09 '24 20:02 nekrich

I've just merged https://github.com/fastlane/fastlane/pull/21691 — to proceed with this PR, could you rebase the PR, and revert the changes in https://github.com/fastlane/fastlane/pull/21691? So this PR ends up being a "revert the revert + fix the issue" 😅

rogerluan avatar Feb 10 '24 13:02 rogerluan

Stumbled upon this and just want to let you know we've passed Feb 20 now :)

StefanWallin avatar Apr 09 '24 14:04 StefanWallin

@nekrich looking forward to this feature 😄 🙏

bokanechase-digistorm avatar Apr 29 '24 06:04 bokanechase-digistorm