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

Allow import of multiple signing certs #2112

Merged
merged 11 commits into from
Dec 23, 2020
Merged

Conversation

rist
Copy link
Contributor

@rist rist commented Dec 7, 2020

Was using wrong syntax and therefore failed

Resolves #2109

Short description 📝

Allow import of multiple certificates - before this PR only the first certificate was successfully imported. The command to check if a certificate was already supported used some unsupported/unknown arguments and therefore failed as soon as the keychain was not empty.

Solution 📦

Use correct syntax to check for the presence of a public key in the keychain. Was unable to find a mechanism to look up private keys (It seems to be possible only by name, but was unable to extract that name from the private key) but found out that when importing the same private key multiple times keychain handles that and only keeps one entry.

Was using wrong syntax and therefore failed
@rist
Copy link
Contributor Author

rist commented Dec 7, 2020

first step is to fix import of public keys.
Private key import is not working yet

@fortmarek fortmarek self-requested a review December 7, 2020 08:16
@rist
Copy link
Contributor Author

rist commented Dec 9, 2020

Looked more into the issue of private keys:
It seems the only workable solution to look up private keys in the keychain is using their friendly name. In my first use cases the friendly name of the key (e.g. "Some Organisation") was basically a substring of the certificate name ("Apple Development: Some Organisation (1234)". But in my real life use case (7 different subsidiaries managing their AppStore account and certificates) I ran into cases where the friendly name of a private key was the name of the account manager.
I also tried to extract the friendly name of the p12 file but digging into to that using the openssl command I never found that friendly name (which in fact is visible in Keychain Assistant).

But then I found a much easier solution - it seems Keychain Access in fact handles duplicate imports of keys and either ignores them or replaces the old instance with a new one.

One last thing was that I had one issue where the name extracted from the Subject of the public key had some encoding issues (\\xC3\\xA4 instead of ä) so I added a helper method cleaning this up. No idea how widespread this problem is but I had one case.

Copy link
Member

@fortmarek fortmarek left a comment

Choose a reason for hiding this comment

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

PR looks great, left just some minor comments 🎉

Sources/TuistSigning/Certificate/CertificateParser.swift Outdated Show resolved Hide resolved
Sources/TuistSigning/SecurityController.swift Outdated Show resolved Hide resolved
Sources/TuistSigning/SecurityController.swift Show resolved Hide resolved
features/generate-1.feature Show resolved Hide resolved
@fortmarek
Copy link
Member

I'd wait for the main branch to be green (although it is not something you can fix in this PR), but otherwise no more comments, thanks!

@fortmarek fortmarek merged commit 47e9850 into main Dec 23, 2020
@danieleformichelli danieleformichelli deleted the bugfix/2109ImportMultipleCerts branch July 22, 2021 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only first matched signing certificate is imported
3 participants