Skip to content

Conversation

jenskuhrjorgensen
Copy link
Contributor

@jenskuhrjorgensen jenskuhrjorgensen commented Dec 17, 2024

This PR makes it possible to run unlock_keychain with add_to_search_list: :replace without an existing default keychain.

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

It is currently not possible to run unlock_keychain with add_to_search_list: :replace if you don't have an existing default keychain. Even when also providing set_default: true. For us this currently means that we have to first run create_keychain with default_keychain: true in order to create a temporary default dummy keychain and then unlock the actual keychain that we want to use with unlock_keychain and then finally remove the unused dummy keychain with delete_keychain.

Description

This PR wraps the read of the original/existing default keychain with a begin-rescue in order to make sure that it doesn't crash when no default keychain is set.

I can add that the reading of the original default keychain is stored in SharedValues::ORIGINAL_DEFAULT_KEYCHAIN but that it is undocumented for the unlock_keychain action (it is however documented for the create_keychain task https://docs.fastlane.tools/actions/create_keychain/#create_keychain).

I tried adding some tests for this (inside fastlane/spec/actions_specs/unlock_keychain_spec.rb) but I'm a rookie Ruby developer so I didn't manage to make it work because the test wouldn't fail even though I didn't have a default keychain set.

Finally, I can add that a lot of tests (bundle exec rspec) fail if you don't have a default keychain set. Also in master of the fastlane repo without my changes from this PR.

Testing Steps

This can be tested by removing the default keychain on your machine:

# Get default keychain
security default-keychain

# Delete default keychain
security delete keychain <default keychain from previous command>

# Validate that no default keychain is set
security default-keychain

and then run a simple lane that creates and unlocks a keychain:

lane :test do
  create_keychain(
    name: "Temporary_keychain",
    password: "abcd1234",
    default_keychain: false
  )

  unlock_keychain( # Unlock an existing keychain and add it to the keychain search list
    path: "Temporary_keychain",
    password: "abcd1234",
    add_to_search_list: :replace,
    set_default: true
  )
end

Note: To set your login keychain as the default again, run security default-keychain -s <default keychain from the first command you ran>, probably something like security default-keychain -s /Users/<your-username>/Library/Keychains/login.keychain-db.

If there is no default keychain, the output will now look like this:

...
[13:26:50]: -----------------------------
[13:26:50]: --- Step: unlock_keychain ---
[13:26:50]: -----------------------------
[13:26:50]: Reading existing default keychain 
[13:26:50]: Unlocking keychain at path: /Users/***/Library/Keychains/Temporary_keychain-db

Instead of failing with:

[13:22:02]: Called from Fastfile at line 383
[13:22:02]: ```
[13:22:02]:     381:	  )
[13:22:02]:     382:
[13:22:02]:  => 383:	  unlock_keychain( # Unlock an existing keychain and add it to the keychain search list
[13:22:02]:     384:	    path: "Temporary_keychain",
[13:22:02]:     385:	    password: "abcd1234",
[13:22:02]: ```
[13:22:02]: Shell command exited with exit status 1 instead of 0.

@jenskuhrjorgensen jenskuhrjorgensen changed the title Rescue if no default keychain is set unlock_keychain without having a default keychain Dec 17, 2024
@jenskuhrjorgensen
Copy link
Contributor Author

Hi @revolter
Do you have a chance to take a look at this one or if there is someone better suited, point me in the direction of that someone? :)

@jenskuhrjorgensen
Copy link
Contributor Author

Hi @mollyIV
Maybe you can have a look at this small pr? 🙏

Copy link
Collaborator

@revolter revolter left a comment

Choose a reason for hiding this comment

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

Unfortunately, this is way beyond me 🙈

But the changes themselves seem pretty safe, and the description it quite thorough (except this outdated part).

As such, I'll approve this, and ask for help from more knowledgeable people.

@revolter revolter changed the title unlock_keychain without having a default keychain [unlock_keychain] fix crash when not having a default keychain Feb 1, 2025
@jenskuhrjorgensen
Copy link
Contributor Author

Thanks for the review, @revolter. I updated the outdated PR description.

UI.message("Reading existing default keychain")
Actions.lane_context[Actions::SharedValues::ORIGINAL_DEFAULT_KEYCHAIN] = Fastlane::Actions.sh("security default-keychain").strip
rescue => e
raise unless e.message.include?("security: SecKeychainCopyDefault: A default keychain could not be found.")
Copy link
Member

Choose a reason for hiding this comment

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

Is there another way we could recognize this error besides checking the error message? Maybe the status code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @mollyIV
That would've been nice, but when I print the exception it just prints this

error : Exit status of command 'security default-keychain' was 1 instead of 0.
security: SecKeychainCopyDefault: A default keychain could not be found.

I don't know if "1" is a unique error code for this case and therefore I added a check for the error message instead. Maybe you know if we can just check for error code? Also, I don't know how to actually get that error code from the e object in begin-rescue.

Here is some documentation on what I believe the security default-keychain command calls:
https://developer.apple.com/documentation/security/seckeychaincopydefault(_:)?language=objc

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks for the explanation. Not sure if the status code 1 is the right on, sounds pretty generic. I think checking error description in this case will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mollyIV I agree. Thanks a lot for the review!

@mollyIV mollyIV merged commit b626fb1 into fastlane:master Feb 7, 2025
3 checks passed
@jenskuhrjorgensen
Copy link
Contributor Author

Hi @mollyIV and @revolter

Is there any plan on when the next release of fastlane (containing this PR) will be out?

Best regards
Jens

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.

3 participants