-
Notifications
You must be signed in to change notification settings - Fork 78
Import security updates #210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
bcf93a5
6f956a4
04a1218
16634b7
866c522
4106199
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| package config | ||
|
|
||
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestResolveRemoteToken(t *testing.T) { | ||
| t.Setenv("WINGS_TOKEN_ID", "") | ||
| t.Setenv("WINGS_TOKEN", "") | ||
|
|
||
| cfg := Configuration{ | ||
| AuthenticationTokenId: "panel-id", | ||
| AuthenticationToken: "panel-token", | ||
| } | ||
| if err := cfg.ResolveToken(true); err != nil { | ||
| t.Fatalf("expected remote credentials to resolve: %v", err) | ||
| } | ||
| if cfg.Token.ID != "panel-id" || cfg.Token.Token != "panel-token" { | ||
| t.Fatalf("unexpected resolved credentials: %#v", cfg.Token) | ||
| } | ||
| } | ||
|
|
||
| func TestResolveRemoteTokenRejectsIndirection(t *testing.T) { | ||
| t.Setenv("WINGS_TOKEN_ID", "") | ||
| t.Setenv("WINGS_TOKEN", "") | ||
|
|
||
| tests := []Configuration{ | ||
| {AuthenticationTokenId: "file:///tmp/id", AuthenticationToken: "panel-token"}, | ||
| {AuthenticationTokenId: "panel-id", AuthenticationToken: "$PANEL_TOKEN"}, | ||
| } | ||
| for _, cfg := range tests { | ||
| if err := cfg.ResolveToken(true); err == nil { | ||
| t.Fatal("expected remote token indirection to be rejected") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestResolveRemoteTokenRequiresEnvironmentMatch(t *testing.T) { | ||
| secret := filepath.Join(t.TempDir(), "token") | ||
| if err := os.WriteFile(secret, []byte("panel-token\n"), 0o600); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| t.Setenv("WINGS_TOKEN_ID", "panel-id") | ||
| t.Setenv("WINGS_TOKEN", "file://"+secret) | ||
|
|
||
| cfg := Configuration{ | ||
| AuthenticationTokenId: "panel-id", | ||
| AuthenticationToken: "panel-token", | ||
| } | ||
| if err := cfg.ResolveToken(true); err != nil { | ||
| t.Fatalf("expected matching environment credentials to resolve: %v", err) | ||
| } | ||
|
|
||
| cfg.AuthenticationToken = "rotated-token" | ||
| if err := cfg.ResolveToken(true); err == nil { | ||
| t.Fatal("expected mismatched environment credentials to be rejected") | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| package docker | ||
|
|
||
| import ( | ||
| "context" | ||
| "os" | ||
| "path" | ||
| "strconv" | ||
| "strings" | ||
| "sync" | ||
|
|
||
| "emperror.dev/errors" | ||
| "github.com/apex/log" | ||
| "github.com/docker/docker/client" | ||
|
|
||
| "github.com/pelican/wings/config" | ||
| ) | ||
|
|
||
| // cgroupV2 reports whether the host uses the unified cgroup v2 hierarchy. | ||
| var cgroupV2 = sync.OnceValue(func() bool { | ||
| _, err := os.Stat("/sys/fs/cgroup/cgroup.controllers") | ||
| return err == nil | ||
| }) | ||
|
|
||
| var burstWarning sync.Once | ||
|
|
||
| // cpuBurstMicroseconds returns the burst allowance in microseconds for the given | ||
| // CFS quota and configured percentage. The kernel rejects a burst larger than the | ||
| // quota, so the value is clamped to it. | ||
| func cpuBurstMicroseconds(quota int64, percent int64) int64 { | ||
| if quota <= 0 || percent <= 0 { | ||
| return 0 | ||
| } | ||
| if percent > 100 { | ||
| percent = 100 | ||
| } | ||
| return quota * percent / 100 | ||
| } | ||
|
|
||
| // resolveCgroupCpuFile parses the contents of a /proc/<pid>/cgroup file and | ||
| // returns the absolute path of the CFS burst file for that process's cgroup. | ||
| func resolveCgroupCpuFile(procCgroup string, v2 bool) (string, error) { | ||
| for _, line := range strings.Split(procCgroup, "\n") { | ||
| parts := strings.SplitN(line, ":", 3) | ||
| if len(parts) != 3 || !strings.HasPrefix(parts[2], "/") || strings.Contains(parts[2], "..") { | ||
| continue | ||
| } | ||
| if v2 { | ||
| if parts[0] == "0" && parts[1] == "" { | ||
| return path.Join("/sys/fs/cgroup", parts[2], "cpu.max.burst"), nil | ||
| } | ||
| continue | ||
| } | ||
| for _, controller := range strings.Split(parts[1], ",") { | ||
| if controller == "cpu" { | ||
| return path.Join("/sys/fs/cgroup/cpu", parts[2], "cpu.cfs_burst_us"), nil | ||
|
Comment on lines
+53
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift Resolve the cgroup v1 CPU mount point. Line 55 hard-codes Read the CPU controller mount point from mount information before constructing the burst-file path. Update 🤖 Prompt for AI Agents |
||
| } | ||
| } | ||
| } | ||
| return "", errors.New("environment/docker: no cpu controller found in cgroup file") | ||
| } | ||
|
|
||
| // writeCpuBurst writes a burst value in microseconds into the cpu cgroup of the | ||
| // given process. This is expected to fail on kernels older than 5.14 or when the | ||
| // cgroup hierarchy is not writable by Wings, so failures are only logged. | ||
| func writeCpuBurst(l *log.Entry, pid int, burst int64) { | ||
| if pid <= 0 { | ||
| return | ||
| } | ||
| if err := writeBurstFile(pid, burst); err != nil { | ||
| logBurstFailure(l.WithField("error", err), burst) | ||
| return | ||
| } | ||
| l.WithField("burst_us", burst).Debug("updated container cpu burst") | ||
| } | ||
|
|
||
| func writeBurstFile(pid int, burst int64) error { | ||
| b, err := os.ReadFile("/proc/" + strconv.Itoa(pid) + "/cgroup") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| f, err := resolveCgroupCpuFile(string(b), cgroupV2()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return os.WriteFile(f, []byte(strconv.FormatInt(burst, 10)), 0o644) | ||
| } | ||
|
|
||
| // logBurstFailure warns the first time a burst cannot be applied and stays at | ||
| // debug otherwise. Failed clears are always quiet since a host that never | ||
| // accepted a burst has nothing to clear. | ||
| func logBurstFailure(l *log.Entry, burst int64) { | ||
| if burst > 0 { | ||
| first := false | ||
| burstWarning.Do(func() { first = true }) | ||
| if first { | ||
| l.Warn("failed to set cpu burst, this requires Linux 5.14 or newer and a writable cgroup hierarchy") | ||
| return | ||
| } | ||
| } | ||
| l.Debug("failed to set cpu burst") | ||
| } | ||
|
|
||
| // SetCpuBurst applies the configured CFS burst to a running container based on | ||
| // the CFS quota in microseconds it was created with. This is a no-op when | ||
| // bursting is disabled or the container has no CPU limit. | ||
| func SetCpuBurst(ctx context.Context, cli *client.Client, containerID string, quota int64) { | ||
| cfg := config.Get().Docker.CpuBurst | ||
| if !cfg.Enabled || quota <= 0 { | ||
| return | ||
| } | ||
| c, err := cli.ContainerInspect(ctx, containerID) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Redundant second
Suggested fix: pass the known pid (and, per the other comment, the actual quota) through from callers — |
||
| if err != nil || c.State == nil { | ||
| return | ||
| } | ||
| writeCpuBurst(log.WithField("container_id", containerID), c.State.Pid, cpuBurstMicroseconds(quota, cfg.Percent)) | ||
| } | ||
|
|
||
| // applyCpuBurst applies the configured CFS burst to the environment's container | ||
| // using its current CPU limit. | ||
| func (e *Environment) applyCpuBurst(ctx context.Context) { | ||
| quota := e.Configuration.Limits().CpuLimit * config.Get().Docker.CpuPeriodMicroseconds() / 100 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Recomputing the quota from current config silently disables bursting for existing containers after a This duplicates the formula in Suggested fix: use the container's actual quota instead of recomputing it. |
||
| SetCpuBurst(ctx, e.client, e.Id, quota) | ||
| } | ||
|
|
||
| // clearCpuBurst zeroes the CFS burst for the given container process. This must | ||
| // happen before a quota change is applied since the kernel rejects a quota lower | ||
| // than the current burst. It runs even when bursting is disabled so a value set | ||
| // before the feature was turned off cannot block future quota changes. | ||
| func (e *Environment) clearCpuBurst(pid int) { | ||
| writeCpuBurst(e.log(), pid, 0) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cgroup path resolution breaks when wings itself runs in a container (a shipped deployment — this repo has a
Dockerfile), and the failure warning is misleading.Per
cgroup_namespaces(7),/proc/<pid>/cgrouppaths are rendered relative to the reader's cgroup namespace. Host-installed wings sees full host paths and works, but containerized wings fails on several fronts: with a private cgroupns the target's path renders as/../..., which this..filter skips (→ “no cpu controller found”); without host PID namespace/proc/<host-pid>isn't even the right process; and the container's/sys/fs/cgroupmount typically exposes only its own subtree. Withcpu_burst.enableddefaulting totrue, every containerized install logs one false “requires Linux 5.14 or newer” warning and the feature silently never works.Separately, nothing validates that the resolved path belongs to the target container before
os.WriteFile— a crashed container plus PID reuse between inspect and write would mutate an unrelated cgroup. A cheap guard for both: verify the resolved path contains the container ID (holds under both/docker/<id>anddocker-<id>.scopelayouts), and mention cgroup-namespace visibility in the warning text.