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

[fastlane_core] fix WWDR certificates import flow #21442

Merged

Conversation

PaulTaykalo
Copy link
Contributor

@PaulTaykalo PaulTaykalo commented Aug 7, 2023

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.

Motivation and Context

When fastlane installs the WWDR certificate it performs it in next steps:

  • Download certificate (i.e. AppleWWDRCAG6.cer)
  • Add random suffix (i.e AppleWWDRCAG6.cer20230109-941-zd7vp8)
  • Try to import certificate security import AppleWWDRCAG6.cer20230109-941-zd7vp8

The issue what we found is that macOS security tool would treat some extensions as a part of expected format
So, when generated extension contains strings like p8 or p7 and also contains - symbol, it treats it as a fromat type
This means, that if generated suffix will contain these strings, security won't be able to import a certificate, even if is valid one.

Here are some examples (with the same contents)

Crt Name Status Error
tmp.cer
tmpp8.cer
tmp.cerp8 security: SecKeychainItemImport: Unknown format in import.
tmp.cerXXXXXp8XXX security: SecKeychainItemImport: Unknown format in import.

Resolves #20960, #5259

Description

Instead of generating suffix to the extension, suffix to the filename is generated instead:

Name Value
Previous tmp.cerDDDDDD-XXXX-RANDOM
Current tmpDDDDDD-XXXX-RANDOM.cer

Testing Steps

Funny calculations 🎲

  • There are 2176782336 random suffixes (6 symbols, a-z0-9 (36 values)
  • There are 8398080 strings with p8 in them
  • There is 8398080/2176782336 = 0,00385802 probability to fail (~0,3858 %) when importing 1 cert
  • We have about 2,29 % probability when importing 6 of them

Thanks

  • @nekrich for the prepared code and fixed tests :)

@nekrich nekrich mentioned this pull request Aug 7, 2023
4 tasks
@PaulTaykalo PaulTaykalo force-pushed the fix/wwdr-installer-certificate branch 2 times, most recently from b5546d0 to 1d0bb37 Compare August 7, 2023 13:02
Copy link

@GregoryMaks GregoryMaks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

@nekrich
Copy link
Contributor

nekrich commented Aug 8, 2023

This is a quick bug-fix PR we use in our fork.

I found a long time not merged PR #21273 that covers this issue too and additionally adds expiration date checks to the added certificates.

Copy link
Member

@KrauseFx KrauseFx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, nice catch! 💪

@majelbstoat
Copy link

Very nice. Reminds me of this: https://medium.engineering/the-unluckiest-paragraphs-751dd36d2d30#6bb3

(Thanks for the fix. Bit me for the first time today, so great timing!)

@ThomasD-WL
Copy link

Thanks for the fix, it's really useful !

Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@rogerluan
Copy link
Member

Really nice catch @PaulTaykalo 💪 thanks for this!

@rogerluan rogerluan changed the title Fix WWDR certificates import flow [fastlane_core] fix WWDR certificates import flow Aug 12, 2023
@rogerluan rogerluan merged commit 239b738 into fastlane:master Aug 12, 2023
8 checks passed
@numandev1
Copy link

when it will release?

@rogerluan
Copy link
Member

@numandev1 we don't have a release cadency but it'll be released in the next release for sure. Not sure when that will take place though, I'll try to make it happen in the next week or so.

Meanwhile, this code lives in master already so you can use it in your environment by updating your Gemfile:

gem 'fastlane', git: 'https://github.com/fastlane/fastlane.git', branch: 'master'

And run bundle install to apply the changes.

bjornoleh added a commit to bjornoleh/iAPS that referenced this pull request Aug 26, 2023
gem 'fastlane', git: 'https://github.com/fastlane/fastlane.git', branch: 'master'

copied from fastlane/fastlane/pull/21442#issuecomment-1681443895
@iOSGeekster
Copy link
Contributor

This is a quick bug-fix PR we use in our fork.

I found a long time not merged PR #21273 that covers this issue too and additionally adds expiration date checks to the added certificates.

Cool! Then merging this into #21273 should be a breeze

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.

Could not install WWDR certificate
9 participants