Agent: export secure binary upgrade installer - #585
Conversation
|
Self-review updates pushed in |
|
The downstream focused AKS/Flex Node E2E passed using Unbounded commit |
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a new secure, parameterized agent upgrade installer that validates HTTPS downloads, verifies archive digests and members, enforces size bounds, and performs an atomic blue/green switch while redacting sensitive URL components.
Changes:
- Introduces
SecureInstallAndSwitchwith strict archive validation, digest verification, and atomic symlink switching. - Exports
InstallFileWithLimitedSizeininternal/utiliofor bounded, atomic installs. - Adds comprehensive tests for secure install flows, input validation, redirects, size limits, and URL redaction.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/agent/internal/utilio/io.go | Exports limited-size, atomic file installer used by secure upgrade flow |
| pkg/agent/agentbinary/upgrade.go | Implements secure download/verify/extract and blue-green switching logic |
| pkg/agent/agentbinary/upgrade_test.go | Adds tests covering secure install/switch behavior and failure modes |
| pkg/agent/agentbinary/agentbinary.go | Updates package/docs to steer new callers to secure installer |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Unbounded now consumes the shared secure installer itself in |
|
Removed the dead legacy daemon staging wrapper and 311 lines of duplicate HTTP/archive/switch tests in |
|
Fixed the failing agent E2E in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
pkg/agent/agentbinary/upgrade.go:405
- The install is committed to
targetPathas soon as the expected member is encountered. If later entries exist (or a later read error occurs), the function returns an error but the inactive slot has already been replaced. To keep failure paths non-mutating, stage into a pending/temp file first and only atomically replacetargetPathafter the entire archive has been validated through EOF (no extra members, no truncation). This aligns better with the documented strict rejection of unexpected entries.
if header.Name != opts.ExpectedMember {
return fmt.Errorf("agent archive contains unexpected member %q", header.Name)
}
if found {
return fmt.Errorf("agent archive contains duplicate member %q", opts.ExpectedMember)
}
if header.Typeflag != tar.TypeReg || header.Size <= 0 || header.Size > opts.MaxExtractedBytes {
return fmt.Errorf("agent archive member %q is not a valid bounded regular file", opts.ExpectedMember)
}
if err := utilio.InstallFileWithLimitedSize(targetPath, tarReader, opts.Mode, opts.MaxExtractedBytes); err != nil {
return fmt.Errorf("install upgraded agent binary: %w", err)
}
pkg/agent/agentbinary/upgrade.go:427
safeArchiveNamecurrently allows.and..(both are non-empty, clean to themselves, and won’t match the../prefix check). Even thoughExpectedMembervalidation makes it unlikely to be exploited, it’s safer and clearer to explicitly reject.and..so unsafe names reliably trigger the intended "unsafe member" error path.
func safeArchiveName(name string) bool {
return name != "" &&
!filepath.IsAbs(name) &&
filepath.Clean(name) == name &&
!strings.Contains(name, `\`) &&
!strings.HasPrefix(name, ".."+string(filepath.Separator))
}
pkg/agent/agentbinary/upgrade.go:234
- The error message
\"invalid download URL\"is not very actionable when debugging parameter issues. Since the goal is to avoid leaking credentials, consider including a short, non-sensitive hint (e.g., "invalid download URL: must be an absolute HTTPS URL") or returning the parse failure reason without echoing the raw URL (or by echoing only a redacted form when parsing succeeds).
parsedURL, err := url.ParseRequestURI(strings.TrimSpace(rawURL))
if err != nil {
return nil, fmt.Errorf("invalid download URL")
}
cmd/kubectl-unbounded/app/machine_operation_create.go:237
- The
downloadURLvalidation message still suggests<url>, but the operation now requires HTTPS per docs and server-side validation. Updating this message to<https-url>would make the CLI feedback consistent with the new contract.
if o.kind == v1alpha3.OperationAgentUpgrade {
if parameters["downloadURL"] == "" {
return fmt.Errorf("AgentUpgrade requires --param downloadURL=<url>")
}
if parameters["sha256"] == "" {
return fmt.Errorf("AgentUpgrade requires --param sha256=<archive-sha256>")
}
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (3)
pkg/agent/internal/utilio/io.go:35
InstallFileWithLimitedSizeusesio.LimitReader(r, maxBytes+1)later, but the current validation allowsmaxBytes == math.MaxInt64, which would overflowmaxBytes+1and break the size limit enforcement. Since this function is now exported and intended for reuse, guard against theMaxInt64case (or switch to a non-+1sentinel strategy).
func InstallFileWithLimitedSize(filename string, r io.Reader, perm os.FileMode, maxBytes int64) error {
if maxBytes <= 0 {
return fmt.Errorf("invalid maxBytes: %d", maxBytes)
}
pkg/agent/agentbinary/upgrade.go:86
ExpectedMembervalidation currently permits values like "." or ".." (both are base names), which can lead to surprising/ambiguous archive validation and error messages. Since the installer requires an exact archive member name, reject "."/".." explicitly in addition to path-prefixed and nested names.
opts.ExpectedMember = strings.TrimSpace(opts.ExpectedMember)
if opts.ExpectedMember == "" || filepath.Base(opts.ExpectedMember) != opts.ExpectedMember {
return normalizedSecureInstallOptions{}, fmt.Errorf("expected archive member must be an exact base name without a path prefix")
}
pkg/agent/agentbinary/upgrade.go:228
RedactedURLis exported but will panic onnilinput (*parsedURL). Even if current internal callers always pass a non-nil URL, guarding makes the helper safe for external use and avoids surprising panics in error paths.
// RedactedURL removes query and fragment data that may contain credentials.
func RedactedURL(parsedURL *url.URL) string {
redacted := *parsedURL
redacted.RawQuery = ""
redacted.Fragment = ""
return redacted.String()
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (3)
pkg/agent/internal/utilio/io.go:35
- InstallFileWithLimitedSize uses
io.LimitReader(r, maxBytes+1)without guarding againstmaxBytes+1overflow. If a caller passesmaxBytes == math.MaxInt64, the addition overflows and the limiter can become negative, defeating the size limit and potentially installing an empty/partial file without error.
func InstallFileWithLimitedSize(filename string, r io.Reader, perm os.FileMode, maxBytes int64) error {
if maxBytes <= 0 {
return fmt.Errorf("invalid maxBytes: %d", maxBytes)
}
pkg/agent/agentbinary/upgrade.go:304
- When
client.Do(req)fails (and the context is still active), the returned error drops the underlying cause entirely. This makes diagnosing TLS/connection failures difficult, and you can still avoid leaking credential-bearing URLs by special-casing*url.Error(use its.Err) while including the error for other cases.
if ctx.Err() != nil {
return "", fmt.Errorf("download agent archive from %s: %w", RedactedURL(parsedURL), ctx.Err())
}
// Redirect and transport errors can contain credential-bearing URLs.
return "", fmt.Errorf("download agent archive from %s failed", RedactedURL(parsedURL))
pkg/agent/agentbinary/upgrade.go:133
Layout.BinaryPathis required by ValidateLayout, but it is not used anywhere in SecureInstallAndSwitch (it only uses Current/LastGood/Blue/Green). This forces callers to provide a seemingly mandatory path that has no effect on the operation.
// ValidateLayout verifies that all binary paths are clean, absolute, and distinct.
func ValidateLayout(paths Layout) error {
values := []string{
paths.BinaryPath,
paths.BluePath,
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Suppressed comments (3)
cmd/kubectl-unbounded/app/machine_operation_test.go:125
- The test name says SHA-256 is required, but the assertion verifies that omitting it is accepted. Rename the test so failures and test listings describe the behavior accurately.
func TestValidateAgentUpgradeRequiresSHA256(t *testing.T) {
pkg/agent/agentbinary/upgrade.go:242
- This rollback-critical branch is not exercised by the new tests: their failure cases keep both current and last-good on blue while targeting green, so
lastGoodProtectedremains false. Add a case with current on green, last-good on blue, and a failing candidate targeting blue, then assert last-good is moved to green before blue is replaced.
lastGoodProtected := err == nil && lastGoodTarget == filepath.Join(canonicalTargetDir, filepath.Base(targetPath))
if lastGoodProtected {
pkg/agent/agentbinary/agentbinary.go:142
- The candidate controls this output, and this error propagates directly into the MachineOperation status. Returning even a capped 4 KiB exposes arbitrary untrusted text, contrary to
designs/agent-upgrade.md:110and the PR's redaction guarantee. Return only the execution error.
details := strings.TrimSpace(output.String())
if details != "" {
return fmt.Errorf("verify agent binary %s: %w: %s", path, err, details)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
pkg/agent/agentbinary/upgrade.go:484
- A NUL type flag (
tar.TypeRegA) is also a valid tar regular-file representation. Rejecting it makes valid release archives fail even though the repository's general extractor accepts both forms atpkg/agent/internal/utilio/tar.go:80-85.
if header.Typeflag != tar.TypeReg || header.Size <= 0 || header.Size > opts.MaxExtractedBytes {
return fmt.Errorf("agent archive member %q is not a valid bounded regular file", opts.ExpectedMember)
pkg/agent/agentbinary/agentbinary.go:146
- Candidate-controlled stdout/stderr is still appended to the returned error. The AgentUpgrade controller persists this error verbatim in
MachineOperationstatus (controller_machineoperation.go:120-125), contradicting the stated guarantee that candidate verification does not expose untrusted output. Return only the wrapped execution error.
details := strings.TrimSpace(output.String())
if details != "" {
return fmt.Errorf("verify agent binary %s: %w: %s", path, err, details)
cmd/kubectl-unbounded/app/machine_operation_test.go:125
- The name says SHA-256 is required, but this test deliberately verifies that omitting it succeeds. Rename the test so failures report the behavior it actually covers.
func TestValidateAgentUpgradeRequiresSHA256(t *testing.T) {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
pkg/agent/agentbinary/upgrade.go:342
- This checks only the original URL, so
http -> https -> httpis still accepted. Once a redirect chain reaches HTTPS, the following hop must not downgrade; use the immediately preceding request (via[len(via)-1]), matchingutilio.CheckRedirectNoHTTPSDowngrade.
if len(via) > 0 && via[0].URL.Scheme == "https" && req.URL.Scheme != "https" {
pkg/agent/agentbinary/upgrade.go:450
- Using
2 * MaxExtractedBytesdoes not leave enough fixed tar overhead for small valid limits. For example, a 600-byte member with a 1 KiB limit produces 2,560 decompressed tar bytes (header, padding, and end blocks) and is rejected even though the member is below the configured limit. Include fixed tar overhead in the total decompression bound while retaining overflow checks.
decompressed := &countingReader{reader: io.LimitReader(gz, 2*opts.MaxExtractedBytes+1)}
cmd/kubectl-unbounded/app/machine_operation_test.go:125
- The test name says SHA-256 is required, but the test explicitly verifies that an HTTP upgrade without SHA-256 is accepted. Rename it to describe the optional behavior so future readers do not infer the opposite contract.
func TestValidateAgentUpgradeRequiresSHA256(t *testing.T) {
Summary
pkg/agent/agentbinaryMotivation
AKS Flex Node needs the same blue/green agent binary mechanics but publishes architecture-specific binary names and requires stricter archive validation. The existing exported installer hardcodes
unbounded-agent, while the more complete orchestration is undercmd/agent/internal.This API lets callers provide binary names, paths, digest, permissions, limits, and an HTTP client without copying archive mechanics. The supplied HTTP client is wrapped so redirects remain HTTPS-only.
Testing
go test ./pkg/agent/agentbinary ./pkg/agent/internal/utiliogolangci-lintfor the changed packagesFull repository lint is blocked locally by missing OpenSSL development headers required by the TPM simulator typecheck.
Consumed by Azure/AKSFlexNode#266.