Skip to content

Fix keychain deletion in multi-certificate workflows#74

Closed
FelixLisczyk wants to merge 1 commit intoApple-Actions:masterfrom
FelixLisczyk:pl-78
Closed

Fix keychain deletion in multi-certificate workflows#74
FelixLisczyk wants to merge 1 commit intoApple-Actions:masterfrom
FelixLisczyk:pl-78

Conversation

@FelixLisczyk
Copy link
Copy Markdown
Contributor

@FelixLisczyk FelixLisczyk commented Apr 7, 2025

Problem Description

When import-codesign-certs@v5 is used multiple times in the same workflow, the same keychain is deleted more than once during the post-job cleanup phase. This results in workflow failures, as the second deletion attempt leads to the following error:

security: SecKeychainDelete: The specified keychain could not be found.
Error: The process '/usr/bin/security' failed with exit code 50

This issue occurs when multiple certificates, such as development and distribution certificates, are imported in separate steps using the same keychain.

Changes Made

  • Added a check to only attempt keychain deletion if it was created by the current action instance.

Example Workflow

This PR fixes issues with workflows structured like:

- name: Import Development Certificate
  id: create_keychain
  uses: apple-actions/import-codesign-certs@v5
  with:
    p12-file-base64: ${{ inputs.p12-file-development }}
    p12-password: ${{ inputs.p12-file-development-password }}
- name: Import Distribution Certificate
  uses: apple-actions/import-codesign-certs@v5
  with:
    create-keychain: false
    keychain-password: "${{ steps.create_keychain.outputs.keychain-password }}"
    p12-file-base64: ${{ inputs.p12-file-distribution }}
    p12-password: ${{ inputs.p12-file-distribution-password }}

Notes

I know it's possible to import multiple certificates at once, as mentioned in the README.md. However, I prefer to keep the certificate files separate since they are also used in other locations.

@daveisfera
Copy link
Copy Markdown
Collaborator

daveisfera commented Apr 11, 2025

I'd prefer to not add this complexity because you can either import multiple certificates with a single action or specify a name using p12-filepath in the second action.

@daveisfera daveisfera closed this Apr 11, 2025
@FelixLisczyk
Copy link
Copy Markdown
Contributor Author

FelixLisczyk commented Apr 12, 2025

Thanks for reviewing my PR. The main issue I'm addressing is that the action currently deletes keychains during cleanup regardless of whether it created them or not. This causes problems when using the action multiple times in a workflow (cleanup fails after the first use) and when users specify custom keychains they don't want deleted.

While merging certificates into a single file works as a workaround, I believe the action itself should handle this logic - only deleting keychains it has created. I've tried using a filepath in the second action, but the cleanup step still fails because the keychain was already deleted.

@daveisfera
Copy link
Copy Markdown
Collaborator

daveisfera commented Apr 13, 2025

Can you help me understand the use case for having separate actions rather than a single with multiple exported together?

Also, if we're going to support this use case, then I'd rather use the existing create-keychain and not do the delete when it's set to false.

daveisfera
daveisfera reviewed Apr 13, 2025
View reviewed changes
@FelixLisczyk
Copy link
Copy Markdown
Contributor Author

FelixLisczyk commented Apr 14, 2025

Thanks for your feedback. To explain my use case: I maintain multiple repos where I store development and distribution certificates as separate secrets. This separation is intentional - some workflows need only one certificate type, and they expire at different times, so combining them creates unnecessary maintenance overhead when updating.

Regarding implementation, I think storing the createKeychain input as state in the run method and checking it during cleanup would work well. Does that approach sound reasonable to you, or did you have something else in mind?

@FelixLisczyk
Copy link
Copy Markdown
Contributor Author

FelixLisczyk commented May 31, 2025

Hi @daveisfera,

Just wanted to check in and see if there are any updates or feedback on this PR. Please let me know if there’s anything I can clarify or improve. Thanks!

@daveisfera
Copy link
Copy Markdown
Collaborator

daveisfera commented Jun 3, 2025

Would having keychain use a unique name solve the problem?

I'd prefer to solve the problem with an existing input or changing a default behavior, rather than adding a new input, if possible.

@FelixLisczyk
Copy link
Copy Markdown
Contributor Author

FelixLisczyk commented Jun 4, 2025

Would having keychain use a unique name solve the problem?

Yes, I can work around this issue by using separate keychains.

I'd prefer to solve the problem with an existing input or changing a default behavior, rather than adding a new input, if possible.

This PR does not add a new input; it only changes the behavior of the existing create-keychain input.

But I am fine with using different keychain names. Thanks!

@daveisfera
Copy link
Copy Markdown
Collaborator

daveisfera commented Jun 5, 2025

I'd prefer to solve the problem with an existing input or changing a default behavior, rather than adding a new input, if possible.

This PR does not add a new input; it only changes the behavior of the existing create-keychain input.

Doh! Can you change the base branch to main or submit a new PR with that being the base?

@FelixLisczyk FelixLisczyk mentioned this pull request Jun 7, 2025
Merged
@FelixLisczyk
Copy link
Copy Markdown
Contributor Author

FelixLisczyk commented Jun 7, 2025

Sure, I've resubmitted this PR (#100).

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

Reviewers

@daveisfera daveisfera daveisfera left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@FelixLisczyk @daveisfera