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

Update project to Swift 5, and replaced CodeSignUpdate shell script with a Swift script to do the same thing but allow errors to be seen #28

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

chipjarred
Copy link

I'm sorry for such a large pull request. I should have broken it up into two parts. One for the migration to Swift 5, and the other for the new CodeSignUpdate script written in Swift. I had an earlier pull request but managed to fix some of the needless diffs in it, so I closed it and submitting this new one.

The changes needed for Swift 5 were really minor. Swift 5 warns about there various temporaries pointers being passed to parameters to initializers needing to out-live the initializer call. This was a problem for the calls to the Authorization API. The problem comes down to Swift having different variable life-time standards than, say, C++. Basically a variable is only guaranteed to live until its last use, at which point it might be immediately deinitialized, rather than for the life of the block that it's in. For a fully Swift API that's not a problem, because reference counts keep things alive, or they're actually copied, but the Authorization API is C and uses pointers. So passing in a String or AuthorizationItem by reference to those functions is translated to a pointer to those things, which gets stored away, but then the String or AuthorizationItem it points to might be immediately deinitialized. The solution was just to wrap the calls and the whole block that depends on them with String's withCString method, or with Swift.withUnsafePointer to ensure the pointers remain valid. One could perhaps accomplish the same thing with withExtendedLifetime, but since the API wants pointers anyway that's what I used.

Your CodeSignUpdate shell script is great, much better than manually running SMJobBlessUtil.py -setreq manually. However it had some limitations owing to the fact that it used I/O redirection to build strings. None of the error messages it produced could ever get to the log, but instead were consumed in variable assignment. It would just give a build error stating the script terminated with exit code 1, and no other information. I re-wrote it as a Swift script so that string building doesn't interfere with I/O. Its error messages show up in the build log now. It also allowed me to do a bit more fine-grained error reporting. The biggest improvement regarding the script, which could have been done with the shell script as well, was removing the hard coding of bundle identifiers. They are now provided by exporting them just prior to calling the script in the Run Script phase. That way users of the script don't need to modify the script itself at all. I added a README for the Scripts folder about it.
Of course since it's intended to run as part of the build process, CodeSignUpdate is run as a script rather than compiling it normally.

… CodeSignUpdate.sh with CodeSignUpdate.swift (intended to be run as a script during the Run Script phase, not actually compiled). It does the same job, but since it doesn't need to redirect I/O to build strings, it is can actually produce useful error message that show up in the build log. The shell script did produce some error messages, but they were never displayed because stdout was redirected to shell variable string content. It adds some additional checks too. It removes hardcoding of bundle IDs in the script. They are now provided by preceding the call to swift CodeSignUpdate.swift in the RunScript phase with export commands that set shell variables, MAIN_BUNDLE_ID, and HELPER_BUNDLE_ID.
… CodeSignUtil generates (basically its just some missing spaces just before some instances of "/* exists */". SMJobBless itself is fine with it, as technically the preceding space isn't required, but SMJobBlessUtil.py complains about it because it's not what it generates when you call it with -setreq, and of course, it checks for exactly what it generates.
…ed the project, so they won't be a diff in an updated pull request.
…orked the project, so it won't show up as diffs in the updated pull request
…d it so there aren't needless diffs in updated pull request
…n main switch statement of CodeSignUpdate.swift as suggested by Nathan Toone (github.com/toonetown) who found the error.
{
let appleDeveloper = "Apple Development:"
let macDeveloper = "Mac Developer:"
let customDeveloper = "Developer ID Application:"

Choose a reason for hiding this comment

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

When signing with a Developer ID certificate, you need to match the logic and structure of the designated requirements that are generated when action == install. However, isValidDeveloperCN only gets called by way of appendMacDeveloper - which isn't called for action == install. I'm not sure that there is benefit in supporting the Developer ID certificate when action == build.

(This is just a suggestion and some feedback. The failure caused by trying to sign with a developer ID cert under action == build is what helped me to realize that my build scripts were not correct for my development versus deployment profiles...with this change, I would not have had that error to guide me in the correct direction).

Choose a reason for hiding this comment

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

Another alternative (which I think might be even more helpful) is to key off of the Developer ID in the signing certificate, and if it is a developer ID cert, calculate like you currently do for action == install, and if it's non-Developer ID, calculate like you currently do for action == build. That would match the correct designated requirements for any certificate (and then you wouldn't key off the ACTION environment variable anymore).

As it stands, I always to my builds with the build target, so I need to export ACTION=install when I'm signing with a developer ID cert (and export ACTION=build when I'm signing with my dev certificates). But - keying off the cert might be nicer than keying off of the ACTION environment variable. Again - just suggestions.

@toonetown
Copy link

BTW - I have to say, this is a VERY helpful pull request. Thank you for providing it. The swift-based signing script is very nice.

@chipjarred
Copy link
Author

Thank you for the kind words, and I'm glad to know it was helpful to you in fixing your build script issues.

I'll need to do some more thinking about the case when action == install, and in general there are lots of improvements that could be made. My Swift version of the script is more or less a straight-forward translation of Erik's shell script so that error messages wouldn't be gobbled up in string construction. As such it retains a lot of that original structure and logic flow. Of course, Swift isn't bash/zsh, and I haven't spent a ton of time making it more Swifty or expanding the checks it does, though I did add one or two additional ones, and moved the hard coded bundle IDs out of the script into the environment.

@chipjarred
Copy link
Author

@toonetown I like your suggestion about keying off of Developer ID. It sounds like you have a clear idea and are more recently in the depths of code signing details than I have been. If you have the time to do it, would you mind writing it up as a pull request on my fork? Otherwise I'll put it on my to do list.

@toonetown
Copy link

I absolutely can. I actually had it done that way earlier but decided against the additional changes in case you or others didn’t want that. I will get something put up in the next couple days.

@chipjarred
Copy link
Author

Great! I look forward to seeing it.

toonetown and others added 5 commits November 29, 2021 12:51
Rather than keying off the "ACTION" environment variable, we can key off the fact that it is a DeveloperID certificate - which causes the binary to be signed with a different Designated Requirement.  Previously, the Developer ID designated requirement was used only when ACTION was set to "install".  This change keys off of the certificate CN, and if it is a Developer ID certificate, it will generate the strings appropriate for that signature.
Automatically handle DeveloperID certificates.
…elperToolController object. Changed AppProtocol to HelperToolControllerProtocol, since it no longer applies to the App or AppDelegate. This decouples the UI code from the helper tool/helper connection API, though I still think it needs a bit more work in that direction.
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.

None yet

2 participants