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>")
}
}
| // The inactive slot may still be last-good. Protect the verified running | ||
| // binary before replacing that slot. | ||
| if err := utilio.UpdateSymlink(paths.LastGoodPath, previousPath); err != nil { | ||
| return SwitchResult{}, fmt.Errorf("protect current agent as last-good: %w", err) | ||
| } |
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}
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.