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

Tooling for Mac Distribution #324

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions tools/mac-distribution/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
# mac-packaging

## Usage

### Packaging

App Bundle (.app)
```shell
mac-distribution package-app tsh tsh.app/
Copy link
Contributor

Choose a reason for hiding this comment

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

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

```

Package Installer (.pkg)

```shell
# Staging files
mkdir "${STAGING_PKG}"
cp "file1" "file2" "${STAGING_PKG}"

# Package
mac-distribution package-pkg --install-location /usr/local/bin "${STAGING_PKG}" "my-app.pkg"
```

### Notarization

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="" ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we support flags at all? Doesn't this just make it more likely that secrets get recorded in shell history files?

```

These flags can also be set through the environment.
```shell
APPLE_USERNAME=""
APPLE_PASSWORD=""
SIGNING_IDENTITY=""
BUNDLE_ID=""
```

If all of these are set then notarization will be enabled and the tool will notarize after packaging.
This is to make it convenient to test locally without having to set up creds to build packages.

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.
Comment on lines +41 to +42
Copy link
Contributor

Choose a reason for hiding this comment

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

Most CI environments set a CI environment variable - what do you think about automatically forcing notarization when $CI is set?

16 changes: 16 additions & 0 deletions tools/mac-distribution/go.mod
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
module github.com/gravitational/shared-workflows/tools/mac-distribution

go 1.23.2
Copy link
Contributor

Choose a reason for hiding this comment

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

1.23.6 is the current latest 1.23 and is what we have in the main teleport repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like 1.24.0 was just released today!


require (
github.com/alecthomas/kong v1.6.1
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

github.com/gravitational/trace v1.5.0
github.com/stretchr/testify v1.8.3
)

require (
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
golang.org/x/net v0.33.0 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
)
22 changes: 22 additions & 0 deletions tools/mac-distribution/go.sum
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
github.com/alecthomas/assert/v2 v2.11.0 h1:2Q9r3ki8+JYXvGsDyBXwH3LcJ+WK5D0gc5E8vS6K3D0=
github.com/alecthomas/assert/v2 v2.11.0/go.mod h1:Bze95FyfUr7x34QZrjL+XP+0qgp/zg8yS+TtBj1WA3k=
github.com/alecthomas/kong v1.6.1 h1:/7bVimARU3uxPD0hbryPE8qWrS3Oz3kPQoxA/H2NKG8=
github.com/alecthomas/kong v1.6.1/go.mod h1:p2vqieVMeTAnaC83txKtXe8FLke2X07aruPWXyMPQrU=
github.com/alecthomas/repr v0.4.0 h1:GhI2A8MACjfegCPVq9f1FLvIBS+DrQ2KQBFZP1iFzXc=
github.com/alecthomas/repr v0.4.0/go.mod h1:Fr0507jx4eOXV7AlPV6AVZLYrLIuIeSOWtW57eE/O/4=
github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/gravitational/trace v1.5.0 h1:JbeL2HDGyzgy7G72Z2hP2gExEyA6Y2p7fCiSjyZwCJw=
github.com/gravitational/trace v1.5.0/go.mod h1:dxezSkKm880IIDx+czWG8fq+pLnXjETBewMgN3jOBlg=
github.com/hexops/gotextdiff v1.0.3 h1:gitA9+qJrrTCsiCl7+kh75nPqQt1cx4ZkudSTLoUqJM=
github.com/hexops/gotextdiff v1.0.3/go.mod h1:pSWU5MAI3yDq+fZBTazCSJysOMbxWL1BSow5/V2vxeg=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/stretchr/testify v1.8.3 h1:RP3t2pwF7cMEbC1dqtB6poj3niw/9gnV4Cjg5oW5gtY=
github.com/stretchr/testify v1.8.3/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
golang.org/x/net v0.33.0 h1:74SYHlV8BIgHIFC/LrYkOGIwL19eTYXQ5wc6TBuO36I=
golang.org/x/net v0.33.0/go.mod h1:HXLR5J+9DxmrqMwG9qjGCxZ+zKXxBru04zlTvWlWuN4=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
61 changes: 61 additions & 0 deletions tools/mac-distribution/internal/exec/exec.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package exec

import (
"bytes"
"log/slog"
"os/exec"
"strings"

"github.com/gravitational/trace"
)

// CommandRunner is a wrapper around [exec.Command] that is useful for testing.
type CommandRunner interface {
RunCommand(path string, args ...string) (string, error)
}

func NewDefaultCommandRunner() *DefaultCommandRunner {
return &DefaultCommandRunner{}
}

type DefaultCommandRunner struct {
}

var _ CommandRunner = &DefaultCommandRunner{}

func (d *DefaultCommandRunner) RunCommand(path string, args ...string) (string, error) {
var stdout bytes.Buffer
var stderr bytes.Buffer

cmd := exec.Command(path, args...)
cmd.Stderr = &stderr
cmd.Stdout = &stdout

err := cmd.Run()
out := strings.TrimSpace(stdout.String())
doggydogworld marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return out, trace.Wrap(err, "failed to run command: %s", stderr.String())
}
return out, nil
}

// DryRunner is a dry runner that does not actually run the command.
// Instead, it logs the command that would have been run.
type DryRunner struct {
log *slog.Logger
}

var _ CommandRunner = &DryRunner{}

// NewDryRunner creates a new dry runner.
func NewDryRunner(logger *slog.Logger) *DryRunner {
return &DryRunner{
log: logger,
}
}

// 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

return "dry run", nil
}
29 changes: 29 additions & 0 deletions tools/mac-distribution/internal/fileutil/copy.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package fileutil

import (
"io"
"os"

"github.com/gravitational/trace"
)

// CopyFile copies a file from src to dst.
func CopyFile(src, dst string) error {
w, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0755) // create or overwrite
doggydogworld marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return trace.Wrap(err)
}
defer w.Close()

r, err := os.OpenFile(src, os.O_RDONLY, 0)
doggydogworld marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return trace.Wrap(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

On error should we be deleting the dst file or leaving it behind?

}
defer r.Close()

if _, err = io.Copy(w, r); err != nil {
return trace.Wrap(err)
}

return nil
}
63 changes: 63 additions & 0 deletions tools/mac-distribution/internal/zipper/zipper.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package zipper

import (
"archive/zip"
"io"
"io/fs"
"os"
"path/filepath"
"strings"

"github.com/gravitational/trace"
)

type DirZipper interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need an interface here? Looks like there's only one implementation of it..

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

ZipDir(dir string, out io.Writer, opts DirZipperOpts) error
}

type DirZipperOpts struct {
// IncludePrefix determines whether to keep the root directory as a prefix in the zip file.
// This is particularly useful for App Bundles where the root directory (.app) should be included.
IncludePrefix bool
}

func NewDirZipper() DirZipper {
return &DefaultZipper{}
}

// DefaultZipper is the default implementation of DirZipper
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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

zipwriter := zip.NewWriter(out)
defer zipwriter.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

The error from zipwriter.Close() is ignored. This can lead to silent data loss if the zip writer cannot write the central directory.

The main problem capturing these errors in a defer is that you may already be returning an earlier error so you may not need to close and handle that error - often the first error is all you need to return. In this specific case I'd suggest you do not bother closing if there is an error since it is not needed to write the central directory if the zipfile is not going to be valid anyway:

func (z *DefaultZipper) ZipDir(dir string, out io.Writer, opts DirZipperOpt) (err error) {
...

defer func() {
    if err == nil {
        err = zipwriter.Close()
    }
}()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented this. Forgot to add errors checking for a lot of the Close()


root := filepath.Clean(dir)

err := filepath.WalkDir(root, fs.WalkDirFunc(func(path string, d fs.DirEntry, err error) error {
// Ignore zipping directories
if d.IsDir() {
return nil
}

if !opts.IncludePrefix {
path, _ = strings.CutPrefix(path, root+string(os.PathSeparator))
doggydogworld marked this conversation as resolved.
Show resolved Hide resolved
}

w, err := zipwriter.Create(path)
if err != nil {
return err
}

f, err := os.OpenFile(path, os.O_RDONLY, 0)
doggydogworld marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
}
defer f.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

this is another ignored error, but perhaps that's ok. If you cannot close a file you've opened for reading, does it matter for a non-server process? If something has gone so wrong that this fails, it is likely the next thing you do will fail too, and if there's nothing more to do, then no worries! But just pointing it out in case I've missed something important.


_, err = io.Copy(w, f)
return err
}))
return trace.Wrap(err)
}
141 changes: 141 additions & 0 deletions tools/mac-distribution/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
package main

import (
"fmt"
"log/slog"

"github.com/alecthomas/kong"
"github.com/gravitational/shared-workflows/tools/mac-distribution/notarize"
"github.com/gravitational/shared-workflows/tools/mac-distribution/packaging"
"github.com/gravitational/trace"
)

var log = slog.Default()

type CLI struct {
// Subcommands
Notarize NotarizeCmd `cmd:"" help:"Utility for notarizing files"`
PackageApp AppBundleCmd `cmd:"" help:"Create an Application Bundle (.app)"`
PackagePkg PackageInstallerCmd `cmd:"" help:"Create a package installer (.pkg)"`

GlobalFlags
}

type NotarizeCmd struct {
Files []string `arg:"" help:"List of files to notarize."`
}

type GlobalFlags struct {
Retry int `group:"notarization options" help:"Retry notarization in case of failure."`
ForceNotarization bool `group:"notarization options" help:"Always attempt notarization. By default notarization will be skipped if notarization creds are not set."`

AppleUsername string `group:"notarization creds" and:"notarization creds" env:"APPLE_USERNAME" help:"Apple Username. Required for notarization. Must use with apple-password."`
ApplePassword string `group:"notarization creds" and:"notarization creds" env:"APPLE_PASSWORD" help:"Apple Password. Required for notarization. Must use with apple-username."`
SigningID string `group:"notarization creds" and:"notarization creds" env:"SIGNING_ID" help:"Signing Identity to use for codesigning. Required for notarization."`
BundleID string `group:"notarization creds" and:"notarization creds" env:"BUNDLE_ID" help:"Bundle ID is a unique identifier used for codesigning & notarization. Required for notarization."`
}

type AppBundleCmd struct {
AppBinary string `arg:"" help:"Binary to use as the main executable for the app bundle."`
Skeleton string `arg:"" help:"Skeleton directory to use as the base for the app bundle."`

Entitlements string `flag:"" help:"Entitlements file to use for the app bundle."`
}

type PackageInstallerCmd struct {
RootPath string `arg:"" help:"Path to the root directory of the package installer."`
PackageOutputPath string `arg:"" help:"Path to the output package installer."`

InstallLocation string `flag:"" required:"" help:"Location where the package contents will be installed."`
ScriptsDir string `flag:"" help:"Path to the scripts directory. Contains preinstall and postinstall scripts."`
Version string `flag:"" help:"Version of the package. Used in determining upgrade behavior."`
}

func main() {
cli := CLI{
GlobalFlags: GlobalFlags{},
}
doggydogworld marked this conversation as resolved.
Show resolved Hide resolved

kctx := kong.Parse(&cli)
err := kctx.Run(&cli.GlobalFlags)
doggydogworld marked this conversation as resolved.
Show resolved Hide resolved
kctx.FatalIfErrorf(err)
}

func (c *AppBundleCmd) Run(g *GlobalFlags) error {
notaryTool, err := g.InitNotaryTool()
if err != nil {
return trace.Wrap(err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather that this at at top of each sub-command Run method, you can add an AfterApply() hook method to GlobalFlags and add a field for the notary tool to that struct too:

type GlobalFlags struct {
...
    notaryTool *notarize.Tool
}

func (g *GlobalFlags) AfterApply() error {
    ... contents of GlobalFlags.InitNotaryTool() goes here ...
    g.notaryTool = notarize.NewTool(...)
}

You can access notaryTool as cli.notaryTool if you change the arg to the Run(cli *CLI) method.

Then consider renaming GlobalFlags to NotaryCmd or something. Kong is quite composable through embedding, where you can embed functionality to the sub-commands as needed and attach the initialization logic to the embedded type. Then embed NotaryCmd in each of the commands that need a notary. This will allow you to extend this program with commands that do not use a notary and you don't then initialize the notary tool. Now it's embedded in the specific commands, you don't really need the cli *CLI arg, but I still like to keep that anyway for flags that apply to all commands like --verbose, etc.

Have a look at what I've done with TemporalCmd here: https://github.com/gravitational/workflows/blob/main/cmd/wctl/main.go#L49 - that struct is embedded in the commands that are temporal commands, which gives a flag to point at temporal and the AfterApply() creates the connection to temporal so the sub-commands can just use it. The GenerateTokenCmd command does not use it


pkg := packaging.NewAppBundlePackager(
&packaging.AppBundleInfo{
Skeleton: c.Skeleton,
Entitlements: c.Entitlements,
AppBinary: c.AppBinary,
},
&packaging.AppBundlePackagerOpts{
NotaryTool: notaryTool,
Logger: log,
},
)

return pkg.Package()
}

func (c *PackageInstallerCmd) Run(g *GlobalFlags) error {
notaryTool, err := g.InitNotaryTool()
if err != nil {
return trace.Wrap(err)
}

pkg := packaging.NewPackageInstallerPackager(
&packaging.PackageInstallerInfo{
RootPath: c.RootPath,
InstallLocation: c.InstallLocation,
OutputPath: c.PackageOutputPath,
ScriptsDir: c.ScriptsDir,
BundleID: g.BundleID, // Only populated for notarization
},
&packaging.PackageInstallerPackagerOpts{
NotaryTool: notaryTool,
Logger: log,
},
)

return pkg.Package()
}

func (c *NotarizeCmd) Run(g *GlobalFlags) error {
tool, err := g.InitNotaryTool()
if err != nil {
return trace.Wrap(err)
}
return trace.Wrap(tool.NotarizeBinaries(c.Files))
}

func (g *GlobalFlags) InitNotaryTool() (*notarize.Tool, error) {
doggydogworld marked this conversation as resolved.
Show resolved Hide resolved
// Dry run if no credentials are provided
dryRun := g.AppleUsername == "" || g.ApplePassword == "" || g.SigningID == ""
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if dryRun && g.ForceNotarization {
return nil, fmt.Errorf("notarization credentials not provided and force-notarization is enabled")
doggydogworld marked this conversation as resolved.
Show resolved Hide resolved
}

if dryRun {
log.Warn("notarization dry run enabled", "reason", "notarization credentials missing")
}

// Initialize notary tool
doggydogworld marked this conversation as resolved.
Show resolved Hide resolved
return notarize.NewTool(
notarize.Creds{
AppleUsername: g.AppleUsername,
ApplePassword: g.ApplePassword,
SigningIdentity: g.SigningID,
BundleID: g.BundleID,
},
notarize.ToolOpts{
Retry: g.Retry,
DryRun: dryRun,
Logger: log,
},
), nil
}
Loading
Loading