From 5a462d9096dd2898ab8cb2572c083cf0d4a8bc18 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 19 Jun 2026 15:35:12 -0700 Subject: [PATCH 01/17] feat: add kfutil upgrade command Adds 'kfutil upgrade' to atomically replace the running binary with any GitHub-tagged release, including pre-releases and older versions. - pkg/upgrade/upgrade.go: fetches release metadata from GitHub API, resolves the goreleaser zip asset for the current GOOS/GOARCH, verifies SHA-256 against the published SUMS file, and applies the update via minio/selfupdate (handles Windows rename-swap natively) - cmd/upgrade.go: cobra command wiring with --version, --dry-run, --force - pkg/upgrade/upgrade_test.go: 13 unit tests covering asset name resolution, checksum verification, zip extraction, and GitHub API responses via httptest mock server Flags: --version any valid GitHub tag (e.g. v1.9.0, v1.9.2-rc.14) --dry-run show what would happen without replacing the binary --force upgrade even when already at the target version --- cmd/upgrade.go | 51 +++++++ go.mod | 2 + go.sum | 13 ++ pkg/upgrade/upgrade.go | 282 ++++++++++++++++++++++++++++++++++++ pkg/upgrade/upgrade_test.go | 176 ++++++++++++++++++++++ 5 files changed, 524 insertions(+) create mode 100644 cmd/upgrade.go create mode 100644 pkg/upgrade/upgrade.go create mode 100644 pkg/upgrade/upgrade_test.go diff --git a/cmd/upgrade.go b/cmd/upgrade.go new file mode 100644 index 00000000..c824bfe7 --- /dev/null +++ b/cmd/upgrade.go @@ -0,0 +1,51 @@ +// Copyright 2025 Keyfactor +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cmd + +import ( + "kfutil/pkg/upgrade" + "kfutil/pkg/version" + + "github.com/spf13/cobra" +) + +var upgradeCmd = &cobra.Command{ + Use: "upgrade", + Short: "Upgrade kfutil to the latest release", + Long: `Fetches a kfutil release from GitHub, verifies its SHA-256 checksum, +and atomically replaces the running binary. + +By default the latest published release is installed. Pass --version with any +valid GitHub tag (e.g. v1.9.0, v1.10.0-beta.1) to install a specific release, +including pre-releases and older versions. + +Examples: + kfutil upgrade # install latest + kfutil upgrade --version v1.8.0 # install a specific tag + kfutil upgrade --dry-run # preview without changing anything`, + RunE: func(cmd *cobra.Command, args []string) error { + targetVersion, _ := cmd.Flags().GetString("version") + dryRun, _ := cmd.Flags().GetBool("dry-run") + force, _ := cmd.Flags().GetBool("force") + return upgrade.Run(version.VERSION, targetVersion, dryRun, force) + }, +} + +func init() { + upgradeCmd.Flags().String("version", "", "GitHub tag to install (default: latest release)") + upgradeCmd.Flags().Bool("dry-run", false, "Show what would be downloaded without replacing the binary") + upgradeCmd.Flags().Bool("force", false, "Upgrade even if already at the target version") + RootCmd.AddCommand(upgradeCmd) +} diff --git a/go.mod b/go.mod index 51a3e5f1..6c758621 100644 --- a/go.mod +++ b/go.mod @@ -16,6 +16,7 @@ require ( github.com/google/uuid v1.6.0 github.com/hinshun/vt10x v0.0.0-20220301184237-5011da428d02 github.com/joho/godotenv v1.5.1 + github.com/minio/selfupdate v0.6.0 github.com/rs/zerolog v1.34.0 github.com/spf13/cobra v1.10.2 github.com/spf13/pflag v1.0.10 @@ -27,6 +28,7 @@ require ( ) require ( + aead.dev/minisign v0.2.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/internal v1.11.2 // indirect github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets v1.4.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/internal v1.2.0 // indirect diff --git a/go.sum b/go.sum index b7adc5d3..5cbac31b 100644 --- a/go.sum +++ b/go.sum @@ -1,3 +1,5 @@ +aead.dev/minisign v0.2.0 h1:kAWrq/hBRu4AARY6AlciO83xhNnW9UaC8YipS2uhLPk= +aead.dev/minisign v0.2.0/go.mod h1:zdq6LdSd9TbuSxchxwhpA9zEb9YXcVGoE8JakuiGaIQ= github.com/AlecAivazis/survey/v2 v2.3.7 h1:6I/u8FvytdGsgonrYsVn2t8t4QiRnh6QSTqkkhIiSjQ= github.com/AlecAivazis/survey/v2 v2.3.7/go.mod h1:xUTIdE4KCOIjsBAE1JYsUPoCqYdZ1reCfTwbto0Fduo= github.com/Azure/azure-sdk-for-go/sdk/azcore v1.20.0 h1:JXg2dwJUmPB9JmtVmdEB16APJ7jurfbY5jnfXpJoRMc= @@ -83,6 +85,8 @@ github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D github.com/mgutz/ansi v0.0.0-20170206155736-9520e82c474b/go.mod h1:01TrycV0kFyexm33Z7vhZRXopbI8J3TDReVlkTgMUxE= github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d h1:5PJl274Y63IEHC+7izoQE9x6ikvDFZS2mDVS3drnohI= github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d/go.mod h1:01TrycV0kFyexm33Z7vhZRXopbI8J3TDReVlkTgMUxE= +github.com/minio/selfupdate v0.6.0 h1:i76PgT0K5xO9+hjzKcacQtO7+MjJ4JKA8Ak8XQ9DDwU= +github.com/minio/selfupdate v0.6.0/go.mod h1:bO02GTIPCMQFTEvE5h4DjYB58bCoZ35XLeBf0buTDdM= github.com/mitchellh/go-testing-interface v1.14.1 h1:jrgshOhYAUVNMAJiKbEu7EqAwgJJ2JqpQmpLJOu07cU= github.com/mitchellh/go-testing-interface v1.14.1/go.mod h1:gfgS7OtZj6MA4U1UrDRp04twqAjfvlZyCfX3sDjEym8= github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c h1:+mdjkGKdHQG3305AYmdv1U2eRNDiU2ErMBj1gwrq8eQ= @@ -115,12 +119,16 @@ go.mozilla.org/pkcs7 v0.9.0/go.mod h1:SNgMg+EgDFwmvSmLRTNKC5fegJjB7v23qTQ0XLGUNH go.yaml.in/yaml/v3 v3.0.4 h1:tfq32ie2Jv2UxXFdLJdh3jXuOzWiL1fo0bu/FbuKpbc= go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= +golang.org/x/crypto v0.0.0-20210220033148-5ea612d1eb83/go.mod h1:jdWPYTVW3xRLrWPugEBEK3UY2ZEsg3UU495nc5E+M+I= golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= +golang.org/x/crypto v0.0.0-20211209193657-4570a0811e8b/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4= golang.org/x/crypto v0.45.0 h1:jMBrvKuj23MTlT0bQEOBcAE0mjg8mK9RXFhRH6nyF3Q= golang.org/x/crypto v0.45.0/go.mod h1:XTGrrkGJve7CYK7J8PEww4aY7gM3qMCElcJQ8n8JdX4= golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4= +golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= +golang.org/x/net v0.0.0-20211112202133-69e39bad7dc2/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y= golang.org/x/net v0.0.0-20220722155237-a158d28d115b/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c= golang.org/x/net v0.47.0 h1:Mx+4dIFzqraBXUugkia1OOvlD6LemFo1ALMHjrXDOhY= golang.org/x/net v0.47.0/go.mod h1:/jNxtkgq5yWUGYkaZGqo27cfGZ1c5Nen03aYrrKpVRU= @@ -130,9 +138,12 @@ golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190222072716-a9d3bda3a223/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= +golang.org/x/sys v0.0.0-20191026070338-33540a1f6037/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200116001909-b77594299b42/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200223170610-d5e6a3e2c0ae/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= +golang.org/x/sys v0.0.0-20210228012217-479acdf4ea46/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= +golang.org/x/sys v0.0.0-20210423082822-04245dca01da/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20210630005230-0f9fa26af87c/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20210927094055-39ccf1dd6fa6/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= @@ -145,12 +156,14 @@ golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.12.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.38.0 h1:3yZWxaJjBmCWXqhN1qh02AkOnCQ1poK6oF+a7xWL6Gc= golang.org/x/sys v0.38.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= +golang.org/x/term v0.0.0-20201117132131-f5c789dd3221/go.mod h1:Nr5EML6q2oocZ2LXRh80K7BxOlk5/8JxuGnuhpl+muw= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= golang.org/x/term v0.37.0 h1:8EGAD0qCmHYZg6J17DvsMy9/wJ7/D/4pV/wfnld5lTU= golang.org/x/term v0.37.0/go.mod h1:5pB4lxRNYYVZuTLmy8oR2BH8dflOR+IbTYFD8fi3254= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= +golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ= golang.org/x/text v0.4.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8= golang.org/x/text v0.31.0 h1:aC8ghyu4JhP8VojJ2lEHBnochRno1sgL6nEi9WGFGMM= diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go new file mode 100644 index 00000000..8b2cce6f --- /dev/null +++ b/pkg/upgrade/upgrade.go @@ -0,0 +1,282 @@ +// Copyright 2025 Keyfactor +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package upgrade + +import ( + "archive/zip" + "bytes" + "crypto/sha256" + "encoding/hex" + "encoding/json" + "fmt" + "io" + "net/http" + "os" + "runtime" + "strings" + + "github.com/minio/selfupdate" +) + +const ( + releasesURL = "https://api.github.com/repos/Keyfactor/kfutil/releases" + binaryName = "kfutil" +) + +// GitHubRelease is the minimal shape we need from the GitHub releases API. +type GitHubRelease struct { + TagName string `json:"tag_name"` + Assets []GitHubAsset `json:"assets"` +} + +type GitHubAsset struct { + Name string `json:"name"` + BrowserDownloadURL string `json:"browser_download_url"` +} + +// Run fetches the target release, verifies the checksum, and replaces the +// running binary. targetVersion may be any valid GitHub tag (e.g. "v1.9.0") +// or empty to use the latest release. +func Run(currentVersion, targetVersion string, dryRun, force bool) error { + release, err := fetchRelease(targetVersion) + if err != nil { + return err + } + + // Strip leading "v" for comparison with version.VERSION which has no prefix. + releaseVer := strings.TrimPrefix(release.TagName, "v") + currentVer := strings.TrimPrefix(currentVersion, "v") + + fmt.Printf("Current version : %s\n", currentVer) + fmt.Printf("Target version : %s\n", releaseVer) + + if !force && targetVersion == "" && releaseVer == currentVer { + fmt.Println("Already at the latest version.") + return nil + } + + assetName := archiveAssetName(release.TagName) + sumsName := fmt.Sprintf("kfutil_%s_SHA256SUMS", strings.TrimPrefix(release.TagName, "v")) + + archiveURL, sumsURL := "", "" + for _, a := range release.Assets { + switch a.Name { + case assetName: + archiveURL = a.BrowserDownloadURL + case sumsName: + sumsURL = a.BrowserDownloadURL + } + } + + if archiveURL == "" { + return fmt.Errorf("no release asset found for %s/%s (looked for %q)\navailable assets:\n%s", + runtime.GOOS, runtime.GOARCH, assetName, listAssets(release.Assets)) + } + + if dryRun { + fmt.Printf("\n[dry-run] Would download : %s\n", archiveURL) + if sumsURL != "" { + fmt.Printf("[dry-run] Would verify : %s\n", sumsURL) + } + fmt.Printf("[dry-run] Would replace : %s\n", currentExecutable()) + return nil + } + + fmt.Printf("Downloading %s ...\n", assetName) + archiveData, err := download(archiveURL) + if err != nil { + return fmt.Errorf("download failed: %w", err) + } + + binary, err := extractBinary(archiveData, binaryName) + if err != nil { + return fmt.Errorf("extract failed: %w", err) + } + + if sumsURL != "" { + fmt.Println("Verifying checksum ...") + sumsData, err := download(sumsURL) + if err != nil { + return fmt.Errorf("checksum download failed: %w", err) + } + if err := verifyChecksum(binary, assetName, sumsData); err != nil { + return err + } + fmt.Println("Checksum OK.") + } + + fmt.Println("Applying update ...") + if err := apply(bytes.NewReader(binary)); err != nil { + return err + } + + fmt.Printf("Upgraded to %s.\n", release.TagName) + return nil +} + +// fetchRelease returns the latest release or a specific tagged release. +func fetchRelease(tag string) (*GitHubRelease, error) { + return fetchReleaseFrom(releasesURL, tag) +} + +// fetchReleaseFrom is the testable core of fetchRelease; baseURL replaces releasesURL. +func fetchReleaseFrom(baseURL, tag string) (*GitHubRelease, error) { + var url string + if tag == "" { + url = baseURL + "/latest" + } else { + url = baseURL + "/tags/" + tag + } + + req, err := http.NewRequest(http.MethodGet, url, nil) + if err != nil { + return nil, err + } + req.Header.Set("Accept", "application/vnd.github+json") + if tok := os.Getenv("GITHUB_TOKEN"); tok != "" { + req.Header.Set("Authorization", "Bearer "+tok) + } + + resp, err := http.DefaultClient.Do(req) + if err != nil { + return nil, fmt.Errorf("GitHub API request failed: %w", err) + } + defer resp.Body.Close() + + switch resp.StatusCode { + case http.StatusOK: + case http.StatusNotFound: + if tag != "" { + return nil, fmt.Errorf("release tag %q not found", tag) + } + return nil, fmt.Errorf("no releases found for this repository") + case http.StatusForbidden, http.StatusTooManyRequests: + return nil, fmt.Errorf("GitHub API rate limited (HTTP %d) — set GITHUB_TOKEN to increase limits", resp.StatusCode) + default: + return nil, fmt.Errorf("GitHub API returned HTTP %d", resp.StatusCode) + } + + var rel GitHubRelease + if err := json.NewDecoder(resp.Body).Decode(&rel); err != nil { + return nil, fmt.Errorf("failed to parse release response: %w", err) + } + return &rel, nil +} + +// archiveAssetName returns the goreleaser archive name for the current platform. +// Pattern: kfutil___.zip (version without leading "v") +func archiveAssetName(tag string) string { + ver := strings.TrimPrefix(tag, "v") + goos := runtime.GOOS + goarch := runtime.GOARCH + return fmt.Sprintf("kfutil_%s_%s_%s.zip", ver, goos, goarch) +} + +// download fetches a URL and returns the body bytes. +func download(url string) ([]byte, error) { + req, err := http.NewRequest(http.MethodGet, url, nil) + if err != nil { + return nil, err + } + if tok := os.Getenv("GITHUB_TOKEN"); tok != "" { + req.Header.Set("Authorization", "Bearer "+tok) + } + + resp, err := http.DefaultClient.Do(req) + if err != nil { + return nil, err + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("HTTP %d from %s", resp.StatusCode, url) + } + return io.ReadAll(resp.Body) +} + +// extractBinary unpacks the binary named or .exe from a zip archive. +func extractBinary(zipData []byte, name string) ([]byte, error) { + r, err := zip.NewReader(bytes.NewReader(zipData), int64(len(zipData))) + if err != nil { + return nil, fmt.Errorf("invalid zip archive: %w", err) + } + + targets := []string{name, name + ".exe"} + for _, f := range r.File { + for _, t := range targets { + if f.Name == t { + rc, err := f.Open() + if err != nil { + return nil, err + } + defer rc.Close() + return io.ReadAll(rc) + } + } + } + return nil, fmt.Errorf("binary %q not found in archive", name) +} + +// verifyChecksum checks the SHA-256 of binary against the goreleaser SUMS file. +// The SUMS file has lines: " " +func verifyChecksum(binary []byte, assetName string, sumsData []byte) error { + h := sha256.Sum256(binary) + got := hex.EncodeToString(h[:]) + + for _, line := range strings.Split(string(sumsData), "\n") { + parts := strings.Fields(line) + if len(parts) == 2 && parts[1] == assetName { + if parts[0] != got { + return fmt.Errorf("checksum mismatch for %s\n expected: %s\n got: %s", assetName, parts[0], got) + } + return nil + } + } + // No entry found — skip silently rather than blocking the upgrade. + return nil +} + +// apply replaces the running binary using minio/selfupdate. +func apply(binary io.Reader) error { + err := selfupdate.Apply(binary, selfupdate.Options{}) + if err != nil { + if rbErr := selfupdate.RollbackError(err); rbErr != nil { + return fmt.Errorf("upgrade failed and rollback also failed: %w", rbErr) + } + if os.IsPermission(err) { + return fmt.Errorf("permission denied writing to %s\nTry re-running with elevated privileges (sudo kfutil upgrade)", currentExecutable()) + } + return fmt.Errorf("upgrade failed: %w", err) + } + return nil +} + +func currentExecutable() string { + exe, err := os.Executable() + if err != nil { + return "kfutil" + } + return exe +} + +func listAssets(assets []GitHubAsset) string { + var sb strings.Builder + for _, a := range assets { + sb.WriteString(" ") + sb.WriteString(a.Name) + sb.WriteString("\n") + } + return sb.String() +} diff --git a/pkg/upgrade/upgrade_test.go b/pkg/upgrade/upgrade_test.go new file mode 100644 index 00000000..cbd7ebf3 --- /dev/null +++ b/pkg/upgrade/upgrade_test.go @@ -0,0 +1,176 @@ +// Copyright 2025 Keyfactor +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package upgrade + +import ( + "archive/zip" + "bytes" + "crypto/sha256" + "encoding/hex" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// ── archiveAssetName ────────────────────────────────────────────────────────── + +func TestArchiveAssetName(t *testing.T) { + name := archiveAssetName("v1.9.0") + expected := fmt.Sprintf("kfutil_1.9.0_%s_%s.zip", runtime.GOOS, runtime.GOARCH) + assert.Equal(t, expected, name) +} + +func TestArchiveAssetName_NoLeadingV(t *testing.T) { + assert.Equal(t, archiveAssetName("v1.2.3"), archiveAssetName("v1.2.3")) + // Both should strip the "v" + withV := archiveAssetName("v1.2.3") + assert.NotContains(t, withV, "_v1.") +} + +// ── verifyChecksum ──────────────────────────────────────────────────────────── + +func TestVerifyChecksum_Match(t *testing.T) { + data := []byte("fake binary content") + h := sha256.Sum256(data) + hashHex := hex.EncodeToString(h[:]) + sums := fmt.Sprintf("%s kfutil_1.9.0_linux_amd64.zip\n", hashHex) + + err := verifyChecksum(data, "kfutil_1.9.0_linux_amd64.zip", []byte(sums)) + require.NoError(t, err) +} + +func TestVerifyChecksum_Mismatch(t *testing.T) { + data := []byte("fake binary content") + sums := "badhash kfutil_1.9.0_linux_amd64.zip\n" + err := verifyChecksum(data, "kfutil_1.9.0_linux_amd64.zip", []byte(sums)) + require.Error(t, err) + assert.Contains(t, err.Error(), "checksum mismatch") +} + +func TestVerifyChecksum_AssetNotInSums(t *testing.T) { + // No matching line → silently passes (non-blocking) + err := verifyChecksum([]byte("data"), "kfutil_1.9.0_freebsd_arm64.zip", []byte("abc123 kfutil_1.9.0_linux_amd64.zip\n")) + require.NoError(t, err) +} + +// ── extractBinary ───────────────────────────────────────────────────────────── + +func makeZip(t *testing.T, filename, content string) []byte { + t.Helper() + var buf bytes.Buffer + w := zip.NewWriter(&buf) + f, err := w.Create(filename) + require.NoError(t, err) + _, err = f.Write([]byte(content)) + require.NoError(t, err) + require.NoError(t, w.Close()) + return buf.Bytes() +} + +func TestExtractBinary_Unix(t *testing.T) { + data := makeZip(t, "kfutil", "binary-content") + got, err := extractBinary(data, "kfutil") + require.NoError(t, err) + assert.Equal(t, []byte("binary-content"), got) +} + +func TestExtractBinary_Windows(t *testing.T) { + data := makeZip(t, "kfutil.exe", "win-binary") + got, err := extractBinary(data, "kfutil") + require.NoError(t, err) + assert.Equal(t, []byte("win-binary"), got) +} + +func TestExtractBinary_NotFound(t *testing.T) { + data := makeZip(t, "readme.txt", "hello") + _, err := extractBinary(data, "kfutil") + require.Error(t, err) + assert.Contains(t, err.Error(), "not found in archive") +} + +func TestExtractBinary_InvalidZip(t *testing.T) { + _, err := extractBinary([]byte("not a zip"), "kfutil") + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid zip archive") +} + +// ── fetchRelease (via mock HTTP server) ─────────────────────────────────────── + +func mockReleaseServer(t *testing.T, tag string, statusCode int) *httptest.Server { + t.Helper() + rel := GitHubRelease{ + TagName: tag, + Assets: []GitHubAsset{ + {Name: fmt.Sprintf("kfutil_1.9.0_%s_%s.zip", runtime.GOOS, runtime.GOARCH), BrowserDownloadURL: "http://example.com/kfutil.zip"}, + {Name: "kfutil_1.9.0_SHA256SUMS", BrowserDownloadURL: "http://example.com/SHA256SUMS"}, + }, + } + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if statusCode != http.StatusOK { + w.WriteHeader(statusCode) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(rel) + })) +} + +func TestFetchRelease_Latest(t *testing.T) { + srv := mockReleaseServer(t, "v1.9.0", http.StatusOK) + defer srv.Close() + + // Temporarily point releasesURL at the mock. + orig := releasesURL + // fetchRelease builds the URL itself, so we test via a helper that accepts a base. + _ = orig // used indirectly; full integration via Run() in integration tests. + + rel, err := fetchReleaseFrom(srv.URL, "") + require.NoError(t, err) + assert.Equal(t, "v1.9.0", rel.TagName) + assert.Len(t, rel.Assets, 2) +} + +func TestFetchRelease_SpecificTag(t *testing.T) { + srv := mockReleaseServer(t, "v1.8.0", http.StatusOK) + defer srv.Close() + + rel, err := fetchReleaseFrom(srv.URL, "v1.8.0") + require.NoError(t, err) + assert.Equal(t, "v1.8.0", rel.TagName) +} + +func TestFetchRelease_NotFound(t *testing.T) { + srv := mockReleaseServer(t, "", http.StatusNotFound) + defer srv.Close() + + _, err := fetchReleaseFrom(srv.URL, "v99.0.0") + require.Error(t, err) + assert.Contains(t, err.Error(), "not found") +} + +func TestFetchRelease_RateLimited(t *testing.T) { + srv := mockReleaseServer(t, "", http.StatusForbidden) + defer srv.Close() + + _, err := fetchReleaseFrom(srv.URL, "") + require.Error(t, err) + assert.Contains(t, err.Error(), "rate limited") +} From 9823890e1b236da9238b3a89f2ba10062da86dde Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 19 Jun 2026 15:39:52 -0700 Subject: [PATCH 02/17] docs: generate CLI docs for upgrade command --- docs/kfutil.md | 1 + docs/kfutil_upgrade.md | 57 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) create mode 100644 docs/kfutil_upgrade.md diff --git a/docs/kfutil.md b/docs/kfutil.md index a775de3f..28a6fce4 100644 --- a/docs/kfutil.md +++ b/docs/kfutil.md @@ -46,4 +46,5 @@ A CLI wrapper around the Keyfactor Platform API. * [kfutil status](kfutil_status.md) - List the status of Keyfactor services. * [kfutil store-types](kfutil_store-types.md) - Keyfactor certificate store types APIs and utilities. * [kfutil stores](kfutil_stores.md) - Keyfactor certificate stores APIs and utilities. +* [kfutil upgrade](kfutil_upgrade.md) - Upgrade kfutil to the latest release * [kfutil version](kfutil_version.md) - Shows version of kfutil diff --git a/docs/kfutil_upgrade.md b/docs/kfutil_upgrade.md new file mode 100644 index 00000000..e9622751 --- /dev/null +++ b/docs/kfutil_upgrade.md @@ -0,0 +1,57 @@ +## kfutil upgrade + +Upgrade kfutil to the latest release + +### Synopsis + +Fetches a kfutil release from GitHub, verifies its SHA-256 checksum, +and atomically replaces the running binary. + +By default the latest published release is installed. Pass --version with any +valid GitHub tag (e.g. v1.9.0, v1.10.0-beta.1) to install a specific release, +including pre-releases and older versions. + +Examples: + kfutil upgrade # install latest + kfutil upgrade --version v1.8.0 # install a specific tag + kfutil upgrade --dry-run # preview without changing anything + +``` +kfutil upgrade [flags] +``` + +### Options + +``` + --dry-run Show what would be downloaded without replacing the binary + --force Upgrade even if already at the target version + -h, --help help for upgrade + --version string GitHub tag to install (default: latest release) +``` + +### Options inherited from parent commands + +``` + --api-path string API Path to use for authenticating to Keyfactor Command. (default is KeyfactorAPI) (default "KeyfactorAPI") + --auth-provider-profile string The profile to use defined in the securely stored config. If not specified the config named 'default' will be used if it exists. (default "default") + --auth-provider-type string Provider type choices: (azid) + --client-id string OAuth2 client-id to use for authenticating to Keyfactor Command. + --client-secret string OAuth2 client-secret to use for authenticating to Keyfactor Command. + --config string Full path to config file in JSON format. (default is $HOME/.keyfactor/command_config.json) + --debug Enable debugFlag logging. + --domain string Domain to use for authenticating to Keyfactor Command. + --exp Enable expEnabled features. (USE AT YOUR OWN RISK, these features are not supported and may change or be removed at any time.) + --format text How to format the CLI output. Currently only text is supported. (default "text") + --hostname string Hostname to use for authenticating to Keyfactor Command. + --no-prompt Do not prompt for any user input and assume defaults or environmental variables are set. + --offline Will not attempt to connect to GitHub for latest release information and resources. + --password string Password to use for authenticating to Keyfactor Command. WARNING: Remember to delete your console history if providing kfcPassword here in plain text. + --profile string Use a specific profile from your config file. If not specified the config named 'default' will be used if it exists. + --skip-tls-verify Disable TLS verification for API requests to Keyfactor Command. + --token-url string OAuth2 token endpoint full URL to use for authenticating to Keyfactor Command. + --username string Username to use for authenticating to Keyfactor Command. +``` + +### SEE ALSO + +* [kfutil](kfutil.md) - Keyfactor CLI utilities From 62fa113badf8a69d3015343aafc683f654b2474d Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 19 Jun 2026 15:40:40 -0700 Subject: [PATCH 03/17] fix: correct copyright year to 2026 in new files --- cmd/upgrade.go | 2 +- pkg/upgrade/upgrade.go | 2 +- pkg/upgrade/upgrade_test.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/upgrade.go b/cmd/upgrade.go index c824bfe7..8b480a46 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -1,4 +1,4 @@ -// Copyright 2025 Keyfactor +// Copyright 2026 Keyfactor // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index 8b2cce6f..ed8e9619 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -1,4 +1,4 @@ -// Copyright 2025 Keyfactor +// Copyright 2026 Keyfactor // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. diff --git a/pkg/upgrade/upgrade_test.go b/pkg/upgrade/upgrade_test.go index cbd7ebf3..1c3e3aba 100644 --- a/pkg/upgrade/upgrade_test.go +++ b/pkg/upgrade/upgrade_test.go @@ -1,4 +1,4 @@ -// Copyright 2025 Keyfactor +// Copyright 2026 Keyfactor // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. From dce69f3187545ea53c26f58dcbb22999f6ddb5e1 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 19 Jun 2026 15:57:13 -0700 Subject: [PATCH 04/17] fix(upgrade): remediate security and compliance findings Security (P0): - SEC-1: verifyChecksum now hashes the zip archive, not the extracted binary, matching what goreleaser records in SHA256SUMS; missing SUMS entry is now an error instead of a silent pass - SEC-2: GITHUB_TOKEN is only forwarded to hosts in an explicit allowlist (api.github.com, github.com, objects.githubusercontent.com) to prevent token exfiltration via a tampered BrowserDownloadURL - SEC-3: upgrade aborts with an error when no SHA256SUMS asset is present in the release, making integrity verification mandatory Compliance (P1/P2): - AUD-6: import zerolog; all audit log entries now use structured fields - AUD-1: log.Info events before and after binary replacement with from_version, to_version, executable, operator, and source_url fields - AUD-2: resolveOperator() captures os/user.Current().Username and includes it in every structured log entry - AUD-3: log.Error before each error return in Run() with a stable event name - AUD-4: log.Warn when --force bypasses the version-match safety guard - AUD-5: log.Debug after each HTTP response in fetchReleaseFrom and download with url, method, and status_code fields Tests: inverted TestVerifyChecksum_AssetNotInSums to require.Error; added TestDownload_TokenNotSentToUntrustedHost to verify the allowlist behaviour. --- pkg/upgrade/upgrade.go | 158 +++++++++++++++++++++++++++++------- pkg/upgrade/upgrade_test.go | 24 +++++- 2 files changed, 151 insertions(+), 31 deletions(-) diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index ed8e9619..b7e172ed 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -23,11 +23,14 @@ import ( "fmt" "io" "net/http" + "net/url" "os" + "os/user" "runtime" "strings" "github.com/minio/selfupdate" + "github.com/rs/zerolog/log" ) const ( @@ -35,10 +38,17 @@ const ( binaryName = "kfutil" ) +// allowedTokenHosts are the only hosts to which GITHUB_TOKEN may be forwarded. +var allowedTokenHosts = map[string]bool{ + "api.github.com": true, + "github.com": true, + "objects.githubusercontent.com": true, +} + // GitHubRelease is the minimal shape we need from the GitHub releases API. type GitHubRelease struct { - TagName string `json:"tag_name"` - Assets []GitHubAsset `json:"assets"` + TagName string `json:"tag_name"` + Assets []GitHubAsset `json:"assets"` } type GitHubAsset struct { @@ -46,12 +56,27 @@ type GitHubAsset struct { BrowserDownloadURL string `json:"browser_download_url"` } +// resolveOperator returns the current OS user name for audit log fields. +func resolveOperator() string { + u, err := user.Current() + if err != nil { + return "unknown" + } + return u.Username +} + // Run fetches the target release, verifies the checksum, and replaces the // running binary. targetVersion may be any valid GitHub tag (e.g. "v1.9.0") // or empty to use the latest release. func Run(currentVersion, targetVersion string, dryRun, force bool) error { + operator := resolveOperator() + release, err := fetchRelease(targetVersion) if err != nil { + log.Error().Err(err). + Str("event", "upgrade.fetch_release_failed"). + Str("operator", operator). + Msg("failed to fetch release metadata") return err } @@ -67,6 +92,15 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { return nil } + // Log when --force bypasses the version-match safety check. + if force && targetVersion == "" && releaseVer == currentVer { + log.Warn(). + Str("event", "upgrade.force_override"). + Str("operator", operator). + Str("version", releaseVer). + Msg("version-match check bypassed via --force") + } + assetName := archiveAssetName(release.TagName) sumsName := fmt.Sprintf("kfutil_%s_SHA256SUMS", strings.TrimPrefix(release.TagName, "v")) @@ -85,11 +119,13 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { runtime.GOOS, runtime.GOARCH, assetName, listAssets(release.Assets)) } + if sumsURL == "" { + return fmt.Errorf("release %s has no SHA256SUMS asset — upgrade aborted", release.TagName) + } + if dryRun { fmt.Printf("\n[dry-run] Would download : %s\n", archiveURL) - if sumsURL != "" { - fmt.Printf("[dry-run] Would verify : %s\n", sumsURL) - } + fmt.Printf("[dry-run] Would verify : %s\n", sumsURL) fmt.Printf("[dry-run] Would replace : %s\n", currentExecutable()) return nil } @@ -97,31 +133,76 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { fmt.Printf("Downloading %s ...\n", assetName) archiveData, err := download(archiveURL) if err != nil { + log.Error().Err(err). + Str("event", "upgrade.download_failed"). + Str("url", archiveURL). + Str("operator", operator). + Msg("archive download failed") return fmt.Errorf("download failed: %w", err) } binary, err := extractBinary(archiveData, binaryName) if err != nil { + log.Error().Err(err). + Str("event", "upgrade.extract_failed"). + Str("operator", operator). + Msg("binary extraction from archive failed") return fmt.Errorf("extract failed: %w", err) } - if sumsURL != "" { - fmt.Println("Verifying checksum ...") - sumsData, err := download(sumsURL) - if err != nil { - return fmt.Errorf("checksum download failed: %w", err) - } - if err := verifyChecksum(binary, assetName, sumsData); err != nil { - return err - } - fmt.Println("Checksum OK.") + fmt.Println("Verifying checksum ...") + sumsData, err := download(sumsURL) + if err != nil { + log.Error().Err(err). + Str("event", "upgrade.checksum_download_failed"). + Str("url", sumsURL). + Str("operator", operator). + Msg("SHA256SUMS download failed") + return fmt.Errorf("checksum download failed: %w", err) } + // Verify the hash of the zip archive, not the extracted binary — + // goreleaser's SHA256SUMS records hashes of the zip archives. + if err := verifyChecksum(archiveData, assetName, sumsData); err != nil { + log.Error().Err(err). + Str("event", "upgrade.checksum_mismatch"). + Str("asset", assetName). + Str("operator", operator). + Msg("checksum verification failed") + return err + } + fmt.Println("Checksum OK.") + + exe := currentExecutable() + log.Info(). + Str("event", "upgrade.applying"). + Str("from_version", currentVer). + Str("to_version", releaseVer). + Str("executable", exe). + Str("operator", operator). + Str("source_url", archiveURL). + Msg("applying binary replacement") fmt.Println("Applying update ...") if err := apply(bytes.NewReader(binary)); err != nil { + log.Error().Err(err). + Str("event", "upgrade.apply_failed"). + Str("from_version", currentVer). + Str("to_version", releaseVer). + Str("executable", exe). + Str("operator", operator). + Msg("binary replacement failed") return err } + log.Info(). + Str("event", "upgrade.applied"). + Str("from_version", currentVer). + Str("to_version", releaseVer). + Str("executable", exe). + Str("operator", operator). + Str("source_url", archiveURL). + Msg("binary replacement complete") + fmt.Printf("Upgraded to %s.\n", release.TagName) return nil } @@ -133,14 +214,14 @@ func fetchRelease(tag string) (*GitHubRelease, error) { // fetchReleaseFrom is the testable core of fetchRelease; baseURL replaces releasesURL. func fetchReleaseFrom(baseURL, tag string) (*GitHubRelease, error) { - var url string + var reqURL string if tag == "" { - url = baseURL + "/latest" + reqURL = baseURL + "/latest" } else { - url = baseURL + "/tags/" + tag + reqURL = baseURL + "/tags/" + tag } - req, err := http.NewRequest(http.MethodGet, url, nil) + req, err := http.NewRequest(http.MethodGet, reqURL, nil) if err != nil { return nil, err } @@ -155,6 +236,13 @@ func fetchReleaseFrom(baseURL, tag string) (*GitHubRelease, error) { } defer resp.Body.Close() + log.Debug(). + Str("event", "upgrade.github_api_response"). + Str("url", reqURL). + Str("method", http.MethodGet). + Int("status_code", resp.StatusCode). + Msg("GitHub API response received") + switch resp.StatusCode { case http.StatusOK: case http.StatusNotFound: @@ -184,14 +272,19 @@ func archiveAssetName(tag string) string { return fmt.Sprintf("kfutil_%s_%s_%s.zip", ver, goos, goarch) } -// download fetches a URL and returns the body bytes. -func download(url string) ([]byte, error) { - req, err := http.NewRequest(http.MethodGet, url, nil) +// download fetches a URL and returns the body bytes. GITHUB_TOKEN is only +// forwarded to hosts in allowedTokenHosts to prevent token exfiltration via +// a tampered BrowserDownloadURL. +func download(rawURL string) ([]byte, error) { + req, err := http.NewRequest(http.MethodGet, rawURL, nil) if err != nil { return nil, err } + if tok := os.Getenv("GITHUB_TOKEN"); tok != "" { - req.Header.Set("Authorization", "Bearer "+tok) + if parsed, err := url.Parse(rawURL); err == nil && allowedTokenHosts[parsed.Hostname()] { + req.Header.Set("Authorization", "Bearer "+tok) + } } resp, err := http.DefaultClient.Do(req) @@ -200,8 +293,15 @@ func download(url string) ([]byte, error) { } defer resp.Body.Close() + log.Debug(). + Str("event", "upgrade.http_response"). + Str("url", rawURL). + Str("method", http.MethodGet). + Int("status_code", resp.StatusCode). + Msg("HTTP response received") + if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("HTTP %d from %s", resp.StatusCode, url) + return nil, fmt.Errorf("HTTP %d from %s", resp.StatusCode, rawURL) } return io.ReadAll(resp.Body) } @@ -229,10 +329,11 @@ func extractBinary(zipData []byte, name string) ([]byte, error) { return nil, fmt.Errorf("binary %q not found in archive", name) } -// verifyChecksum checks the SHA-256 of binary against the goreleaser SUMS file. +// verifyChecksum checks the SHA-256 of archiveData against the goreleaser SUMS file. // The SUMS file has lines: " " -func verifyChecksum(binary []byte, assetName string, sumsData []byte) error { - h := sha256.Sum256(binary) +// A missing entry is an error — the SUMS file must cover the target asset. +func verifyChecksum(archiveData []byte, assetName string, sumsData []byte) error { + h := sha256.Sum256(archiveData) got := hex.EncodeToString(h[:]) for _, line := range strings.Split(string(sumsData), "\n") { @@ -244,8 +345,7 @@ func verifyChecksum(binary []byte, assetName string, sumsData []byte) error { return nil } } - // No entry found — skip silently rather than blocking the upgrade. - return nil + return fmt.Errorf("no checksum entry found for %s in SHA256SUMS", assetName) } // apply replaces the running binary using minio/selfupdate. diff --git a/pkg/upgrade/upgrade_test.go b/pkg/upgrade/upgrade_test.go index 1c3e3aba..70bbb5da 100644 --- a/pkg/upgrade/upgrade_test.go +++ b/pkg/upgrade/upgrade_test.go @@ -66,9 +66,11 @@ func TestVerifyChecksum_Mismatch(t *testing.T) { } func TestVerifyChecksum_AssetNotInSums(t *testing.T) { - // No matching line → silently passes (non-blocking) + // Missing entry must be an error — a SUMS file that doesn't cover the target + // asset must not silently pass, as an attacker could strip the entry. err := verifyChecksum([]byte("data"), "kfutil_1.9.0_freebsd_arm64.zip", []byte("abc123 kfutil_1.9.0_linux_amd64.zip\n")) - require.NoError(t, err) + require.Error(t, err) + assert.Contains(t, err.Error(), "no checksum entry found") } // ── extractBinary ───────────────────────────────────────────────────────────── @@ -112,6 +114,24 @@ func TestExtractBinary_InvalidZip(t *testing.T) { assert.Contains(t, err.Error(), "invalid zip archive") } +// ── download (token host allowlist) ────────────────────────────────────────── + +func TestDownload_TokenNotSentToUntrustedHost(t *testing.T) { + var receivedAuth string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + receivedAuth = r.Header.Get("Authorization") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("data")) + })) + defer srv.Close() + + t.Setenv("GITHUB_TOKEN", "super-secret-token") + + _, err := download(srv.URL + "/asset.zip") + require.NoError(t, err) + assert.Empty(t, receivedAuth, "GITHUB_TOKEN must not be sent to untrusted host") +} + // ── fetchRelease (via mock HTTP server) ─────────────────────────────────────── func mockReleaseServer(t *testing.T, tag string, statusCode int) *httptest.Server { From 089f4d25bd20e4174caa935658e3edf97ae9f3c4 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 19 Jun 2026 16:05:14 -0700 Subject: [PATCH 05/17] fix(upgrade): close remaining compliance gaps from second audit pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add upgrade.run_started event at Run() entry with operator, versions, force, and dry_run fields — establishes baseline for anomaly detection - Add log.Info for the already-current early exit (upgrade.already_current) - Add log.Info for the dry-run exit path (upgrade.dry_run) - Add log.Error for archiveURL-not-found and sumsURL-missing returns (upgrade.asset_not_found, upgrade.sums_missing) - Expand --force logging to fire whenever the flag is set, not only when the version-equality guard would have blocked — captures --version usage - Thread operator through fetchReleaseFrom and download so all HTTP-level log entries carry identity context - Log a warning when resolveOperator() fails so auditors can distinguish the "unknown" sentinel from a real user named "unknown" --- pkg/upgrade/upgrade.go | 68 ++++++++++++++++++++++++++++++------- pkg/upgrade/upgrade_test.go | 15 ++++---- 2 files changed, 61 insertions(+), 22 deletions(-) diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index b7e172ed..0315cd0b 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -40,8 +40,8 @@ const ( // allowedTokenHosts are the only hosts to which GITHUB_TOKEN may be forwarded. var allowedTokenHosts = map[string]bool{ - "api.github.com": true, - "github.com": true, + "api.github.com": true, + "github.com": true, "objects.githubusercontent.com": true, } @@ -60,6 +60,9 @@ type GitHubAsset struct { func resolveOperator() string { u, err := user.Current() if err != nil { + log.Warn().Err(err). + Str("event", "upgrade.operator_resolution_failed"). + Msg("could not resolve OS user identity — audit logs will use 'unknown'") return "unknown" } return u.Username @@ -71,7 +74,16 @@ func resolveOperator() string { func Run(currentVersion, targetVersion string, dryRun, force bool) error { operator := resolveOperator() - release, err := fetchRelease(targetVersion) + log.Info(). + Str("event", "upgrade.run_started"). + Str("operator", operator). + Str("current_version", currentVersion). + Str("target_version", targetVersion). + Bool("force", force). + Bool("dry_run", dryRun). + Msg("upgrade run initiated") + + release, err := fetchRelease(targetVersion, operator) if err != nil { log.Error().Err(err). Str("event", "upgrade.fetch_release_failed"). @@ -88,17 +100,24 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { fmt.Printf("Target version : %s\n", releaseVer) if !force && targetVersion == "" && releaseVer == currentVer { + log.Info(). + Str("event", "upgrade.already_current"). + Str("operator", operator). + Str("version", currentVer). + Msg("binary is already at the latest version — no action taken") fmt.Println("Already at the latest version.") return nil } - // Log when --force bypasses the version-match safety check. - if force && targetVersion == "" && releaseVer == currentVer { + // Log when --force is set regardless of whether it overrides the guard, + // so the flag's presence is always traceable in the audit record. + if force { log.Warn(). Str("event", "upgrade.force_override"). Str("operator", operator). - Str("version", releaseVer). - Msg("version-match check bypassed via --force") + Str("current_version", currentVer). + Str("target_version", releaseVer). + Msg("--force flag set: safety checks may be bypassed") } assetName := archiveAssetName(release.TagName) @@ -115,15 +134,36 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { } if archiveURL == "" { + log.Error(). + Str("event", "upgrade.asset_not_found"). + Str("operator", operator). + Str("tag", release.TagName). + Str("asset_name", assetName). + Str("os", runtime.GOOS). + Str("arch", runtime.GOARCH). + Msg("no matching release asset found for current platform") return fmt.Errorf("no release asset found for %s/%s (looked for %q)\navailable assets:\n%s", runtime.GOOS, runtime.GOARCH, assetName, listAssets(release.Assets)) } if sumsURL == "" { + log.Error(). + Str("event", "upgrade.sums_missing"). + Str("operator", operator). + Str("tag", release.TagName). + Msg("release has no SHA256SUMS asset — upgrade aborted") return fmt.Errorf("release %s has no SHA256SUMS asset — upgrade aborted", release.TagName) } if dryRun { + log.Info(). + Str("event", "upgrade.dry_run"). + Str("operator", operator). + Str("from_version", currentVer). + Str("to_version", releaseVer). + Str("executable", currentExecutable()). + Str("archive_url", archiveURL). + Msg("dry-run: no changes applied") fmt.Printf("\n[dry-run] Would download : %s\n", archiveURL) fmt.Printf("[dry-run] Would verify : %s\n", sumsURL) fmt.Printf("[dry-run] Would replace : %s\n", currentExecutable()) @@ -131,7 +171,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { } fmt.Printf("Downloading %s ...\n", assetName) - archiveData, err := download(archiveURL) + archiveData, err := download(archiveURL, operator) if err != nil { log.Error().Err(err). Str("event", "upgrade.download_failed"). @@ -151,7 +191,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { } fmt.Println("Verifying checksum ...") - sumsData, err := download(sumsURL) + sumsData, err := download(sumsURL, operator) if err != nil { log.Error().Err(err). Str("event", "upgrade.checksum_download_failed"). @@ -208,12 +248,12 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { } // fetchRelease returns the latest release or a specific tagged release. -func fetchRelease(tag string) (*GitHubRelease, error) { - return fetchReleaseFrom(releasesURL, tag) +func fetchRelease(tag, operator string) (*GitHubRelease, error) { + return fetchReleaseFrom(releasesURL, tag, operator) } // fetchReleaseFrom is the testable core of fetchRelease; baseURL replaces releasesURL. -func fetchReleaseFrom(baseURL, tag string) (*GitHubRelease, error) { +func fetchReleaseFrom(baseURL, tag, operator string) (*GitHubRelease, error) { var reqURL string if tag == "" { reqURL = baseURL + "/latest" @@ -241,6 +281,7 @@ func fetchReleaseFrom(baseURL, tag string) (*GitHubRelease, error) { Str("url", reqURL). Str("method", http.MethodGet). Int("status_code", resp.StatusCode). + Str("operator", operator). Msg("GitHub API response received") switch resp.StatusCode { @@ -275,7 +316,7 @@ func archiveAssetName(tag string) string { // download fetches a URL and returns the body bytes. GITHUB_TOKEN is only // forwarded to hosts in allowedTokenHosts to prevent token exfiltration via // a tampered BrowserDownloadURL. -func download(rawURL string) ([]byte, error) { +func download(rawURL, operator string) ([]byte, error) { req, err := http.NewRequest(http.MethodGet, rawURL, nil) if err != nil { return nil, err @@ -298,6 +339,7 @@ func download(rawURL string) ([]byte, error) { Str("url", rawURL). Str("method", http.MethodGet). Int("status_code", resp.StatusCode). + Str("operator", operator). Msg("HTTP response received") if resp.StatusCode != http.StatusOK { diff --git a/pkg/upgrade/upgrade_test.go b/pkg/upgrade/upgrade_test.go index 70bbb5da..259aebba 100644 --- a/pkg/upgrade/upgrade_test.go +++ b/pkg/upgrade/upgrade_test.go @@ -127,7 +127,7 @@ func TestDownload_TokenNotSentToUntrustedHost(t *testing.T) { t.Setenv("GITHUB_TOKEN", "super-secret-token") - _, err := download(srv.URL + "/asset.zip") + _, err := download(srv.URL+"/asset.zip", "testuser") require.NoError(t, err) assert.Empty(t, receivedAuth, "GITHUB_TOKEN must not be sent to untrusted host") } @@ -157,12 +157,9 @@ func TestFetchRelease_Latest(t *testing.T) { srv := mockReleaseServer(t, "v1.9.0", http.StatusOK) defer srv.Close() - // Temporarily point releasesURL at the mock. - orig := releasesURL - // fetchRelease builds the URL itself, so we test via a helper that accepts a base. - _ = orig // used indirectly; full integration via Run() in integration tests. + // fetchRelease builds the URL itself; test via fetchReleaseFrom which accepts a base URL. - rel, err := fetchReleaseFrom(srv.URL, "") + rel, err := fetchReleaseFrom(srv.URL, "", "testuser") require.NoError(t, err) assert.Equal(t, "v1.9.0", rel.TagName) assert.Len(t, rel.Assets, 2) @@ -172,7 +169,7 @@ func TestFetchRelease_SpecificTag(t *testing.T) { srv := mockReleaseServer(t, "v1.8.0", http.StatusOK) defer srv.Close() - rel, err := fetchReleaseFrom(srv.URL, "v1.8.0") + rel, err := fetchReleaseFrom(srv.URL, "v1.8.0", "testuser") require.NoError(t, err) assert.Equal(t, "v1.8.0", rel.TagName) } @@ -181,7 +178,7 @@ func TestFetchRelease_NotFound(t *testing.T) { srv := mockReleaseServer(t, "", http.StatusNotFound) defer srv.Close() - _, err := fetchReleaseFrom(srv.URL, "v99.0.0") + _, err := fetchReleaseFrom(srv.URL, "v99.0.0", "testuser") require.Error(t, err) assert.Contains(t, err.Error(), "not found") } @@ -190,7 +187,7 @@ func TestFetchRelease_RateLimited(t *testing.T) { srv := mockReleaseServer(t, "", http.StatusForbidden) defer srv.Close() - _, err := fetchReleaseFrom(srv.URL, "") + _, err := fetchReleaseFrom(srv.URL, "", "testuser") require.Error(t, err) assert.Contains(t, err.Error(), "rate limited") } From 25b799e6b8ba6d0543e5eec2790642c59abcac43 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 19 Jun 2026 16:15:05 -0700 Subject: [PATCH 06/17] fix(upgrade): close third-round compliance findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - cmd/upgrade.go: explicitly set zerolog level to InfoLevel (non-debug) or DebugLevel (--debug) instead of relying on the unset default; ensures audit events are durably emitted regardless of future changes to the informDebug initialization pattern used by other commands - upgrade.fetch_release_failed: add tag field so the attempted release tag is always captured in the forensic record - upgrade.extract_failed, upgrade.apply_failed: add source_url field for full binary provenance on failed privileged operations - upgrade.dry_run: rename archive_url → source_url for consistency with applying/applied/apply_failed events (single SIEM query field) - upgrade.checksum_mismatch: add source_url field so the archive provenance is captured on the security event - upgrade.rollback_failed: add dedicated event inside apply() for the rollback-also-failed sub-path so on-call engineers can distinguish a broken binary state from an ordinary upgrade failure - fetchReleaseFrom, download: add latency_ms field to all HTTP log events (SOC2 CC9.2 vendor API response metadata) - TestDownload_TokenSentToTrustedHost: add positive allowlist test verifying token IS forwarded to hosts in allowedTokenHosts --- cmd/upgrade.go | 9 +++++++++ pkg/upgrade/upgrade.go | 17 ++++++++++++++--- pkg/upgrade/upgrade_test.go | 20 ++++++++++++++++++++ 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/cmd/upgrade.go b/cmd/upgrade.go index 8b480a46..2eb5e0a7 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -18,6 +18,7 @@ import ( "kfutil/pkg/upgrade" "kfutil/pkg/version" + "github.com/rs/zerolog" "github.com/spf13/cobra" ) @@ -39,6 +40,14 @@ Examples: targetVersion, _ := cmd.Flags().GetString("version") dryRun, _ := cmd.Flags().GetBool("dry-run") force, _ := cmd.Flags().GetBool("force") + // Upgrade always emits Info+ audit events regardless of --debug. + // Other commands use informDebug() which sets Disabled when debug is off, + // but the binary-replacement audit trail must be unconditionally durable. + if debugFlag { + zerolog.SetGlobalLevel(zerolog.DebugLevel) + } else { + zerolog.SetGlobalLevel(zerolog.InfoLevel) + } return upgrade.Run(version.VERSION, targetVersion, dryRun, force) }, } diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index 0315cd0b..05838549 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -28,6 +28,7 @@ import ( "os/user" "runtime" "strings" + "time" "github.com/minio/selfupdate" "github.com/rs/zerolog/log" @@ -88,6 +89,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { log.Error().Err(err). Str("event", "upgrade.fetch_release_failed"). Str("operator", operator). + Str("tag", targetVersion). Msg("failed to fetch release metadata") return err } @@ -109,8 +111,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { return nil } - // Log when --force is set regardless of whether it overrides the guard, - // so the flag's presence is always traceable in the audit record. + // Log whenever --force is set so the flag is always traceable in the audit record. if force { log.Warn(). Str("event", "upgrade.force_override"). @@ -162,7 +163,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { Str("from_version", currentVer). Str("to_version", releaseVer). Str("executable", currentExecutable()). - Str("archive_url", archiveURL). + Str("source_url", archiveURL). Msg("dry-run: no changes applied") fmt.Printf("\n[dry-run] Would download : %s\n", archiveURL) fmt.Printf("[dry-run] Would verify : %s\n", sumsURL) @@ -186,6 +187,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { log.Error().Err(err). Str("event", "upgrade.extract_failed"). Str("operator", operator). + Str("source_url", archiveURL). Msg("binary extraction from archive failed") return fmt.Errorf("extract failed: %w", err) } @@ -207,6 +209,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { Str("event", "upgrade.checksum_mismatch"). Str("asset", assetName). Str("operator", operator). + Str("source_url", archiveURL). Msg("checksum verification failed") return err } @@ -230,6 +233,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { Str("to_version", releaseVer). Str("executable", exe). Str("operator", operator). + Str("source_url", archiveURL). Msg("binary replacement failed") return err } @@ -270,6 +274,7 @@ func fetchReleaseFrom(baseURL, tag, operator string) (*GitHubRelease, error) { req.Header.Set("Authorization", "Bearer "+tok) } + start := time.Now() resp, err := http.DefaultClient.Do(req) if err != nil { return nil, fmt.Errorf("GitHub API request failed: %w", err) @@ -281,6 +286,7 @@ func fetchReleaseFrom(baseURL, tag, operator string) (*GitHubRelease, error) { Str("url", reqURL). Str("method", http.MethodGet). Int("status_code", resp.StatusCode). + Int64("latency_ms", time.Since(start).Milliseconds()). Str("operator", operator). Msg("GitHub API response received") @@ -328,6 +334,7 @@ func download(rawURL, operator string) ([]byte, error) { } } + start := time.Now() resp, err := http.DefaultClient.Do(req) if err != nil { return nil, err @@ -339,6 +346,7 @@ func download(rawURL, operator string) ([]byte, error) { Str("url", rawURL). Str("method", http.MethodGet). Int("status_code", resp.StatusCode). + Int64("latency_ms", time.Since(start).Milliseconds()). Str("operator", operator). Msg("HTTP response received") @@ -395,6 +403,9 @@ func apply(binary io.Reader) error { err := selfupdate.Apply(binary, selfupdate.Options{}) if err != nil { if rbErr := selfupdate.RollbackError(err); rbErr != nil { + log.Error().Err(rbErr). + Str("event", "upgrade.rollback_failed"). + Msg("binary replacement failed and rollback also failed — binary may be corrupted") return fmt.Errorf("upgrade failed and rollback also failed: %w", rbErr) } if os.IsPermission(err) { diff --git a/pkg/upgrade/upgrade_test.go b/pkg/upgrade/upgrade_test.go index 259aebba..b4c292fa 100644 --- a/pkg/upgrade/upgrade_test.go +++ b/pkg/upgrade/upgrade_test.go @@ -116,6 +116,26 @@ func TestExtractBinary_InvalidZip(t *testing.T) { // ── download (token host allowlist) ────────────────────────────────────────── +func TestDownload_TokenSentToTrustedHost(t *testing.T) { + var receivedAuth string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + receivedAuth = r.Header.Get("Authorization") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("data")) + })) + defer srv.Close() + + // Temporarily register the test server's host (127.0.0.1) as trusted. + allowedTokenHosts["127.0.0.1"] = true + t.Cleanup(func() { delete(allowedTokenHosts, "127.0.0.1") }) + + t.Setenv("GITHUB_TOKEN", "super-secret-token") + + _, err := download(srv.URL+"/asset.zip", "testuser") + require.NoError(t, err) + assert.Equal(t, "Bearer super-secret-token", receivedAuth, "GITHUB_TOKEN must be forwarded to trusted host") +} + func TestDownload_TokenNotSentToUntrustedHost(t *testing.T) { var receivedAuth string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From b6af0386695baac5116395bd559a1b5641eea4d1 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 19 Jun 2026 16:15:53 -0700 Subject: [PATCH 07/17] revert(upgrade): restore informDebug(debugFlag) pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Logging is disabled when --debug is not passed, consistent with all other kfutil commands. The compliance gap (audit trail silently dropped) is an accepted risk — noted in project memory. --- cmd/upgrade.go | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/cmd/upgrade.go b/cmd/upgrade.go index 2eb5e0a7..85a46052 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -18,7 +18,6 @@ import ( "kfutil/pkg/upgrade" "kfutil/pkg/version" - "github.com/rs/zerolog" "github.com/spf13/cobra" ) @@ -40,14 +39,7 @@ Examples: targetVersion, _ := cmd.Flags().GetString("version") dryRun, _ := cmd.Flags().GetBool("dry-run") force, _ := cmd.Flags().GetBool("force") - // Upgrade always emits Info+ audit events regardless of --debug. - // Other commands use informDebug() which sets Disabled when debug is off, - // but the binary-replacement audit trail must be unconditionally durable. - if debugFlag { - zerolog.SetGlobalLevel(zerolog.DebugLevel) - } else { - zerolog.SetGlobalLevel(zerolog.InfoLevel) - } + informDebug(debugFlag) return upgrade.Run(version.VERSION, targetVersion, dryRun, force) }, } From 9965cdf53c2aca526d9d3afd14719964b081d825 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 19 Jun 2026 16:18:52 -0700 Subject: [PATCH 08/17] fix(upgrade): close fourth-round compliance findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - apply(): add operator param so upgrade.rollback_failed carries identity - upgrade.download_failed: rename url → source_url for SIEM field consistency - upgrade.checksum_download_failed: rename url → sums_url (distinct resource) - upgrade.github_api_response, upgrade.http_response: promote Debug → Info so external API interactions appear in production log pipelines (SOC2 CC9.2) - upgrade.apply_failed: add failure_reason field ("permission_denied" or "apply_error") so incident responders can distinguish failure modes from structured fields rather than error message strings - normalizeTag(): log "latest" instead of "" when no --version was passed so upgrade.run_started and upgrade.fetch_release_failed are unambiguous - TestDownload_NonOKStatus: test that download() errors on non-200 responses --- pkg/upgrade/upgrade.go | 31 +++++++++++++++++++++++-------- pkg/upgrade/upgrade_test.go | 11 +++++++++++ 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index 05838549..0a8ee54b 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -69,6 +69,15 @@ func resolveOperator() string { return u.Username } +// normalizeTag returns the tag as-is, or "latest" when the tag is empty, +// so log fields are unambiguous when no --version flag was passed. +func normalizeTag(tag string) string { + if tag == "" { + return "latest" + } + return tag +} + // Run fetches the target release, verifies the checksum, and replaces the // running binary. targetVersion may be any valid GitHub tag (e.g. "v1.9.0") // or empty to use the latest release. @@ -79,7 +88,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { Str("event", "upgrade.run_started"). Str("operator", operator). Str("current_version", currentVersion). - Str("target_version", targetVersion). + Str("target_version", normalizeTag(targetVersion)). Bool("force", force). Bool("dry_run", dryRun). Msg("upgrade run initiated") @@ -89,7 +98,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { log.Error().Err(err). Str("event", "upgrade.fetch_release_failed"). Str("operator", operator). - Str("tag", targetVersion). + Str("tag", normalizeTag(targetVersion)). Msg("failed to fetch release metadata") return err } @@ -176,8 +185,8 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { if err != nil { log.Error().Err(err). Str("event", "upgrade.download_failed"). - Str("url", archiveURL). Str("operator", operator). + Str("source_url", archiveURL). Msg("archive download failed") return fmt.Errorf("download failed: %w", err) } @@ -197,8 +206,8 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { if err != nil { log.Error().Err(err). Str("event", "upgrade.checksum_download_failed"). - Str("url", sumsURL). Str("operator", operator). + Str("sums_url", sumsURL). Msg("SHA256SUMS download failed") return fmt.Errorf("checksum download failed: %w", err) } @@ -226,7 +235,11 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { Msg("applying binary replacement") fmt.Println("Applying update ...") - if err := apply(bytes.NewReader(binary)); err != nil { + if err := apply(bytes.NewReader(binary), operator); err != nil { + failureReason := "apply_error" + if os.IsPermission(err) { + failureReason = "permission_denied" + } log.Error().Err(err). Str("event", "upgrade.apply_failed"). Str("from_version", currentVer). @@ -234,6 +247,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { Str("executable", exe). Str("operator", operator). Str("source_url", archiveURL). + Str("failure_reason", failureReason). Msg("binary replacement failed") return err } @@ -281,7 +295,7 @@ func fetchReleaseFrom(baseURL, tag, operator string) (*GitHubRelease, error) { } defer resp.Body.Close() - log.Debug(). + log.Info(). Str("event", "upgrade.github_api_response"). Str("url", reqURL). Str("method", http.MethodGet). @@ -341,7 +355,7 @@ func download(rawURL, operator string) ([]byte, error) { } defer resp.Body.Close() - log.Debug(). + log.Info(). Str("event", "upgrade.http_response"). Str("url", rawURL). Str("method", http.MethodGet). @@ -399,12 +413,13 @@ func verifyChecksum(archiveData []byte, assetName string, sumsData []byte) error } // apply replaces the running binary using minio/selfupdate. -func apply(binary io.Reader) error { +func apply(binary io.Reader, operator string) error { err := selfupdate.Apply(binary, selfupdate.Options{}) if err != nil { if rbErr := selfupdate.RollbackError(err); rbErr != nil { log.Error().Err(rbErr). Str("event", "upgrade.rollback_failed"). + Str("operator", operator). Msg("binary replacement failed and rollback also failed — binary may be corrupted") return fmt.Errorf("upgrade failed and rollback also failed: %w", rbErr) } diff --git a/pkg/upgrade/upgrade_test.go b/pkg/upgrade/upgrade_test.go index b4c292fa..62704d85 100644 --- a/pkg/upgrade/upgrade_test.go +++ b/pkg/upgrade/upgrade_test.go @@ -136,6 +136,17 @@ func TestDownload_TokenSentToTrustedHost(t *testing.T) { assert.Equal(t, "Bearer super-secret-token", receivedAuth, "GITHUB_TOKEN must be forwarded to trusted host") } +func TestDownload_NonOKStatus(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + })) + defer srv.Close() + + _, err := download(srv.URL+"/asset.zip", "testuser") + require.Error(t, err) + assert.Contains(t, err.Error(), "403") +} + func TestDownload_TokenNotSentToUntrustedHost(t *testing.T) { var receivedAuth string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From cc8812d379a8c765ac9c0fabecdca9e5be58de30 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 19 Jun 2026 16:22:37 -0700 Subject: [PATCH 09/17] fix(upgrade): close fifth-round compliance findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - fetchReleaseFrom, download: log upgrade.github_api_network_error / upgrade.http_network_error (with latency_ms) on transport failure so network errors are captured even when the HTTP response is never received - fetchReleaseFrom: log upgrade.release_parse_failed when JSON decode fails so the failure category (network vs application) is distinguishable - upgrade.github_api_response, upgrade.http_response: emit at log.Warn when status_code >= 400 so SIEM error thresholds trigger on GitHub API errors (previously always log.Info regardless of status) - apiClient / downloadClient: replace http.DefaultClient (no timeout) with explicit http.Client{Timeout} — 30s for API calls, 5m for binary downloads; ensures timed-out requests produce an error event rather than silently hanging --- pkg/upgrade/upgrade.go | 49 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 43 insertions(+), 6 deletions(-) diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index 0a8ee54b..be9067d5 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -31,6 +31,7 @@ import ( "time" "github.com/minio/selfupdate" + "github.com/rs/zerolog" "github.com/rs/zerolog/log" ) @@ -39,6 +40,12 @@ const ( binaryName = "kfutil" ) +// apiClient is used for GitHub API metadata calls (short, bounded latency). +var apiClient = &http.Client{Timeout: 30 * time.Second} + +// downloadClient is used for binary asset downloads (larger payloads). +var downloadClient = &http.Client{Timeout: 5 * time.Minute} + // allowedTokenHosts are the only hosts to which GITHUB_TOKEN may be forwarded. var allowedTokenHosts = map[string]bool{ "api.github.com": true, @@ -289,14 +296,26 @@ func fetchReleaseFrom(baseURL, tag, operator string) (*GitHubRelease, error) { } start := time.Now() - resp, err := http.DefaultClient.Do(req) + resp, err := apiClient.Do(req) if err != nil { + log.Error().Err(err). + Str("event", "upgrade.github_api_network_error"). + Str("operator", operator). + Str("url", reqURL). + Str("method", http.MethodGet). + Int64("latency_ms", time.Since(start).Milliseconds()). + Msg("GitHub API network request failed") return nil, fmt.Errorf("GitHub API request failed: %w", err) } defer resp.Body.Close() - log.Info(). - Str("event", "upgrade.github_api_response"). + var ev *zerolog.Event + if resp.StatusCode >= 400 { + ev = log.Warn() + } else { + ev = log.Info() + } + ev.Str("event", "upgrade.github_api_response"). Str("url", reqURL). Str("method", http.MethodGet). Int("status_code", resp.StatusCode). @@ -319,6 +338,12 @@ func fetchReleaseFrom(baseURL, tag, operator string) (*GitHubRelease, error) { var rel GitHubRelease if err := json.NewDecoder(resp.Body).Decode(&rel); err != nil { + log.Error().Err(err). + Str("event", "upgrade.release_parse_failed"). + Str("operator", operator). + Str("url", reqURL). + Int("status_code", resp.StatusCode). + Msg("failed to parse GitHub release response body") return nil, fmt.Errorf("failed to parse release response: %w", err) } return &rel, nil @@ -349,14 +374,26 @@ func download(rawURL, operator string) ([]byte, error) { } start := time.Now() - resp, err := http.DefaultClient.Do(req) + resp, err := downloadClient.Do(req) if err != nil { + log.Error().Err(err). + Str("event", "upgrade.http_network_error"). + Str("operator", operator). + Str("url", rawURL). + Str("method", http.MethodGet). + Int64("latency_ms", time.Since(start).Milliseconds()). + Msg("HTTP network request failed") return nil, err } defer resp.Body.Close() - log.Info(). - Str("event", "upgrade.http_response"). + var ev *zerolog.Event + if resp.StatusCode >= 400 { + ev = log.Warn() + } else { + ev = log.Info() + } + ev.Str("event", "upgrade.http_response"). Str("url", rawURL). Str("method", http.MethodGet). Int("status_code", resp.StatusCode). From 65b284447785911446fee47ae4da136d7fbc3eb2 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 19 Jun 2026 16:25:45 -0700 Subject: [PATCH 10/17] fix(upgrade): close sixth-round compliance findings - upgrade.rollback_succeeded: log Info event when apply() fails but rollback succeeds, so auditors can distinguish safe vs corrupted binary outcomes - sanitizeURL(): strip query-string params before logging any URL field; prevents presigned CDN query-string credentials from appearing in log storage - github_token_present: add Bool field to all HTTP log events so credential use is traceable without logging the token value - fetchReleaseFrom: url.PathEscape(tag) before appending to URL so user-supplied --version values cannot alter the request URL structure - extractBinary: wrap rc with io.LimitReader(maxBinaryBytes=100MiB) to bound memory use and produce an explicit error rather than OOM on malformed or zip-bomb archives --- pkg/upgrade/upgrade.go | 63 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 51 insertions(+), 12 deletions(-) diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index be9067d5..87ce8f91 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -38,6 +38,9 @@ import ( const ( releasesURL = "https://api.github.com/repos/Keyfactor/kfutil/releases" binaryName = "kfutil" + + // maxBinaryBytes caps the extracted binary size to prevent OOM on malformed archives. + maxBinaryBytes = 100 * 1024 * 1024 // 100 MiB ) // apiClient is used for GitHub API metadata calls (short, bounded latency). @@ -85,6 +88,18 @@ func normalizeTag(tag string) string { return tag } +// sanitizeURL strips query-string parameters before a URL is written to a log field. +// This prevents presigned CDN URLs (which carry time-limited credentials in their +// query strings) from appearing in log storage. +func sanitizeURL(rawURL string) string { + parsed, err := url.Parse(rawURL) + if err != nil { + return rawURL + } + parsed.RawQuery = "" + return parsed.String() +} + // Run fetches the target release, verifies the checksum, and replaces the // running binary. targetVersion may be any valid GitHub tag (e.g. "v1.9.0") // or empty to use the latest release. @@ -179,7 +194,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { Str("from_version", currentVer). Str("to_version", releaseVer). Str("executable", currentExecutable()). - Str("source_url", archiveURL). + Str("source_url", sanitizeURL(archiveURL)). Msg("dry-run: no changes applied") fmt.Printf("\n[dry-run] Would download : %s\n", archiveURL) fmt.Printf("[dry-run] Would verify : %s\n", sumsURL) @@ -193,7 +208,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { log.Error().Err(err). Str("event", "upgrade.download_failed"). Str("operator", operator). - Str("source_url", archiveURL). + Str("source_url", sanitizeURL(archiveURL)). Msg("archive download failed") return fmt.Errorf("download failed: %w", err) } @@ -203,7 +218,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { log.Error().Err(err). Str("event", "upgrade.extract_failed"). Str("operator", operator). - Str("source_url", archiveURL). + Str("source_url", sanitizeURL(archiveURL)). Msg("binary extraction from archive failed") return fmt.Errorf("extract failed: %w", err) } @@ -214,7 +229,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { log.Error().Err(err). Str("event", "upgrade.checksum_download_failed"). Str("operator", operator). - Str("sums_url", sumsURL). + Str("sums_url", sanitizeURL(sumsURL)). Msg("SHA256SUMS download failed") return fmt.Errorf("checksum download failed: %w", err) } @@ -225,7 +240,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { Str("event", "upgrade.checksum_mismatch"). Str("asset", assetName). Str("operator", operator). - Str("source_url", archiveURL). + Str("source_url", sanitizeURL(archiveURL)). Msg("checksum verification failed") return err } @@ -238,7 +253,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { Str("to_version", releaseVer). Str("executable", exe). Str("operator", operator). - Str("source_url", archiveURL). + Str("source_url", sanitizeURL(archiveURL)). Msg("applying binary replacement") fmt.Println("Applying update ...") @@ -253,7 +268,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { Str("to_version", releaseVer). Str("executable", exe). Str("operator", operator). - Str("source_url", archiveURL). + Str("source_url", sanitizeURL(archiveURL)). Str("failure_reason", failureReason). Msg("binary replacement failed") return err @@ -265,7 +280,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { Str("to_version", releaseVer). Str("executable", exe). Str("operator", operator). - Str("source_url", archiveURL). + Str("source_url", sanitizeURL(archiveURL)). Msg("binary replacement complete") fmt.Printf("Upgraded to %s.\n", release.TagName) @@ -283,7 +298,9 @@ func fetchReleaseFrom(baseURL, tag, operator string) (*GitHubRelease, error) { if tag == "" { reqURL = baseURL + "/latest" } else { - reqURL = baseURL + "/tags/" + tag + // URL-encode the tag so path-separator or query characters in user + // input cannot alter the request URL structure. + reqURL = baseURL + "/tags/" + url.PathEscape(tag) } req, err := http.NewRequest(http.MethodGet, reqURL, nil) @@ -291,7 +308,9 @@ func fetchReleaseFrom(baseURL, tag, operator string) (*GitHubRelease, error) { return nil, err } req.Header.Set("Accept", "application/vnd.github+json") + tokenPresent := false if tok := os.Getenv("GITHUB_TOKEN"); tok != "" { + tokenPresent = true req.Header.Set("Authorization", "Bearer "+tok) } @@ -304,6 +323,7 @@ func fetchReleaseFrom(baseURL, tag, operator string) (*GitHubRelease, error) { Str("url", reqURL). Str("method", http.MethodGet). Int64("latency_ms", time.Since(start).Milliseconds()). + Bool("github_token_present", tokenPresent). Msg("GitHub API network request failed") return nil, fmt.Errorf("GitHub API request failed: %w", err) } @@ -320,6 +340,7 @@ func fetchReleaseFrom(baseURL, tag, operator string) (*GitHubRelease, error) { Str("method", http.MethodGet). Int("status_code", resp.StatusCode). Int64("latency_ms", time.Since(start).Milliseconds()). + Bool("github_token_present", tokenPresent). Str("operator", operator). Msg("GitHub API response received") @@ -367,8 +388,10 @@ func download(rawURL, operator string) ([]byte, error) { return nil, err } + tokenPresent := false if tok := os.Getenv("GITHUB_TOKEN"); tok != "" { if parsed, err := url.Parse(rawURL); err == nil && allowedTokenHosts[parsed.Hostname()] { + tokenPresent = true req.Header.Set("Authorization", "Bearer "+tok) } } @@ -379,9 +402,10 @@ func download(rawURL, operator string) ([]byte, error) { log.Error().Err(err). Str("event", "upgrade.http_network_error"). Str("operator", operator). - Str("url", rawURL). + Str("url", sanitizeURL(rawURL)). Str("method", http.MethodGet). Int64("latency_ms", time.Since(start).Milliseconds()). + Bool("github_token_present", tokenPresent). Msg("HTTP network request failed") return nil, err } @@ -394,10 +418,11 @@ func download(rawURL, operator string) ([]byte, error) { ev = log.Info() } ev.Str("event", "upgrade.http_response"). - Str("url", rawURL). + Str("url", sanitizeURL(rawURL)). Str("method", http.MethodGet). Int("status_code", resp.StatusCode). Int64("latency_ms", time.Since(start).Milliseconds()). + Bool("github_token_present", tokenPresent). Str("operator", operator). Msg("HTTP response received") @@ -423,7 +448,15 @@ func extractBinary(zipData []byte, name string) ([]byte, error) { return nil, err } defer rc.Close() - return io.ReadAll(rc) + // LimitReader prevents OOM on malformed or zip-bomb archives. + data, err := io.ReadAll(io.LimitReader(rc, maxBinaryBytes)) + if err != nil { + return nil, err + } + if int64(len(data)) >= maxBinaryBytes { + return nil, fmt.Errorf("binary %q exceeds maximum allowed size (%d bytes)", name, maxBinaryBytes) + } + return data, nil } } } @@ -460,6 +493,12 @@ func apply(binary io.Reader, operator string) error { Msg("binary replacement failed and rollback also failed — binary may be corrupted") return fmt.Errorf("upgrade failed and rollback also failed: %w", rbErr) } + // Rollback succeeded — log so auditors can distinguish this outcome + // from a rollback failure. + log.Info(). + Str("event", "upgrade.rollback_succeeded"). + Str("operator", operator). + Msg("apply failed but binary was successfully rolled back to prior version") if os.IsPermission(err) { return fmt.Errorf("permission denied writing to %s\nTry re-running with elevated privileges (sudo kfutil upgrade)", currentExecutable()) } From 58c6dc3d6dd2fbfdd4df78865641cf0bca30a811 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 19 Jun 2026 16:29:09 -0700 Subject: [PATCH 11/17] fix(upgrade): close seventh-round compliance findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - sanitizeURL applied to reqURL in fetchReleaseFrom (all 3 log sites: github_api_network_error, github_api_response, release_parse_failed); consistent with download() which already sanitized its URL fields - download(): wrap resp.Body with io.LimitReader(maxBinaryBytes) before io.ReadAll so the raw HTTP response (archive + SUMS) is bounded; closes the gap where the zip-level limit in extractBinary was not yet applied to the network layer - upgrade.applying: add Bool("force", force) field so the event is self-contained for change management audit without requiring cross-event lookup to upgrade.run_started - currentExecutable(): emit upgrade.executable_resolution_failed warning when os.Executable() fails so auditors can distinguish real path from the "kfutil" fallback sentinel in log records Skipped (accepted): - C-1: logs silent without --debug — user explicitly accepted this risk - H-1: ConsoleWriter destroys structured fields — codebase-wide infra concern - M-1: duplicate fetch_release_failed events — intentional layering; removing the Run-level log would leave HTTP status error paths with no audit record --- pkg/upgrade/upgrade.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index 87ce8f91..d14231ce 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -254,6 +254,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { Str("executable", exe). Str("operator", operator). Str("source_url", sanitizeURL(archiveURL)). + Bool("force", force). Msg("applying binary replacement") fmt.Println("Applying update ...") @@ -320,7 +321,7 @@ func fetchReleaseFrom(baseURL, tag, operator string) (*GitHubRelease, error) { log.Error().Err(err). Str("event", "upgrade.github_api_network_error"). Str("operator", operator). - Str("url", reqURL). + Str("url", sanitizeURL(reqURL)). Str("method", http.MethodGet). Int64("latency_ms", time.Since(start).Milliseconds()). Bool("github_token_present", tokenPresent). @@ -336,7 +337,7 @@ func fetchReleaseFrom(baseURL, tag, operator string) (*GitHubRelease, error) { ev = log.Info() } ev.Str("event", "upgrade.github_api_response"). - Str("url", reqURL). + Str("url", sanitizeURL(reqURL)). Str("method", http.MethodGet). Int("status_code", resp.StatusCode). Int64("latency_ms", time.Since(start).Milliseconds()). @@ -362,7 +363,7 @@ func fetchReleaseFrom(baseURL, tag, operator string) (*GitHubRelease, error) { log.Error().Err(err). Str("event", "upgrade.release_parse_failed"). Str("operator", operator). - Str("url", reqURL). + Str("url", sanitizeURL(reqURL)). Int("status_code", resp.StatusCode). Msg("failed to parse GitHub release response body") return nil, fmt.Errorf("failed to parse release response: %w", err) @@ -429,7 +430,7 @@ func download(rawURL, operator string) ([]byte, error) { if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("HTTP %d from %s", resp.StatusCode, rawURL) } - return io.ReadAll(resp.Body) + return io.ReadAll(io.LimitReader(resp.Body, maxBinaryBytes)) } // extractBinary unpacks the binary named or .exe from a zip archive. @@ -510,6 +511,9 @@ func apply(binary io.Reader, operator string) error { func currentExecutable() string { exe, err := os.Executable() if err != nil { + log.Warn().Err(err). + Str("event", "upgrade.executable_resolution_failed"). + Msg("could not resolve executable path — audit logs will record 'kfutil' as executable") return "kfutil" } return exe From ce589b3a20240978530778fd783bcf4389bc33d1 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 19 Jun 2026 16:30:59 -0700 Subject: [PATCH 12/17] fix(upgrade): close eighth-round compliance findings - dry-run fmt.Printf: use sanitizeURL() for archiveURL and sumsURL so presigned CDN query-string credentials never reach console output - download() error string: use sanitizeURL(rawURL) in the HTTP status error so query-string credentials are stripped from stderr / error aggregators - fetchReleaseFrom: log upgrade.github_api_request_build_failed on http.NewRequest error so malformed-URL failures leave an audit trace - download: log upgrade.http_request_build_failed on http.NewRequest error Skipped (accepted / out of scope): - M-1: force_override test requires Run() end-to-end mock harness --- pkg/upgrade/upgrade.go | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index d14231ce..5b07028c 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -196,8 +196,8 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { Str("executable", currentExecutable()). Str("source_url", sanitizeURL(archiveURL)). Msg("dry-run: no changes applied") - fmt.Printf("\n[dry-run] Would download : %s\n", archiveURL) - fmt.Printf("[dry-run] Would verify : %s\n", sumsURL) + fmt.Printf("\n[dry-run] Would download : %s\n", sanitizeURL(archiveURL)) + fmt.Printf("[dry-run] Would verify : %s\n", sanitizeURL(sumsURL)) fmt.Printf("[dry-run] Would replace : %s\n", currentExecutable()) return nil } @@ -306,6 +306,11 @@ func fetchReleaseFrom(baseURL, tag, operator string) (*GitHubRelease, error) { req, err := http.NewRequest(http.MethodGet, reqURL, nil) if err != nil { + log.Warn().Err(err). + Str("event", "upgrade.github_api_request_build_failed"). + Str("operator", operator). + Str("url", sanitizeURL(reqURL)). + Msg("failed to construct GitHub API request") return nil, err } req.Header.Set("Accept", "application/vnd.github+json") @@ -386,6 +391,11 @@ func archiveAssetName(tag string) string { func download(rawURL, operator string) ([]byte, error) { req, err := http.NewRequest(http.MethodGet, rawURL, nil) if err != nil { + log.Warn().Err(err). + Str("event", "upgrade.http_request_build_failed"). + Str("operator", operator). + Str("url", sanitizeURL(rawURL)). + Msg("failed to construct HTTP request") return nil, err } @@ -428,7 +438,7 @@ func download(rawURL, operator string) ([]byte, error) { Msg("HTTP response received") if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("HTTP %d from %s", resp.StatusCode, rawURL) + return nil, fmt.Errorf("HTTP %d from %s", resp.StatusCode, sanitizeURL(rawURL)) } return io.ReadAll(io.LimitReader(resp.Body, maxBinaryBytes)) } From 0bb5ebc08abbf2799a8c5e57d55f30100663b1c5 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 19 Jun 2026 16:43:52 -0700 Subject: [PATCH 13/17] fix(upgrade): close ninth-round compliance findings H1: add from_version, to_version, executable to rollback_failed and rollback_succeeded by threading those values into apply(); all three are in scope in Run() where the call site lives H2: add operator param to currentExecutable() so upgrade.executable_resolution_failed carries operator for identity correlation M1: emit upgrade.checksum_verified Info event (with operator, asset, source_url) after verifyChecksum returns nil, closing the gap in the checksum success audit trail M2: add TestSanitizeURL_StripsQueryParams and TestSanitizeURL_ParseErrorFallback to cover presigned-URL stripping and parse-error fallback behaviour --- pkg/upgrade/upgrade.go | 55 +++++++++++++++++++++++-------------- pkg/upgrade/upgrade_test.go | 19 +++++++++++++ 2 files changed, 54 insertions(+), 20 deletions(-) diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index 5b07028c..db55708f 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -88,9 +88,9 @@ func normalizeTag(tag string) string { return tag } -// sanitizeURL strips query-string parameters before a URL is written to a log field. -// This prevents presigned CDN URLs (which carry time-limited credentials in their -// query strings) from appearing in log storage. +// sanitizeURL strips query-string parameters before a URL is written to a log +// field or console output. This prevents presigned CDN URLs (which carry +// time-limited credentials in their query strings) from appearing in logs. func sanitizeURL(rawURL string) string { parsed, err := url.Parse(rawURL) if err != nil { @@ -100,6 +100,19 @@ func sanitizeURL(rawURL string) string { return parsed.String() } +// currentExecutable resolves the path to the running binary for audit log fields. +func currentExecutable(operator string) string { + exe, err := os.Executable() + if err != nil { + log.Warn().Err(err). + Str("event", "upgrade.executable_resolution_failed"). + Str("operator", operator). + Msg("could not resolve executable path — audit logs will record 'kfutil' as executable") + return "kfutil" + } + return exe +} + // Run fetches the target release, verifies the checksum, and replaces the // running binary. targetVersion may be any valid GitHub tag (e.g. "v1.9.0") // or empty to use the latest release. @@ -187,18 +200,20 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { return fmt.Errorf("release %s has no SHA256SUMS asset — upgrade aborted", release.TagName) } + exe := currentExecutable(operator) + if dryRun { log.Info(). Str("event", "upgrade.dry_run"). Str("operator", operator). Str("from_version", currentVer). Str("to_version", releaseVer). - Str("executable", currentExecutable()). + Str("executable", exe). Str("source_url", sanitizeURL(archiveURL)). Msg("dry-run: no changes applied") fmt.Printf("\n[dry-run] Would download : %s\n", sanitizeURL(archiveURL)) fmt.Printf("[dry-run] Would verify : %s\n", sanitizeURL(sumsURL)) - fmt.Printf("[dry-run] Would replace : %s\n", currentExecutable()) + fmt.Printf("[dry-run] Would replace : %s\n", exe) return nil } @@ -244,9 +259,14 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { Msg("checksum verification failed") return err } + log.Info(). + Str("event", "upgrade.checksum_verified"). + Str("operator", operator). + Str("asset", assetName). + Str("source_url", sanitizeURL(archiveURL)). + Msg("SHA-256 checksum verified successfully") fmt.Println("Checksum OK.") - exe := currentExecutable() log.Info(). Str("event", "upgrade.applying"). Str("from_version", currentVer). @@ -258,7 +278,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { Msg("applying binary replacement") fmt.Println("Applying update ...") - if err := apply(bytes.NewReader(binary), operator); err != nil { + if err := apply(bytes.NewReader(binary), operator, currentVer, releaseVer, exe); err != nil { failureReason := "apply_error" if os.IsPermission(err) { failureReason = "permission_denied" @@ -494,13 +514,16 @@ func verifyChecksum(archiveData []byte, assetName string, sumsData []byte) error } // apply replaces the running binary using minio/selfupdate. -func apply(binary io.Reader, operator string) error { +func apply(binary io.Reader, operator, fromVersion, toVersion, exe string) error { err := selfupdate.Apply(binary, selfupdate.Options{}) if err != nil { if rbErr := selfupdate.RollbackError(err); rbErr != nil { log.Error().Err(rbErr). Str("event", "upgrade.rollback_failed"). Str("operator", operator). + Str("from_version", fromVersion). + Str("to_version", toVersion). + Str("executable", exe). Msg("binary replacement failed and rollback also failed — binary may be corrupted") return fmt.Errorf("upgrade failed and rollback also failed: %w", rbErr) } @@ -509,26 +532,18 @@ func apply(binary io.Reader, operator string) error { log.Info(). Str("event", "upgrade.rollback_succeeded"). Str("operator", operator). + Str("from_version", fromVersion). + Str("to_version", toVersion). + Str("executable", exe). Msg("apply failed but binary was successfully rolled back to prior version") if os.IsPermission(err) { - return fmt.Errorf("permission denied writing to %s\nTry re-running with elevated privileges (sudo kfutil upgrade)", currentExecutable()) + return fmt.Errorf("permission denied writing to %s\nTry re-running with elevated privileges (sudo kfutil upgrade)", exe) } return fmt.Errorf("upgrade failed: %w", err) } return nil } -func currentExecutable() string { - exe, err := os.Executable() - if err != nil { - log.Warn().Err(err). - Str("event", "upgrade.executable_resolution_failed"). - Msg("could not resolve executable path — audit logs will record 'kfutil' as executable") - return "kfutil" - } - return exe -} - func listAssets(assets []GitHubAsset) string { var sb strings.Builder for _, a := range assets { diff --git a/pkg/upgrade/upgrade_test.go b/pkg/upgrade/upgrade_test.go index 62704d85..bf26a5ad 100644 --- a/pkg/upgrade/upgrade_test.go +++ b/pkg/upgrade/upgrade_test.go @@ -222,3 +222,22 @@ func TestFetchRelease_RateLimited(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "rate limited") } + +// ── sanitizeURL ─────────────────────────────────────────────────────────────── + +func TestSanitizeURL_StripsQueryParams(t *testing.T) { + raw := "https://objects.githubusercontent.com/github-production-release-asset/abc123/kfutil.zip?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKID&X-Amz-Signature=deadbeef" + got := sanitizeURL(raw) + assert.Equal(t, "https://objects.githubusercontent.com/github-production-release-asset/abc123/kfutil.zip", got) + assert.NotContains(t, got, "X-Amz") + assert.NotContains(t, got, "Signature") + assert.NotContains(t, got, "?") +} + +func TestSanitizeURL_ParseErrorFallback(t *testing.T) { + // An unparseable URL must be returned as-is — better to log a weird string + // than to panic or drop the URL entirely from audit output. + raw := "://not a valid url" + got := sanitizeURL(raw) + assert.Equal(t, raw, got) +} From 8b426f518ca8ee8f276bf0031a7795cc50ddea81 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 19 Jun 2026 16:46:25 -0700 Subject: [PATCH 14/17] fix(upgrade): close tenth-round compliance findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit H1: emit upgrade.github_api_rejected Error event before each non-200 return in fetchReleaseFrom switch — gives SIEM rules an outcome-specific event name distinct from upgrade.github_api_response H2: emit upgrade.http_request_failed Error event before the non-200 return in download — same outcome-specificity fix for asset fetches M1: upgrade.rollback_succeeded changed from Info to Warn — a rollback is a degraded-state recovery action, not a routine operation M2: emit upgrade.apply_started Info inside apply() before selfupdate.Apply() is called — records the exact point the filesystem write commences M5: add TestExtractBinary_ExceedsMaxSize to assert the maxBinaryBytes size cap returns an error rather than an oversized slice Low: strip URL fragments in sanitizeURL(); add TestSanitizeURL_StripsFragment to cover the new behaviour --- pkg/upgrade/upgrade.go | 45 ++++++++++++++++++++++++++++++++----- pkg/upgrade/upgrade_test.go | 35 +++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 5 deletions(-) diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index db55708f..6a314ba2 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -97,6 +97,7 @@ func sanitizeURL(rawURL string) string { return rawURL } parsed.RawQuery = "" + parsed.Fragment = "" return parsed.String() } @@ -373,13 +374,34 @@ func fetchReleaseFrom(baseURL, tag, operator string) (*GitHubRelease, error) { switch resp.StatusCode { case http.StatusOK: case http.StatusNotFound: + var errMsg string if tag != "" { - return nil, fmt.Errorf("release tag %q not found", tag) + errMsg = fmt.Sprintf("release tag %q not found", tag) + } else { + errMsg = "no releases found for this repository" } - return nil, fmt.Errorf("no releases found for this repository") + log.Error(). + Str("event", "upgrade.github_api_rejected"). + Str("operator", operator). + Int("status_code", resp.StatusCode). + Str("url", sanitizeURL(reqURL)). + Msg("GitHub API rejected the upgrade request") + return nil, fmt.Errorf("%s", errMsg) case http.StatusForbidden, http.StatusTooManyRequests: + log.Error(). + Str("event", "upgrade.github_api_rejected"). + Str("operator", operator). + Int("status_code", resp.StatusCode). + Str("url", sanitizeURL(reqURL)). + Msg("GitHub API rejected the upgrade request") return nil, fmt.Errorf("GitHub API rate limited (HTTP %d) — set GITHUB_TOKEN to increase limits", resp.StatusCode) default: + log.Error(). + Str("event", "upgrade.github_api_rejected"). + Str("operator", operator). + Int("status_code", resp.StatusCode). + Str("url", sanitizeURL(reqURL)). + Msg("GitHub API rejected the upgrade request") return nil, fmt.Errorf("GitHub API returned HTTP %d", resp.StatusCode) } @@ -458,6 +480,12 @@ func download(rawURL, operator string) ([]byte, error) { Msg("HTTP response received") if resp.StatusCode != http.StatusOK { + log.Error(). + Str("event", "upgrade.http_request_failed"). + Str("operator", operator). + Int("status_code", resp.StatusCode). + Str("url", sanitizeURL(rawURL)). + Msg("HTTP download request returned non-success status") return nil, fmt.Errorf("HTTP %d from %s", resp.StatusCode, sanitizeURL(rawURL)) } return io.ReadAll(io.LimitReader(resp.Body, maxBinaryBytes)) @@ -515,6 +543,13 @@ func verifyChecksum(archiveData []byte, assetName string, sumsData []byte) error // apply replaces the running binary using minio/selfupdate. func apply(binary io.Reader, operator, fromVersion, toVersion, exe string) error { + log.Info(). + Str("event", "upgrade.apply_started"). + Str("operator", operator). + Str("from_version", fromVersion). + Str("to_version", toVersion). + Str("executable", exe). + Msg("binary write commencing via selfupdate") err := selfupdate.Apply(binary, selfupdate.Options{}) if err != nil { if rbErr := selfupdate.RollbackError(err); rbErr != nil { @@ -527,9 +562,9 @@ func apply(binary io.Reader, operator, fromVersion, toVersion, exe string) error Msg("binary replacement failed and rollback also failed — binary may be corrupted") return fmt.Errorf("upgrade failed and rollback also failed: %w", rbErr) } - // Rollback succeeded — log so auditors can distinguish this outcome - // from a rollback failure. - log.Info(). + // Rollback succeeded — log at Warn so auditors can distinguish a + // recovery action from routine Info events. + log.Warn(). Str("event", "upgrade.rollback_succeeded"). Str("operator", operator). Str("from_version", fromVersion). diff --git a/pkg/upgrade/upgrade_test.go b/pkg/upgrade/upgrade_test.go index bf26a5ad..b5331e60 100644 --- a/pkg/upgrade/upgrade_test.go +++ b/pkg/upgrade/upgrade_test.go @@ -241,3 +241,38 @@ func TestSanitizeURL_ParseErrorFallback(t *testing.T) { got := sanitizeURL(raw) assert.Equal(t, raw, got) } + +func TestSanitizeURL_StripsFragment(t *testing.T) { + raw := "https://github.com/Keyfactor/kfutil/releases/tag/v1.9.0#readme" + got := sanitizeURL(raw) + assert.NotContains(t, got, "#") + assert.NotContains(t, got, "readme") +} + +// ── extractBinary size cap ──────────────────────────────────────────────────── + +func TestExtractBinary_ExceedsMaxSize(t *testing.T) { + // Build a zip with an entry that is exactly maxBinaryBytes bytes. + // extractBinary must return an error rather than returning a slice of that size. + var buf bytes.Buffer + w := zip.NewWriter(&buf) + f, err := w.Create("kfutil") + require.NoError(t, err) + // Write maxBinaryBytes bytes — this will trigger the size-cap check. + chunk := make([]byte, 4096) + written := 0 + for written < maxBinaryBytes { + n := maxBinaryBytes - written + if n > len(chunk) { + n = len(chunk) + } + _, err = f.Write(chunk[:n]) + require.NoError(t, err) + written += n + } + require.NoError(t, w.Close()) + + _, err = extractBinary(buf.Bytes(), "kfutil") + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeds maximum allowed size") +} From c25d185e3d4465172c27ca607302e5854a2794ed Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 19 Jun 2026 16:49:17 -0700 Subject: [PATCH 15/17] fix(upgrade): close eleventh-round compliance findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit H1: add source_url to upgrade.rollback_failed and upgrade.rollback_succeeded by adding sourceURL param to apply(); forensic reconstruction of a failed upgrade can now trace the artifact without cross-event correlation H2: detect silently-truncated downloads — after io.ReadAll+LimitReader, check len(data) >= maxBinaryBytes and emit upgrade.download_size_limit_reached Error before returning an error; prevents a 100 MiB-capped payload from reaching checksum verification with a misleading mismatch event M1: add TestDownload_BodyTruncatedAtLimit to assert the HTTP body size cap rejects oversized payloads with the correct error message M2: add comments on the paired Warn/Error emit blocks in fetchReleaseFrom and download explaining why both events are intentional (latency_ms lives in the Warn, the control decision in the Error) --- pkg/upgrade/upgrade.go | 31 ++++++++++++++++++++++++++++--- pkg/upgrade/upgrade_test.go | 23 +++++++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index 6a314ba2..21f5267a 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -279,7 +279,7 @@ func Run(currentVersion, targetVersion string, dryRun, force bool) error { Msg("applying binary replacement") fmt.Println("Applying update ...") - if err := apply(bytes.NewReader(binary), operator, currentVer, releaseVer, exe); err != nil { + if err := apply(bytes.NewReader(binary), operator, currentVer, releaseVer, exe, sanitizeURL(archiveURL)); err != nil { failureReason := "apply_error" if os.IsPermission(err) { failureReason = "permission_denied" @@ -356,6 +356,10 @@ func fetchReleaseFrom(baseURL, tag, operator string) (*GitHubRelease, error) { } defer resp.Body.Close() + // Two events fire for non-200 responses: github_api_response (Warn) captures + // transport telemetry (latency, headers); github_api_rejected (Error) captures + // the control decision. Both are intentional — removing the Warn event would + // drop latency_ms from the audit trail. var ev *zerolog.Event if resp.StatusCode >= 400 { ev = log.Warn() @@ -464,6 +468,10 @@ func download(rawURL, operator string) ([]byte, error) { } defer resp.Body.Close() + // Two events fire for non-200 responses: http_response (Warn) captures + // transport telemetry (latency, headers); http_request_failed (Error) captures + // the control decision. Both are intentional — removing the Warn event would + // drop latency_ms from the audit trail. var ev *zerolog.Event if resp.StatusCode >= 400 { ev = log.Warn() @@ -488,7 +496,21 @@ func download(rawURL, operator string) ([]byte, error) { Msg("HTTP download request returned non-success status") return nil, fmt.Errorf("HTTP %d from %s", resp.StatusCode, sanitizeURL(rawURL)) } - return io.ReadAll(io.LimitReader(resp.Body, maxBinaryBytes)) + data, err := io.ReadAll(io.LimitReader(resp.Body, maxBinaryBytes)) + if err != nil { + return nil, err + } + // LimitReader silently caps at maxBinaryBytes; detect and reject truncated payloads. + if int64(len(data)) >= maxBinaryBytes { + log.Error(). + Str("event", "upgrade.download_size_limit_reached"). + Str("operator", operator). + Str("url", sanitizeURL(rawURL)). + Int64("limit_bytes", maxBinaryBytes). + Msg("HTTP response body reached size cap — download rejected") + return nil, fmt.Errorf("response body from %s exceeded maximum allowed size (%d bytes)", sanitizeURL(rawURL), maxBinaryBytes) + } + return data, nil } // extractBinary unpacks the binary named or .exe from a zip archive. @@ -542,13 +564,14 @@ func verifyChecksum(archiveData []byte, assetName string, sumsData []byte) error } // apply replaces the running binary using minio/selfupdate. -func apply(binary io.Reader, operator, fromVersion, toVersion, exe string) error { +func apply(binary io.Reader, operator, fromVersion, toVersion, exe, sourceURL string) error { log.Info(). Str("event", "upgrade.apply_started"). Str("operator", operator). Str("from_version", fromVersion). Str("to_version", toVersion). Str("executable", exe). + Str("source_url", sourceURL). Msg("binary write commencing via selfupdate") err := selfupdate.Apply(binary, selfupdate.Options{}) if err != nil { @@ -559,6 +582,7 @@ func apply(binary io.Reader, operator, fromVersion, toVersion, exe string) error Str("from_version", fromVersion). Str("to_version", toVersion). Str("executable", exe). + Str("source_url", sourceURL). Msg("binary replacement failed and rollback also failed — binary may be corrupted") return fmt.Errorf("upgrade failed and rollback also failed: %w", rbErr) } @@ -570,6 +594,7 @@ func apply(binary io.Reader, operator, fromVersion, toVersion, exe string) error Str("from_version", fromVersion). Str("to_version", toVersion). Str("executable", exe). + Str("source_url", sourceURL). Msg("apply failed but binary was successfully rolled back to prior version") if os.IsPermission(err) { return fmt.Errorf("permission denied writing to %s\nTry re-running with elevated privileges (sudo kfutil upgrade)", exe) diff --git a/pkg/upgrade/upgrade_test.go b/pkg/upgrade/upgrade_test.go index b5331e60..25efd05e 100644 --- a/pkg/upgrade/upgrade_test.go +++ b/pkg/upgrade/upgrade_test.go @@ -147,6 +147,29 @@ func TestDownload_NonOKStatus(t *testing.T) { assert.Contains(t, err.Error(), "403") } +func TestDownload_BodyTruncatedAtLimit(t *testing.T) { + // Serve exactly maxBinaryBytes bytes — download must return an error rather + // than silently returning a truncated payload that would later fail checksum. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + chunk := make([]byte, 4096) + written := 0 + for written < maxBinaryBytes { + n := maxBinaryBytes - written + if n > len(chunk) { + n = len(chunk) + } + _, _ = w.Write(chunk[:n]) + written += n + } + })) + defer srv.Close() + + _, err := download(srv.URL+"/big.zip", "testuser") + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeded maximum allowed size") +} + func TestDownload_TokenNotSentToUntrustedHost(t *testing.T) { var receivedAuth string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From 9d451540a4eeee3bbd90d379c7d345ea7a913c77 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 19 Jun 2026 16:51:45 -0700 Subject: [PATCH 16/17] fix(upgrade): close twelfth-round compliance findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit M1: emit upgrade.http_body_read_error Error before returning a mid-body read failure; distinguishes a truncated transfer from a refused connection in forensic reconstruction (SOC2 CC7.3) M3: add captureLog() test helper that redirects the global zerolog logger to a buffer for test duration; use it to assert upgrade.http_request_failed, upgrade.download_size_limit_reached, and upgrade.github_api_rejected are actually emitted — test coverage now proves audit events fire, not just that errors propagate M4: add .Err(err) to upgrade.rollback_succeeded so the root cause of the apply failure is in the structured log record alongside the rollback outcome (SOC1 completeness and accuracy) --- pkg/upgrade/upgrade.go | 10 ++++++++-- pkg/upgrade/upgrade_test.go | 20 ++++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index 21f5267a..96a0a7a2 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -498,6 +498,11 @@ func download(rawURL, operator string) ([]byte, error) { } data, err := io.ReadAll(io.LimitReader(resp.Body, maxBinaryBytes)) if err != nil { + log.Error().Err(err). + Str("event", "upgrade.http_body_read_error"). + Str("operator", operator). + Str("url", sanitizeURL(rawURL)). + Msg("HTTP response body read failed after successful connection") return nil, err } // LimitReader silently caps at maxBinaryBytes; detect and reject truncated payloads. @@ -587,8 +592,9 @@ func apply(binary io.Reader, operator, fromVersion, toVersion, exe, sourceURL st return fmt.Errorf("upgrade failed and rollback also failed: %w", rbErr) } // Rollback succeeded — log at Warn so auditors can distinguish a - // recovery action from routine Info events. - log.Warn(). + // recovery action from routine Info events. Include the original error + // so the root cause is in the structured record (SOC1 completeness). + log.Warn().Err(err). Str("event", "upgrade.rollback_succeeded"). Str("operator", operator). Str("from_version", fromVersion). diff --git a/pkg/upgrade/upgrade_test.go b/pkg/upgrade/upgrade_test.go index 25efd05e..1d93bd32 100644 --- a/pkg/upgrade/upgrade_test.go +++ b/pkg/upgrade/upgrade_test.go @@ -26,10 +26,24 @@ import ( "runtime" "testing" + "github.com/rs/zerolog" + "github.com/rs/zerolog/log" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +// captureLog redirects the global zerolog logger to a buffer for the duration +// of the test, then restores it. Returns the buffer so callers can assert on +// logged event names. +func captureLog(t *testing.T) *bytes.Buffer { + t.Helper() + var buf bytes.Buffer + orig := log.Logger + log.Logger = zerolog.New(&buf) + t.Cleanup(func() { log.Logger = orig }) + return &buf +} + // ── archiveAssetName ────────────────────────────────────────────────────────── func TestArchiveAssetName(t *testing.T) { @@ -137,6 +151,7 @@ func TestDownload_TokenSentToTrustedHost(t *testing.T) { } func TestDownload_NonOKStatus(t *testing.T) { + logBuf := captureLog(t) srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusForbidden) })) @@ -145,11 +160,13 @@ func TestDownload_NonOKStatus(t *testing.T) { _, err := download(srv.URL+"/asset.zip", "testuser") require.Error(t, err) assert.Contains(t, err.Error(), "403") + assert.Contains(t, logBuf.String(), "upgrade.http_request_failed", "audit event must fire on non-200 download") } func TestDownload_BodyTruncatedAtLimit(t *testing.T) { // Serve exactly maxBinaryBytes bytes — download must return an error rather // than silently returning a truncated payload that would later fail checksum. + logBuf := captureLog(t) srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) chunk := make([]byte, 4096) @@ -168,6 +185,7 @@ func TestDownload_BodyTruncatedAtLimit(t *testing.T) { _, err := download(srv.URL+"/big.zip", "testuser") require.Error(t, err) assert.Contains(t, err.Error(), "exceeded maximum allowed size") + assert.Contains(t, logBuf.String(), "upgrade.download_size_limit_reached", "audit event must fire on body size cap") } func TestDownload_TokenNotSentToUntrustedHost(t *testing.T) { @@ -238,12 +256,14 @@ func TestFetchRelease_NotFound(t *testing.T) { } func TestFetchRelease_RateLimited(t *testing.T) { + logBuf := captureLog(t) srv := mockReleaseServer(t, "", http.StatusForbidden) defer srv.Close() _, err := fetchReleaseFrom(srv.URL, "", "testuser") require.Error(t, err) assert.Contains(t, err.Error(), "rate limited") + assert.Contains(t, logBuf.String(), "upgrade.github_api_rejected", "audit event must fire on rate-limited response") } // ── sanitizeURL ─────────────────────────────────────────────────────────────── From 67c603c13cba7181a6abe7a5a092c60db9829fda Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 19 Jun 2026 16:55:43 -0700 Subject: [PATCH 17/17] =?UTF-8?q?fix(upgrade):=20close=20thirteenth-round?= =?UTF-8?q?=20compliance=20findings=20=E2=80=94=20stopping=20loop?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit H1: capture applyErr before RollbackError unwraps it; add apply_error string field to upgrade.rollback_failed so forensic reconstruction has the root cause alongside the rollback failure (SOC1 completeness) M3: split github_token_present into github_token_present (env var set?) and github_token_forwarded (actually sent in header?) in both fetchReleaseFrom and download; log events for untrusted-host downloads now correctly record that a token existed but was withheld, rather than masking token availability (SOC2 CC6.6 boundary protection telemetry) M4: add comment to TestDownload_TokenSentToTrustedHost explaining why these two tests must not run in parallel (allowedTokenHosts mutation) --- pkg/upgrade/upgrade.go | 24 +++++++++++++++++------- pkg/upgrade/upgrade_test.go | 4 ++++ 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index 96a0a7a2..f2113577 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -335,10 +335,12 @@ func fetchReleaseFrom(baseURL, tag, operator string) (*GitHubRelease, error) { return nil, err } req.Header.Set("Accept", "application/vnd.github+json") - tokenPresent := false - if tok := os.Getenv("GITHUB_TOKEN"); tok != "" { - tokenPresent = true - req.Header.Set("Authorization", "Bearer "+tok) + tokenPresent := os.Getenv("GITHUB_TOKEN") != "" + tokenForwarded := false + if tokenPresent { + // GitHub API is always a trusted host — token is always forwarded when present. + tokenForwarded = true + req.Header.Set("Authorization", "Bearer "+os.Getenv("GITHUB_TOKEN")) } start := time.Now() @@ -351,6 +353,7 @@ func fetchReleaseFrom(baseURL, tag, operator string) (*GitHubRelease, error) { Str("method", http.MethodGet). Int64("latency_ms", time.Since(start).Milliseconds()). Bool("github_token_present", tokenPresent). + Bool("github_token_forwarded", tokenForwarded). Msg("GitHub API network request failed") return nil, fmt.Errorf("GitHub API request failed: %w", err) } @@ -372,6 +375,7 @@ func fetchReleaseFrom(baseURL, tag, operator string) (*GitHubRelease, error) { Int("status_code", resp.StatusCode). Int64("latency_ms", time.Since(start).Milliseconds()). Bool("github_token_present", tokenPresent). + Bool("github_token_forwarded", tokenForwarded). Str("operator", operator). Msg("GitHub API response received") @@ -445,10 +449,12 @@ func download(rawURL, operator string) ([]byte, error) { return nil, err } - tokenPresent := false - if tok := os.Getenv("GITHUB_TOKEN"); tok != "" { + tok := os.Getenv("GITHUB_TOKEN") + tokenPresent := tok != "" + tokenForwarded := false + if tokenPresent { if parsed, err := url.Parse(rawURL); err == nil && allowedTokenHosts[parsed.Hostname()] { - tokenPresent = true + tokenForwarded = true req.Header.Set("Authorization", "Bearer "+tok) } } @@ -463,6 +469,7 @@ func download(rawURL, operator string) ([]byte, error) { Str("method", http.MethodGet). Int64("latency_ms", time.Since(start).Milliseconds()). Bool("github_token_present", tokenPresent). + Bool("github_token_forwarded", tokenForwarded). Msg("HTTP network request failed") return nil, err } @@ -484,6 +491,7 @@ func download(rawURL, operator string) ([]byte, error) { Int("status_code", resp.StatusCode). Int64("latency_ms", time.Since(start).Milliseconds()). Bool("github_token_present", tokenPresent). + Bool("github_token_forwarded", tokenForwarded). Str("operator", operator). Msg("HTTP response received") @@ -580,9 +588,11 @@ func apply(binary io.Reader, operator, fromVersion, toVersion, exe, sourceURL st Msg("binary write commencing via selfupdate") err := selfupdate.Apply(binary, selfupdate.Options{}) if err != nil { + applyErr := err // preserve original cause before RollbackError unwraps it if rbErr := selfupdate.RollbackError(err); rbErr != nil { log.Error().Err(rbErr). Str("event", "upgrade.rollback_failed"). + Str("apply_error", applyErr.Error()). Str("operator", operator). Str("from_version", fromVersion). Str("to_version", toVersion). diff --git a/pkg/upgrade/upgrade_test.go b/pkg/upgrade/upgrade_test.go index 1d93bd32..edec1c72 100644 --- a/pkg/upgrade/upgrade_test.go +++ b/pkg/upgrade/upgrade_test.go @@ -130,6 +130,10 @@ func TestExtractBinary_InvalidZip(t *testing.T) { // ── download (token host allowlist) ────────────────────────────────────────── +// TestDownload_TokenSentToTrustedHost and TestDownload_TokenNotSentToUntrustedHost +// must NOT run in parallel: the former mutates allowedTokenHosts (127.0.0.1 → true) +// which would cause the latter to see the test server as trusted and silently invert +// its assertion. The t.Cleanup restores state, but only after the test completes. func TestDownload_TokenSentToTrustedHost(t *testing.T) { var receivedAuth string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {