Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

nekrich
Copy link
Contributor

@nekrich nekrich commented Jan 16, 2024

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

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 nekrich changed the title [match] select cert with the most furthest expiration date in the future [wip] [match] select cert with the most furthest expiration date in the future Jan 16, 2024
@nekrich nekrich changed the title [wip] [match] select cert with the most furthest expiration date in the future [match] select cert with the most furthest expiration date in the future Jan 16, 2024
@nekrich nekrich marked this pull request as ready for review January 16, 2024 12:25
@nekrich
Copy link
Contributor Author

nekrich commented Jan 16, 2024

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

@lacostej
Copy link
Collaborator

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?

@nekrich
Copy link
Contributor Author

nekrich commented Jan 16, 2024

I'm good with both options )

@nekrich
Copy link
Contributor Author

nekrich commented Jan 16, 2024

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

@nekrich
Copy link
Contributor Author

nekrich commented Jan 16, 2024

And new PR: #21812

@nekrich nekrich marked this pull request as draft February 8, 2024 00:16
@nekrich
Copy link
Contributor Author

nekrich commented Feb 8, 2024

Hi, @rogerluan and @lacostej. We are experiencing some issues with encryption. I don't know at this moment if it's this PR, or #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 😥.

@lacostej
Copy link
Collaborator

lacostej commented Feb 9, 2024

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

@lacostej
Copy link
Collaborator

lacostej commented Feb 9, 2024

@nekrich so to summarize.

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.

@nekrich
Copy link
Contributor Author

nekrich commented Feb 9, 2024

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

@rogerluan
Copy link
Member

I've just merged #21691 — to proceed with this PR, could you rebase the PR, and revert the changes in #21691? So this PR ends up being a "revert the revert + fix the issue" 😅

@StefanWallin
Copy link

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

@bokanechase-digistorm
Copy link

bokanechase-digistorm commented Apr 29, 2024

@nekrich looking forward to this feature 😄 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants