From 75333d294dc57adc5e8233c2b8235f829eb3ec18 Mon Sep 17 00:00:00 2001 From: Christopher Kreft Date: Thu, 16 Jul 2026 18:23:35 +0200 Subject: [PATCH] Verify update packages with ECDSA signatures --- .github/actions/sign-update/action.yml | 36 +++ .github/workflows/builds.yml | 80 ++++++- docs/updates/_index.md | 39 ++++ pkg/snclient/config.go | 3 +- pkg/snclient/t/configs/snclient_incl.ini | 3 + pkg/snclient/task_updates.go | 270 ++++++++++++++++------- pkg/snclient/task_updates_test.go | 126 +++++++++++ pkg/snclient/update_signature.go | 77 +++++++ pkg/snclient/update_signature_test.go | 60 +++++ t/02_daemon_test.go | 3 + 10 files changed, 617 insertions(+), 80 deletions(-) create mode 100644 .github/actions/sign-update/action.yml create mode 100644 pkg/snclient/task_updates_test.go create mode 100644 pkg/snclient/update_signature.go create mode 100644 pkg/snclient/update_signature_test.go diff --git a/.github/actions/sign-update/action.yml b/.github/actions/sign-update/action.yml new file mode 100644 index 00000000..7686eb78 --- /dev/null +++ b/.github/actions/sign-update/action.yml @@ -0,0 +1,36 @@ +name: Sign update package +description: Create a detached ECDSA signature with OpenSSL + +inputs: + artifact: + description: Package file to sign + required: true + private-key-pem: + description: PEM-encoded ECDSA private key + required: true + public-key-sha256: + description: Expected SHA-256 fingerprint of the PKIX DER public key + required: true + +runs: + using: composite + steps: + - shell: bash + env: + ARTIFACT: ${{ inputs.artifact }} + EXPECTED_PUBLIC_KEY_SHA256: ${{ inputs.public-key-sha256 }} + UPDATE_SIGNING_KEY_PEM: ${{ inputs.private-key-pem }} + run: | + set -euo pipefail + umask 077 + key="${RUNNER_TEMP}/snclient-update-signing-key.pem" + public_key="${RUNNER_TEMP}/snclient-update-public-key.der" + trap 'rm -f "${key}" "${public_key}"' EXIT + printf '%s\n' "${UPDATE_SIGNING_KEY_PEM}" > "${key}" + openssl pkey -in "${key}" -pubout -outform DER -out "${public_key}" + actual_fingerprint="$(sha256sum "${public_key}" | cut -d' ' -f1)" + if [[ "${actual_fingerprint}" != "${EXPECTED_PUBLIC_KEY_SHA256}" ]]; then + echo "::error::Update signing key does not match the built-in public key" + exit 1 + fi + openssl dgst -sha256 -sign "${key}" -out "${ARTIFACT}.sig" "${ARTIFACT}" diff --git a/.github/workflows/builds.yml b/.github/workflows/builds.yml index 1c82eb2b..58d8b05c 100644 --- a/.github/workflows/builds.yml +++ b/.github/workflows/builds.yml @@ -17,6 +17,9 @@ on: permissions: contents: read +env: + UPDATE_PUBLIC_KEY_SHA256: "f74bfbce4461efa8d357d4a639ba8b1f74668385dcd3a85cd238accc05fdb2c0" + jobs: get-version: runs-on: ubuntu-latest @@ -605,6 +608,59 @@ jobs: name: "${{ env.BIN }}" + sign-dev-update-assets: + if: github.event_name == 'push' && github.ref == 'refs/heads/main' + strategy: + fail-fast: false + matrix: + include: + - asset_os: windows + go-arch: i386 + extension: msi + - asset_os: windows + go-arch: x86_64 + extension: msi + - asset_os: windows + go-arch: aarch64 + extension: msi + - asset_os: linux + go-arch: i386 + extension: rpm + - asset_os: linux + go-arch: x86_64 + extension: rpm + - asset_os: linux + go-arch: aarch64 + extension: rpm + - asset_os: osx + go-arch: x86_64 + extension: pkg + - asset_os: osx + go-arch: aarch64 + extension: pkg + needs: [get-version, pkg-msi, pkg-rpm, pkg-osx] + runs-on: ubuntu-latest + environment: update-dev + env: + ASSET: "snclient-${{needs.get-version.outputs.version}}-${{ matrix.asset_os }}-${{ matrix.go-arch }}.${{ matrix.extension }}" + steps: + - uses: actions/checkout@v7 + - uses: actions/download-artifact@v8 + with: + name: "${{ env.ASSET }}" + - uses: ./.github/actions/sign-update + with: + artifact: ${{ env.ASSET }} + private-key-pem: ${{ secrets.SNCLIENT_UPDATE_PRIVATE_KEY_PEM }} + public-key-sha256: ${{ env.UPDATE_PUBLIC_KEY_SHA256 }} + - uses: actions/upload-artifact@v7.0.1 + with: + name: "${{ env.ASSET }}.sig" + path: "${{ env.ASSET }}.sig" + if-no-files-found: error + archive: false + + # remove those artifacts which have been converted to .deb or .rpm files clean-tmp-files: strategy: @@ -784,6 +840,7 @@ jobs: go-arch: [i386, x86_64, aarch64] runs-on: ubuntu-latest needs: [get-version,make-release] + environment: update-stable permissions: contents: write env: @@ -794,7 +851,12 @@ jobs: - uses: actions/download-artifact@v8 with: name: "${{ env.BIN }}.msi" - - run: gh release upload v${{needs.get-version.outputs.version}} ${{ env.BIN }}.msi + - uses: ./.github/actions/sign-update + with: + artifact: ${{ env.BIN }}.msi + private-key-pem: ${{ secrets.SNCLIENT_UPDATE_PRIVATE_KEY_PEM }} + public-key-sha256: ${{ env.UPDATE_PUBLIC_KEY_SHA256 }} + - run: gh release upload v${{needs.get-version.outputs.version}} ${{ env.BIN }}.msi ${{ env.BIN }}.msi.sig upload-linux-deb-assets: @@ -828,6 +890,7 @@ jobs: go-arch: [i386, x86_64, aarch64] runs-on: ubuntu-latest needs: [get-version,make-release] + environment: update-stable permissions: contents: write env: @@ -838,7 +901,12 @@ jobs: - uses: actions/download-artifact@v8 with: name: "${{ env.BIN }}.rpm" - - run: gh release upload v${{needs.get-version.outputs.version}} ${{ env.BIN }}.rpm + - uses: ./.github/actions/sign-update + with: + artifact: ${{ env.BIN }}.rpm + private-key-pem: ${{ secrets.SNCLIENT_UPDATE_PRIVATE_KEY_PEM }} + public-key-sha256: ${{ env.UPDATE_PUBLIC_KEY_SHA256 }} + - run: gh release upload v${{needs.get-version.outputs.version}} ${{ env.BIN }}.rpm ${{ env.BIN }}.rpm.sig upload-linux-apk-assets: @@ -872,6 +940,7 @@ jobs: go-arch: [x86_64, aarch64] runs-on: ubuntu-latest needs: [get-version,make-release] + environment: update-stable permissions: contents: write env: @@ -882,7 +951,12 @@ jobs: - uses: actions/download-artifact@v8 with: name: "${{ env.BIN }}.pkg" - - run: gh release upload v${{needs.get-version.outputs.version}} ${{ env.BIN }}.pkg + - uses: ./.github/actions/sign-update + with: + artifact: ${{ env.BIN }}.pkg + private-key-pem: ${{ secrets.SNCLIENT_UPDATE_PRIVATE_KEY_PEM }} + public-key-sha256: ${{ env.UPDATE_PUBLIC_KEY_SHA256 }} + - run: gh release upload v${{needs.get-version.outputs.version}} ${{ env.BIN }}.pkg ${{ env.BIN }}.pkg.sig upload-binary-release-assets: diff --git a/docs/updates/_index.md b/docs/updates/_index.md index dfe2dc25..a4bb3650 100644 --- a/docs/updates/_index.md +++ b/docs/updates/_index.md @@ -28,6 +28,7 @@ Create or edit `/etc/snclient/snclient_local.ini` (on windows: `C:\Program Files [/settings/updates] automatic updates = enabled automatic restart = enabled +verify signature = true ``` This will update SNClient to the latest stable release. @@ -39,6 +40,7 @@ add the dev channel as well. [/settings/updates] automatic updates = enabled automatic restart = enabled +verify signature = true channel = stable,dev [/settings/updates/channel/dev] @@ -50,6 +52,43 @@ In order to use the dev channel, you need to create a github token here: [github Unfortunately it is not possible to download the build artifacts without a token. +Update signatures are verified by default with the public key built into +SNClient. Missing or invalid signatures abort the update. For manual recovery +from an unsigned source, verification can be explicitly disabled with +`verify signature = false` in `/settings/updates`. + +The release workflow expects the same PEM-encoded ECDSA P-256 private key in both +GitHub environments: + +- `SNCLIENT_UPDATE_PRIVATE_KEY_PEM` in the `update-stable` environment +- `SNCLIENT_UPDATE_PRIVATE_KEY_PEM` in the `update-dev` environment + +The stable workflow signs the final RPM, MSI and PKG release assets. Development +signatures are created only for package artifacts built from a push to the +`main` branch. OpenSSL generates each binary `.sig` sidecar as an ASN.1/DER ECDSA +signature over the SHA-256 digest of the exact package bytes. + +Generate the signing key once outside the repository: + +```sh +openssl genpkey -algorithm EC -pkeyopt ec_paramgen_curve:P-256 -out update-private.pem +openssl pkey -in update-private.pem -pubout -out update-public.pem +``` + +Store the full private PEM as the secret in both environments. The client +stores the public key as base64-encoded PKIX DER in `update_signature.go`: + +```sh +openssl pkey -in update-private.pem -pubout -outform DER | base64 -w0 +``` + +The workflow also pins the SHA-256 fingerprint of the same DER public key and +fails before signing if the configured secret does not match: + +```sh +openssl pkey -in update-private.pem -pubout -outform DER | sha256sum +``` + ### Debian / Ubuntu On Debian and Ubuntu you can make use of the `unattended-upgrades` package. diff --git a/pkg/snclient/config.go b/pkg/snclient/config.go index 70d448ef..328887ad 100644 --- a/pkg/snclient/config.go +++ b/pkg/snclient/config.go @@ -60,7 +60,8 @@ var DefaultConfig = map[string]ConfigData{ "nasty characters": DefaultNastyCharacters, }, "/settings/updates": { - "channel": "stable", + "channel": "stable", + "verify signature": "true", }, "/settings/updates/channel": { "stable": "https://api.github.com/repos/ConSol-monitoring/snclient/releases", diff --git a/pkg/snclient/t/configs/snclient_incl.ini b/pkg/snclient/t/configs/snclient_incl.ini index 1767f18b..cde9052a 100644 --- a/pkg/snclient/t/configs/snclient_incl.ini +++ b/pkg/snclient/t/configs/snclient_incl.ini @@ -91,6 +91,9 @@ automatic restart = disabled ; channel - comma separated list of channel to search for updates. channel = stable +; verify signature - Verify downloaded updates with the built-in channel key. +verify signature = true + ; pre release - Control if pre releases from the stable channel should be considered as well. pre release = false diff --git a/pkg/snclient/task_updates.go b/pkg/snclient/task_updates.go index 2b3fc4eb..ab6a220e 100644 --- a/pkg/snclient/task_updates.go +++ b/pkg/snclient/task_updates.go @@ -34,7 +34,8 @@ const ( UpdateCheckIntervalRegular = 55 * time.Second // Maximum file size for updates (prevent tar bombs) - UpdateFileMaxSize = 100e6 + UpdateFileMaxSize = 100e6 + updateSignatureArchiveMaxSize = 1e6 // MainBranch sets the github main branch, only artifacts from that branch will be considered MainBranch = "main" @@ -54,6 +55,7 @@ func init() { "automatic restart": "disabled", "channel": "stable", "pre release": "false", + "verify signature": "true", "update interval": "1h", "update hours": "0-24", "update days": "mon-sun", @@ -75,6 +77,7 @@ type UpdateHandler struct { automaticRestart bool channel string preRelease bool + verifySignature bool updateInterval float64 updateHours []UpdateHours updateDays []UpdateDays @@ -85,10 +88,26 @@ type UpdateHandler struct { } type updatesAvailable struct { - channel string - url string - version string - header map[string]string + channel string + url string + signatureURL string + version string + header map[string]string + artifactArchives bool +} + +type githubArtifact struct { + URL string `json:"archive_download_url"` + Name string `json:"name"` + Expired bool `json:"expired"` + WorkflowRun struct { + ID int64 `json:"id"` + Branch string `json:"head_branch"` + } `json:"workflow_run"` +} + +type githubActions struct { + Artifacts []githubArtifact `json:"artifacts"` } type cachedURLVersion struct { @@ -98,7 +117,8 @@ type cachedURLVersion struct { func NewUpdateHandler() Module { return &UpdateHandler{ - urlCache: make(map[string]cachedURLVersion), + verifySignature: true, + urlCache: make(map[string]cachedURLVersion), } } @@ -130,6 +150,14 @@ func (u *UpdateHandler) setConfig(section *ConfigSection) error { u.preRelease = preRelease } + verifySignature, ok, err := section.GetBool("verify signature") + switch { + case err != nil: + return fmt.Errorf("verify signature: %s", err.Error()) + case ok: + u.verifySignature = verifySignature + } + autoUpdate, ok, err := section.GetBool("automatic updates") switch { case err != nil: @@ -398,9 +426,9 @@ func (u *UpdateHandler) checkUpdate(ctx context.Context, url string, preRelease } else if ok, _ := regexp.MatchString(`^https://api\.github\.com/repos/.*/actions/artifacts`, url); ok { updates, err = u.checkUpdateGithubActions(ctx, url, channel) } else if ok, _ := regexp.MatchString(`^file:`, url); ok { - updates, err = u.checkUpdateFile(ctx, url) + updates, err = u.checkUpdateFile(ctx, url, channel) } else { - updates, err = u.checkUpdateCustomURL(ctx, url) + updates, err = u.checkUpdateCustomURL(ctx, url, channel) } if err != nil { @@ -471,11 +499,20 @@ func (u *UpdateHandler) checkUpdateGithubRelease(ctx context.Context, url, chann continue } + assetURLs := make(map[string]string, len(release.Assets)) + for _, asset := range release.Assets { + assetURLs[asset.Name] = asset.URL + } + log.Debugf("checking assets for release: %s", release.TagName) foundOne := false for _, asset := range release.Assets { if u.isUsableGithubAsset(strings.ToLower(asset.Name)) { - updates = append(updates, updatesAvailable{url: asset.URL, version: release.TagName}) + updates = append(updates, updatesAvailable{ + url: asset.URL, + signatureURL: assetURLs[asset.Name+".sig"], + version: release.TagName, + }) foundOne = true } } @@ -507,18 +544,7 @@ func (u *UpdateHandler) checkUpdateGithubActions(ctx context.Context, url, chann logHTTPResponse(resp) - type GithubArtifact struct { - URL string `json:"archive_download_url"` - Name string `json:"name"` - WorkflowRun struct { - Banch string `json:"head_branch"` - } `json:"workflow_run"` - } - - type GithubActions struct { - Artifacts []GithubArtifact `json:"artifacts"` - } - var artifacts GithubActions + var artifacts githubActions err = json.NewDecoder(resp.Body).Decode(&artifacts) if err != nil { return nil, fmt.Errorf("json: %s", err.Error()) @@ -530,11 +556,24 @@ func (u *UpdateHandler) checkUpdateGithubActions(ctx context.Context, url, chann } reActionVersion := regexp.MustCompile(`^snclient\-(.*?)\-\w+-\w+\.\w+`) + artifactsByRunAndName := make(map[string]githubArtifact, len(artifacts.Artifacts)) + for _, artifact := range artifacts.Artifacts { + if artifact.Expired || artifact.WorkflowRun.Branch != MainBranch { + continue + } + key := fmt.Sprintf("%d:%s", artifact.WorkflowRun.ID, artifact.Name) + artifactsByRunAndName[key] = artifact + } for i := range artifacts.Artifacts { artifact := artifacts.Artifacts[i] - if artifact.WorkflowRun.Banch != MainBranch { - log.Debugf("[update] skipped artifact from none-main branch: %s", artifact.WorkflowRun.Banch) + if artifact.Expired { + log.Debugf("[update] skipped expired artifact: %s", artifact.Name) + + continue + } + if artifact.WorkflowRun.Branch != MainBranch { + log.Debugf("[update] skipped artifact from none-main branch: %s", artifact.WorkflowRun.Branch) continue } @@ -542,7 +581,15 @@ func (u *UpdateHandler) checkUpdateGithubActions(ctx context.Context, url, chann matches := reActionVersion.FindStringSubmatch(artifact.Name) if len(matches) > 1 { version := matches[1] - updates = append(updates, updatesAvailable{url: artifact.URL, version: version, header: header}) + signatureKey := fmt.Sprintf("%d:%s.sig", artifact.WorkflowRun.ID, artifact.Name) + signatureArtifact := artifactsByRunAndName[signatureKey] + updates = append(updates, updatesAvailable{ + url: artifact.URL, + signatureURL: signatureArtifact.URL, + version: version, + header: header, + artifactArchives: true, + }) } } } @@ -555,7 +602,7 @@ func (u *UpdateHandler) checkUpdateGithubActions(ctx context.Context, url, chann } // check available update from any url -func (u *UpdateHandler) checkUpdateCustomURL(ctx context.Context, url string) (updates []updatesAvailable, err error) { +func (u *UpdateHandler) checkUpdateCustomURL(ctx context.Context, url, channel string) (updates []updatesAvailable, err error) { log.Tracef("[update] checking custom url at: %s", url) resp, err := u.snc.httpDo(ctx, u.httpOptions, "HEAD", url, nil) if err != nil { @@ -576,7 +623,7 @@ func (u *UpdateHandler) checkUpdateCustomURL(ctx context.Context, url string) (u if resp.ContentLength > 0 && resp.ContentLength == stat.Size() { log.Tracef("[update] content size matches %s: %d vs. %s: %d", url, resp.ContentLength, executable, stat.Size()) - return []updatesAvailable{{url: url, version: u.snc.Version()}}, nil + return []updatesAvailable{{url: url, signatureURL: url + ".sig", version: u.snc.Version()}}, nil } refresh := false @@ -610,11 +657,11 @@ func (u *UpdateHandler) checkUpdateCustomURL(ctx context.Context, url string) (u if !refresh { log.Tracef("[update] using cached entry for %s", url) - return []updatesAvailable{{url: url, version: cacheEntry.version}}, nil + return []updatesAvailable{{url: url, signatureURL: url + ".sig", version: cacheEntry.version}}, nil } log.Tracef("[update] need to refresh cache for %s", url) - version, err := u.getVersionFromURL(ctx, url) + version, err := u.getVersionFromURL(ctx, url, channel) if err != nil { return nil, fmt.Errorf("failed to fetch version: %s", err.Error()) } @@ -624,11 +671,11 @@ func (u *UpdateHandler) checkUpdateCustomURL(ctx context.Context, url string) (u responseSize: resp.ContentLength, } - return []updatesAvailable{{url: url, version: version}}, nil + return []updatesAvailable{{url: url, signatureURL: url + ".sig", version: version}}, nil } // check available update from local or remote filesystem -func (u *UpdateHandler) checkUpdateFile(ctx context.Context, url string) (updates []updatesAvailable, err error) { +func (u *UpdateHandler) checkUpdateFile(ctx context.Context, url, channel string) (updates []updatesAvailable, err error) { localPath := strings.TrimPrefix(url, "file://") log.Tracef("[update] checking local file at: %s", localPath) _, err = os.Stat(localPath) @@ -636,72 +683,122 @@ func (u *UpdateHandler) checkUpdateFile(ctx context.Context, url string) (update return nil, fmt.Errorf("could not find update file: %s", err.Error()) } - // copy to tmp location - tempUpdate := filepath.Join(os.TempDir(), "snclient-tmpupdate") + GlobalMacros["file-ext"] - os.Remove(tempUpdate) // remove if it already exists for some reason - err = utils.CopyFile(localPath, tempUpdate) + version, err := u.getVersionFromURL(ctx, url, channel) if err != nil { - return nil, fmt.Errorf("copy update file failed: %s", err.Error()) + return nil, err } - err = u.extractUpdate(ctx, tempUpdate) - if err != nil { - return nil, fmt.Errorf("extracting update failed: %s", err.Error()) + return []updatesAvailable{{url: url, signatureURL: url + ".sig", version: version}}, nil +} + +// fetch update file into tmp file +func (u *UpdateHandler) downloadUpdate(ctx context.Context, update *updatesAvailable) (binPath string, err error) { + executable := GlobalMacros["exe-full"] + updateFile := u.snc.buildUpdateFile(executable) + defer func() { + if err != nil { + LogError(os.Remove(updateFile)) + } + }() + if downloadErr := u.downloadResource(ctx, update.url, update.header, updateFile, UpdateFileMaxSize); downloadErr != nil { + return "", downloadErr + } + if update.artifactArchives { + if extractErr := u.extractZip(updateFile); extractErr != nil { + return "", fmt.Errorf("extracting github update artifact: %s", extractErr.Error()) + } + } + + if u.verifySignature { + if update.signatureURL == "" { + return "", fmt.Errorf("update signature is missing for %s channel", update.channel) + } + if signatureErr := u.downloadAndVerifySignature(ctx, update, updateFile); signatureErr != nil { + return "", signatureErr + } + } else { + log.Warnf("[update] update signature verification is disabled") } - // get version from that executable - version, err := u.verifyUpdate(ctx, tempUpdate) + if extractErr := u.extractUpdate(ctx, updateFile); extractErr != nil { + return "", extractErr + } + + return updateFile, nil +} + +func (u *UpdateHandler) downloadAndVerifySignature(ctx context.Context, update *updatesAvailable, updateFile string) error { + signatureFile, err := os.CreateTemp("", "snclient-update-signature") if err != nil { - return nil, err + return fmt.Errorf("creating update signature file: %s", err.Error()) + } + signaturePath := signatureFile.Name() + defer os.Remove(signaturePath) + if closeErr := signatureFile.Close(); closeErr != nil { + return fmt.Errorf("closing update signature file: %s", closeErr.Error()) } - return []updatesAvailable{{url: url, version: version}}, nil + if downloadErr := u.downloadResource(ctx, update.signatureURL, update.header, signaturePath, updateSignatureArchiveMaxSize); downloadErr != nil { + return fmt.Errorf("fetching update signature: %s", downloadErr.Error()) + } + if update.artifactArchives { + if extractErr := u.extractZipWithMaxSize(signaturePath, updateSignatureMaxSize); extractErr != nil { + return fmt.Errorf("extracting github signature artifact: %s", extractErr.Error()) + } + } + + if verifyErr := verifyUpdateSignature(updateFile, signaturePath); verifyErr != nil { + return verifyErr + } + log.Debugf("[update] verified %s update signature", update.channel) + + return nil } -// fetch update file into tmp file -func (u *UpdateHandler) downloadUpdate(ctx context.Context, update *updatesAvailable) (binPath string, err error) { - url := update.url +func (u *UpdateHandler) downloadResource( + ctx context.Context, + url string, + header map[string]string, + destination string, + maxSize int64, +) error { var src io.ReadCloser if localPath, ok := strings.CutPrefix(url, "file://"); ok { - log.Tracef("[update] fetching update from %s", localPath) - file, err2 := os.Open(localPath) - if err2 != nil { - return "", fmt.Errorf("open failed %s: %s", localPath, err2.Error()) + log.Tracef("[update] fetching update resource from %s", localPath) + file, err := os.Open(localPath) + if err != nil { + return fmt.Errorf("open failed %s: %s", localPath, err.Error()) } src = file + defer file.Close() } else { - log.Tracef("[update] downloading update from %s", url) - resp, err2 := u.snc.httpDo(ctx, u.httpOptions, "GET", url, update.header) - if err2 != nil { - return "", fmt.Errorf("fetching update failed %s: %s", url, err2.Error()) + log.Tracef("[update] downloading update resource from %s", url) + resp, err := u.snc.httpDo(ctx, u.httpOptions, "GET", url, header) + if err != nil { + return fmt.Errorf("fetching update failed %s: %s", url, err.Error()) } - defer resp.Body.Close() src = resp.Body + defer resp.Body.Close() } - executable := GlobalMacros["exe-full"] - updateFile := u.snc.buildUpdateFile(executable) - saveFile, err := os.Create(updateFile) + saveFile, err := os.OpenFile(destination, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0o600) if err != nil { - return "", fmt.Errorf("open: %s", err.Error()) + return fmt.Errorf("open: %s", err.Error()) } log.Tracef("[update] saving to %s", saveFile.Name()) - - _, err = io.Copy(saveFile, src) - if err != nil { - saveFile.Close() - - return "", fmt.Errorf("read: %s", err.Error()) - } - saveFile.Close() - - err = u.extractUpdate(ctx, updateFile) - if err != nil { - return "", err + written, copyErr := io.Copy(saveFile, io.LimitReader(src, maxSize+1)) + closeErr := saveFile.Close() + switch { + case copyErr != nil: + return fmt.Errorf("read: %s", copyErr.Error()) + case closeErr != nil: + return fmt.Errorf("close: %s", closeErr.Error()) + case written > maxSize: + return fmt.Errorf("update resource exceeds maximum size of %d bytes", maxSize) } - return updateFile, nil + return nil } func (u *UpdateHandler) extractUpdate(ctx context.Context, updateFile string) (err error) { @@ -782,9 +879,13 @@ func (u *UpdateHandler) verifyUpdate(ctx context.Context, newBinPath string) (ve return version, nil } -func (u *UpdateHandler) getVersionFromURL(ctx context.Context, url string) (version string, err error) { +func (u *UpdateHandler) getVersionFromURL(ctx context.Context, url, channel string) (version string, err error) { log.Tracef("[update] trying to determine version for url %s", url) - filePath, err := u.downloadUpdate(ctx, &updatesAvailable{url: url}) + filePath, err := u.downloadUpdate(ctx, &updatesAvailable{ + channel: channel, + url: url, + signatureURL: url + ".sig", + }) if err != nil { return "", err } @@ -911,6 +1012,10 @@ func (u *UpdateHandler) updatePreChecks() bool { } func (u *UpdateHandler) extractZip(fileName string) error { + return u.extractZipWithMaxSize(fileName, UpdateFileMaxSize) +} + +func (u *UpdateHandler) extractZipWithMaxSize(fileName string, maxSize int64) error { zipHandle, err := zip.OpenReader(fileName) if err != nil { return fmt.Errorf("zip: %s", err.Error()) @@ -920,6 +1025,9 @@ func (u *UpdateHandler) extractZip(fileName string) error { if len(zipHandle.File) != 1 { return fmt.Errorf("expect zip must contain exactly one file, have: %d", len(zipHandle.File)) } + if zipHandle.File[0].UncompressedSize64 > uint64(maxSize) { //nolint:gosec // maxSize is a positive internal limit. + return fmt.Errorf("zip content exceeds maximum size of %d bytes", maxSize) + } tempFile, err := os.CreateTemp("", "snclient-unzip") if err != nil { @@ -932,12 +1040,17 @@ func (u *UpdateHandler) extractZip(fileName string) error { } defer src.Close() - _, err = io.Copy(tempFile, src) + written, err := io.Copy(tempFile, io.LimitReader(src, maxSize+1)) if err != nil { tempFile.Close() return fmt.Errorf("read: %s", err.Error()) } + if written > maxSize { + tempFile.Close() + + return fmt.Errorf("zip content exceeds maximum size of %d bytes", maxSize) + } tempFile.Close() defer os.Remove(tempFile.Name()) @@ -1182,6 +1295,10 @@ func (u *UpdateHandler) extractXar(ctx context.Context, fileName string) error { } func (u *UpdateHandler) isUsableGithubAsset(name string) bool { + if strings.HasSuffix(name, ".sig") { + return false + } + archVariants := []string{pkgArch(runtime.GOARCH), runtime.GOARCH} osVariants := []string{runtime.GOOS} @@ -1223,8 +1340,9 @@ func (u *UpdateHandler) sanitizeChannel(flag string) (channel, updateFile string if _, err := os.ReadFile(channel); err == nil { updateFile = channel best = &updatesAvailable{ - channel: "file", - url: "file://" + updateFile, + channel: "file", + url: "file://" + updateFile, + signatureURL: "file://" + updateFile + ".sig", } } diff --git a/pkg/snclient/task_updates_test.go b/pkg/snclient/task_updates_test.go new file mode 100644 index 00000000..18696551 --- /dev/null +++ b/pkg/snclient/task_updates_test.go @@ -0,0 +1,126 @@ +package snclient + +import ( + "context" + "crypto/tls" + "fmt" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestUpdateSignatureConfig(t *testing.T) { + handler, ok := NewUpdateHandler().(*UpdateHandler) + require.True(t, ok) + assert.True(t, handler.verifySignature) + + section := NewConfig(true).Section("/settings/updates") + section.Set("verify signature", "false") + require.NoError(t, handler.setConfig(section)) + assert.False(t, handler.verifySignature) + + section.Set("verify signature", "invalid") + err := handler.setConfig(section) + assert.ErrorContains(t, err, "verify signature") +} + +func TestGithubReleaseSignaturePairing(t *testing.T) { + assetName := testUpdateAssetName() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprintf(w, `[{"tag_name":"v1.2.3","assets":[`+ + `{"name":%q,"browser_download_url":%q},`+ + `{"name":%q,"browser_download_url":%q}`+ + `]}]`, assetName, "https://example.test/"+assetName, + assetName+".sig", "https://example.test/"+assetName+".sig") + })) + defer server.Close() + + handler := testUpdateHandler() + updates, err := handler.checkUpdateGithubRelease(context.Background(), server.URL, "stable", false) + require.NoError(t, err) + require.Len(t, updates, 1) + assert.Equal(t, "https://example.test/"+assetName+".sig", updates[0].signatureURL) +} + +func TestGithubActionsSignaturePairing(t *testing.T) { + assetName := testUpdateAssetName() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprintf(w, `{"artifacts":[`+ + `{"name":%q,"archive_download_url":%q,"workflow_run":{"id":42,"head_branch":"main"}},`+ + `{"name":%q,"archive_download_url":%q,"workflow_run":{"id":42,"head_branch":"main"}}`+ + `]}`, assetName, "https://example.test/package", + assetName+".sig", "https://example.test/signature") + })) + defer server.Close() + + handler := testUpdateHandler() + handler.snc.config.Section("/settings/updates/channel/dev").Set("github token", "test-token") + updates, err := handler.checkUpdateGithubActions(context.Background(), server.URL, "dev") + require.NoError(t, err) + require.Len(t, updates, 1) + assert.Equal(t, "https://example.test/signature", updates[0].signatureURL) + assert.True(t, updates[0].artifactArchives) +} + +func TestDownloadUpdateRequiresSignature(t *testing.T) { + source := filepath.Join(t.TempDir(), "snclient.rpm") + require.NoError(t, os.WriteFile(source, []byte("unsigned package"), 0o600)) + + originalExecutable := GlobalMacros["exe-full"] + originalExtension := GlobalMacros["file-ext"] + GlobalMacros["exe-full"] = filepath.Join(t.TempDir(), "snclient") + GlobalMacros["file-ext"] = "" + t.Cleanup(func() { + GlobalMacros["exe-full"] = originalExecutable + GlobalMacros["file-ext"] = originalExtension + }) + + handler := testUpdateHandler() + _, err := handler.downloadUpdate(context.Background(), &updatesAvailable{ + channel: "stable", + url: "file://" + source, + }) + require.ErrorContains(t, err, "signature is missing") + + handler.verifySignature = false + _, err = handler.downloadUpdate(context.Background(), &updatesAvailable{ + channel: "stable", + url: "file://" + source, + }) + require.Error(t, err) + assert.NotContains(t, err.Error(), "signature is missing") +} + +func testUpdateHandler() *UpdateHandler { + return &UpdateHandler{ + snc: NewAgentSimple(&AgentFlags{}), + verifySignature: true, + httpOptions: &HTTPClientOptions{ + tlsConfig: &tls.Config{MinVersion: tls.VersionTLS12}, + reqTimeout: 5, + }, + urlCache: make(map[string]cachedURLVersion), + } +} + +func testUpdateAssetName() string { + osName := runtime.GOOS + extension := "rpm" + if runtime.GOOS == "windows" { + extension = "msi" + } + if runtime.GOOS == "darwin" { + osName = "osx" + extension = "pkg" + } + + return fmt.Sprintf("snclient-1.2.3-%s-%s.%s", osName, pkgArch(runtime.GOARCH), extension) +} diff --git a/pkg/snclient/update_signature.go b/pkg/snclient/update_signature.go new file mode 100644 index 00000000..ca7e90ac --- /dev/null +++ b/pkg/snclient/update_signature.go @@ -0,0 +1,77 @@ +package snclient + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/sha256" + "crypto/x509" + "encoding/base64" + "fmt" + "io" + "os" +) + +const ( + updateSignatureMaxSize = 1024 + + // Fork validation key. Upstream releases must replace it with a maintainer-controlled key. + updatePublicKeyBase64 = "MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAELNoFBtr9CSXY9910vzXBiSfctTh9iq3/VOOBf+4EQGbj17xG4FLd0pCgOxLBi40sagqmGJrJAQs07iPIHk0qrg==" +) + +func verifyUpdateSignature(updatePath, signaturePath string) error { + publicKey, err := updatePublicKey() + if err != nil { + return err + } + + signatureFile, err := os.Open(signaturePath) + if err != nil { + return fmt.Errorf("reading update signature: %s", err.Error()) + } + defer signatureFile.Close() + signatureData, err := io.ReadAll(io.LimitReader(signatureFile, updateSignatureMaxSize+1)) + if err != nil { + return fmt.Errorf("reading update signature: %s", err.Error()) + } + if len(signatureData) > updateSignatureMaxSize { + return fmt.Errorf("update signature exceeds maximum size of %d bytes", updateSignatureMaxSize) + } + + return verifyUpdateSignatureWithKey(updatePath, signatureData, publicKey) +} + +func verifyUpdateSignatureWithKey(updatePath string, signature []byte, publicKey *ecdsa.PublicKey) error { + file, err := os.Open(updatePath) + if err != nil { + return fmt.Errorf("opening update for signature verification: %s", err.Error()) + } + defer file.Close() + + digest := sha256.New() + if _, err = io.Copy(digest, file); err != nil { + return fmt.Errorf("hashing update for signature verification: %s", err.Error()) + } + + if !ecdsa.VerifyASN1(publicKey, digest.Sum(nil), signature) { + return fmt.Errorf("update signature verification failed") + } + + return nil +} + +func updatePublicKey() (*ecdsa.PublicKey, error) { + publicKeyDER, err := base64.StdEncoding.DecodeString(updatePublicKeyBase64) + if err != nil { + return nil, fmt.Errorf("decoding update public key: %s", err.Error()) + } + parsedKey, err := x509.ParsePKIXPublicKey(publicKeyDER) + if err != nil { + return nil, fmt.Errorf("parsing update public key: %s", err.Error()) + } + publicKey, ok := parsedKey.(*ecdsa.PublicKey) + if !ok || publicKey.Curve != elliptic.P256() { + return nil, fmt.Errorf("invalid update public key") + } + + return publicKey, nil +} diff --git a/pkg/snclient/update_signature_test.go b/pkg/snclient/update_signature_test.go new file mode 100644 index 00000000..696c1d2a --- /dev/null +++ b/pkg/snclient/update_signature_test.go @@ -0,0 +1,60 @@ +package snclient + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/sha256" + "crypto/x509" + "encoding/base64" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + updateSignatureTestPublicKey = "MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEyrMXXwiKA1GJ4J04JuivIeznP4hvVJbSNQVRRs9HvJsOP1HJFnqwcjvFtaZtArURBFc1KVzN4+HYRVACtAdCFw==" + updateSignatureTestVector = "MEQCIHFVLuEjKUx0fvKClMYd/LpBrfJZn/drWgBIKvZJKd02AiA6qoJTbJlNtK2Ao3hHcboTGf/y2KSy+K//5ie7ClP7iw==" +) + +func TestVerifyUpdateSignature(t *testing.T) { + privateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + + updatePath := t.TempDir() + "/snclient.rpm" + updateData := []byte("signed update package") + require.NoError(t, os.WriteFile(updatePath, updateData, 0o600)) + + digest := sha256.Sum256(updateData) + signature, err := ecdsa.SignASN1(rand.Reader, privateKey, digest[:]) + require.NoError(t, err) + + require.NoError(t, verifyUpdateSignatureWithKey(updatePath, signature, &privateKey.PublicKey)) + + require.NoError(t, os.WriteFile(updatePath, []byte("tampered update package"), 0o600)) + err = verifyUpdateSignatureWithKey(updatePath, signature, &privateKey.PublicKey) + assert.ErrorContains(t, err, "signature verification failed") +} + +func TestUpdatePublicKey(t *testing.T) { + publicKey, err := updatePublicKey() + require.NoError(t, err) + assert.Equal(t, elliptic.P256(), publicKey.Curve) +} + +func TestVerifyUpdateSignatureCompatibilityVector(t *testing.T) { + publicKeyDER, err := base64.StdEncoding.DecodeString(updateSignatureTestPublicKey) + require.NoError(t, err) + parsedKey, err := x509.ParsePKIXPublicKey(publicKeyDER) + require.NoError(t, err) + publicKey, ok := parsedKey.(*ecdsa.PublicKey) + require.True(t, ok) + signature, err := base64.StdEncoding.DecodeString(updateSignatureTestVector) + require.NoError(t, err) + + updatePath := t.TempDir() + "/update.pkg" + require.NoError(t, os.WriteFile(updatePath, []byte("update package"), 0o600)) + require.NoError(t, verifyUpdateSignatureWithKey(updatePath, signature, publicKey)) +} diff --git a/t/02_daemon_test.go b/t/02_daemon_test.go index 9160622c..eb67ef8e 100644 --- a/t/02_daemon_test.go +++ b/t/02_daemon_test.go @@ -46,6 +46,9 @@ check_echo_win = cmd /c echo '%ARG1%' check_echo_args = echo '%ARGS%' check_echo_args_win = cmd /c echo '%ARGS%' +[/settings/updates] +verify signature = false + [/settings/updates/channel] local = file://./tmpupdates/snclient${file-ext} `