Tooling for Mac Distribution #324
Conversation
|
|
||
| By default, notarization is disabled and will output dryrun logs. To enable it you must either set the following options: | ||
| ```shell | ||
| mac-distribution --apple-username="" --apple-password="" --signing-identity="" --bundle-id="" ... |
There was a problem hiding this comment.
Should we support flags at all? Doesn't this just make it more likely that secrets get recorded in shell history files?
| However this isn't desirable in CI environments where notarization must happen. Enabling dryrun will "silently" cause a failure. | ||
| For convenience the `--force-notarization` flag is provided to fail in the scenario where creds are missing. |
There was a problem hiding this comment.
Most CI environments set a CI environment variable - what do you think about automatically forcing notarization when $CI is set?
| // Retry sets the max number of attempts for a succesful notarization | ||
| retry int |
There was a problem hiding this comment.
| // Retry sets the max number of attempts for a succesful notarization | |
| retry int | |
| maxRetries int |
A well-named variable often eliminates the need for extra comments.
|
|
||
| r, err := os.OpenFile(src, os.O_RDONLY, 0) | ||
| if err != nil { | ||
| return trace.Wrap(err) |
There was a problem hiding this comment.
On error should we be deleting the dst file or leaving it behind?
| "github.com/gravitational/trace" | ||
| ) | ||
|
|
||
| type DirZipper interface { |
There was a problem hiding this comment.
Do we need an interface here? Looks like there's only one implementation of it..
There was a problem hiding this comment.
Agree. This seems a little too Object Oriented for what is really just a function. I'd suggest getting rid of the DefaultZipper type too as if the interface goes, there's no point to the type either.
|
|
||
| App Bundle (.app) | ||
| ```shell | ||
| mac-distribution package-app tsh tsh.app/ |
There was a problem hiding this comment.
Instead of having a separate tool for every artifact, what do you think about having a single build tool with subcommands that we can extend as we convert more stuff? E.g. something like:
$ tbuild build-mac ... --notarize
| type DefaultZipper struct{} | ||
|
|
||
| // ZipDir will zip the directory into the specified output file | ||
| func (z *DefaultZipper) ZipDir(dir string, out io.Writer, opts DirZipperOpts) error { |
There was a problem hiding this comment.
Nit: Not a huge deal but I'd consider making "opts" into functional options so you can keep parameter list clean when you need to not pass any options.
|
|
||
| func (g *GlobalFlags) InitNotaryTool() (*notarize.Tool, error) { | ||
| // Dry run if no credentials are provided | ||
| dryRun := g.AppleUsername == "" || g.ApplePassword == "" || g.SigningID == "" |
There was a problem hiding this comment.
More of a nit, but I would probably prefer if the dry run mode was an explicit flag and creds required if it's not set. Less room for mistake IMO.
|
|
||
| // The Apple Notary Service requires a zip file for notarization. | ||
| // The files will be staged in a temporary directory and zipped for submission. | ||
| notaryDir, err := os.MkdirTemp("", "notarize-binaries-*") |
There was a problem hiding this comment.
Can we include something in the dir name to differentiate between different builds? Like a version number? Just for potential debugging/troubleshooting.
|
|
||
| // RunCommand logs the command that would have been run. | ||
| func (d *DryRunner) RunCommand(path string, args ...string) (string, error) { | ||
| d.log.Info("dry run", "path", path, "args", args) |
There was a problem hiding this comment.
Looking at usage, this will potentially log all sensitive CLI arguments like Apple username and password, right? For example, when we call notarytool. We should not do that.
| "--output-format", "json", | ||
| } | ||
|
|
||
| stdout, err := t.cmdRunner.RunCommand("xcrun", args...) |
There was a problem hiding this comment.
Like mentioned above, in dry run mode this looks like it will dump all arguments, including password, into the logs.
| "--apple-password", t.Creds.ApplePassword, | ||
| "--output-format", "json", | ||
| } | ||
| stdout, err := t.cmdRunner.RunCommand("xcrun", args...) |
There was a problem hiding this comment.
Same as above regarding arguments logging.
| go 1.23.2 | ||
|
|
||
| require ( | ||
| github.com/alecthomas/kong v1.6.1 |
There was a problem hiding this comment.
current latest release is 1.7.0, although 1.8.0 was tagged 4 hours ago, but nothing on the releases page yet (although historically, kong has not used github releases; there's only 2 there at the moment).
| @@ -0,0 +1,16 @@ | |||
| module github.com/gravitational/shared-workflows/tools/mac-distribution | |||
|
|
|||
| go 1.23.2 | |||
There was a problem hiding this comment.
1.23.6 is the current latest 1.23 and is what we have in the main teleport repo.
There was a problem hiding this comment.
Looks like 1.24.0 was just released today!
| "github.com/gravitational/trace" | ||
| ) | ||
|
|
||
| type DirZipper interface { |
There was a problem hiding this comment.
Agree. This seems a little too Object Oriented for what is really just a function. I'd suggest getting rid of the DefaultZipper type too as if the interface goes, there's no point to the type either.
| // The Apple Notary Service requires a zip file for notarization. | ||
| // The files will be staged in a temporary directory and zipped for submission. | ||
| notaryDir, err := os.MkdirTemp("", "notarize-binaries-*") | ||
| if err != nil { | ||
| return trace.Wrap(err) | ||
| } | ||
| defer os.RemoveAll(notaryDir) | ||
|
|
||
| // Stage files for notarization | ||
| for _, file := range files { | ||
| dest := filepath.Join(notaryDir, filepath.Base(file)) | ||
| if err := fileutil.CopyFile(file, dest); err != nil { | ||
| return trace.Wrap(err) | ||
| } | ||
| } | ||
|
|
||
| // Zip the staged directory where the binaries are located | ||
| notaryfile, err := os.CreateTemp("", "notarize-binaries-*.zip") | ||
| if err != nil { | ||
| return trace.Wrap(err) | ||
| } | ||
| defer notaryfile.Close() | ||
| defer os.Remove(notaryfile.Name()) | ||
|
|
||
| if err := t.zip.ZipDir(notaryDir, notaryfile, zipper.DirZipperOpts{}); err != nil { | ||
| return trace.Wrap(err) | ||
| } |
There was a problem hiding this comment.
Since we're doing all this with Go code now, I feel it is unnecessary to create a temp dir and copy all the files into there to zip them. We can create zip files using archive/zip.Writer.CreateHeader to specify the filename inside the zip file without needing the files to be in a similar place. Perhaps this is a future optimization though - I'll leave that up to you, but I think we should do this as it is a lot less copying of bytes, and less clean-up that may go wrong leaving garbage.
| if err := os.MkdirAll(binDir, 0755); err != nil { | ||
| return trace.Wrap(err) | ||
| } | ||
|
|
||
| binDest := filepath.Join(binDir, filepath.Base(a.Info.AppBinary)) | ||
| if err := fileutil.CopyFile(a.Info.AppBinary, binDest); err != nil { |
There was a problem hiding this comment.
Should all this be cleaned up when done? Or at least if there's an error?
* Copying files preserves perms by default and removes dest on errors * Appbundle sets perms for binary correctly * Copy should properly propagate Close errors * Notarize binaries no longer creates a staging dir for zipping * Moving around packaging files * Unexporting submissionResponseData * Using byte slice for stdout * Fixing zipper archiveName * Cleaning up notarytool * Cleaning up some packaging names * Cleaning up main
No description provided.